Fix OAuth token expiry and implement refresh token rotation#3102
Fix OAuth token expiry and implement refresh token rotation#3102HydroSulphide wants to merge 2 commits intorommapp:masterfrom
Conversation
… token doesn't lead to internal server error
Greptile SummaryThis PR fixes two bugs in the OAuth implementation — missing expiry enforcement on access tokens and an internal server error when expired tokens were used via the bearer auth path — and adds proper refresh token rotation so that each refresh consumes the old token and issues a new pair. Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant /token Endpoint
participant OAuthHandler
participant Redis
participant DB
Note over Client, DB: Initial login (password grant)
Client->>+/token Endpoint: POST /token (grant_type=password)
/token Endpoint->>OAuthHandler: create_access_token(...)
OAuthHandler-->>/ token Endpoint: access_token (30 min)
/token Endpoint->>OAuthHandler: create_refresh_token(...)
OAuthHandler->>Redis: SETEX refresh-jti:{jti} 7d "valid"
OAuthHandler-->>/ token Endpoint: refresh_token (7 days)
/token Endpoint-->>-Client: {access_token, refresh_token, expires_in, refresh_expires_in}
Note over Client, DB: Token refresh (rotation)
Client->>+/token Endpoint: POST /token (grant_type=refresh_token)
/token Endpoint->>OAuthHandler: consume_refresh_token(token)
OAuthHandler->>OAuthHandler: decode JWT + verify exp/iss/type
OAuthHandler->>Redis: GET refresh-jti:{jti}
Redis-->>OAuthHandler: "valid"
OAuthHandler->>Redis: DEL refresh-jti:{jti}
OAuthHandler->>DB: get_user_by_username(sub)
DB-->>OAuthHandler: user
OAuthHandler-->>/ token Endpoint: (user, claims)
/token Endpoint->>OAuthHandler: create_access_token(...)
OAuthHandler-->>/ token Endpoint: new access_token
/token Endpoint->>OAuthHandler: create_refresh_token(...)
OAuthHandler->>Redis: SETEX refresh-jti:{new_jti} 7d "valid"
OAuthHandler-->>/ token Endpoint: new refresh_token
/token Endpoint-->>-Client: {new access_token, new refresh_token, ...}
Note over Client, DB: Bearer token used in API request
Client->>+HybridAuthBackend: GET /api/... Authorization: Bearer {access_token}
HybridAuthBackend->>OAuthHandler: get_current_active_user_from_bearer_token(token)
OAuthHandler->>OAuthHandler: decode + verify exp/iss
OAuthHandler->>DB: get_user_by_username(sub)
DB-->>OAuthHandler: user
OAuthHandler-->>HybridAuthBackend: (user, claims)
HybridAuthBackend-->>-Client: 200 OK (authenticated)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| redis_client.delete(f"refresh-jti:{jti}") | ||
|
|
||
| username = payload.claims.get("sub") | ||
| if not username: | ||
| raise OAuthCredentialsException | ||
|
|
||
| user = db_user_handler.get_user_by_username(username) | ||
| if user is None: | ||
| raise OAuthCredentialsException | ||
|
|
There was a problem hiding this comment.
Refresh token consumed before user validation completes
The JTI is deleted from Redis (line 318) before the username and user database lookups (lines 320–326). If the database lookup raises an unexpected exception (e.g., a transient DB error), the refresh token is permanently consumed but no new token pair is ever issued. The client would be left with no valid refresh token and no way to recover short of re-authenticating from scratch.
The delete should be deferred until all validations have passed:
| redis_client.delete(f"refresh-jti:{jti}") | |
| username = payload.claims.get("sub") | |
| if not username: | |
| raise OAuthCredentialsException | |
| user = db_user_handler.get_user_by_username(username) | |
| if user is None: | |
| raise OAuthCredentialsException | |
| username = payload.claims.get("sub") | |
| if not username: | |
| raise OAuthCredentialsException | |
| user = db_user_handler.get_user_by_username(username) | |
| if user is None: | |
| raise OAuthCredentialsException | |
| if not user.enabled: | |
| raise UserDisabledException | |
| redis_client.delete(f"refresh-jti:{jti}") | |
| return user, payload.claims |
| try: | ||
| user, claims = await oauth_handler.get_current_active_user_from_bearer_token(token) | ||
| except HTTPException as exc: | ||
| if exc.status_code == 401: | ||
| return None | ||
|
|
||
| if user is None or claims is None: | ||
| return None |
There was a problem hiding this comment.
Non-401 HTTPException causes NameError on fallthrough
The except block only explicitly returns when exc.status_code == 401. If a different status code is raised (e.g., a future change adds a 403-level exception), the except block silently finishes without returning, and execution falls through to if user is None or claims is None — but at that point user and claims are unbound, causing a NameError.
While all current exceptions from get_current_active_user_from_bearer_token happen to be 401 (both OAuthCredentialsException and UserDisabledException are 401), the guard should be made defensive so this cannot silently break with future changes:
| try: | |
| user, claims = await oauth_handler.get_current_active_user_from_bearer_token(token) | |
| except HTTPException as exc: | |
| if exc.status_code == 401: | |
| return None | |
| if user is None or claims is None: | |
| return None | |
| try: | |
| user, claims = await oauth_handler.get_current_active_user_from_bearer_token(token) | |
| except HTTPException as exc: | |
| if exc.status_code == 401: | |
| return None | |
| raise |
gantoine
added
the
auth
|
@HydroSulphide i'll have a look once the comments and failing tests are fixed! |
2 participants
Description
The current state of OAuth token implementation is that they are always valid, because of a missing check against their expiration date. I implemented this check in
get_current_active_user_from_bearer_token(). I also implemented refresh token rotation.This PR changes the OAuth token flow so expired access tokens are rejected correctly, refresh tokens are consumed and rotated on refresh, and old refresh tokens can no longer be reused. The
/tokenendpoint now usesconsume_refresh_token()for the refresh flow, creates a new refresh token on every successful refresh, and returnsrefresh_token,expires_in, andrefresh_expires_inin the response. I also updated theTokenResponsetype accordingly.Additionally, I added a follow-up fix for bearer authentication in
hybrid_auth.py: ifget_current_active_user_from_bearer_token()raises anHTTPExceptionwith status code401, it is now handled gracefully instead of causing an internal server error during API calls with expired access tokens.I tested the changes locally with a token rotation test flow that verifies valid access tokens work before expiry, expired access tokens are rejected by the server, refresh tokens produce a new token pair, old refresh tokens fail after rotation, and expired refresh tokens are rejected.
Most importantly: I fixed a typo in a comment.
Checklist