fix: treat Keycloak refresh_expires_in=0 as never-expires sentinel#4060
fix: treat Keycloak refresh_expires_in=0 as never-expires sentinel#4060strawgate wants to merge 10 commits into
Conversation
The initial auth-code exchange path had a bug where val==0 kept refresh_expires_in as None, causing the fallback to be applied despite the Keycloak never-expires sentinel. Now uses the same keycloak_never_expires flag pattern as the other two code paths. Co-authored-by: Copilot <[email protected]>
51f073e to
07e3808
Compare
|
Edited to reflect the latest CI run (#25815958614). The previous TTL test failure is fixed; a new static-analysis failure has appeared. tl;dr: Root Cause: Both fixture definitions live inside Fix: Move the new transparent-refresh tests (and their Log excerptRelated files
|
…fault Co-authored-by: Copilot <[email protected]>
|
I'm a little worried about handling this 0 sentinel globally, will investigate |
… on subsequent refreshes When Keycloak returns refresh_expires_in=0 (offline token, never expires) on every token response, the previous code only guarded the initial exchange path. On subsequent exchange_refresh_token and transparent refresh cycles, the code would fall through to 'keep existing expiry' — inheriting the wall-clock timestamp set at initial exchange. After ~1 year that decayed to ~0 seconds, issuing FastMCP RTs with 1-second TTL and forcing re-auth even though the Keycloak offline token was still valid. Fix: add elif val == 0 to both refresh paths that clears refresh_token_expires_at to None. The fallback branch then issues a fresh full fallback-TTL FastMCP RT on every cycle, matching the 'always-valid' semantics of offline tokens. Also improves the debug log message to distinguish 'never expires' from 'expiry not provided' so operators can see exactly what Keycloak sent. Adds a regression test: test_refresh_expires_in_zero_subsequent_refresh_does_not_shrink Co-authored-by: Copilot <[email protected]>
Keycloak returns refresh_expires_in=0 for offline_access tokens to signal 'this refresh token never expires'. Base OAuthProxy has no way to know this is intentional rather than a malformed response, so it falls through to the standard 1-year wall-clock fallback — which causes the FastMCP refresh token TTL to shrink on every subsequent refresh cycle until it reaches ~0 after one year, forcing re-authentication even though the Keycloak offline token is still valid. This commit: - Adds KeycloakOAuthProxy(OAuthProxy) to providers/keycloak.py with a convenience __init__ that derives OIDC endpoints from realm_url - Adds _zero_refresh_expiry_means_never_expires: bool = False class attr on OAuthProxy; KeycloakOAuthProxy sets it to True - Adds refresh_token_never_expires: bool = False to UpstreamTokenSet so the intent is visible in stored state - When the flag is set and val==0: marks the token as never-expiring and clears refresh_token_expires_at so subsequent refresh cycles always get a fresh full fallback-TTL FastMCP RT instead of a decaying one - Base OAuthProxy is completely unchanged for val==0: falls through to the existing 1-year wall-clock fallback as before Tests: - test_refresh_expires_in_zero_issues_refresh_token: KeycloakOAuthProxy correctly issues a refresh token and marks upstream as never-expiring - test_refresh_expires_in_zero_subsequent_refresh_does_not_shrink: TTL stays at ~1 year after repeated refresh cycles - test_base_proxy_does_not_treat_zero_as_never_expires: confirms base OAuthProxy behaviour is unaffected Co-authored-by: Copilot <[email protected]>
…rride Replaces the _zero_refresh_expiry_means_never_expires class attribute with a proper override hook. The base class stays free of any provider-specific knowledge; KeycloakOAuthProxy overrides the method and returns True for val==0. Co-authored-by: Copilot <[email protected]>
…ess DCR 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.7 <[email protected]>
…xy docs 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.7 <[email protected]>
…backs 🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.7 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f19df2dce7
ℹ️ 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".
🤖 Generated with Claude Code Co-Authored-By: Claude Opus 4.7 <[email protected]>
Fixes handling of Keycloak's
refresh_expires_in=0for offline tokens.The bug: Keycloak returns
refresh_expires_in=0foroffline_accesstokens to mean "this refresh token never expires." The original code used Python's int truthiness (if int(val):), so0was treated as falsy and fell through to the 1-year wall-clock fallback. On subsequent refresh cycles the code inherited the decaying remaining time from that original timestamp — after ~1 year the FastMCP RT would be issued with a near-zero TTL, forcing re-authentication even though the Keycloak offline token was still valid.Why not handle this globally:
refresh_expires_inis not defined by RFC 6749 — it's a Keycloak extension. No spec-compliant provider would send0to mean "never expires." Treating0as a special sentinel in the baseOAuthProxywould change behavior for all providers in an undocumented way.The fix:
KeycloakOAuthProxy(OAuthProxy), a new subclass inproviders/keycloak.py, overrides_upstream_refresh_token_never_expires()to returnTrueforval == 0. This keeps all Keycloak-specific knowledge in the Keycloak provider and leavesOAuthProxyunchanged for every other provider. Arefresh_token_never_expires: boolfield onUpstreamTokenSetpersists the intent in stored state so that all three refresh paths (initial exchange,exchange_refresh_token, transparent access-token refresh) consistently issue a fresh full-TTL FastMCP RT on every cycle.KeycloakOAuthProxyalso provides a convenience constructor: passrealm_urland it derives the authorization, token, and revocation endpoints automatically.Refs #4054 — fixes Bug 3 (Keycloak
refresh_expires_in=0). Bugs 1, 2 and 4 are tracked separately on that issue. Note this also introducesKeycloakOAuthProxyas a first-class provider withrealm_url-based endpoint derivation.