feat(auth): Revoke refresh token on password change #928
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances the JWT authentication system to properly revoke refresh tokens when a user's password changes. The implementation adds password validation to the token refresh process to prevent infinite loops in client applications caused by tokens remaining valid after password changes.
- Adds password hash validation to
TokenRefreshSerializerwhenCHECK_REVOKE_TOKENis enabled - Implements automatic token blacklisting when password changes are detected
- Improves error handling for deleted users by raising consistent
AuthenticationFailedexceptions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rest_framework_simplejwt/serializers.py | Adds password validation logic and token blacklisting in TokenRefreshSerializer |
| tests/test_serializers.py | Adds comprehensive tests for password change scenarios and updates existing deleted user test |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@sshishov could you please try using simplejwt by installing from this PR and check that your problem solved correctly? |
The new automated tests included in this PR already cover the exact scenario reported by @sshishov. The Furthermore, the These tests confirm the fix is working as intended. |
|
I see error message pattern in this line djangorestframework-simplejwt/rest_framework_simplejwt/serializers.py Lines 108 to 110 in d1412c4 djangorestframework-simplejwt/rest_framework_simplejwt/serializers.py Lines 124 to 129 in d1412c4 I think you should add the error message to the dict and follow existing pattern on this code djangorestframework-simplejwt/rest_framework_simplejwt/serializers.py Lines 151 to 154 in d1412c4 |
I fix it. |
|
I think you should revert the code that you changed in authentication.py because i mean only update the code in serializers.py. Also i only see that pattern in serializers.py, and it will make this PR more specific |
For context: implementing this fix required duplicating the There may be an opportunity here for a more centralized error message pattern to improve consistency. |
|
Hmm okay. It looks good to me. But i will leave to @Andrew-Chen-Wang because he is the maintainer |
| default_error_messages = { | ||
| "no_active_account": _("No active account found for the given token.") | ||
| "no_active_account": _("No active account found for the given token."), | ||
| "password_changed": _("The user's password has been changed."), |
There was a problem hiding this comment.
re-run the locale generation since this is a new gettext (i.e. the line where the gettext is written has moved)
There was a problem hiding this comment.
I noticed that the project uses a custom script (scripts/i18n_updater.py) for locale updates, likely to avoid unnecessary changes in translation files. Since this is a multi-language project, I won't run makemessages or compilemessages directly. Please let me know if you prefer me to run the custom script or if locale updates are handled separately by maintainers.
There was a problem hiding this comment.
Update: I ran the i18n_updater.py script, but it generated no changes.
This is because the The user's password has been changed. string already exists in the translation catalog from a previous PR (originating from authentication.py:146), so makemessages correctly finds no new strings to add. No further action should be needed.
| try: | ||
| user = get_user_model().objects.get( | ||
| **{api_settings.USER_ID_FIELD: user_id} | ||
| ) | ||
| except get_user_model().DoesNotExist: | ||
| # This handles the case where the user has been deleted. | ||
| raise AuthenticationFailed( | ||
| self.error_messages["no_active_account"], "no_active_account" | ||
| ) |
There was a problem hiding this comment.
raising AuthenticationFailed is a breaking change. What is the problem with DoesNotExist error?
There was a problem hiding this comment.
Great question! Here's why I think 401 is better than 404:
Security-wise: Using 401 instead of 404 prevents attackers from figuring out if a token exists in our system. If we return 404, they know the token doesn't exist, which is information we shouldn't leak.
Standards-wise: RFC 7235 says authentication errors should be 401, not 404. This keeps us consistent with web standards.
User experience: When users get a 401, they know it's an authentication problem and can try logging in again. A 404 might confuse them into thinking the API endpoint doesn't exist.
Code-wise: DoesNotExist is a database error that shouldn't bubble up to users. AuthenticationFailed is the right error for auth issues.
There was a problem hiding this comment.
Whilst I agree that 401 is more secure, that's only in the context of the user not being logged in. This serializer requires accepting a refresh token.
I agree on the rest. Approve of the change. Can you add this to the CHANGELOG.md?
There was a problem hiding this comment.
I'll apply it right away.
On a related note, I think there might be a pre-existing issue with how AuthenticationFailed is handled here. I'll explain in a follow-up comment.
There was a problem hiding this comment.
Following up on my previous comment, I'd like to discuss a pre-existing issue regarding how AuthenticationFailed exceptions are handled, which can lead to an unexpected 403 Forbidden response instead of a 401 Unauthorized.
The Root Cause in Django REST Framework
As per the DRF documentation on authentication responses, the framework distinguishes between 401 and 403 responses for unauthenticated requests. A 401 response must include a WWW-Authenticate header, while a 403 response does not.
The logic that enforces this is in DRF's APIView.handle_exception method:
# In rest_framework.views.APIView
def handle_exception(self, exc):
if isinstance(exc, (exceptions.NotAuthenticated,
exceptions.AuthenticationFailed)):
# WWW-Authenticate header for 401 responses, else coerce to 403
auth_header = self.get_authenticate_header(self.request)
if auth_header:
exc.auth_header = auth_header
else:
exc.status_code = status.HTTP_403_FORBIDDEN
# ...This code explicitly changes the status code to 403 FORBIDDEN if an AuthenticationFailed exception is caught and get_authenticate_header() returns a "falsy" value (which happens if no Authorization header was provided in the request).
The Impact on simplejwt
When we raise AuthenticationFailed in our serializers (for cases like a deleted user or a password change), we correctly signal an authentication failure. However, if the client makes this request without an Authorization header, DRF's logic will convert our intended 401 into a 403.
This behavior is inconsistent and can be problematic:
- Security: A 401 response is more appropriate as it does not leak information about the existence of a resource.
- Standards (RFC 7235): 401 is the standard response for authentication-related failures.
- User Experience: A 401 clearly tells the client to re-authenticate. A 403 is ambiguous and could be misinterpreted as a permanent permission issue.
Lack of Test Coverage
This specific scenario is not currently covered by the project's test suite. We could add a test like the following to demonstrate this behavior:
# In tests/test_integration.py
def test_it_should_return_403_if_authenticate_header_is_not_set(self):
"""
Tests that a 403 Forbidden is returned if authentication fails
and the `authenticate_header` method returns no value.
"""
# We mock the `authenticate_header` method to return None. This
# triggers the specific code path in DRF's exception handler
# that converts a 401 Unauthorized to a 403 Forbidden.
from unittest.mock import patch
with patch(
"rest_framework_simplejwt.authentication.JWTAuthentication.authenticate_header",
return_value=None,
):
self.authenticate_with_token(api_settings.AUTH_HEADER_TYPES[0], "an_invalid_token_string")
res = self.view_get()
# We now expect a 403 Forbidden response instead of a 401.
self.assertEqual(res.status_code, 403)
self.assertEqual(res.data["detail"].code, "token_not_valid")Potential Solution
To ensure a consistent 401 response in all authentication failure scenarios, one solution could be to use a custom exception class that bypasses DRF's status code coercion, something like this:
from rest_framework.exceptions import APIException
class CustomNotAuthenticated(APIException):
status_code = status.HTTP_401_UNAUTHORIZED
default_detail = _('Authentication credentials were not provided.')
default_code = 'not_authenticated'I wanted to bring this to your attention as it seems to be a broader architectural point. I'm happy to discuss this further.
There was a problem hiding this comment.
Let’s continue the 401 vs 403 discussion in a separate issue.
For this PR, could you give your final feedback? If there’s any problem with the current changes, I’ll update them.
There was a problem hiding this comment.
I’ve done the rebase, this PR is now updated @Andrew-Chen-Wang
There was a problem hiding this comment.
@Andrew-Chen-Wang
Could you please give me some feedback and share your thoughts on the latest change?
Added user validation logic to TokenRefreshSlidingSerializer. |
|
@mahdirahimi1999 perhaps renaming djangorestframework-simplejwt/rest_framework_simplejwt/serializers.py Lines 134 to 137 in f4e9886 djangorestframework-simplejwt/rest_framework_simplejwt/serializers.py Lines 204 to 207 in f4e9886 Just a matter of name though |
Fix it. |
mahdirahimi1999
requested review from
pauloxnet
and removed request for
ajay09 and
pauloxnet
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Implements the same user validation logic (active status, password change) in to ensure consistent behavior with the standard .
Simplifies the conditional check by removing temporary variables for the token hash and user password hash.
for more information, see https://pre-commit.ci
… for missing users
mahdirahimi1999
force-pushed
the
fix-refresh-token-revocation
branch
from
88eff03 to
ea764f1
Compare
| user = ( | ||
| get_user_model() | ||
| .objects.filter(**{api_settings.USER_ID_FIELD: user_id}) | ||
| .first() | ||
| ) | ||
| if not user or not api_settings.USER_AUTHENTICATION_RULE(user): | ||
| try: | ||
| user = get_user_model().objects.get( | ||
| **{api_settings.USER_ID_FIELD: user_id} | ||
| ) | ||
| except get_user_model().DoesNotExist: | ||
| # This handles the case where the user has been deleted. | ||
| raise AuthenticationFailed( | ||
| self.error_messages["no_active_account"], | ||
| "no_active_account", | ||
| self.error_messages["no_active_account"], "no_active_account" | ||
| ) | ||
|
|
||
| data = {"access": str(refresh.access_token)} | ||
| if not api_settings.USER_AUTHENTICATION_RULE(user): | ||
| raise AuthenticationFailed( | ||
| self.error_messages["no_active_account"], "no_active_account" | ||
| ) |
There was a problem hiding this comment.
I think your changes are the try/except; please ensure it uses the code from #861 to be more DRY
There was a problem hiding this comment.
Hi Andrew,
Thanks for the feedback. I considered the suggestion to use .filter().first(), but in this case .get() is more appropriate. Since we're querying by primary key, .get() is both the most direct and idiomatic Django approach—it clearly expects a single unique record and raises DoesNotExist if it's missing.
In contrast, .filter().first() builds a queryset, allows multiple matches, and is less explicit. So this isn't about code repetition, but about choosing the most suitable and efficient method for a primary key lookup.
You're welcome, glad I could help |
|
@Andrew-Chen-Wang |
3 participants
Closes #927
Description
This pull request addresses an issue where refresh tokens remain valid even after a user changes their password, despite
CHECK_REVOKE_TOKENbeing enabled. This behavior can lead to infinite loops on client applications, where an expired access token prompts a token refresh, which succeeds, but the new access token is still invalid.Changes
This PR introduces a password validation step into the
TokenRefreshSerializerto ensure refresh tokens are properly invalidated when a user's password changes.The key changes are:
validatemethod inTokenRefreshSerializernow fetches the user associated with the refresh token.CHECK_REVOKE_TOKENisTrue, it compares the password hash stored in the token's payload with the user's current password hash in the database.AuthenticationFailedexception is raised with the new error codepassword_changed.User.DoesNotExistand raising a consistentAuthenticationFailederror.Testing