Implement access token context encoding framework #8
Conversation
closes #37118 Signed-off-by: mposolda <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a comprehensive access token context encoding framework for Keycloak's OIDC implementation. The framework embeds contextual metadata directly into access token IDs using a 6-character prefix format followed by the raw token ID (e.g., {sessionType}{tokenType}{grantType}:{rawTokenId}).
Key Components Added:
- AccessTokenContext: A data container that encapsulates token metadata including session type (online/offline/transient), token type (regular/lightweight), grant type, and raw token ID
- TokenContextEncoderProvider SPI: A pluggable interface for encoding/decoding token context information
- DefaultTokenContextEncoderProvider: The default implementation using 2-character shortcuts for each context type
- TokenContextEncoderProviderFactory: Factory interface following Keycloak's standard provider pattern
Grant Type System Enhancement:
All OAuth2GrantTypeFactory implementations now provide compact shortcuts (e.g., "ac" for authorization code, "cc" for client credentials, "rt" for refresh token) through a new getShortcut() method. This enables efficient encoding of grant type information in tokens.
Integration Points:
- Modified TokenManager to use the new encoder during token generation
- Updated all grant type implementations to set grant type context in client sessions
- Enhanced test infrastructure to validate the new encoded token ID format
- Made
getShouldUseLightweightTokenmethod public to support token type determination
Architecture Benefits:
This framework allows retrieval of token metadata without database lookups or proprietary JWT claims, potentially improving performance while maintaining OAuth2/OIDC compliance. The design supports backward compatibility with existing tokens that don't have encoded prefixes.
Confidence score: 2/5
- This PR contains critical bugs that will cause immediate production failures, including incorrect parameter validation and broken test logic
- Several implementation issues could lead to runtime exceptions and test failures
- Files requiring immediate attention:
AccessTokenContext.java(constructor validation bug),AssertEvents.java(inverted logic in matcher), andDefaultTokenContextEncoderProviderTest.java(brittle test setup)
27 files reviewed, 7 comments
| 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"); |
There was a problem hiding this comment.
syntax: Wrong parameter validated - should be rawTokenId instead of grantType
| Objects.requireNonNull(grantType, "Null rawTokenId not allowed"); | |
| Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed"); |
| String tokenId = "ofrtac:5678"; | ||
| AccessTokenContext ctx = provider.getTokenContextFromTokenId(tokenId); | ||
| Assert.fail("Not expected to success due incorrect grant type"); | ||
| } catch (RuntimeException iae) { |
There was a problem hiding this comment.
style: Catching generic RuntimeException is too broad. The implementation throws IllegalArgumentException specifically - catch that instead for more precise testing.
| } catch (RuntimeException iae) { | |
| } catch (IllegalArgumentException iae) { |
| try { | ||
| String tokenId = "ofrtac:5678"; | ||
| AccessTokenContext ctx = provider.getTokenContextFromTokenId(tokenId); | ||
| Assert.fail("Not expected to success due incorrect grant type"); |
There was a problem hiding this comment.
syntax: Typo: 'Not expected to success' should be 'Not expected to succeed'
| Assert.fail("Not expected to success due incorrect grant type"); | |
| Assert.fail("Not expected to succeed due to incorrect grant type"); |
| Map<String, String> grantsByShortcuts; | ||
| Map<String, String> grantsToShortcuts; |
There was a problem hiding this comment.
style: These fields should be private to follow encapsulation principles
| Map<String, String> grantsByShortcuts; | |
| Map<String, String> grantsToShortcuts; | |
| private Map<String, String> grantsByShortcuts; | |
| private Map<String, String> grantsToShortcuts; |
| String[] items = item.split(":"); | ||
| if (items.length != 2) return false; | ||
| // Grant type shortcut starts at character 4th char and is 2-chars long | ||
| if (items[0].substring(3, 5).equals(expectedGrantShortcut)) return false; |
There was a problem hiding this comment.
logic: Critical logic error: this condition returns false when grant shortcuts MATCH. Should be !items[0].substring(3, 5).equals(expectedGrantShortcut)
| if (items[0].substring(3, 5).equals(expectedGrantShortcut)) return false; | |
| if (!items[0].substring(3, 5).equals(expectedGrantShortcut)) return false; |
| TokenContextEncoderProvider encoder = session.getProvider(TokenContextEncoderProvider.class); | ||
| AccessTokenContext tokenCtx = encoder.getTokenContextFromClientSessionContext(clientSessionCtx, KeycloakModelUtils.generateId()); | ||
| token.id(encoder.encodeTokenId(tokenCtx)); |
There was a problem hiding this comment.
logic: No null check for encoder provider - could cause NPE if provider not registered
| * @return usually like 3-letters shortcut of specific grants. It can be useful for example in the tokens when the amount of characters should be limited and hence using full grant name | ||
| * is not ideal. Shortcut should be unique across grants. |
There was a problem hiding this comment.
style: Javadoc mentions "usually like 3-letters shortcut" but some implementations use 2-letter shortcuts ("ac", "cc", "rt", "te", "pc", "ci", "ro"). Consider updating documentation to reflect actual usage pattern.
| * @return usually like 3-letters shortcut of specific grants. It can be useful for example in the tokens when the amount of characters should be limited and hence using full grant name | |
| * is not ideal. Shortcut should be unique across grants. | |
| * @return usually like 2-3 letters shortcut of specific grants. It can be useful for example in the tokens when the amount of characters should be limited and hence using full grant name | |
| * is not ideal. Shortcut should be unique across grants. |
2 participants
Test 8