Invalidate auth tokens on password change and reset#5128
Draft
RishabhJain2018 wants to merge 2 commits into
Draft
Invalidate auth tokens on password change and reset#5128RishabhJain2018 wants to merge 2 commits into
RishabhJain2018 wants to merge 2 commits into
Conversation
Revoke ExpiringToken, JwtToken, and blacklisted JWT refresh tokens on password change and password reset confirm so other browsers and devices cannot keep using old credentials. Frontend redirects to login after a successful password change and clears stored tokens. Adds unit tests for the revocation helper and API flows. Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a ChangesToken Revocation on Password Change/Reset
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/accounts/token_revocation.py (1)
22-28: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid one blacklist lookup per outstanding token.
Line 23 adds an N+1 read before the blacklist writes. Preload already-blacklisted token IDs once, then iterate only unblacklisted outstanding tokens.
As per path instructions,
apps/**: “Django backend. Pay attention to: Database query efficiency (N+1 queries, missing select_related/prefetch_related)”.♻️ Proposed query reduction
- for outstanding_token in OutstandingToken.objects.filter(user=user): - if BlacklistedToken.objects.filter(token=outstanding_token).exists(): - continue + blacklisted_token_ids = set( + BlacklistedToken.objects.filter(token__user=user).values_list( + "token_id", flat=True + ) + ) + + for outstanding_token in OutstandingToken.objects.filter( + user=user + ).exclude(id__in=blacklisted_token_ids): try: RefreshToken(outstanding_token.token).blacklist() except TokenError: BlacklistedToken.objects.get_or_create(token=outstanding_token)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/accounts/token_revocation.py` around lines 22 - 28, The revocation loop in outstanding token processing is doing an N+1 blacklist read via BlacklistedToken.objects.filter(token=outstanding_token).exists() for every token; update the token revocation flow in token_revocation.py to fetch the already-blacklisted outstanding token IDs once up front, then iterate only over unblacklisted OutstandingToken entries before calling RefreshToken(...).blacklist() and the TokenError fallback. Keep the logic centered around the existing OutstandingToken and BlacklistedToken queryset usage so the loop no longer performs a per-token blacklist lookup.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/accounts/serializers.py`:
- Around line 350-352: The password update and token revocation are not atomic
in the serializer save path, so a failure in revocation can leave the new
password committed while old tokens still work. Update the relevant serializer
save methods in apps/accounts/serializers.py to wrap both
self.set_password_form.save() and revoke_all_user_tokens(self.user) in a single
transaction using the existing save method(s) as the entry point, ensuring both
operations succeed or fail together.
In `@tests/unit/accounts/test_token_revocation.py`:
- Around line 25-29: The test fixture user is being created with an unhashed
password via User.objects.create, which will break old_password validation in
the revocation flow. Update the setup in the test class to use
User.objects.create_user or create the User and call set_password before saving
so the password-change and token revocation checks operate on a valid hashed
password.
---
Nitpick comments:
In `@apps/accounts/token_revocation.py`:
- Around line 22-28: The revocation loop in outstanding token processing is
doing an N+1 blacklist read via
BlacklistedToken.objects.filter(token=outstanding_token).exists() for every
token; update the token revocation flow in token_revocation.py to fetch the
already-blacklisted outstanding token IDs once up front, then iterate only over
unblacklisted OutstandingToken entries before calling
RefreshToken(...).blacklist() and the TokenError fallback. Keep the logic
centered around the existing OutstandingToken and BlacklistedToken queryset
usage so the loop no longer performs a per-token blacklist lookup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c399ed0-956c-4d55-8359-ac0a91af19b8
📒 Files selected for processing (8)
apps/accounts/serializers.pyapps/accounts/token_revocation.pyfrontend/src/js/controllers/changePwdCtrl.jsfrontend/src/js/controllers/profileCtrl.jsfrontend/tests/controllers-test/changePwdCtrl.test.jsfrontend/tests/controllers-test/profileCtrl.test.jssettings/common.pytests/unit/accounts/test_token_revocation.py
Comment on lines
+350
to
+352
| def save(self): | ||
| self.set_password_form.save() | ||
| revoke_all_user_tokens(self.user) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether request-level transactions already cover these password endpoints.
rg -n --glob 'settings/**' 'ATOMIC_REQUESTS|DATABASES'
rg -n --glob 'apps/**/*.py' 'transaction\.atomic|revoke_all_user_tokens|CustomPassword(Change|ResetConfirm)Serializer'Repository: Cloud-CV/EvalAI
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files | rg '(^|/)(apps/accounts/serializers\.py|.*settings.*\.py|.*settings/.*\.py|.*database.*\.py)$'
printf '\n== serializers outline ==\n'
ast-grep outline apps/accounts/serializers.py --view expanded || true
printf '\n== relevant settings hits ==\n'
rg -n 'ATOMIC_REQUESTS|DATABASES|AUTOCOMMIT|atomic\(' -g '*.py' .
printf '\n== revocation helper hits ==\n'
rg -n 'def revoke_all_user_tokens|revoke_all_user_tokens\(' -g '*.py' apps .Repository: Cloud-CV/EvalAI
Length of output: 4480
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== serializers section ==\n'
sed -n '340,370p' apps/accounts/serializers.py
printf '\n== token revocation helper ==\n'
sed -n '1,220p' apps/accounts/token_revocation.py
printf '\n== settings common ==\n'
sed -n '1,220p' settings/common.py
printf '\n== settings env files atomics ==\n'
for f in settings/dev.py settings/prod.py settings/staging.py settings/test.py; do
printf '\n--- %s ---\n' "$f"
rg -n 'ATOMIC_REQUESTS|DATABASES|AUTOCOMMIT|transaction' "$f" || true
doneRepository: Cloud-CV/EvalAI
Length of output: 8491
Wrap password save/revocation in a transaction.
apps/accounts/serializers.py:350-362 Both serializers commit the new password before revoke_all_user_tokens(). If any revocation write fails, the password change can persist while old credentials remain usable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/accounts/serializers.py` around lines 350 - 352, The password update and
token revocation are not atomic in the serializer save path, so a failure in
revocation can leave the new password committed while old tokens still work.
Update the relevant serializer save methods in apps/accounts/serializers.py to
wrap both self.set_password_form.save() and revoke_all_user_tokens(self.user) in
a single transaction using the existing save method(s) as the entry point,
ensuring both operations succeed or fail together.
Comment on lines
+25
to
+29
| self.user = User.objects.create( | ||
| username="someuser", | ||
| email="user@test.com", | ||
| password="secret_password", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant test file and nearby context
git ls-files tests/unit/accounts/test_token_revocation.py
wc -l tests/unit/accounts/test_token_revocation.py
sed -n '1,220p' tests/unit/accounts/test_token_revocation.py
# Find the user model / auth helpers used in this area
rg -n "create_user|set_password|check_password|old_password|token revocation|revocation" tests src . -g '!**/node_modules/**'Repository: Cloud-CV/EvalAI
Length of output: 21870
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the password-change / reset serializers and any helper they use.
sed -n '300,390p' apps/accounts/serializers.py
# Inspect the token revocation helper for the side effects the tests assert.
sed -n '1,220p' apps/accounts/token_revocation.py
# Inspect the model used to store JWT tokens.
sed -n '1,220p' apps/accounts/models.pyRepository: Cloud-CV/EvalAI
Length of output: 7547
Create the fixture user with a hashed password. User.objects.create(...) leaves password unusable for old_password checks, so these password-change tests will fail before the revocation assertions run. Use create_user(...) or call set_password().
🧰 Tools
🪛 Ruff (0.15.18)
[error] 28-28: Possible hardcoded password assigned to argument: "password"
(S106)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/accounts/test_token_revocation.py` around lines 25 - 29, The test
fixture user is being created with an unhashed password via User.objects.create,
which will break old_password validation in the revocation flow. Update the
setup in the test class to use User.objects.create_user or create the User and
call set_password before saving so the password-change and token revocation
checks operate on a valid hashed password.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the reported Failure to Invalidate Session on Password Change vulnerability.
When a user changes or resets their password, EvalAI now revokes all outstanding credentials for that account:
ExpiringToken)JwtToken)OutstandingTokenblacklist)Other browsers/devices can no longer keep using old tokens after a password change. The browser that performed the change is also logged out and redirected to the login page.
Changes
revoke_all_user_tokens()helper inapps/accounts/token_revocation.pyPasswordChangeSerializerandPasswordResetConfirmSerializerthat call the helper after saving the new passwordREST_AUTH_SERIALIZERSin settingslocalStorageand redirect to loginTest plan
pytest tests/unit/accounts/test_token_revocation.pychangePwdCtrlandprofileCtrlSlack Thread
Summary by CodeRabbit
New Features
Bug Fixes
Tests