Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds per-session JWT revocation and a cache-backed refresh grace/deduplication system: session-scoped access/refresh tokens, per-token whitelisting, enforce_at session management with immediate revoke, refresh/logout API endpoints, frontend logout call, tests, and SIMPLE_JWT config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Backend as Backend (token_refresh_api_view)
participant TokenLogic as Token System (jwt_session)
participant Cache as Cache/Redis
participant DB as Database
Client->>Backend: POST /auth/refresh { refresh_token }
Backend->>TokenLogic: refresh_tokens_with_grace_period(token_str)
TokenLogic->>TokenLogic: Parse & validate SessionRefreshToken
TokenLogic->>TokenLogic: Check session enforce_at / revocation
TokenLogic->>Cache: Check grace cache for cached response
alt cached response exists
Cache-->>TokenLogic: Cached tokens
TokenLogic-->>Backend: Return cached tokens
else no cached response
TokenLogic->>TokenLogic: Acquire short lock (dedupe)
TokenLogic->>TokenLogic: Re-check revocation & cache
TokenLogic->>DB: Verify user is active
TokenLogic->>TokenLogic: Generate new access (+ optional refresh)
TokenLogic->>Cache: Whitelist old iat & store new tokens in grace cache
TokenLogic->>DB: set_session_enforce_at(session_id, now)
TokenLogic-->>Backend: Return new tokens
end
Backend-->>Client: 200 { access, refresh }
sequenceDiagram
participant Client
participant Backend as Backend (logout_api_view)
participant JWTAuth as JWT Authentication
participant TokenLogic as Token System (jwt_session)
participant DB as Database
Client->>Backend: POST /auth/logout Authorization: Bearer {access_token}
Backend->>JWTAuth: Authenticate / extract access token
JWTAuth->>TokenLogic: Decode SessionAccessToken => session_id
Backend->>TokenLogic: revoke_session(session_id)
TokenLogic->>DB: set_session_enforce_at(session_id, 0)
TokenLogic-->>Backend: Revoked
Backend-->>Client: 204 No Content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✏️ Tip: You can disable this entire section by setting Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…rough an exception Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…at/auth-revoke-tokens
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@authentication/jwt_session.py`:
- Around line 146-171: The cached grace-return is happening before
signature/expiry validation; change the flow in the validate-refresh logic:
after constructing SessionRefreshToken(refresh_token_str, verify=False) and
extracting session_id and computing grace_key, keep the
is_token_revoked(refresh) check, then call refresh.verify() (wrap TokenError ->
InvalidToken as currently done) before reading cache.get(grace_key); only after
successful verify return the cached JSON. Update the block around
SessionRefreshToken, is_token_revoked, refresh.verify, grace_key and cache.get
to reflect this ordering.
- Around line 186-193: Wrap the User.objects.get(pk=user_id) call in a
try/except that catches User.DoesNotExist and converts it to an
AuthenticationFailed exception (e.g., same message "No active account found for
the given token.") so missing users during refresh return 401 instead of causing
a 500; keep the existing check of api_settings.USER_AUTHENTICATION_RULE(user)
unchanged and only run it when the user lookup succeeds in the jwt refresh
handling code in authentication/jwt_session.py.
In `@authentication/views/common.py`:
- Around line 354-364: The code can raise TokenError when constructing
SessionAccessToken(raw_token) and currently bubbles up; wrap the
SessionAccessToken(raw_token) creation and subsequent token.get("session_id")
usage in a try/except that catches the TokenError from SimpleJWT (import
TokenError) and handle it idempotently (e.g., swallow the error and do not call
revoke_session, or return a controlled response such as HTTP 204/401 depending
on view flow). Update the block around
JWTAuthentication.get_header/get_raw_token and SessionAccessToken to catch
TokenError and ensure revoke_session is only called for valid tokens.
🧹 Nitpick comments (2)
front_end/src/app/(main)/accounts/actions.ts (1)
167-175: Consider logging server-side logout failures.
Silent catch hides revocation issues; a low-noise log keeps UX intact while preserving observability.🔧 Suggested tweak
- try { - await ServerAuthApi.logout(); - } catch {} + try { + await ServerAuthApi.logout(); + } catch (err) { + console.warn("Server logout failed", err); + }tests/unit/test_auth/test_views.py (1)
152-155: Add explicit assertion for session_id to make failures clearer.This keeps the test intent explicit if the token structure changes.
💡 Proposed tweak
# Extract session_id from access token token = SessionAccessToken(access_token, verify=False) session_id = token.get("session_id") + assert session_id, "Expected session_id in access token"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
authentication/jwt_session.pyauthentication/services.pyauthentication/urls.pyauthentication/views/common.pyfront_end/src/app/(main)/accounts/actions.tsfront_end/src/services/api/auth/auth.server.tsmetaculus_web/settings.pytests/unit/test_auth/test_views.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-15T19:29:58.940Z
Learnt from: hlbmtc
Repo: Metaculus/metaculus PR: 4075
File: authentication/urls.py:24-26
Timestamp: 2026-01-15T19:29:58.940Z
Learning: In this codebase, DRF is configured to use IsAuthenticated as the default in REST_FRAMEWORK['DEFAULT_PERMISSION_CLASSES'] within metaculus_web/settings.py. Therefore, explicit permission_classes([IsAuthenticated]) decorators are unnecessary on DRF views unless a view needs to override the default. When reviewing Python files, verify that views relying on the default are not redundantly decorated, and flag cases where permissions are being over-specified or when a non-default permission is explicitly required.
Applied to files:
tests/unit/test_auth/test_views.pyauthentication/views/common.pyauthentication/services.pymetaculus_web/settings.pyauthentication/urls.pyauthentication/jwt_session.py
📚 Learning: 2026-01-19T16:13:59.519Z
Learnt from: hlbmtc
Repo: Metaculus/metaculus PR: 4087
File: metaculus_web/settings.py:150-155
Timestamp: 2026-01-19T16:13:59.519Z
Learning: In the Metaculus codebase `metaculus_web/settings.py`, `SessionAuthentication` is intentionally listed before `JWTAuthentication` in `REST_FRAMEWORK["DEFAULT_AUTHENTICATION_CLASSES"]` because session auth is expected to be used specifically for the Django admin dashboard, while JWT handles the primary web user authentication.
Applied to files:
authentication/views/common.pyauthentication/services.pymetaculus_web/settings.pyauthentication/jwt_session.py
📚 Learning: 2026-01-16T20:30:29.385Z
Learnt from: hlbmtc
Repo: Metaculus/metaculus PR: 4087
File: authentication/views/social.py:47-55
Timestamp: 2026-01-16T20:30:29.385Z
Learning: In the Metaculus codebase, `SocialTokenOnlyAuthView` from the `rest_social_auth` library uses `AllowAny` permission by default, so views inheriting from it (like `SocialCodeAuth` in `authentication/views/social.py`) do not need to explicitly set `permission_classes = (AllowAny,)` for OAuth code exchange to work with unauthenticated requests.
Applied to files:
authentication/views/common.pymetaculus_web/settings.pyauthentication/urls.py
🧬 Code graph analysis (5)
tests/unit/test_auth/test_views.py (3)
tests/unit/conftest.py (2)
anon_client(81-84)user1(46-48)authentication/services.py (1)
get_tokens_for_user(130-139)authentication/jwt_session.py (4)
SessionAccessToken(100-109)get_session_enforce_at(49-53)verify(106-109)verify(129-132)
authentication/views/common.py (1)
authentication/jwt_session.py (3)
refresh_tokens_with_grace_period(135-212)revoke_session(215-220)SessionAccessToken(100-109)
front_end/src/app/(main)/accounts/actions.ts (1)
front_end/src/services/auth_tokens.ts (1)
getAuthCookieManager(162-165)
authentication/services.py (1)
authentication/jwt_session.py (2)
SessionRefreshToken(112-132)for_user(124-127)
authentication/urls.py (1)
authentication/views/common.py (2)
logout_api_view(348-366)token_refresh_api_view(328-344)
🪛 GitHub Check: CodeQL
authentication/jwt_session.py
[warning] 149-149: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 171-171: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.14.13)
authentication/jwt_session.py
109-109: Avoid specifying long messages outside the exception class
(TRY003)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Do not catch blind exception: Exception
(BLE001)
149-149: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
160-160: Avoid specifying long messages outside the exception class
(TRY003)
171-171: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
190-192: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Docker Image
- GitHub Check: Backend Checks
- GitHub Check: Frontend Checks
- GitHub Check: integration-tests
🔇 Additional comments (13)
front_end/src/services/api/auth/auth.server.ts (1)
6-6: LGTM — logout hook is cleanly wired.Also applies to: 165-167
authentication/jwt_session.py (7)
14-35: Clear constants and key helpers.
37-47: Whitelist helpers look good.
49-63: Revocation timestamp caching is straightforward.
66-97: Revocation decision flow is easy to follow.
100-133: Token subclasses integrate revocation checks cleanly.
215-220: Logout revocation hook is straightforward.
194-210: Prevent newly issued tokens from being revoked by enforce_at drift.
enforce_atis set after minting tokens; if the clock ticks to the next second, new tokens can end up withiat < enforce_atand be immediately revoked.authentication/services.py (1)
134-138: LGTM — session-aware refresh token creation.metaculus_web/settings.py (1)
194-201: AUTH_TOKEN_CLASSES wiring looks good.authentication/views/common.py (3)
14-22: No issues found in these imports.
326-345: token_refresh_api_view looks solid.Clear validation and consistent error translation.
347-352: [Your rewritten review comment text here]
[Exactly ONE classification tag]
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
🧹 Preview Environment Cleaned UpThe preview environment for this PR has been destroyed.
Cleanup triggered by PR close at 2026-01-27T18:43:08Z |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@authentication/jwt_session.py`:
- Around line 181-195: Capture a single timestamp up front and use it for both
creating the new token iat and for setting the session enforce_at to avoid the
race between refresh.set_iat() and set_session_enforce_at(); specifically,
compute one UTC timestamp (e.g., aware_utcnow() or
datetime.now(timezone.utc).timestamp()) then apply it to the refresh token
(instead of calling refresh.set_iat() with no args, set the iat claim explicitly
on the refresh object or call set_iat(timestamp) if supported) and pass the same
integer timestamp into set_session_enforce_at(session_id, int(timestamp));
ensure any JSON/data writes and whitelist_token(session_id, old_token_iat)
remain unchanged.
♻️ Duplicate comments (1)
authentication/jwt_session.py (1)
172-177: HandleUser.DoesNotExistduring refresh.
User.objects.get(pk=user_id)raisesUser.DoesNotExistif the user was deleted, resulting in a 500 error. Wrap in try-except to return a proper 401.🔧 Suggested fix
# Check user is still active user_id = refresh.get(api_settings.USER_ID_CLAIM) if user_id: - user = User.objects.get(pk=user_id) + try: + user = User.objects.get(pk=user_id) + except User.DoesNotExist: + raise AuthenticationFailed( + "No active account found for the given token." + ) from None if not api_settings.USER_AUTHENTICATION_RULE(user): raise AuthenticationFailed( "No active account found for the given token." )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
authentication/jwt_session.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-19T16:13:59.519Z
Learnt from: hlbmtc
Repo: Metaculus/metaculus PR: 4087
File: metaculus_web/settings.py:150-155
Timestamp: 2026-01-19T16:13:59.519Z
Learning: In the Metaculus codebase `metaculus_web/settings.py`, `SessionAuthentication` is intentionally listed before `JWTAuthentication` in `REST_FRAMEWORK["DEFAULT_AUTHENTICATION_CLASSES"]` because session auth is expected to be used specifically for the Django admin dashboard, while JWT handles the primary web user authentication.
Applied to files:
authentication/jwt_session.py
📚 Learning: 2026-01-15T19:29:58.940Z
Learnt from: hlbmtc
Repo: Metaculus/metaculus PR: 4075
File: authentication/urls.py:24-26
Timestamp: 2026-01-15T19:29:58.940Z
Learning: In this codebase, DRF is configured to use IsAuthenticated as the default in REST_FRAMEWORK['DEFAULT_PERMISSION_CLASSES'] within metaculus_web/settings.py. Therefore, explicit permission_classes([IsAuthenticated]) decorators are unnecessary on DRF views unless a view needs to override the default. When reviewing Python files, verify that views relying on the default are not redundantly decorated, and flag cases where permissions are being over-specified or when a non-default permission is explicitly required.
Applied to files:
authentication/jwt_session.py
🪛 Ruff (0.14.13)
authentication/jwt_session.py
77-77: Avoid specifying long messages outside the exception class
(TRY003)
109-109: Avoid specifying long messages outside the exception class
(TRY003)
132-132: Avoid specifying long messages outside the exception class
(TRY003)
164-164: Avoid specifying long messages outside the exception class
(TRY003)
175-177: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Docker Image / Build Docker Image
- GitHub Check: integration-tests
- GitHub Check: Frontend Checks
- GitHub Check: Backend Checks
🔇 Additional comments (9)
authentication/jwt_session.py (9)
1-21: LGTM!Imports are well-organized and constants are clearly named with appropriate documentation.
23-35: LGTM!Key generation helpers are well-encapsulated and follow consistent naming conventions.
37-46: LGTM!Whitelist functions correctly manage the cache with appropriate timeouts matching the grace period.
49-63: LGTM!Session enforce_at management is correctly implemented with appropriate TTL based on token lifetime.
100-110: LGTM!
SessionAccessTokencorrectly extends verification to include revocation check after standard JWT validation.
112-133: LGTM!
SessionRefreshTokencorrectly implements session-based token creation and revocation verification.
200-205: LGTM!
revoke_sessioncorrectly implements immediate revocation by settingenforce_atto 0.
161-161:cache.lock()requires a Redis-backed cache.The
cache.lock()method is not part of Django's standard cache API—it's provided bydjango-redis. If the cache backend changes away from Redis, this will raiseAttributeError. The project currently usesdjango-redisas configured in settings, but this is a hard dependency for this code path.
75-78: Remove concern about token migration strategy.The
session_idrequirement is not a breaking change to existing tokens. All token creation in the codebase goes throughget_tokens_for_user()which usesSessionRefreshToken.for_user(), which always assigns asession_id(line 126). The check at lines 75–77 is a defensive validation to prevent misconfiguration (e.g., accidental use of the parentRefreshTokenclass directly), as shown in the test at lines 217–224. No pre-existing tokens withoutsession_idexist in this system.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/unit/test_auth/test_views.py`:
- Around line 149-153: The test currently reads the session_id from
SessionAccessToken via token.get("session_id") and then queries
get_session_enforce_at(session_id) which can produce a false-positive if
session_id is missing; add an explicit assertion that session_id is present
(e.g., assert session_id is not None or truthy) immediately after obtaining it
to ensure the claim exists before calling get_session_enforce_at.
🧹 Nitpick comments (1)
tests/unit/test_auth/test_views.py (1)
135-136: Preferreverse(...)over a hard-coded logout URL.Using a named route keeps tests stable if the URL prefix or path changes.
♻️ Suggested change
class TestLogout: - url = "/api/auth/logout/" + url = reverse("auth-logout")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/test_auth/test_views.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-15T19:29:58.940Z
Learnt from: hlbmtc
Repo: Metaculus/metaculus PR: 4075
File: authentication/urls.py:24-26
Timestamp: 2026-01-15T19:29:58.940Z
Learning: In this codebase, DRF is configured to use IsAuthenticated as the default in REST_FRAMEWORK['DEFAULT_PERMISSION_CLASSES'] within metaculus_web/settings.py. Therefore, explicit permission_classes([IsAuthenticated]) decorators are unnecessary on DRF views unless a view needs to override the default. When reviewing Python files, verify that views relying on the default are not redundantly decorated, and flag cases where permissions are being over-specified or when a non-default permission is explicitly required.
Applied to files:
tests/unit/test_auth/test_views.py
🧬 Code graph analysis (1)
tests/unit/test_auth/test_views.py (3)
tests/unit/conftest.py (1)
anon_client(81-84)authentication/services.py (1)
get_tokens_for_user(130-139)authentication/jwt_session.py (4)
SessionAccessToken(100-109)get_session_enforce_at(49-53)verify(106-109)verify(129-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Docker Image / Build Docker Image
- GitHub Check: integration-tests
- GitHub Check: Backend Checks
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
3 participants
This PR implements a session-based JWT token management system with server-side revocation. The main goal:
More details -- https://www.notion.so/metaculus/Auth-migration-roadmap-2e96aaf4f10180728140ddbdb51f5045?source=copy_link#2f06aaf4f10180bfb380df24e5662caf
part of #4008
Summary by CodeRabbit
New Features
Client
Tests
✏️ Tip: You can customize this high-level summary in your review settings.