fix: add refresh token logic#1595
Conversation
WalkthroughAdds IBM JWT refresh capability triggered during request authentication. Defines platform refresh configuration (URL and period window), implements an async helper to POST expired tokens to the refresh endpoint with retry logic, and integrates it into the authentication dependency chain to refresh tokens and update session cookies before expiry. ChangesIBM JWT Refresh Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
github-actions
Bot
added
the
backend
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/dependencies.py (1)
411-418: 💤 Low valueMove
datetimeandPLATFORM_REFRESH_PERIODimports to module/function top.These imports sit on a hot path (every authenticated request whose JWT carries an
exp).importis cached after the first hit so the cost is small, but inlining them inside a nestedifmakes the control flow harder to read and breaks the pattern used elsewhere in this function (other settings are imported once near the top of_get_ibm_user). Suggest hoistingimport datetimeto the module imports and addingPLATFORM_REFRESH_PERIODto the existingfrom config.settings import (...)block at lines 361–366.🤖 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 `@src/dependencies.py` around lines 411 - 418, The in-function imports for datetime and PLATFORM_REFRESH_PERIOD inside the exp check should be hoisted: move "import datetime" to the module top-level imports and add PLATFORM_REFRESH_PERIOD to the existing "from config.settings import (...)" import group (used elsewhere in _get_ibm_user), then remove the nested imports and reference datetime and PLATFORM_REFRESH_PERIOD directly in the exp handling (the exp = claims.get("exp") / if exp - now < PLATFORM_REFRESH_PERIOD block).
🤖 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.
Inline comments:
In `@src/auth/ibm_auth.py`:
- Around line 109-119: The token-extraction can return the whole JSON body or
crash when data isn't a dict; update the logic in the function that refreshes
IBM tokens so you first attempt resp.json() and only treat it as a dict (use
data.get("token"/"access_token"/"refresh_token")) if isinstance(data, dict); if
json parsing fails or data is not a dict, treat resp.text as the fallback. After
extracting a candidate token (from dict keys or resp.text), validate it is a
string that looks like a JWT by requiring exactly two '.' characters (three
segments) before returning; tighten exception handling to catch
json.JSONDecodeError for json parsing and avoid broad excepts that swallow
AttributeError.
- Around line 105-125: The refresh_ibm_jwt retry loop is too long and can stall
request authentication; update refresh_ibm_jwt (called from _get_ibm_user) to
use a much smaller retry policy (e.g., 2–3 attempts) and a shorter per-attempt
timeout (e.g., 2–5s), only sleep/backoff between attempts and skip the
asyncio.sleep after the final attempt, and ensure immediate return None on final
failure; additionally add a short-lived in-memory de-dup / negative-result cache
keyed by the current token or user ID so concurrent calls to
refresh_ibm_jwt/PLATFORM_REFRESH_URL reuse a single in-flight attempt or the
recent negative result (TTL ~30–60s) to prevent fan-out.
In `@src/dependencies.py`:
- Around line 411-438: The code refreshes the IBM JWT via refresh_ibm_jwt and
reassigns ibm_token but continues to use user_id, email, and name that were
extracted earlier from the old claims; decode the newly returned token's claims
and re-derive user_id/email/name after a successful refresh (i.e., right after
setting the cookie/ibm_token) so downstream construction of the User object uses
the refreshed token's claims; reference refresh_ibm_jwt, ibm_token, claims,
user_id, email, name, response.set_cookie, IBM_SESSION_COOKIE_NAME and ensure
any logic that returns or raises (e.g., HTTPException) uses the up-to-date
derived values.
- Around line 423-429: The refreshed cookie set via response.set_cookie for
IBM_SESSION_COOKIE_NAME omits persistence and scoping attributes; update the
logic around response.set_cookie (where IBM_SESSION_COOKIE_NAME is written) to
(1) extract the JWT "exp" (if present) and compute max_age/expires and pass them
into response.set_cookie, (2) preserve or mirror the original cookie's
path/domain if available (e.g., from the incoming request cookies or headers) or
fall back to configurable defaults, and (3) make secure and samesite
configurable (or derive secure from the request scheme) instead of hardcoding
secure=True so local dev over HTTP works. Ensure these changes are applied where
the new_token is set so the rewritten cookie retains the same scope and lifetime
as the original.
---
Nitpick comments:
In `@src/dependencies.py`:
- Around line 411-418: The in-function imports for datetime and
PLATFORM_REFRESH_PERIOD inside the exp check should be hoisted: move "import
datetime" to the module top-level imports and add PLATFORM_REFRESH_PERIOD to the
existing "from config.settings import (...)" import group (used elsewhere in
_get_ibm_user), then remove the nested imports and reference datetime and
PLATFORM_REFRESH_PERIOD directly in the exp handling (the exp =
claims.get("exp") / if exp - now < PLATFORM_REFRESH_PERIOD block).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba9a66cc-7160-40ac-9bf5-891ffdb22253
📒 Files selected for processing (3)
src/auth/ibm_auth.pysrc/config/settings.pysrc/dependencies.py
| async with httpx.AsyncClient() as client: | ||
| for attempt in range(10): | ||
| try: | ||
| resp = await client.post(PLATFORM_REFRESH_URL, headers=headers, timeout=30.0) | ||
| if resp.status_code == 200: | ||
| data = resp.json() | ||
| new_token = ( | ||
| data.get("token") | ||
| or data.get("access_token") | ||
| or data.get("refresh_token") | ||
| or resp.text.strip().strip('"') | ||
| ) | ||
| if "." in new_token: | ||
| logger.info("Successfully refreshed IBM JWT token.") | ||
| return new_token | ||
| except Exception as e: | ||
| logger.warning(f"Failed to refresh IBM token (attempt {attempt + 1}): {e}") | ||
|
|
||
| await asyncio.sleep(10) | ||
|
|
||
| return None |
There was a problem hiding this comment.
Retry loop can stall request authentication for minutes.
refresh_ibm_jwt runs inside the request-authentication path (called from _get_ibm_user). With 10 attempts, a 30s per-attempt timeout, and a 10s asyncio.sleep between attempts, a misbehaving/unreachable PLATFORM_REFRESH_URL can keep a single request hanging for ~400s before the dependency gives up. Worse, every concurrent request whose token is in the refresh window will independently enter this loop — there is no in-flight de-duplication or short-circuit — so an outage of the refresh endpoint becomes an effective DoS on the API. Also, asyncio.sleep(10) runs even after the final failed attempt, wasting another 10s before returning None.
Consider: lower the retry count and per-attempt timeout dramatically (e.g., 2–3 attempts with a small backoff), skip the sleep after the last attempt, and add a short-lived in-memory de-dup / negative-result cache keyed by the token (or user) so one failing refresh doesn’t fan out to every concurrent request.
♻️ Sketch
- async with httpx.AsyncClient() as client:
- for attempt in range(10):
- try:
- resp = await client.post(PLATFORM_REFRESH_URL, headers=headers, timeout=30.0)
+ max_attempts = 3
+ async with httpx.AsyncClient(timeout=10.0) as client:
+ for attempt in range(max_attempts):
+ try:
+ resp = await client.post(PLATFORM_REFRESH_URL, headers=headers)
if resp.status_code == 200:
...
if "." in (new_token or ""):
logger.info("Successfully refreshed IBM JWT token.")
return new_token
+ else:
+ logger.warning(
+ "IBM JWT refresh returned non-200",
+ attempt=attempt + 1,
+ status_code=resp.status_code,
+ )
except Exception as e:
logger.warning(f"Failed to refresh IBM token (attempt {attempt + 1}): {e}")
-
- await asyncio.sleep(10)
+ if attempt < max_attempts - 1:
+ await asyncio.sleep(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async with httpx.AsyncClient() as client: | |
| for attempt in range(10): | |
| try: | |
| resp = await client.post(PLATFORM_REFRESH_URL, headers=headers, timeout=30.0) | |
| if resp.status_code == 200: | |
| data = resp.json() | |
| new_token = ( | |
| data.get("token") | |
| or data.get("access_token") | |
| or data.get("refresh_token") | |
| or resp.text.strip().strip('"') | |
| ) | |
| if "." in new_token: | |
| logger.info("Successfully refreshed IBM JWT token.") | |
| return new_token | |
| except Exception as e: | |
| logger.warning(f"Failed to refresh IBM token (attempt {attempt + 1}): {e}") | |
| await asyncio.sleep(10) | |
| return None | |
| max_attempts = 3 | |
| async with httpx.AsyncClient(timeout=10.0) as client: | |
| for attempt in range(max_attempts): | |
| try: | |
| resp = await client.post(PLATFORM_REFRESH_URL, headers=headers) | |
| if resp.status_code == 200: | |
| data = resp.json() | |
| new_token = ( | |
| data.get("token") | |
| or data.get("access_token") | |
| or data.get("refresh_token") | |
| or resp.text.strip().strip('"') | |
| ) | |
| if "." in (new_token or ""): | |
| logger.info("Successfully refreshed IBM JWT token.") | |
| return new_token | |
| else: | |
| logger.warning( | |
| "IBM JWT refresh returned non-200", | |
| attempt=attempt + 1, | |
| status_code=resp.status_code, | |
| ) | |
| except Exception as e: | |
| logger.warning(f"Failed to refresh IBM token (attempt {attempt + 1}): {e}") | |
| if attempt < max_attempts - 1: | |
| await asyncio.sleep(1) | |
| return None |
🤖 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 `@src/auth/ibm_auth.py` around lines 105 - 125, The refresh_ibm_jwt retry loop
is too long and can stall request authentication; update refresh_ibm_jwt (called
from _get_ibm_user) to use a much smaller retry policy (e.g., 2–3 attempts) and
a shorter per-attempt timeout (e.g., 2–5s), only sleep/backoff between attempts
and skip the asyncio.sleep after the final attempt, and ensure immediate return
None on final failure; additionally add a short-lived in-memory de-dup /
negative-result cache keyed by the current token or user ID so concurrent calls
to refresh_ibm_jwt/PLATFORM_REFRESH_URL reuse a single in-flight attempt or the
recent negative result (TTL ~30–60s) to prevent fan-out.
| if resp.status_code == 200: | ||
| data = resp.json() | ||
| new_token = ( | ||
| data.get("token") | ||
| or data.get("access_token") | ||
| or data.get("refresh_token") | ||
| or resp.text.strip().strip('"') | ||
| ) | ||
| if "." in new_token: | ||
| logger.info("Successfully refreshed IBM JWT token.") | ||
| return new_token |
There was a problem hiding this comment.
Token-extraction fallback can return the entire JSON body as the “token”.
When the server returns a JSON object that does not contain token / access_token / refresh_token, data.get(...) yields None, so the expression falls through to resp.text.strip().strip('"'). For a JSON response such as {"foo": "a.b.c"} the fallback returns the literal body string, which contains a . and therefore passes the "." in new_token shape check — the function then returns a JSON document as if it were a JWT. Additionally, if resp.json() returns a non-dict (e.g., the server returns a bare JSON string token), data.get raises AttributeError and the whole attempt is lost in the broad except Exception.
Suggest narrowing the response handling: use the text fallback only when the response is non-JSON / non-dict, and require the extracted value to be a string of the expected JWT shape (three dot-separated segments).
♻️ Suggested handling
- resp = await client.post(PLATFORM_REFRESH_URL, headers=headers, timeout=30.0)
- if resp.status_code == 200:
- data = resp.json()
- new_token = (
- data.get("token")
- or data.get("access_token")
- or data.get("refresh_token")
- or resp.text.strip().strip('"')
- )
- if "." in new_token:
- logger.info("Successfully refreshed IBM JWT token.")
- return new_token
+ resp = await client.post(PLATFORM_REFRESH_URL, headers=headers, timeout=30.0)
+ if resp.status_code == 200:
+ new_token: str | None = None
+ try:
+ data = resp.json()
+ except ValueError:
+ data = None
+ if isinstance(data, dict):
+ new_token = (
+ data.get("token")
+ or data.get("access_token")
+ or data.get("refresh_token")
+ )
+ elif isinstance(data, str):
+ new_token = data
+ else:
+ new_token = resp.text.strip().strip('"')
+ if isinstance(new_token, str) and new_token.count(".") == 2:
+ logger.info("Successfully refreshed IBM JWT token.")
+ return new_token🤖 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 `@src/auth/ibm_auth.py` around lines 109 - 119, The token-extraction can return
the whole JSON body or crash when data isn't a dict; update the logic in the
function that refreshes IBM tokens so you first attempt resp.json() and only
treat it as a dict (use data.get("token"/"access_token"/"refresh_token")) if
isinstance(data, dict); if json parsing fails or data is not a dict, treat
resp.text as the fallback. After extracting a candidate token (from dict keys or
resp.text), validate it is a string that looks like a JWT by requiring exactly
two '.' characters (three segments) before returning; tighten exception handling
to catch json.JSONDecodeError for json parsing and avoid broad excepts that
swallow AttributeError.
| exp = claims.get("exp") | ||
| if exp: | ||
| import datetime | ||
|
|
||
| from config.settings import PLATFORM_REFRESH_PERIOD | ||
|
|
||
| now = datetime.datetime.now(datetime.UTC).timestamp() | ||
| if exp - now < PLATFORM_REFRESH_PERIOD: | ||
| logger.info("IBM JWT token is expiring soon, attempting refresh...") | ||
| new_token = await ibm_auth.refresh_ibm_jwt(ibm_token) | ||
| if new_token: | ||
| ibm_token = new_token | ||
| response.set_cookie( | ||
| key=IBM_SESSION_COOKIE_NAME, | ||
| value=new_token, | ||
| httponly=True, | ||
| secure=True, | ||
| samesite="lax", | ||
| ) | ||
| logger.info("Successfully refreshed IBM JWT token and updated cookie.") | ||
| else: | ||
| if now >= exp: | ||
| if required: | ||
| raise HTTPException( | ||
| status_code=401, | ||
| detail="IBM session expired. Please log in again.", | ||
| ) | ||
| return None |
There was a problem hiding this comment.
Stale claims used after a successful refresh.
When refresh_ibm_jwt returns a new token, ibm_token is reassigned but user_id, email, and name were already computed from the old token's claims (lines 407–409). The downstream branches at lines 472 and 511 then build the User from those stale claims. In the normal case the refreshed token represents the same subject so this is benign, but if the refresh response ever returns a token bound to a different identity (e.g., impersonation, role-switch flows), the request will silently run as the old identity. Decoding the refreshed token and re-deriving user_id/email/name from its claims would close that gap.
🤖 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 `@src/dependencies.py` around lines 411 - 438, The code refreshes the IBM JWT
via refresh_ibm_jwt and reassigns ibm_token but continues to use user_id, email,
and name that were extracted earlier from the old claims; decode the newly
returned token's claims and re-derive user_id/email/name after a successful
refresh (i.e., right after setting the cookie/ibm_token) so downstream
construction of the User object uses the refreshed token's claims; reference
refresh_ibm_jwt, ibm_token, claims, user_id, email, name, response.set_cookie,
IBM_SESSION_COOKIE_NAME and ensure any logic that returns or raises (e.g.,
HTTPException) uses the up-to-date derived values.
| response.set_cookie( | ||
| key=IBM_SESSION_COOKIE_NAME, | ||
| value=new_token, | ||
| httponly=True, | ||
| secure=True, | ||
| samesite="lax", | ||
| ) |
There was a problem hiding this comment.
Refreshed cookie loses persistence/scoping attributes.
response.set_cookie here omits max_age/expires, so the rewritten IBM_SESSION_COOKIE_NAME cookie becomes a session-only cookie — when the browser is closed the user is logged out, even though the JWT itself may still be valid for hours. It also doesn’t mirror whatever path/domain Traefik used when originally setting this cookie, which can produce a sibling cookie that browsers will send for some routes but not others (and secure=True will be wrong for local dev over HTTP). If the new JWT carries an exp, set it as max_age / expires, and consider making cookie attributes configurable so they match Traefik’s issuance.
♻️ Sketch
- if new_token:
- ibm_token = new_token
- response.set_cookie(
- key=IBM_SESSION_COOKIE_NAME,
- value=new_token,
- httponly=True,
- secure=True,
- samesite="lax",
- )
+ if new_token:
+ ibm_token = new_token
+ new_claims = ibm_auth.decode_ibm_jwt(new_token) or {}
+ new_exp = new_claims.get("exp")
+ max_age = int(new_exp - now) if new_exp else None
+ response.set_cookie(
+ key=IBM_SESSION_COOKIE_NAME,
+ value=new_token,
+ httponly=True,
+ secure=True,
+ samesite="lax",
+ max_age=max_age,
+ path="/",
+ )🤖 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 `@src/dependencies.py` around lines 423 - 429, The refreshed cookie set via
response.set_cookie for IBM_SESSION_COOKIE_NAME omits persistence and scoping
attributes; update the logic around response.set_cookie (where
IBM_SESSION_COOKIE_NAME is written) to (1) extract the JWT "exp" (if present)
and compute max_age/expires and pass them into response.set_cookie, (2) preserve
or mirror the original cookie's path/domain if available (e.g., from the
incoming request cookies or headers) or fall back to configurable defaults, and
(3) make secure and samesite configurable (or derive secure from the request
scheme) instead of hardcoding secure=True so local dev over HTTP works. Ensure
these changes are applied where the new_token is set so the rewritten cookie
retains the same scope and lifetime as the original.
1 participant
This pull request introduces support for proactively refreshing IBM JWT tokens when they are about to expire, improving authentication robustness and user experience. It adds new environment variables for refresh configuration, implements an asynchronous token refresh function, and ensures that refreshed tokens are set in cookies. The changes also update authentication dependencies to accommodate the new logic.
IBM JWT Token Refresh Support:
refresh_ibm_jwtinsrc/auth/ibm_auth.pyto refresh IBM JWT tokens using thePLATFORM_REFRESH_URLenvironment variable, with retry logic and logging.src/dependencies.pyto check token expiry and, if the token is close to expiring, attempt to refresh it and update the session cookie if successful.Configuration Enhancements:
PLATFORM_REFRESH_URLandPLATFORM_REFRESH_PERIODinsrc/config/settings.pyto control the refresh endpoint and threshold for token refresh.Dependency and API Adjustments:
src/dependencies.py(_get_ibm_user,get_current_user, andget_optional_user) to accept and use theResponseobject, allowing them to set cookies after a token refresh. [1] [2] [3] [4] [5]Summary by CodeRabbit
Release Notes