Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
github-actions
Bot
added
front-end
Docker builds report
|
📝 WalkthroughWalkthroughThe backend enables token revocation when a password changes. The dashboard now requires confirmation before submitting a password change, logs the user out after success, and displays a notification at login. Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: High Merge Risk: 🟡 Moderate · up to Changing a password now signs out other sessions. Resetting a password does not, so a device that was already logged in stays logged in after a reset. Issue
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
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
3e7486dc-9d8d-4e94-96ba-f5a2b256a864
📒 Files selected for processing (5)
api/app/settings/common.pyapi/tests/unit/custom_auth/test_unit_custom_auth_views.pyfrontend/common/constants.tsfrontend/web/components/pages/AccountSettingsPage.tsxfrontend/web/components/pages/home-page/HomePage.tsx
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| "confirmation": "users.emails.ConfirmationEmail", | ||
| }, | ||
| "SET_PASSWORD_RETYPE": True, | ||
| "LOGOUT_ON_PASSWORD_CHANGE": True, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Revoke tokens after password resets too.
LOGOUT_ON_PASSWORD_CHANGE affects Djoser's set_password action, but not reset_password_confirm. After a successful password reset, an existing DRF token therefore remains valid. Add token revocation to the reset-confirmation path and test a request made with the old token. (raw.githubusercontent.com)
| onYes: () => { | ||
| setIsSaving(true) | ||
| _data | ||
| .post(`${Project.api}auth/users/set_password/`, { | ||
| current_password: currentPassword, | ||
| new_password: newPassword1, | ||
| re_new_password: newPassword2, | ||
| }) | ||
| .then(() => { | ||
| setIsSaving(false) | ||
| // Changing the password revokes the auth token on every device, | ||
| // including this one, so log out to show the login page. | ||
| sessionStorage.setItem(PASSWORD_CHANGED_SESSION_KEY, 'true') | ||
| AppActions.logout() | ||
| }) | ||
| .catch(() => { | ||
| setIsSaving(false) | ||
| setPasswordError( | ||
| 'There was an error setting your password, please check your details.', | ||
| ) | ||
| }) | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear passwordError when the user confirms a new attempt.
The onYes handler sets passwordError on failure. It never clears the error before a retry. A stale error stays visible while the new request runs. Call setPasswordError(null) at the start of onYes.
A second concern: the logout depends on the .then callback. If AppActions.logout() throws, the .catch handler shows a "password" error although the password changed. This is unlikely. Keep the logout call outside the request error path if you want exact error reporting.
Proposed fix
--- "a/frontend/web/components/pages/AccountSettingsPage.tsx"
+++ "b/frontend/web/components/pages/AccountSettingsPage.tsx"
@@ -147,8 +147,9 @@
password.
</div>
),
onYes: () => {
+ setPasswordError(null)
setIsSaving(true)
_data
.post(`${Project.api}auth/users/set_password/`, {
current_password: currentPassword,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onYes: () => { | |
| setIsSaving(true) | |
| _data | |
| .post(`${Project.api}auth/users/set_password/`, { | |
| current_password: currentPassword, | |
| new_password: newPassword1, | |
| re_new_password: newPassword2, | |
| }) | |
| .then(() => { | |
| setIsSaving(false) | |
| // Changing the password revokes the auth token on every device, | |
| // including this one, so log out to show the login page. | |
| sessionStorage.setItem(PASSWORD_CHANGED_SESSION_KEY, 'true') | |
| AppActions.logout() | |
| }) | |
| .catch(() => { | |
| setIsSaving(false) | |
| setPasswordError( | |
| 'There was an error setting your password, please check your details.', | |
| ) | |
| }) | |
| }, | |
| onYes: () => { | |
| setPasswordError(null) | |
| setIsSaving(true) | |
| _data | |
| .post(`${Project.api}auth/users/set_password/`, { | |
| current_password: currentPassword, | |
| new_password: newPassword1, | |
| re_new_password: newPassword2, | |
| }) | |
| .then(() => { | |
| setIsSaving(false) | |
| // Changing the password revokes the auth token on every device, | |
| // including this one, so log out to show the login page. | |
| sessionStorage.setItem(PASSWORD_CHANGED_SESSION_KEY, 'true') | |
| AppActions.logout() | |
| }) | |
| .catch(() => { | |
| setIsSaving(false) | |
| setPasswordError( | |
| 'There was an error setting your password, please check your details.', | |
| ) | |
| }) | |
| }, |
Source: Learnings
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8698 +/- ##
=======================================
Coverage 98.85% 98.85%
=======================================
Files 1663 1663
Lines 68711 68720 +9
=======================================
+ Hits 67921 67930 +9
Misses 790 790 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21402 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-16 — run #21402 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21402 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #21402 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #21401 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #21401 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #21401 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #21401 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression20 screenshots compared. See report for details. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
11e6a933-d532-4f74-b3f2-6e8eb7349872
📒 Files selected for processing (1)
api/tests/unit/custom_auth/test_unit_custom_auth_views.py
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.
| staff_user: FFAdminUser, | ||
| ) -> None: | ||
| # Given | ||
| staff_user.password = make_password("old-password") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Revoke tokens after password resets as well.
This test covers set_password only, but issue #8651 also requires revocation after reset_password_confirm. Djoser 2.3.0 and 2.3.4 revoke tokens in set_password; reset_password_confirm saves the new password without revoking tokens. An existing token can therefore still authenticate after a reset. (raw.githubusercontent.com) Add revocation to the reset flow and assert that a pre-reset token receives 401.
This branch has not been deployed
1 participant
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #8651
How did you test this code?
Logged in, changed password, verified the token was deleted in Django-admin, after logging in with the new password a new token is issued. When the user wants to change the password they get a modal warning them they will be logged out and when they are redirected to the login page a banner says they need to log back in with the new password.