fix: handle refresh responses without id_token#307
Open
balazsotakomaiya wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds handling for refresh responses that omit Changes
Sequence Diagram(s)sequenceDiagram
participant Manager
participant Network as TokenEndpoint
participant Store
participant Validator
Manager->>Store: load cached user/token
Store-->>Manager: cached token (+ old id_token)
Manager->>Network: POST refresh (refresh_token)
Network-->>Manager: success response (no id_token, new access_token)
Manager->>Manager: set allowExpiredIdToken=true on merged token
Manager->>Store: persist merged token (includes allowExpiredIdToken)
Manager->>Validator: validate token (uses allowExpiredIdToken)
Validator-->>Manager: suppress JWT-expired error when allowExpiredIdToken=true
Manager-->>Manager: keep session active (no expired event)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
packages/oidc_core/lib/src/managers/user_manager_base.dart (1)
1292-1301:⚠️ Potential issue | 🟡 MinorDefer offline-mode recovery until the refreshed user is accepted.
recordSuccessfulServerContact()runs beforecreateUserFromToken(). If claim validation or userinfo validation later returnsnull/throws, the manager still clears offline state and resets failure counters even though no usable session was produced.handleTokenExpiring()already records success after validation; this path should do the same.💡 Suggested fix
- // Successful refresh - update last server contact and exit offline mode - recordSuccessfulServerContact(newToken: token); - return await createUserFromToken( + final refreshedUser = await createUserFromToken( token: token, nonce: null, userInfo: null, attributes: null, metadata: discoveryDocument, currentUserOverride: existingUser, ); + if (refreshedUser != null) { + recordSuccessfulServerContact(newToken: refreshedUser.token); + } + return refreshedUser;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/oidc_core/lib/src/managers/user_manager_base.dart` around lines 1292 - 1301, The code currently calls recordSuccessfulServerContact() before attempting to validate/build the refreshed session via createUserFromToken(), which can clear offline state even if createUserFromToken() fails or returns null; move the call to recordSuccessfulServerContact() so it only runs after createUserFromToken() has successfully returned a non-null user (i.e., call recordSuccessfulServerContact(newToken: token) immediately before returning the created user and do not call it if createUserFromToken() throws or returns null), mirroring the behavior in handleTokenExpiring() and ensuring failure counters/offline mode are only reset on an actually accepted refreshed user.packages/oidc_core/lib/src/models/user.dart (1)
161-190:⚠️ Potential issue | 🟠 MajorPersist
idTokenOverridein the merged token.
replaceToken()usesidTokenOverrideforOidcUser.idToken/parsedIdToken, butsaveUser()later persistsuser.token.toJson(). Right now the merged JSON is built only from the old/new token payloads, so asettings.getIdTokenoverride is lost on disk and a reload falls back to the staleid_token.💡 Suggested fix
final mergedTokenJson = { // keep old data and override the new data. ...token.toJson(), ...newToken.toJson(), + OidcConstants_AuthParameters.idToken: idToken, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/oidc_core/lib/src/models/user.dart` around lines 161 - 190, The merged token JSON currently only combines token.toJson() and newToken.toJson(), so an idTokenOverride passed into replaceToken() (the idToken variable used to set OidcUser.idToken and parsedIdToken) is not persisted; update the mergedTokenJson before creating OidcToken.fromJson(...) to include the idTokenOverride value (set the appropriate id_token key in mergedTokenJson when idTokenOverride != null) so the persisted token reflects the override; reference mergedTokenJson, idToken (or idTokenOverride), OidcUser._ and OidcToken.fromJson to locate where to add this assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/oidc_core/lib/src/managers/user_manager_base.dart`:
- Around line 1043-1050: createUserFromToken currently derives
allowExpiredIdToken inside the method and can wrongly enable it for non-refresh
flows; change createUserFromToken to accept an explicit bool parameter
allowExpiredIdToken (default false) and pass that to currentUser.replaceToken
instead of computing reusesExistingIdToken locally. Update the refresh code
path(s) that call createUserFromToken to pass allowExpiredIdToken: true, and
leave loginPassword and loginDeviceCodeFlow calls unchanged (or explicitly pass
false) so only refresh callers enable expired-id-token acceptance; adjust
callers referencing createUserFromToken accordingly.
- Around line 1560-1561: The JWT expiry detection is inconsistent:
_isJwtExpiredError(Exception error) currently checks
error.message.startsWith('JWT expired.') (with a period) while another check
uses startsWith('JWT expired') (no period); update _isJwtExpiredError to use a
broader prefix (e.g., startsWith('JWT expired')) or normalize the message check
(trim trailing punctuation) and then replace the ad-hoc check with a call to
_isJwtExpiredError so both validation paths (the helper and the
offline/reused-id_token path) use the same logic (reference _isJwtExpiredError
and the other JWT expiry check in the offline/reused-id_token validation).
---
Outside diff comments:
In `@packages/oidc_core/lib/src/managers/user_manager_base.dart`:
- Around line 1292-1301: The code currently calls
recordSuccessfulServerContact() before attempting to validate/build the
refreshed session via createUserFromToken(), which can clear offline state even
if createUserFromToken() fails or returns null; move the call to
recordSuccessfulServerContact() so it only runs after createUserFromToken() has
successfully returned a non-null user (i.e., call
recordSuccessfulServerContact(newToken: token) immediately before returning the
created user and do not call it if createUserFromToken() throws or returns
null), mirroring the behavior in handleTokenExpiring() and ensuring failure
counters/offline mode are only reset on an actually accepted refreshed user.
In `@packages/oidc_core/lib/src/models/user.dart`:
- Around line 161-190: The merged token JSON currently only combines
token.toJson() and newToken.toJson(), so an idTokenOverride passed into
replaceToken() (the idToken variable used to set OidcUser.idToken and
parsedIdToken) is not persisted; update the mergedTokenJson before creating
OidcToken.fromJson(...) to include the idTokenOverride value (set the
appropriate id_token key in mergedTokenJson when idTokenOverride != null) so the
persisted token reflects the override; reference mergedTokenJson, idToken (or
idTokenOverride), OidcUser._ and OidcToken.fromJson to locate where to add this
assignment.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 242c64ad-7088-4b9d-866d-b861ec3090c6
📒 Files selected for processing (6)
packages/oidc/test/oidc_test.dartpackages/oidc_core/lib/src/constants.dartpackages/oidc_core/lib/src/endpoints/token/token.dartpackages/oidc_core/lib/src/managers/user_manager_base.dartpackages/oidc_core/lib/src/models/user.dartpackages/oidc_core/test/model_test.dart
Comment on lines
+1043
to
+1050
| final reusesExistingIdToken = | ||
| idTokenOverride == null && token.idToken == null; | ||
| newUser = await currentUser.replaceToken( | ||
| token, | ||
| idTokenOverride: idTokenOverride, | ||
| strictVerification: settings.strictJwtVerification, | ||
| cacheStore: store, | ||
| allowExpiredIdToken: reusesExistingIdToken, |
There was a problem hiding this comment.
Only enable allowExpiredIdToken on refresh paths.
createUserFromToken() is also used by loginPassword() (Lines 301-311) and loginDeviceCodeFlow() (Lines 411-417), both with nonce: null. If one of those flows runs while a user is already loaded and the response omits id_token, this path now persists the previous session’s JWT with allowExpiredIdToken: true, so a later reload can keep validating stale claims against the new token set. Please thread this flag explicitly from refresh callers instead of inferring it here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/oidc_core/lib/src/managers/user_manager_base.dart` around lines 1043
- 1050, createUserFromToken currently derives allowExpiredIdToken inside the
method and can wrongly enable it for non-refresh flows; change
createUserFromToken to accept an explicit bool parameter allowExpiredIdToken
(default false) and pass that to currentUser.replaceToken instead of computing
reusesExistingIdToken locally. Update the refresh code path(s) that call
createUserFromToken to pass allowExpiredIdToken: true, and leave loginPassword
and loginDeviceCodeFlow calls unchanged (or explicitly pass false) so only
refresh callers enable expired-id-token acceptance; adjust callers referencing
createUserFromToken accordingly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/oidc_core/lib/src/managers/user_manager_base.dart (1)
1043-1050:⚠️ Potential issue | 🟠 MajorKeep
allowExpiredIdTokenrefresh-only.
createUserFromToken()still infers this from “missingid_token+ existing user”, but this helper is also used byloginPassword()(Lines 301-311) andloginDeviceCodeFlow()(Lines 411-417). If either flow runs while a user is already loaded and the response omitsid_token, this will persist the previous session JWT withallowExpiredIdToken: true, so later reloads can keep validating stale claims against the new token set. Please make this an explicit opt-in from the refresh callers instead of deriving it here.Suggested direction
Future<OidcUser?> createUserFromToken({ required OidcToken token, required String? nonce, required Map<String, dynamic>? attributes, required Map<String, dynamic>? userInfo, required OidcProviderMetadata metadata, OidcUser? currentUserOverride, + bool allowExpiredIdToken = false, bool validateAndSave = true, }) async { final currentUser = currentUserOverride ?? this.currentUser; OidcUser? newUser; final idTokenOverride = await settings.getIdToken?.call(token); if (currentUser == null) { ... } else { - final reusesExistingIdToken = - idTokenOverride == null && token.idToken == null; newUser = await currentUser.replaceToken( token, idTokenOverride: idTokenOverride, strictVerification: settings.strictJwtVerification, cacheStore: store, - allowExpiredIdToken: reusesExistingIdToken, + allowExpiredIdToken: allowExpiredIdToken, ); }Then have only the refresh paths pass
allowExpiredIdToken: token.idToken == null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/oidc_core/lib/src/managers/user_manager_base.dart` around lines 1043 - 1050, createUserFromToken currently infers allowExpiredIdToken from a missing id_token + existing user, which lets non-refresh callers (loginPassword, loginDeviceCodeFlow) accidentally reuse expired id tokens; change createUserFromToken to not derive this value internally and require callers to pass allowExpiredIdToken explicitly, then update loginPassword and loginDeviceCodeFlow to pass false and update the refresh paths to pass (token.idToken == null) so only refresh flows opt into allowExpiredIdToken; reference createUserFromToken, loginPassword, loginDeviceCodeFlow and the refresh-call sites when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/oidc_core/lib/src/managers/user_manager_base.dart`:
- Around line 1043-1050: createUserFromToken currently infers
allowExpiredIdToken from a missing id_token + existing user, which lets
non-refresh callers (loginPassword, loginDeviceCodeFlow) accidentally reuse
expired id tokens; change createUserFromToken to not derive this value
internally and require callers to pass allowExpiredIdToken explicitly, then
update loginPassword and loginDeviceCodeFlow to pass false and update the
refresh paths to pass (token.idToken == null) so only refresh flows opt into
allowExpiredIdToken; reference createUserFromToken, loginPassword,
loginDeviceCodeFlow and the refresh-call sites when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca8c45f4-efcc-455a-b1a6-cbc7d968a0b3
📒 Files selected for processing (1)
packages/oidc_core/lib/src/managers/user_manager_base.dart
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #306
Description
This change fixes the case where a successful refresh token response does not include id_token.
Previously, the library could still reject the session later if it had to keep using the previous id_token and that token was already expired. There was also no regression coverage around token event behavior for this refresh path.
What changed
Why
Some providers do not return id_token on refresh. In that case, a successful refresh should not cause the session to be rejected solely because the manager had to keep using the prior id_token.
Type of Change
Summary by CodeRabbit
New Features
Tests