-
Notifications
You must be signed in to change notification settings - Fork 173
feat: enforce IPSIE session_expiry ceiling in credentials managers#983
feat: enforce IPSIE session_expiry ceiling in credentials managers #983utkrishtsahu wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds IPSIE ChangesIPSIE session_expiry ceiling enforcement
Sequence Diagram(s)sequenceDiagram
participant App
participant CredentialsManager
participant BaseCredentialsManager
participant Storage
App->>CredentialsManager: saveCredentials(credentials)
CredentialsManager->>BaseCredentialsManager: validateSessionExpiryAtCreation(idToken)
alt session_expiry already breached
BaseCredentialsManager-->>App: throw SESSION_EXPIRED
else valid
CredentialsManager->>Storage: persist credentials
CredentialsManager->>BaseCredentialsManager: persistSessionExpiry(idToken)
BaseCredentialsManager->>Storage: store KEY_SESSION_EXPIRY
end
App->>CredentialsManager: getCredentials(...)
CredentialsManager->>BaseCredentialsManager: isSessionExpired(idToken)
BaseCredentialsManager->>Storage: read KEY_SESSION_EXPIRY
alt ceiling breached
CredentialsManager->>Storage: clearCredentials + remove KEY_SESSION_EXPIRY
CredentialsManager-->>App: SESSION_EXPIRED
else ceiling not reached
CredentialsManager->>BaseCredentialsManager: stampPinnedSessionExpiry(credentials)
CredentialsManager-->>App: Credentials with sessionExpiresAt
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.kt (1)
277-305:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftClose the secure SSO/API refresh bypasses.
The secure
getCredentials()path now enforcessession_expiry, but SSO and API credential flows can still serve cached API credentials or use the refresh token after the upstream ceiling. Also re-check afterrequest.execute()before saving renewed credentials so a renewal that crosses the ceiling in-flight cannot succeed.Example guard placement for renewed credentials
val fresh = request.execute() + if (isSessionExpired(fresh.idToken)) { + clearCredentials() + callback.onFailure(CredentialsManagerException.SESSION_EXPIRED) + return@execute + } val expiresAt = fresh.expiresAt.timeFor API credentials, add a stored-ceiling guard before prompting/serving cached API credentials, and a decrypted/live-token guard before any refresh-token exchange.
Also applies to: 897-954, 984-1019, 1043-1079
🤖 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 `@auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.kt` around lines 277 - 305, After the request.execute() call in the SSO exchange flow within the serialExecutor block, add a session_expiry validation check before the saveSsoCredentials(sessionCredentials) call to ensure the renewed credentials respect the upstream ceiling limit, similar to the guard checks performed in the secure getCredentials() path. Apply this same validation pattern to the other credential renewal flows mentioned in related methods around lines 897-954, 984-1019, and 1043-1079 to ensure all SSO and API refresh token exchanges enforce the session_expiry ceiling both before serving cached credentials and after obtaining renewed credentials.auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.kt (1)
135-155:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftApply the session ceiling to every refresh-token path.
getSsoCredentials()andgetApiCredentials()still use the refresh token without checkingsession_expiry, andgetCredentials()does not re-check afterrequest.execute(). This allows SSO/API exchanges—or a slow credential renewal that crosses the ceiling after the pre-check—to return tokens past the hard session ceiling. AddisSessionExpired(...)guards before SSO/API cached returns and refreshes, and again immediately after each refresh response before saving or invoking success.Example guard placement for the credential refresh path
val fresh = request.execute() + if (isSessionExpired(fresh.idToken)) { + clearCredentials() + callback.onFailure(CredentialsManagerException.SESSION_EXPIRED) + return@execute + } val expiresAt = fresh.expiresAt.timeApply the same pattern before
ssoExchange(...), before returning cachedAPICredentials, and before saving renewed API credentials.Also applies to: 511-541, 596-662
🤖 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 `@auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.kt` around lines 135 - 155, Add session expiry ceiling checks in all refresh-token paths within CredentialsManager to prevent returning tokens past the hard session ceiling. Specifically, add isSessionExpired(...) guard checks in the getSsoCredentials() and getApiCredentials() methods before returning cached credentials, before invoking ssoExchange() and similar refresh requests, and immediately after request.execute() completes but before invoking saveSsoCredentials() or the success callback. This ensures the session_expiry validation is consistently applied across all SSO, API, and standard credential renewal flows rather than only at entry points.
🤖 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
`@auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt`:
- Around line 338-360: The persistSessionExpiry() method needs to distinguish
between initial credential saves and refresh token saves to maintain proper
session ceiling semantics. Add a parameter to the persistSessionExpiry() method
(such as preserveExistingWhenMissing with a default value) that controls whether
to preserve the old ceiling when the ID token omits the session_expiry claim.
Update the method implementation so it only preserves the existing value when
the parameter is true and the claim is missing, otherwise it replaces the value.
Then locate all callers of persistSessionExpiry() and determine if they
represent fresh login saves (pass false to clear stale ceilings) or refresh
saves (pass true to preserve the earliest ceiling).
In
`@auth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.kt`:
- Line 3582: The verifyNoMoreInteractions(client) assertions are currently
positioned before the method call under test (manager.getCredentials(callback)),
which prevents them from validating the client usage triggered by that call.
Move the verifyNoMoreInteractions(client) assertions to execute after the
manager.getCredentials(callback) call in each affected test method so that the
mock verification can properly capture any unexpected client interactions. This
ordering issue exists in multiple test methods as indicated by the line
references, so apply the same reordering change to all affected test cases to
ensure consistent test validation.
---
Outside diff comments:
In
`@auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.kt`:
- Around line 135-155: Add session expiry ceiling checks in all refresh-token
paths within CredentialsManager to prevent returning tokens past the hard
session ceiling. Specifically, add isSessionExpired(...) guard checks in the
getSsoCredentials() and getApiCredentials() methods before returning cached
credentials, before invoking ssoExchange() and similar refresh requests, and
immediately after request.execute() completes but before invoking
saveSsoCredentials() or the success callback. This ensures the session_expiry
validation is consistently applied across all SSO, API, and standard credential
renewal flows rather than only at entry points.
In
`@auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.kt`:
- Around line 277-305: After the request.execute() call in the SSO exchange flow
within the serialExecutor block, add a session_expiry validation check before
the saveSsoCredentials(sessionCredentials) call to ensure the renewed
credentials respect the upstream ceiling limit, similar to the guard checks
performed in the secure getCredentials() path. Apply this same validation
pattern to the other credential renewal flows mentioned in related methods
around lines 897-954, 984-1019, and 1043-1079 to ensure all SSO and API refresh
token exchanges enforce the session_expiry ceiling both before serving cached
credentials and after obtaining renewed credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
i️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba3f07e7-b7b9-4e4d-b060-ca7213b5671f
📒 Files selected for processing (11)
EXAMPLES.mdauth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.ktauth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.ktauth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManagerException.ktauth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.ktauth0/src/main/java/com/auth0/android/request/internal/Jwt.ktauth0/src/main/java/com/auth0/android/result/Credentials.ktauth0/src/test/java/com/auth0/android/authentication/storage/CredentialsManagerTest.ktauth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.ktauth0/src/test/java/com/auth0/android/request/internal/JwtTest.ktauth0/src/test/java/com/auth0/android/result/CredentialsTest.kt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is 30s is the agreed leeway across ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes — 30s aligns implementation ,the ~30s negative leeway. The leeway is intentionally negative so we treat the session as expired slightly before the wall-clock ceiling, never after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says non-positive but the check is against 0 also. Shouldn't this be sessionExpiry < 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intentional — "non-positive" means <= 0. We exclude 0 (epoch 1970) too, because it's not a valid live ceiling and storage.retrieveLong(KEY_SESSION_EXPIRY) returns 0 for the unset/migration case. Treating <= 0 as "no ceiling" mirrors the existing guard in willExpire, so we don't log out users who never had a ceiling persisted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validateSessionExpiryAtCreation now throws an exception . Update the comment and signature of saveCredentials for the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Added @throws(CredentialsManagerException::class) and documented that saveCredentials can now throw SESSION_EXPIRED (when the ID token is already past its session_expiry ceiling at creation) and INVALID_CREDENTIALS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. saveCredentials here already had @throws; updated the KDoc to document the SESSION_EXPIRED case as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the intent here ? isSessionExpired(null) will always return false . So this check is completely redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't always false. With a null idToken, isSessionExpired falls through to storage.retrieveLong(KEY_SESSION_EXPIRY) — the ceiling persisted at login. We pass null deliberately because the credentials blob is encrypted and can't be decoded in hasValidCredentials, so the persisted value is the only ceiling available here. Covered by the tests shouldFailWithSessionExpiredWithoutBiometricsOrRenewWhenStoredCeilingReached and ...WhenStoredCeilingFallsWithinLeeway, which assert this returns SESSION_EXPIRED from a stored ceiling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the spec, the expiry time we get during login is the final value and subsequent token refreshes shouldn't overwrite this initial value. This check , currently considers the id token which might contain a new value than the initially stored one thus giving a higher ceiling. So shouldn't the check be something like
val sessionExpiry = storage.retrieveLong(KEY_SESSION_EXPIRY)
?: sessionExpiryFromIdToken(idToken)
?: return false
@kishore7snehil could you confirm this behaviour / condition ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, went with your suggestion. We now read the pinned value first and only fall back to the ID token claim, so a refresh can't bump the ceiling up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creation-time check uses a bare sessionExpiry <= issuedAtSeconds, but the read-time check at line 348 applies the 30s leeway. So a ceiling 1-30s past iat saves here, then gets rejected on the next read. Should we apply the same leeway in both places so they agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. Both checks now use the same 30s leeway so they agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one gap is an implausibly large (milliseconds) value - worth a test pinning that it's treated as no ceiling, since that's the silent fail-open mode for this control.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test for this — pins that a milliseconds value parses as null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 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
`@auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.kt`:
- Around line 340-343: In the isSessionExpired function, the issue is that
non-positive values retrieved from storage (such as 0 or negative sentinel
values) are treated as valid session expiry times instead of being treated as
"not set" and falling back to sessionExpiryFromIdToken(idToken). Modify the
storage retrieval logic to check if the retrieved sessionExpiry value is
non-positive (less than or equal to 0), and if so, treat it as null so the elvis
operator will proceed to evaluate sessionExpiryFromIdToken(idToken) as the
fallback. This same pattern should be applied to all other occurrences of this
storage retrieval pattern in the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
i️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b0324fe-2a9c-4804-94d4-3c41674004f6
📒 Files selected for processing (9)
auth0/src/main/java/com/auth0/android/authentication/storage/BaseCredentialsManager.ktauth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.ktauth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.ktauth0/src/main/java/com/auth0/android/request/internal/Jwt.ktauth0/src/main/java/com/auth0/android/result/Credentials.ktauth0/src/test/java/com/auth0/android/authentication/storage/CredentialsManagerTest.ktauth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.ktauth0/src/test/java/com/auth0/android/request/internal/JwtTest.ktauth0/src/test/java/com/auth0/android/result/CredentialsTest.kt
🚧 Files skipped from review as they are similar to previous changes (5)
- auth0/src/test/java/com/auth0/android/request/internal/JwtTest.kt
- auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.kt
- auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.kt
- auth0/src/test/java/com/auth0/android/authentication/storage/SecureCredentialsManagerTest.kt
- auth0/src/test/java/com/auth0/android/authentication/storage/CredentialsManagerTest.kt
@kishore7snehil
kishore7snehil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Closes the gap between the Swift session_expiry implementation and the Android reference (auth0/Auth0.Android#983): - Persist the session_expiry ceiling to the Keychain at store() time, pinned to the initial-login value and never raised by a refresh that re-emits a later claim. - Resolve the ceiling pinned-value-first with the live ID token as a fallback, so a refreshed token that omits the claim still enforces it. - Enforce on credentials(), ssoCredentials(), apiCredentials(), and hasValid(); clear the stored credentials when the ceiling is reached. - clear() removes the persisted ceiling key. - Parse the claim via NSNumber so a fractional value is truncated rather than dropped; reject implausible (>= 1e10) millisecond values. - Add public Credentials.sessionExpiresAt (Unix seconds). - Extend tests and docs (EXAMPLES.md, README.md, CHANGELOG.md). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
Changes
This change adds support for the IPSIE SL1 session_expiry claim in the credentials managers. When an upstream identity provider (e.g. an enterprise/OIDC connection) asserts a session_expiry claim on the ID token, the SDK now treats it as a hard session ceiling: an absolute lifetime, expressed in Unix seconds, beyond which the local session is no longer valid and cannot be extended by a refresh-token renewal. This is important for downstream/federated scenarios where the upstream IdP dictates a maximum session lifetime that Auth0-issued tokens must respect, independently of the access-token expiry.
Behavior
Public API added
Internal/affected classes (no breaking signature changes)
Behavioral / upgrade note: For a user whose connection asserts session_expiry, a getCredentials call that previously succeeded can now fail with SESSION_EXPIRED once the ceiling is reached. Callers should treat SESSION_EXPIRED as a prompt to re-authenticate. Documented in EXAMPLES.md (new "Upstream session expiry" section).
References
SDK-9590
Testing
Reviewers can run the existing unit suite — ./gradlew :auth0:testDebugUnitTest — which now includes coverage for the new behavior:
End-to-end, the feature was manually verified on a real device (Pixel 7a, Android 16) against a live tenant. Because the platform does not yet emit session_expiry natively, the claim was injected via a Post-Login Action (api.idToken.setCustomClaim('session_expiry', Math.floor(Date.now()/1000) + N)); all behaviors above were confirmed on both CredentialsManager and SecureCredentialsManager.
This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors
Summary by CodeRabbit
session_expiryceiling enforcement across credential, SSO, and API-credential flows, including persisted/pinned ceiling and clock-skew leeway.Credentials.sessionExpiresAtand addedCredentialsManagerException.SESSION_EXPIRED.hasValidCredentialsnow returns false after the ceiling is breached.session_expiryceiling behavior and leeway.