Repository navigation
feat(auth): P4-T2 Admin Role Versioning - #236
Conversation
Adds adminRoleVersion field to users schema to detect role changes and prevent timing attacks during admin role modifications. Changes: - Add adminRoleVersion field to users table (default: 0) - Update SessionClaims to include adminRoleVersion - Create updateUserRole function that bumps version on role changes - Create validateAdminAccess function for DB-level validation - Enhance verifyAdminAuth to validate adminRoleVersion at request time - Update BaseAuthDetails and VerifiedUser interfaces - Add comprehensive integration and unit tests - Generate database migration (0044) Security: This prevents race conditions where a user's admin status changes between token issuance and request validation. Addresses vulnerability #12 from security hardening plan.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds admin role versioning: a non-null integer Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthService as Auth Service\n(verifyAdminAuth)
participant DB as Database
participant AdminRole as Admin Role\n(validateAdminAccess)
Client->>AuthService: Request with token (includes claimed adminRoleVersion)
AuthService->>DB: verifyAuth / fetch session + user including adminRoleVersion
DB-->>AuthService: session/user + claimedAdminRoleVersion
AuthService->>AdminRole: validateAdminAccess(userId, claimedAdminVersion)
AdminRole->>DB: Query user role & adminRoleVersion
DB-->>AdminRole: role, adminRoleVersion
AdminRole->>AdminRole: compare role == 'admin' && versions match
AdminRole-->>AuthService: true / false
alt Validation passes
AuthService-->>Client: VerifiedUser (includes adminRoleVersion)
else Validation fails
AuthService-->>Client: null (unauthorized)
end
sequenceDiagram
participant Admin as Admin User
participant AdminAPI as Admin API\n(updateUserRole)
participant DB as Database
participant Validator as validateAdminAccess
Admin->>AdminAPI: Request role change for user
AdminAPI->>DB: UPDATE users SET role = newRole, adminRoleVersion = adminRoleVersion + 1 RETURNING ...
DB-->>AdminAPI: Updated user { id, role, adminRoleVersion }
AdminAPI-->>Admin: Response with updated user + adminRoleVersion
Note over Admin,Validator: Subsequent auth with old claimed version
Admin->>Validator: authenticate with old claimedVersion
Validator->>DB: SELECT role, adminRoleVersion WHERE id = ...
DB-->>Validator: role, currentAdminRoleVersion
Validator->>Validator: detect mismatch -> deny
Validator-->>Admin: Access denied
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dc479602a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Validate adminRoleVersion against the database to ensure | ||
| // the role hasn't changed since the token was issued | ||
| const isValidAdmin = await validateAdminAccess(user.id, user.adminRoleVersion); |
There was a problem hiding this comment.
Bind adminRoleVersion to token issuance
The new admin-role check doesn’t actually validate a token’s issuance version because verifyAdminAuth passes user.adminRoleVersion that was just read from the database via authenticateSessionRequest/sessionService.validateSession (which returns session.user.adminRoleVersion). That means validateAdminAccess compares the current DB value to itself, so sessions issued before a role change will still be accepted after promotion (or any change) as soon as the DB is updated, which defeats the intended “invalidate old admin tokens” behavior. If you want role changes to invalidate existing sessions, store the adminRoleVersion in the session/token at creation time and compare it to the current DB value (or bump/revoke sessions directly) rather than re-reading it on each request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified - this is correct. The current implementation fetches adminRoleVersion fresh from the database during session validation (session-service.ts:91,114) and then compares it to the same fresh DB value in validateAdminAccess. This means role changes take effect immediately but don't invalidate existing sessions.
Fix requires adding adminRoleVersion to the sessions table and storing it at session creation time (like tokenVersion). This is a schema migration + code change. Creating a follow-up task.
- Add adminRoleVersion: 0 to mock user objects in user-validator.test.ts - Rename isNewUser to _isNewUser in desktop auth exchange (unused param) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Desktop tsconfig has noUnusedLocals: true which doesn't allow underscore-prefixed unused variables. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The isNewUser param is sent from web OAuth callback to indicate new signups vs returning users. Pass it through to the dashboard URL so the web app can show appropriate welcome flow. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add adminRoleVersion: 0 to SessionAuthResult, MCPAuthResult, SessionClaims, and User mock objects across 60 test files to match updated schema. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Adds adminRoleVersion field to users schema to detect role changes
and prevent timing attacks during admin role modifications.
Changes:
Security: This prevents race conditions where a user's admin status
changes between token issuance and request validation.
Addresses vulnerability #12 from security hardening plan.
Summary by CodeRabbit
New Features
Data
Tests
✏️ Tip: You can customize this high-level summary in your review settings.