Repository navigation
Add idp alias to store federated tokens in user session notes - #50592
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect upstream IdP token retrieval in multi-IdP/account-linking scenarios by namespacing federated token session notes with the IdP alias (e.g., FEDERATED_ACCESS_TOKEN:{alias}), preventing overwrites across linked providers.
Changes:
- Introduces alias-namespaced session-note keys for federated tokens and adds helper getters/setters to read/write them with backward-compatible fallback.
- Updates OAuth2/OIDC/SAML/Twitter broker providers and token-exchange flow to store/read tokens via the namespaced keys.
- Updates the V2 broker token storage test to prefer the namespaced note and fall back to the legacy un-namespaced note.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/base/src/test/java/org/keycloak/tests/broker/InterfaceIdentityProviderStoreTokenV2Test.java | Reads token from {key}:{alias} first, then legacy key for compatibility. |
| services/src/main/java/org/keycloak/social/twitter/TwitterIdentityProvider.java | Switches to helper-based access-token retrieval and namespaced storage. |
| services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/AbstractTokenExchangeProvider.java | Stores external-exchange access token under alias-namespaced session note. |
| services/src/main/java/org/keycloak/broker/saml/SAMLIdentityProvider.java | Stores federated access token using namespaced helper. |
| services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java | Uses namespaced helpers for refresh/id/access/expiration notes. |
| services/src/main/java/org/keycloak/broker/oidc/AbstractOAuth2IdentityProvider.java | Adds namespaced note key helpers for refresh/id/expiration and uses them in flows. |
| server-spi-private/src/main/java/org/keycloak/broker/provider/AbstractIdentityProvider.java | Adds namespaced access-token key + helper read/write with legacy fallback guarded by provider alias. |
graziang
marked this pull request as draft
July 6, 2026 08:04
graziang
marked this pull request as ready for review
July 6, 2026 11:12
Comment on lines
+141
to
+147
| protected String getFederatedRefreshToken(UserSessionModel userSession) { | ||
| String token = userSession.getNote(getFederatedRefreshTokenKey()); | ||
| if (token != null) { | ||
| return token; | ||
| } | ||
| return userSession.getNote(FEDERATED_REFRESH_TOKEN); | ||
| } |
Comment on lines
+149
to
+155
| protected String getFederatedTokenExpiration(UserSessionModel userSession) { | ||
| String value = userSession.getNote(getFederatedTokenExpirationKey()); | ||
| if (value != null) { | ||
| return value; | ||
| } | ||
| return userSession.getNote(FEDERATED_TOKEN_EXPIRATION); | ||
| } |
Comment on lines
+157
to
+163
| protected String getFederatedIdToken(UserSessionModel userSession) { | ||
| String token = userSession.getNote(getFederatedIdTokenKey()); | ||
| if (token != null) { | ||
| return token; | ||
| } | ||
| return userSession.getNote(OIDCIdentityProvider.FEDERATED_ID_TOKEN); | ||
| } |
Comment on lines
+437
to
+439
| private void setFederatedAccessTokenNotes(UserSessionModel userSession, String idpAlias, String token) { | ||
| userSession.setNote(UserAuthenticationIdentityProvider.FEDERATED_ACCESS_TOKEN + ":" + idpAlias, token); | ||
| } |
Closes keycloak#50533 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Contributor
There was a problem hiding this comment.
Thanks @graziang for the PR. I've reviewed the changes and have a few comments. Quick question: Should we add tests to InterfaceIdentityProviderStoreTokenV2Test for multi-IdP token isolation (e.g., linking IdP A + B in the same session)?
| return user; | ||
| } | ||
|
|
||
| private void setFederatedAccessTokenNotes(UserSessionModel userSession, String idpAlias, String token) { |
Contributor
There was a problem hiding this comment.
I agree with Copilot comment, rename setFederatedAccessTokenNotes to setFederatedAccessTokenNote (singular)
| return OIDCIdentityProvider.FEDERATED_ID_TOKEN + ":" + getConfig().getAlias(); | ||
| } | ||
|
|
||
| protected String getFederatedRefreshToken(UserSessionModel userSession) { |
Contributor
There was a problem hiding this comment.
While an alias check is performed in AbstractIdentityProvider.getFederatedAccessToken(), a similar check is missing from getFederatedRefreshToken(), getFederatedTokenExpiration(), and getFederatedIdToken(). Is this intentional?"
Closes keycloak#50618 Signed-off-by: Giuseppe Graziano <g.graziano94@gmail.com>
Comment on lines
+333
to
+337
| // Try alias-namespaced key first, then fall back to legacy un-namespaced key | ||
| String token = userSession.getNote(UserAuthenticationIdentityProvider.FEDERATED_ACCESS_TOKEN + ":" + IDP_ALIAS); | ||
| if (token == null) { | ||
| token = userSession.getNote(UserAuthenticationIdentityProvider.FEDERATED_ACCESS_TOKEN); | ||
| } |
jimmychakkalakal
approved these changes
Jul 13, 2026
Contributor
There was a problem hiding this comment.
Thanks @graziang for the PR and changes. LGTM, I approve changes from my side
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
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.
Closes #50533
Fixed an issue where linking multiple identity providers would cause tokens to get mixed up, asking for IdP A's token could return IdP B's token instead.
All federated token notes now use colon-separated keys (FEDERATED_ACCESS_TOKEN:{alias}). Fallback reads to old noted for backward compatibility.