Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Token-type handling still loses explicit values in supported flows, can retain stale session values, and rejects valid capitalization in a test.
4 open findings
What changed in this PR
Addresses #53954 by adding a default Bearer token type to broker responses while preserving explicit identity-provider token types.
Changes:
- Adds token-type propagation through login, storage, sessions, and refresh.
- Adds unit and integration coverage for fallback and preservation behavior.
- Extends legacy token-exchange assertions.
| File | Description |
|---|---|
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/SocialLoginTest.java | Adds social token-type assertions. |
| testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerTokenExchangeTest.java | Adds legacy exchange assertions. |
| tests/base/src/test/java/org/keycloak/tests/broker/OAuth2IdentityProviderStoreTokenV2Test.java | Tests fallback and custom-type preservation. |
| tests/base/src/test/java/org/keycloak/tests/broker/IdentityProviderStoreTokenV2Test.java | Tests OIDC session-note propagation. |
| services/src/test/java/org/keycloak/test/broker/oidc/AbstractOAuth2IdentityProviderTest.java | Tests serialization and response defaults. |
| services/src/main/java/org/keycloak/social/gitlab/GitLabIdentityProvider.java | Propagates GitLab token types. |
| services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java | Propagates OIDC token types. |
| services/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java | Carries token types through storage, sessions, and refresh. |
| server-spi-private/src/main/java/org/keycloak/broker/provider/AbstractIdentityProvider.java | Defaults missing token types to Bearer. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
e7f0a3a to
9c3c986
Compare
9c3c986 to
8d3dbb9
Compare
8d3dbb9 to
c1d54d4
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The focused changes preserve explicit token types and provide the required fallback, with no blocking issues identified; automated tests were not executed.
0 open findings
1 resolved since last review
🧠 Review effort: Balanced
There was a problem hiding this comment.
Thanks, @atiqur-rahman-pro.
In addition to the inline comments: why do we need to consider DPoP (or any non-Bearer token type) in brokering at all? The broker never sends a DPoP proof to the upstream IdP, so a compliant IdP will always issue Bearer, and Keycloak would have no key to use a DPoP-bound token anyway.
Closes keycloak#53954 Signed-off-by: Atiqur Rahman <rahman.atiqur.pro@gmail.com>
c1d54d4 to
57cc591
Compare
|
Hi @pedroigor, Thanks for the clarification. I’ve simplified the PR accordingly. It now only defaults a missing token_type to Bearer in buildTokenResponse(), with focused regression coverage for the V1 and V2 flows. I removed the token-type session state, custom token propagation, and related parsing logic. Could you please take another look? |
|
Thanks, @atiqur-rahman-pro. |
Closes #53954
Summary of Changes
Bearertoken_type fallback:AbstractIdentityProvider.buildTokenResponse(...), ensuretokenResponse.getTokenType()falls back toTokenUtil.TOKEN_TYPE_BEARER("Bearer") if missing, conforming to RFC 6749 §5.1 and RFC 8693 §2.2.1.AbstractOAuth2IdentityProvider, preserve external IdP custom token types (e.g.DPoP) across stored token exchanges (exchangeStoredToken), session-backed token exchanges (exchangeSessionToken), and refresh token requests (refreshToken).FEDERATED_TOKEN_TYPEin user session notes to maintain token type consistency across session lifetimes.OIDCIdentityProvider.extractIdentityFromProfile(...)andGitLabIdentityProvider.extractIdentityFromProfile(...), populateFEDERATED_TOKEN_TYPEinto identity context data during standard login so that broker V2 endpoints preserve the IdP's token type.AbstractOAuth2IdentityProviderTestcovering token type serialization, fallback from dynamic claims, and defaultBearerhandling.OAuth2IdentityProviderStoreTokenV2TestandIdentityProviderStoreTokenV2Testverifying standard login note propagation and custom token type preservation.KcOidcBrokerTokenExchangeTest,SocialLoginTest) for legacy token exchange assertions.