Skip to content

Revoke access token if user password is changed#719

Merged
Andrew-Chen-Wang merged 18 commits intojazzband:masterfrom
mahdirahimi1999:feature/revoke-access-token
Jun 27, 2023
Merged

Revoke access token if user password is changed#719
Andrew-Chen-Wang merged 18 commits intojazzband:masterfrom
mahdirahimi1999:feature/revoke-access-token

Conversation

@mahdirahimi1999
Copy link
Copy Markdown
Member

In this pull request, I have added a new key to the JWT token payload called hash_password, which is generated through the following function:

def get_md5_hash_password(password: str) -> str:
    """
    Returns MD5 hash of the given password
    """
    return hashlib.md5(password.encode()).hexdigest().upper()

Then, during JWT token validation, I check if the current user password matches the value in the hash_password field of the JWT token payload. If these two values are not equal, it means that the user has changed their password.

Additionally, the tests related to JWT token validation have also been updated accordingly with these changes.

mahdirahimi1999 and others added 17 commits April 21, 2023 03:54
* Update `django.po` for id translation

* Update `django.mo` for Bahasa Indonesia (id) translations.
Co-authored-by: Mahdi <mahdi@Mahdis-MacBook-Pro.local>
In ""TOKEN_OBTAIN_SERIALIZER": "rest_framework_simplejwt.serializers.MyTokenObtainPairSerializer"," replaced "rest_framework_simplejwt" to "my_app" to make it clearer that it should be a custom path, since the Django app folder having the same name as the library was confusing and hard to fix if copy and pasting in a hurry.
* Added write_only=True for better doc generation

Auto doc generators can perform better and generate more accurate docs
by having this argument.
Username field in TokenObtainSerializer and token in TokenVerifySerializer
has been changed.

* Added write_only=True to TokenBlacklistSerializer's refresh field
* Add support for Django 4.2

* Exclude DRF 3.13 & Django 4.2 CI combination
* [pre-commit.ci] pre-commit autoupdate

updates:
- [github.com/asottile/yesqa: v1.4.0 → v1.5.0](asottile/yesqa@v1.4.0...v1.5.0)
- [github.com/psf/black: 22.12.0 → 23.3.0](psf/black@22.12.0...23.3.0)
- [github.com/asottile/pyupgrade: v3.3.1 → v3.7.0](asottile/pyupgrade@v3.3.1...v3.7.0)

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Support `override_api_settings` as decorator

* Update test_authentication

* black formatting  test_authentication

* Use drf status instead of literal status

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update test_integration

* Update test_serializers

* Update test_integration

* Update test_token_blacklist

* Update test_tokens

* Update test_views

* add `setUpTestData` to `TestToken`

* fix typo `self` should be `cls`

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@mahdirahimi1999
Copy link
Copy Markdown
Member Author

@Andrew-Chen-Wang
What should I do???

@Andrew-Chen-Wang
Copy link
Copy Markdown
Member

you should run the tests locally and debug. Follow our docs

@mahdirahimi1999
Copy link
Copy Markdown
Member Author

you should run the tests locally and debug. Follow our docs

OK

@mahdirahimi1999
Copy link
Copy Markdown
Member Author

All tests passed.

@Andrew-Chen-Wang

@Andrew-Chen-Wang
Copy link
Copy Markdown
Member

removing tests is not valid.

@mahdirahimi1999
Copy link
Copy Markdown
Member Author

All tests passed.

I already added that line of the test to the test_get_user method, and these changes are implemented in the test_get_user_with_check_revoke_token.

@mahdirahimi1999
Copy link
Copy Markdown
Member Author

All tests passed.

I already added that line of the test to the test_get_user method, and these changes are implemented in the test_get_user_with_check_revoke_token.

@Andrew-Chen-Wang

@Andrew-Chen-Wang
Copy link
Copy Markdown
Member

sorry didn't notice; only read latest commit. Thanks again:)

Copy link
Copy Markdown
Member

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks again!

@thecarpetjasp
Copy link
Copy Markdown
Contributor

Could you please change this from MD5 to SHA256?

MD5 is not considered secure and is considered a very fast hash to compute, so makes it more ideal for brute-force attempts.

I think given the level of importance to this data, which is normally secured in a database, is now outside, there is every more reason to use a much more secure hashing algorithm for this instead, such as SHA256.

Would be a very quick fix and would greatly appreciate.

@Andrew-Chen-Wang
Copy link
Copy Markdown
Member

Though I'd agree, here are a few items to consider:

  • User passwords should already be hashed with one of Django's recommended password hashers like argon2 or PBKDF. If the user decides not to use that...
  • The JWTs should also already be encrypted. The claims are actually just part of the JWT payload which is encrypted by your RSA/HMAC keys. The only way to unencrypt the token is if your keys were leaked which is a much larger / system problem, and, even then, the pen testers would still need to break through the password hash which is only a problem if you allow users to set bad / haveibeenpwned passwords.
  • SHA256 (at this point, why not SHA-3 or BLAKE2; not sure which are in hashlib?) would increase the CPU load on the server. I think it's unnecessary. Feel free to open the issue (and copy my comment) to field others' feedback.

There wouldn't be any breaking changes. A PR would only involve checking first the new hash function then the MD hash. Really, it's a performance thing, and a PR which should be small is always appreciated.

@thecarpetjasp
Copy link
Copy Markdown
Contributor

Could you expand a little more on JWT being encrypted? As far as I'm aware, the JWT is only encoding with base64, which can easily be decoded.

I hear your point regarding SHA256 adding load on the CPU. I suppose I feel a little restless thinking that the users hashed password could potentially be leaked, given MD5 insecurities, which still isn't so devastating in itself, as the users password is still hashed, or should be as you mentioned.

Personally I would feel a lot better if it was something more recent than MD5 - but I do agree on the CPU load argument, which might make MD5 more suitable for this use. I guess this feature is also optional at the end of the day!

Thanks for considering the request and for your response!

@Andrew-Chen-Wang
Copy link
Copy Markdown
Member

Andrew-Chen-Wang commented Feb 18, 2025

Could you expand a little more on JWT being encrypted? As far as I'm aware, the JWT is only encoding with base64, which can easily be decoded.

Different authentication systems create JWTs differently. With this library, we instruct users to create an HMAC or RSA (or other cryptography-supported algorithms) to encrypt our JWT. You can take a look at backend.py on how we use PyJWT and pass our algorithm to encrypt the JWT payload.

The final JWT is what you're thinking of being base64 encoded, but typically JWT payloads are encrypted.

As a note, the "payload" is a term for the middle segment of the JWT itself!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants