Bug: Valid Refresh Tokens despite user changing password

[tichnas]

Description

Password is added along with id as the identity for refresh token to make it invalid on password change.

Fixes #903

Type of Change:

• Code
• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Ran python -m unittest discover tests which gave OK result

Checklist:

• My PR follows the style guidelines of this project
• I have performed a self-review of my own code or materials
• I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• Any dependent changes have been merged

Code/Quality Assurance Only

• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[codecov]

Codecov Report

Merging #950 (9872c7e) into develop (27c0337) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #950   +/-   ##
========================================
  Coverage    96.00%   96.01%           
========================================
  Files           96       96           
  Lines         5307     5319   +12     
========================================
+ Hits          5095     5107   +12     
  Misses         212      212           

Impacted Files	Coverage Δ
app/api/resources/user.py	89.09% <100.00%> (+0.15%)	⬆️
tests/users/test_api_refresh.py	98.07% <100.00%> (+0.29%)	⬆️
app/api/models/user.py	100.00% <0.00%> (ø)
app/api/api_extension.py	100.00% <0.00%> (ø)
app/api/resources/common.py	100.00% <0.00%> (ø)

[tichnas]

@devkapilbansal @isabelcosta As far as I understand, I've to add a test when the password is changed, and we are using the same (old) refresh token (a test where we go inside the if condition). Am I correct? Is there anything else needed?

Thanks in advance :)

[devkapilbansal]

@devkapilbansal @isabelcosta As far as I understand, I've to add a test when the password is changed, and we are using the same (old) refresh token (a test where we go inside the if condition). Am I correct? Is there anything else needed?

Thanks in advance :)

Yes @tichnas you have too.

[devkapilbansal]

Going in the right direction 👍

[tichnas]

@isabelcosta @devkapilbansal Please review. Thanks :)

[devkapilbansal]

@tichnas don't use hard coded status codes in test. You have used 401(hard coded). Replace it by using it from HTTP_STATUS as done in API files

[tichnas]

@devkapilbansal It's like this in all test files. Should I change them all or just this one?

[devkapilbansal]

@devkapilbansal It's like this in all test files. Should I change them all or just this one?

Change for the tests you added. It would be better to open an issue for them

[tichnas]

Thanks for the suggestion @devkapilbansal. I've updated the test case and have also created an issue for all other test cases.

[devkapilbansal]

Thanks @tichnas , although it looks good I will test it soon.
@epicadk can you please help in reviewing this pull request?

[devkapilbansal]

Thanks @tichnas Tested locally and it works

The changes made in this PR were tested locally. Following are the results:

1. Code review - Done

2. All possible responses (positive and negative tests) were tested as below:

• Test1 Description Refresh Token Validation after Password Change
  Screenshot/gif:
  Refresh Token Valid before Password Change
  

Refresh Token invalid after Password Change

_Expected Result_:  Token should be invalid after password change
_Actual Result_: Same as expected

3. Status of PR Changed to: Ready to Merge.

[devkapilbansal]

@vj-codes @isabelcosta Kindly review this PR

[vj-codes]

Thank you for your contibution @tichnas 🎉

[vj-codes]

Adding label ready to merge since testing is done above

[isabelcosta]

@tichnas @devkapilbansal and @vj-codes I put the "on Hold" label because I am not sure this is a secure solution, to include the password hash in the token. This is because a jwt token, when decoded can reveal the content of the identity. You can see here https://jwt.io/ the composition of a token. I would like to have other opinions on this, perhaps we could reach out to Zulip community and ask. I'll post on Zulip and look more into this.

cc @ramitsawhney27 @m-murad @SanketDG in case you have any idea about this 🙏

[epicadk]

@tichnas @devkapilbansal and @vj-codes I put the "on Hold" label because I am not sure this is a secure solution, to include the password hash in the token. This is because a jwt token, when decoded can reveal the content of the identity. You can see here https://jwt.io/ the composition of a token. I would like to have other opinions on this, perhaps we could reach out to Zulip community and ask. I'll post on Zulip and look more into this.

cc @ramitsawhney27 @m-murad @SanketDG in case you have any idea about this 🙏

uh oh! 😅 I see it now. This was not what I meant. The content of the token need not be changed. What needs to be changed is the refresh signing key. Thanks @isabelcosta for catching this.

[epicadk]

The contents of the token do not need to be modified. Just append the users password hash while you sign the refresh token that is all.

[mtreacy002]

Hi @isabelcosta, @tichnas, @devkapilbansal, @epicadk, and @vj-codes. Just thought I might share my two cents here 😉. Apologies if it doesn't make sense though, still a newbie here with python and jwt 🤣🤣🤣.

[isabelcosta]

I will close this PR due to inactivity. Thank you @tichnas for your contribution 🙌
@mtreacy002, @devkapilbansal, @vj-codes and @epicadk for your inputs in this PR!