Repository navigation
Fix openid scope loss on token refresh - #52622
Conversation
Closes keycloak#52574 Signed-off-by: Himanshu Mishra <mishrahimanshu0076@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
A critical scope-handling race remains, and the regression test lacks the initial refresh-token scope assertion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.java:212
- The same shared client-session note is used in the non-parameterized path, so concurrent/sequential authorization-code requests can likewise make a token's OIDC decision depend on whichever request updated the note last instead of the code's saved scope. Preserve
requestedScopeStringfor code/token requests and special-case refresh handling rather than switching this branch to the mutable note.
String scopeSent = clientSession.getNote(OAuth2Constants.SCOPE);
if (TokenUtil.isOIDCRequest(scopeSent)) {
scopeParam = TokenUtil.attachOIDCScope(scopeParam);
tests/base/src/test/java/org/keycloak/tests/oauth/RefreshTokenTest.java:784
- The refreshed refresh token is also only checked for non-null, so the test does not verify that
openidremains in its scope as claimed. Add a scope assertion on this parsed token.
RefreshToken refreshedRefreshToken = oauth.parseRefreshToken(refreshResponse.getRefreshToken());
assertNotNull(refreshedRefreshToken);
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
… DefaultClientSessionContext and verify with test assertions Signed-off-by: Himanshu Mishra <mishrahimanshu0076@gmail.com>
a90bacf to
3ba30a8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
OIDC detection still relies on a mutable client-session note and must use refresh-context state to avoid concurrent authorization races.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Use the original refresh-token scope instead of the mutable client-session note for the scope decision.
Review details
Suppressed comments (1)
services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.java:221
- This fallback reintroduces the shared client-session note as the source of a per-token scope decision.
TokenManager.attachAuthenticationSession()overwrites that note when another authorization request uses the same client session, so an older refresh token that was issued withoutopenidcan gain it after a later OIDC request—or lose it after a later non-OIDC request—despiteoldRefreshToken.getScope()being the authoritative grant. Carry the original refresh-token scope into the context (before narrowing) and use that for this check instead of reading the mutable note.
if (OAuth2Constants.REFRESH_TOKEN.equals(getAttribute(Constants.GRANT_TYPE, String.class))) {
return TokenUtil.isOIDCRequest(clientSession.getNote(OAuth2Constants.SCOPE));
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ding test case Signed-off-by: Himanshu Mishra <mishrahimanshu0076@gmail.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Extend the regression test to assert the refreshed ID token and its access-token hash.
Review details
Suppressed comments (1)
tests/base/src/test/java/org/keycloak/tests/oauth/RefreshTokenTest.java:823
- This regression test exercises the new
oldRefreshToken-based OIDC decision, but it only checks the access-token scope and UserInfo. If the change atAbstractRefreshTokenProvider.java:178failed to generate the ID token (or its access-token hash) after the client-session scope note was overwritten, this test would still pass; assert the refreshed ID token and its access-token hash here as well.
assertScopes("openid profile", refreshResponse.getScope());
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
…refresh Signed-off-by: Himanshu Mishra <mishrahimanshu0076@gmail.com>
Signed-off-by: Himanshu Mishra <mishrahimanshu0076@gmail.com>
There was a problem hiding this comment.
🟢 Approval recommended
The implementation and regression coverage address the scope-loss regression; the remaining documentation nit is non-blocking.
Review details
Suppressed comments (1)
services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.java:222
- The PR description says this fallback uses
clientSession.getNote(OAuth2Constants.SCOPE), but the implementation readsORIGINAL_REFRESH_TOKEN_SCOPE, populated fromoldRefreshToken.getScope(). That distinction is material—the latter is what lets the second regression test work after a later authorization overwrites the mutable client-session note—so please update the description to match the implementation (or change the implementation if note-based behavior is intended).
return TokenUtil.isOIDCRequest(getAttribute(ORIGINAL_REFRESH_TOKEN_SCOPE, String.class));
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
🟢 Approval recommended
The fix and regression coverage address the reported scope-loss issue without unresolved blocking concerns.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
What does this PR do?
Fixes #52574, where the
openidscope is lost from a refreshed access token when the refresh request specifies a narrower scope withoutopenid.For example:
The refreshed access token should retain the
openidscope.Why is this needed?
The regression was introduced by
b49a51bfd3, which introducedrequestedScopeStringto prevent scope values from being mixed between concurrent token requests.During token refresh,
requestedScopeStringis populated from the narrowedoldTokenScope. When the refresh request contains onlyscope=profile,openidis removed fromoldTokenScope.DefaultClientSessionContext.getScopeString()then usesrequestedScopeStringto determine whether the request is an OIDC request. Sinceopenidis no longer present, it is not added to the refreshed access token scope.The original authorization scope is still available from
clientSession.getNote(OAuth2Constants.SCOPE).What changed?
Use
clientSession.getNote(OAuth2Constants.SCOPE)for the OIDC scope check inDefaultClientSessionContext.getScopeString().The existing
requestedScopeStringbehavior is preserved for per-request scope processing and parameterized scope isolation.A regression test was added to verify that
openidis retained when refreshing a token withscope=profile.Testing
The regression test covers:
scope=openid profileopenidpresent in the initial access tokenopenidpresent in the initial refresh tokenscope=profileopenidretained in the refreshed access tokenopenidretained in the refreshed refresh tokenThe existing parameterized scope isolation behavior is also verified to ensure the fix does not regress the changes introduced by #48955.