fix(auth): revoke OIDC sessions on user access changes - #555
Conversation
|
Warning Review limit reached
More reviews will be available in 36 minutes and 54 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds OIDC session revocation to ChangesOIDC Session Revocation on Token Regeneration and Admin Demotion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openrag/services/orchestrators/user_service.py (1)
185-193: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMake the access change and OIDC revocation atomic.
Line 190 and Lines 204-210 apply the user mutation before revoking browser sessions. If
revoke_oidc_sessions_by_user()throws afterregenerate_user_token()/update_user()has already committed, the API returns an error but the security-sensitive change is only half applied, leaving existing OIDC sessions alive. That reintroduces the exact gap this PR is meant to close for incident-response flows.Wrap the user-row update and
revoke_by_userin one transaction/unit-of-work, or add a compensating rollback before surfacing the error.Also applies to: 194-210
🤖 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 `@openrag/services/orchestrators/user_service.py` around lines 185 - 193, The token regeneration flow in user_service’s regenerate_token and the related update path are not atomic because the user mutation happens before revoke_oidc_sessions_by_user/revoke_by_user, so a failure leaves the access change committed while sessions remain active. Update the user-row change and OIDC/session revocation to run in one transaction or unit-of-work, or add a rollback/compensating undo if revocation fails, so the security-sensitive operations in UserService and _auth_service stay consistent.
🤖 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.
Outside diff comments:
In `@openrag/services/orchestrators/user_service.py`:
- Around line 185-193: The token regeneration flow in user_service’s
regenerate_token and the related update path are not atomic because the user
mutation happens before revoke_oidc_sessions_by_user/revoke_by_user, so a
failure leaves the access change committed while sessions remain active. Update
the user-row change and OIDC/session revocation to run in one transaction or
unit-of-work, or add a rollback/compensating undo if revocation fails, so the
security-sensitive operations in UserService and _auth_service stay consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f64ea8b-d140-48a6-a41c-5fc74a37d81a
📒 Files selected for processing (4)
openrag/services/orchestrators/auth_service.pyopenrag/services/orchestrators/user_service.pytests/unit/services/orchestrators/test_auth_service.pytests/unit/services/orchestrators/test_user_service.py
255a449 to
0af189a
Compare
|
Addressed the valid consistency issue. The revocation now happens before the access mutation, so a revocation failure no longer leaves a rotated token or admin demotion committed while old OIDC browser sessions remain active. This keeps the flow fail-closed without adding a larger cross-repository unit-of-work in this PR. I also added ordering coverage around token regeneration and admin demotion. |
0af189a to
49a0104
Compare
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
LGTM.
Revocation works.
I haven't tested it though with the new admin-ui to see the consequences on front
Why
#486 is an incident-response gap: changing a user's access should not leave their existing browser session alive until it naturally expires.
The refactor branch already had the persistence method to revoke OIDC sessions by user, but the user-management flow was not calling it.
What changed
This wires the revocation into the two access-change paths that matter here:
Regular profile edits keep the current behavior and do not revoke sessions.
Closes #486.
Summary by CodeRabbit
New Features
Bug Fixes
Tests