ENG-681 Re implement pw reset session invalidation #6526
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements comprehensive session invalidation logic to address the security vulnerability CVE-2025-57766. When a user's password is reset or updated, all existing authentication sessions are immediately invalidated to prevent unauthorized access from potentially compromised tokens.
The implementation operates at multiple layers:
Backend Authentication Layer: A new is_token_invalidated() function in oauth/utils.py checks if tokens were issued before the user's most recent password reset by comparing token timestamps against the password_reset_at field. This function is integrated into the token extraction process to automatically reject outdated tokens with HTTP 403 responses.
OAuth Client Management: During password resets, the user's associated OAuth client is deleted entirely in both user-initiated (update_user_password) and admin-forced (force_update_password) scenarios. This provides an additional layer of session invalidation beyond timestamp checking.
Frontend Session Management: Both password update modals (UpdatePasswordModal and NewPasswordModal) now implement proper logout flows when users change their own passwords. This includes clearing localStorage, dispatching logout actions, and redirecting to the login page to ensure client-side state remains consistent with server-side invalidation.
Model Relationships: A circular import issue between ClientDetail and FidesUser models was resolved using TYPE_CHECKING blocks to maintain the relationship needed for password reset timestamp validation.
The changes are thoroughly tested with a new test case that verifies old tokens become invalid immediately after password resets. This security enhancement ensures that password changes - whether due to compromise, security incidents, or routine updates - immediately terminate all existing sessions, forcing users to re-authenticate with their new credentials.
PR Description Notes:
- The description sections ("Description Of Changes", "Code Changes", "Steps to Confirm") are marked as "TBD" and should be completed before merge
Important Files Changed
Files Changed
| Filename | Score | Overview |
|---|---|---|
CHANGELOG.md |
5/5 | Documents the security fix with CVE reference and cleans up duplicate section headers |
src/fides/api/oauth/utils.py |
4/5 | Core implementation of token invalidation logic with timestamp checking |
clients/admin-ui/src/features/user-management/NewPasswordModal.tsx |
4/5 | Adds logout flow when admin resets their own password |
src/fides/api/api/v1/endpoints/user_endpoints.py |
4/5 | Implements OAuth client deletion during password resets |
src/fides/api/models/client.py |
5/5 | Resolves circular import issue for user relationship access |
tests/ops/api/v1/endpoints/test_user_endpoints.py |
4/5 | Adds comprehensive test coverage for session invalidation |
clients/admin-ui/src/features/user-management/UpdatePasswordModal.tsx |
4/5 | Implements logout flow after successful password updates |
Confidence score: 4/5
- This PR implements critical security functionality with multiple layers of protection and comprehensive testing
- Score reflects well-structured implementation with proper error handling, but complexity across multiple components requires careful validation
- Pay close attention to the OAuth token validation logic in
utils.pyand ensure all password reset flows properly trigger session invalidation
Sequence Diagram
sequenceDiagram
participant User
participant AdminUI as Admin UI
participant API as Fides API
participant DB as Database
participant Auth as Auth System
Note over User, Auth: Password Reset Session Invalidation Flow
User->>AdminUI: Click "Reset password" button
AdminUI->>AdminUI: Open NewPasswordModal
User->>AdminUI: Enter new password details
AdminUI->>API: POST /api/v1/user/{id}/force-reset-password
API->>DB: Validate user exists
DB-->>API: User found
API->>API: Hash new password
API->>DB: Update user password
API->>DB: Set password_reset_at timestamp
Note over API, DB: Session Invalidation Logic
API->>DB: Find user's OAuth client
API->>DB: Delete OAuth client (invalidates all sessions)
DB-->>API: Password updated, client deleted
API-->>AdminUI: Success response
AdminUI->>AdminUI: Check if admin reset own password
alt Admin reset own password
AdminUI->>AdminUI: Clear localStorage
AdminUI->>Auth: Dispatch logout()
AdminUI->>AdminUI: Redirect to login page
else Admin reset other user's password
AdminUI->>AdminUI: Show success toast
end
Note over User, Auth: Token Validation on Subsequent Requests
User->>API: Make request with old token
API->>Auth: Extract token and load client
Auth->>DB: Load client by ID
DB-->>Auth: Client not found (deleted)
Auth-->>API: Authorization error
API-->>User: 403 Forbidden
alt User tries with valid new session
User->>AdminUI: Login with new password
AdminUI->>API: POST /api/v1/login
API->>DB: Create new OAuth client
API-->>AdminUI: New token
AdminUI->>API: Make request with new token
API->>Auth: Validate token
Auth-->>API: Valid
API-->>AdminUI: Success
end
Context used:
Rule - Review the entire PR not just the last commits (link)
7 files reviewed, 2 comments
| "Unable to delete user client during password reset for user {}: {}", | ||
| current_user.id, | ||
| exc, |
There was a problem hiding this comment.
style: Using bare Exception catch is overly broad. Consider catching more specific exceptions like database errors to avoid masking unexpected issues
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6526 +/- ##
==========================================
- Coverage 87.66% 87.64% -0.03%
==========================================
Files 483 483
Lines 30908 30931 +23
Branches 3482 3486 +4
==========================================
+ Hits 27096 27108 +12
- Misses 3067 3074 +7
- Partials 745 749 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This re implements commit 0ffb561 Thanks Eliana and Tom
thabofletcher
force-pushed
the
ENG-681-pw-reset-session-invalidation
branch
from
7316e8d to
421be70
Compare
thabofletcher
changed the title
thabofletcher
changed the title
There was a problem hiding this comment.
I'm approving, thanks for making the changes. I think we should ticket a follow-up to look at the timezone comparison thing a bit more in depth as it feels like it might be kind of fragile? though not as bad as the original security vuln.
Also let's make sure we get proper coverage :)
| from fides.config import CONFIG | ||
| from tests.conftest import _generate_auth_header |
There was a problem hiding this comment.
this is cursor being annoying with the local imports, can we move them top-level? I have an item in my to-do list to add a rule to fides so it doesn't do this
| assert resp_reset.status_code == HTTP_200_OK | ||
| # Exception is handled and request succeeds; log assertion not required for coverage |
There was a problem hiding this comment.
log assertion is not required but we could add it at very little cost right ? since it's the only "evidence" that we're hitting the exception code path, and codecov's reporting we aren't ... so it might be a good sanity check to leave in place , you can use the loguru_caplog fixture
fides
|
||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 00m 55s |
| Commit |
|
| Committer | Thabo Fletcher |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
5
|
| View all changes introduced in this branch ↗︎ | |
Closes ENG-681
Description Of Changes
This is to re-implement ENG-681, which was reverted due to its impact on FidesPlus/unit tests. This has been refactored to not require the changes the the OAuth client test fixture.
Code Changes
Invalidates exisitng sessions for a user when their password has been reset
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works