Skip to content

Implement access token context encoding framework#3

Open
zaibkhan wants to merge 1 commit into
feature-token-context-baselinefrom
feature-token-context-implementation
Open

Implement access token context encoding framework#3
zaibkhan wants to merge 1 commit into
feature-token-context-baselinefrom
feature-token-context-implementation

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

closes #37118

Signed-off-by: mposolda <mposolda@gmail.com>
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Introduce grant shortcuts, tag token exchange
What’s good: Consistent getShortcut() implementations added across grant factories and tests validate encode/decode paths; SPI and service registrations wired correctly.
Review Status: ❌ Requires changes
Overall Priority: High
Review Update:
• Coverage: Reviewed all 28 files across 3 batches

Issues (Critical & High only)

Severity Issue Why it matters
High Correctness — Incorrect null-check for rawTokenId in AccessTokenContext …/encode/AccessTokenContext.java
Wrong null-check target: rawTokenId is never validated and grantType is validated twice. This can mask a null rawTokenId and fail fast with a misleading message.
High Testing — Grant shortcut check in matcher uses wrong indices and in... …/mappers/AbstractOIDCProtocolMapper.java
The substring indices for the 2-char grant shortcut are off (should be [4,6)), and the condition is inverted. As written, a correct shortcut causes the matcher to fail, breaking tests that validate token ID format.

Showing top 2 issues. Critical: 0, High: 2. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: Consider documenting and centrally enumerating all shortcut codes to prevent collisions and drift across factories; ensure any unused imports are cleaned to keep build warnings low.
  • Testing: Coverage looks solid for encoding/decoding token context including unknown and legacy tokens; consider adding cases for newly introduced shortcuts (e.g., 'te', 'dg', 'ci') to prevent regressions.
  • Documentation: Adding brief documentation for the new shortcut codes would help operators and integrators understand their meaning and ensure consistency.
  • Open questions: Are these shortcut codes considered stable API and guaranteed unique across all grants? Should the token exchange grant type attribute be set for all exchange paths for consistent auditing?

Confidence: 3/5 — Needs work before merge (2 high · status: Requires changes · scope: large PR (28 files))

React with 👍 or 👎 if you found this review useful.

Objects.requireNonNull(sessionType, "Null sessionType not allowed");
Objects.requireNonNull(tokenType, "Null tokenType not allowed");
Objects.requireNonNull(grantType, "Null grantType not allowed");
Objects.requireNonNull(grantType, "Null rawTokenId not allowed");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High: Wrong null-check target: rawTokenId is never validated and grantType is validated twice. This can mask a null rawTokenId and fail fast with a misleading message.

Suggested change
Objects.requireNonNull(grantType, "Null rawTokenId not allowed");
```java
Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed");

@@ -1045,7 +1048,11 @@ protected IDToken applyMapper(IDToken token, Map.Entry<ProtocolMapperModel, Prot
protected AccessToken initToken(KeycloakSession session, RealmModel realm, ClientModel client, UserModel user, UserSessionModel userSession,
ClientSessionContext clientSessionCtx, UriInfo uriInfo) {
AccessToken token = new AccessToken();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: Possible NPE if the provider is not registered; this would break token issuance. Consider a defensive fallback to the original UUID-only ID to preserve runtime behavior if the provider is unavailable.

java
TokenContextEncoderProvider encoder = session.getProvider(TokenContextEncoderProvider.class);
if (encoder == null) {
token.id(KeycloakModelUtils.generateId());
} else {
AccessTokenContext tokenCtx = encoder.getTokenContextFromClientSessionContext(clientSessionCtx, KeycloakModelUtils.generateId());
token.id(encoder.encodeTokenId(tokenCtx));
}

}
grantShortcut = grantsToShortcuts.get(grantType);
}
return grantShortcut;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: Returning null here propagates an absent shortcut to callers and risks NPEs or malformed token context encoding when an unknown or newly registered grant type is encountered; prefer a stable UNKNOWN fallback.

});
grantType = grantsByShortcuts.get(shortcut);
}
return grantType;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: Symmetric to getShortcutByGrantType, returning null for an unknown shortcut can lead to NPEs or inconsistent encoding; use the UNKNOWN sentinel for stability.

*/
public class DefaultTokenContextEncoderProviderFactory implements TokenContextEncoderProviderFactory {

private KeycloakSessionFactory sessionFactory;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: sessionFactory is written in postInit and read in mapping methods potentially from other threads; without a visibility guarantee this can be observed as null, causing sporadic NPEs. Mark as volatile to ensure safe publication.

Suggested change
private KeycloakSessionFactory sessionFactory;
private volatile KeycloakSessionFactory sessionFactory;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants