Implement access token context encoding framework#8
Conversation
closes #37118 Signed-off-by: mposolda <mposolda@gmail.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis change introduces a token context encoding system for OIDC access tokens, storing session type, token type, and grant type information as compact shortcuts within token IDs, enabling context retrieval without relying on non-standard token claims. Changes
Sequence Diagram(s)sequenceDiagram
participant TokenMgr as Token Manager
participant Encoder as TokenContextEncoderProvider
participant Factory as DefaultTokenContextEncoderProviderFactory
participant GrantFactory as OAuth2GrantTypeFactory
participant Context as AccessTokenContext
TokenMgr->>Encoder: encodeTokenId(AccessTokenContext)
Encoder->>Factory: getShortcutByGrantType(grantType)
Factory->>GrantFactory: getShortcut()
GrantFactory-->>Factory: "ac"
Factory-->>Encoder: "ac"
Encoder->>Context: getSessionType(), getTokenType()
Context-->>Encoder: SessionType, TokenType
Encoder->>Encoder: format "SS TT GT:rawId"
Encoder-->>TokenMgr: encodedTokenId
Note over TokenMgr,Context: Later: Decode token
TokenMgr->>Encoder: getTokenContextFromTokenId(encodedTokenId)
Encoder->>Encoder: parse "SS TT GT:rawId"
Encoder->>Factory: getSessionTypeByShortcut("SS")
Factory-->>Encoder: SessionType
Encoder->>Factory: getTokenTypeByShortcut("TT")
Factory-->>Encoder: TokenType
Encoder->>Factory: getGrantTypeByShortcut("GT")
Factory-->>Encoder: grantType
Encoder->>Context: new AccessTokenContext(...)
Context-->>Encoder: AccessTokenContext
Encoder-->>TokenMgr: AccessTokenContext
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java:
- Around line 69-78: The constructor AccessTokenContext currently calls
Objects.requireNonNull(grantType) twice and fails to validate rawTokenId, so
replace the duplicate null-check with Objects.requireNonNull(rawTokenId, "Null
rawTokenId not allowed") to ensure rawTokenId cannot be null; update the
AccessTokenContext(SessionType, TokenType, String, String) constructor to
validate rawTokenId before assigning this.rawTokenId and leave the other checks
unchanged.
In
@services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java:
- Around line 44-63: In getTokenContextFromClientSessionContext, avoid NPEs by
checking clientSessionContext.getClientSession() and the returned
AuthenticatedClientSessionModel.getUserSession() for null before dereferencing;
if clientSession is null throw an IllegalArgumentException stating
"clientSessionContext must have a valid client session", and if userSession is
null throw an IllegalArgumentException stating "client session must have a user
session", then proceed to compute sessionType, useLightweightToken (via
AbstractOIDCProtocolMapper.getShouldUseLightweightToken(session)), tokenType,
grantType and return the new AccessTokenContext as before.
In
@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java:
- Around line 476-492: The matcher isAccessTokenId has inverted logic and wrong
substring indices: in matchesSafely(String item) ensure you first split and
confirm items.length == 2, then extract the grant shortcut using
items[0].substring(4, 6) (chars at indices 4-5) and return false if that
substring does NOT equal expectedGrantShortcut (i.e., if (!items[0].substring(4,
6).equals(expectedGrantShortcut)) return false), finally return
isUUID().matches(items[1]); update the inline comment to reflect the correct
indices and grant position.
🧹 Nitpick comments (10)
services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java (1)
867-871: Good typo fix.The variable rename from
requestedAucienceClientstorequestedAudienceClientsimproves code clarity.Note: Line 1272 has a similar variable
resquestedAudienceClientswith a different typo ("resquested" instead of "requested"). Consider fixing it in the same PR for consistency.services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/StandardTokenExchangeProvider.java (1)
50-50: Minor inconsistency in constant usage.The code uses
OAuth2Constants.TOKEN_EXCHANGE_GRANT_TYPEdirectly, whilePreAuthorizedCodeGrantTypeusesPreAuthorizedCodeGrantTypeFactory.GRANT_TYPE. Although functionally equivalent, consider usingTokenExchangeGrantTypeFactory.GRANT_TYPEfor consistency across grant type implementations (sinceTokenExchangeGrantTypeFactoryis already imported).♻️ Suggested consistency improvement
- clientSessionCtx.setAttribute(Constants.GRANT_TYPE, OAuth2Constants.TOKEN_EXCHANGE_GRANT_TYPE); + clientSessionCtx.setAttribute(Constants.GRANT_TYPE, TokenExchangeGrantTypeFactory.GRANT_TYPE);Also applies to: 237-237
server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java (1)
29-33: Minor Javadoc inaccuracy regarding shortcut length.The Javadoc states "usually like 3-letters shortcut" but the implementations consistently use 2-letter shortcuts (e.g., "ac", "cc", "rt", "dg", "te"). Consider updating the documentation to reflect the actual convention.
📝 Suggested documentation fix
/** - * @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 + * @return usually a 2-letter 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. */ String getShortcut();Additionally, since this interface is part of the SPI, adding a new abstract method is a breaking change for any external
OAuth2GrantTypeFactoryimplementations. Verify this is the intended behavior or consider providing a default implementation if backward compatibility is required.services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantTypeFactory.java (1)
58-61: Consider defining a constant for the shortcut value.Other factories like
CibaGrantTypeFactorydefine aGRANT_SHORTCUTconstant (e.g.,public static final String GRANT_SHORTCUT = "ci") and return it fromgetShortcut(). Using a constant here would improve consistency across the codebase and allow tests/dependent code to reference the shortcut symbolically.Suggested refactor
public class PreAuthorizedCodeGrantTypeFactory implements OAuth2GrantTypeFactory, EnvironmentDependentProviderFactory { public static final String GRANT_TYPE = "urn:ietf:params:oauth:grant-type:pre-authorized_code"; public static final String CODE_REQUEST_PARAM = "pre-authorized_code"; + public static final String GRANT_SHORTCUT = "pc"; ... @Override public String getShortcut() { - return "pc"; + return GRANT_SHORTCUT; }services/src/main/java/org/keycloak/protocol/oidc/grants/TokenExchangeGrantTypeFactory.java (1)
40-43: Consider defining a constant for consistency withCibaGrantTypeFactory.Same as
PreAuthorizedCodeGrantTypeFactory— extracting the shortcut into apublic static final String GRANT_SHORTCUT = "te"constant would align with the pattern used inCibaGrantTypeFactoryand improve testability.Suggested refactor
public class TokenExchangeGrantTypeFactory implements OAuth2GrantTypeFactory, EnvironmentDependentProviderFactory { + public static final String GRANT_SHORTCUT = "te"; + @Override public String getId() { return OAuth2Constants.TOKEN_EXCHANGE_GRANT_TYPE; } @Override public String getShortcut() { - return "te"; + return GRANT_SHORTCUT; }services/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantTypeFactory.java (1)
38-41: Consider extracting the shortcut to a public constant for consistency.The implementation is correct. However,
AuthorizationCodeGrantTypeFactoryexposes its shortcut via apublic static final String GRANT_SHORTCUTconstant, which allows external code (e.g., tests, token encoding logic) to reference the shortcut without hardcoding the string. Consider applying the same pattern here for consistency and to avoid magic strings in consuming code.Suggested change
public class ResourceOwnerPasswordCredentialsGrantTypeFactory implements OAuth2GrantTypeFactory { + public static final String GRANT_SHORTCUT = "ro"; + @Override public String getId() { return OAuth2Constants.PASSWORD; } @Override public String getShortcut() { - return "ro"; + return GRANT_SHORTCUT; }services/src/main/java/org/keycloak/protocol/oidc/grants/ClientCredentialsGrantTypeFactory.java (1)
38-41: Consider extracting the shortcut to a public constant for consistency.Similar to
ResourceOwnerPasswordCredentialsGrantTypeFactory, consider extracting"cc"to apublic static final String GRANT_SHORTCUTconstant for consistency withAuthorizationCodeGrantTypeFactoryand to enable type-safe references from tests and other consumers.Suggested change
public class ClientCredentialsGrantTypeFactory implements OAuth2GrantTypeFactory { + public static final String GRANT_SHORTCUT = "cc"; + @Override public String getId() { return OAuth2Constants.CLIENT_CREDENTIALS; } @Override public String getShortcut() { - return "cc"; + return GRANT_SHORTCUT; }services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (2)
38-50: Test setup directly manipulates factory fields.The setup directly assigns to
factory.grantsByShortcutsandfactory.grantsToShortcuts. This works but couples the test to the internal field structure. Consider adding a test-friendly constructor or factory method if these fields are intended to be encapsulated.
56-59: JUnitassertEqualsargument order is reversed.JUnit convention is
assertEquals(expected, actual), but the assertions use(actual, expected). This won't break tests, but failure messages will show "expected X but was Y" with the values swapped, making debugging confusing.♻️ Suggested fix (apply to all assertEquals calls in this file)
- Assert.assertEquals(ctx.getSessionType(), AccessTokenContext.SessionType.TRANSIENT); - Assert.assertEquals(ctx.getTokenType(), AccessTokenContext.TokenType.LIGHTWEIGHT); - Assert.assertEquals(ctx.getGrantType(), OAuth2Constants.CLIENT_CREDENTIALS); - Assert.assertEquals(ctx.getRawTokenId(), "1234"); + Assert.assertEquals(AccessTokenContext.SessionType.TRANSIENT, ctx.getSessionType()); + Assert.assertEquals(AccessTokenContext.TokenType.LIGHTWEIGHT, ctx.getTokenType()); + Assert.assertEquals(OAuth2Constants.CLIENT_CREDENTIALS, ctx.getGrantType()); + Assert.assertEquals("1234", ctx.getRawTokenId());services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.java (1)
38-42: Consider restricting field visibility.
grantsByShortcutsandgrantsToShortcutsare package-private. If this is intentional for testing, consider adding a comment. Otherwise, these should beprivateto encapsulate internal state.♻️ Suggested change
- Map<String, String> grantsByShortcuts; - Map<String, String> grantsToShortcuts; + private Map<String, String> grantsByShortcuts; + private Map<String, String> grantsToShortcuts;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
server-spi-private/src/main/java/org/keycloak/models/Constants.javaserver-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantType.javaserver-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/TokenManager.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderProvider.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderProviderFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderSpi.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/AuthorizationCodeGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/ClientCredentialsGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/PermissionGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantType.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/RefreshTokenGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantType.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/TokenExchangeGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractOIDCProtocolMapper.javaservices/src/main/java/org/keycloak/protocol/oidc/tokenexchange/StandardTokenExchangeProvider.javaservices/src/main/resources/META-INF/services/org.keycloak.protocol.oidc.encode.TokenContextEncoderProviderFactoryservices/src/main/resources/META-INF/services/org.keycloak.provider.Spiservices/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.javatestsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java
🧰 Additional context used
🧬 Code graph analysis (7)
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java (1)
server-spi-private/src/main/java/org/keycloak/models/Constants.java (1)
Constants(31-209)
services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/StandardTokenExchangeProvider.java (2)
services/src/main/java/org/keycloak/protocol/oidc/grants/TokenExchangeGrantTypeFactory.java (1)
TokenExchangeGrantTypeFactory(33-70)server-spi-private/src/main/java/org/keycloak/models/Constants.java (1)
Constants(31-209)
services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java (3)
services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java (1)
AccessTokenContext(29-95)server-spi-private/src/main/java/org/keycloak/models/Constants.java (1)
Constants(31-209)server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java (1)
KeycloakModelUtils(103-1219)
services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (3)
tests/utils/src/main/java/org/keycloak/tests/utils/Assert.java (1)
Assert(52-176)services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java (1)
DefaultTokenContextEncoderProvider(31-124)services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java (1)
AccessTokenContext(29-95)
services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.java (2)
services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java (1)
AccessTokenContext(29-95)services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java (1)
DefaultTokenContextEncoderProvider(31-124)
services/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantType.java (1)
server-spi-private/src/main/java/org/keycloak/models/Constants.java (1)
Constants(31-209)
services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantType.java (1)
server-spi-private/src/main/java/org/keycloak/models/Constants.java (1)
Constants(31-209)
🔇 Additional comments (29)
services/src/main/java/org/keycloak/protocol/oidc/grants/PermissionGrantTypeFactory.java (1)
38-41: Implementation is correct and follows the established pattern.The
getShortcut()method returns "pg" (permission grant) and the shortcut is unique across all grant type factories in the codebase. All shortcuts are properly distributed: ac, rt, dg, ci, ro, te, pc, pg, cc.services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractOIDCProtocolMapper.java (1)
84-87: The change from instance to public static method is safe and appropriate.Verification confirms no subclass overrides this method. The static modifier is correct since the method only uses the
sessionparameter without accessing instance state. Making itpublicenables access fromDefaultTokenContextEncoderProviderand the new token context encoding framework, which is the intended use case shown in the codebase.services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java (3)
67-68: LGTM!New imports for the token context encoding framework are appropriately added.
248-248: LGTM!Setting the grant type attribute during refresh token validation is appropriate. This enables the token context encoder to capture the correct grant type when the new access token is created via
createClientAccessTokenat line 251.
1051-1055: No changes needed—SPI is properly configured and provider is always available.The
TokenContextEncoderProviderSPI is correctly registered inMETA-INF/serviceswithDefaultTokenContextEncoderProviderFactory, and the factory'screate()method always returns a non-null instance. This is a mandatory core provider with no alternative implementations and cannot be disabled or unavailable. The code at lines 1051-1055 is safe without a null check.services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantTypeFactory.java (1)
37-47: LGTM!The implementation correctly adds the "dg" shortcut for Device Grant, following the established 2-letter convention used by other grant type factories (e.g., "ac" for authorization code, "cc" for client credentials). The
getShortcut()method properly implements the interface contract.server-spi-private/src/main/java/org/keycloak/models/Constants.java (1)
206-207: LGTM!The
GRANT_TYPEconstant appropriately centralizes access toOAuth2Constants.GRANT_TYPEfor session context usage, following the established pattern in this file (similar to how other OAuth2-related constants are aliased).services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantType.java (1)
32-32: LGTM!The grant type is correctly set on the session context using
Constants.GRANT_TYPEas the key, enabling the token context encoding framework to track and encode the grant type used for token issuance.Also applies to: 82-82
services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/StandardTokenExchangeProvider.java (1)
237-237: No action needed - all token exchange code paths set the grant type.The
exchangeClientToClientmethod delegates to eitherexchangeClientToOIDCClient(which setsGRANT_TYPEat line 237) orexchangeClientToSAML2Client. InStandardTokenExchangeProvider, the SAML2 path explicitly throws an exception rejecting SAML2 as unsupported. Thus, only the OIDC path executes successfully, and it always sets the grant type. The concern is not applicable.services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantTypeFactory.java (1)
37-47: LGTM!Good pattern: defining
GRANT_SHORTCUTas a constant allows tests and dependent code to reference it symbolically, and thegetShortcut()method correctly returns this constant.services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java (1)
109-109: LGTM — grant type correctly stored before token creation.Setting the
GRANT_TYPEattribute onclientSessionCtxbefore callingcreateClientAccessTokenensures the token context encoder can access this information. The placement is correct.services/src/main/resources/META-INF/services/org.keycloak.provider.Spi (1)
28-28: LGTM!The
TokenContextEncoderSpiis correctly registered in the SPI service file, enabling Keycloak's provider discovery mechanism to load the new token context encoding SPI.services/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantType.java (1)
133-135: LGTM!The grant type attribute is correctly set on the
ClientSessionContextimmediately after session attachment, making it available for downstream token context encoding. The placement beforegetUserSession()and token generation ensures the context is properly populated for the token building phase.services/src/main/java/org/keycloak/protocol/oidc/grants/AuthorizationCodeGrantTypeFactory.java (1)
32-42: LGTM!Good implementation pattern with the public constant
GRANT_SHORTCUT. This allows tests and token encoding logic to referenceAuthorizationCodeGrantTypeFactory.GRANT_SHORTCUTdirectly rather than hardcoding the string. This is the preferred approach that other grant type factories should follow.services/src/main/resources/META-INF/services/org.keycloak.protocol.oidc.encode.TokenContextEncoderProviderFactory (1)
1-20: LGTM!Standard Java SPI service provider registration. The file correctly registers
DefaultTokenContextEncoderProviderFactoryas the implementation for theTokenContextEncoderProviderFactoryinterface, enabling discovery by the Keycloak provider framework. The implementation class exists in the expected location and correctly implements the required interface.services/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderProviderFactory.java (1)
27-28: LGTM!Clean SPI factory interface definition following established Keycloak patterns.
services/src/main/java/org/keycloak/protocol/oidc/grants/RefreshTokenGrantTypeFactory.java (1)
33-43: LGTM!The grant shortcut implementation follows the established pattern and integrates cleanly with the token context encoding framework.
server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantType.java (2)
85-103: LGTM!The
grantTypefield is properly initialized from form parameters. The nullable behavior is acceptable since downstream token context encoding handles missing grant types by defaulting toUNKNOWN.
172-175: LGTM!Simple getter following the established pattern in this class.
services/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderSpi.java (1)
29-50: LGTM!Clean SPI definition following Keycloak conventions. Marking as internal is appropriate for this token encoding infrastructure.
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java (2)
33-36: LGTM!Imports for grant type factories are correctly added to support the new token ID validation.
145-146: Usage is correct, but depends on the matcher fix above.The grant shortcut constant from
AuthorizationCodeGrantTypeFactory.GRANT_SHORTCUTis appropriately used here.services/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderProvider.java (1)
31-38: LGTM!Well-designed provider interface with clear method contracts for encoding/decoding token context. The Javadoc provides useful context about the purpose.
services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (1)
52-113: Test coverage is comprehensive.Good coverage of success cases (client credentials, offline tokens), error handling (incorrect grant type), edge cases (unknown grant type, old token format), and round-trip encoding verification.
services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java (2)
65-98: LGTM!The decoding logic is well-structured:
- Gracefully handles legacy/unencoded token IDs by returning UNKNOWN context
- Properly validates the encoded context length
- Clear error messages for invalid shortcuts
100-118: LGTM!Encoding logic correctly validates that UNKNOWN session/token types cannot be encoded, ensuring only valid contexts are persisted. The token format
SSTTGT:rawIdis clear and consistent with the decoding logic.services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java (1)
36-67: LGTM!The enums are well-designed with 2-character shortcuts that enable compact token ID encoding. The UNKNOWN values provide a clear fallback for legacy or unrecognized tokens.
services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.java (2)
64-86: LGTM!The initialization logic correctly builds bidirectional mappings and the validation at lines 82-85 effectively catches configuration errors (duplicate shortcuts or IDs) early at startup rather than letting them cause subtle runtime bugs.
106-137: Thread-safe lazy refresh pattern is appropriate.The dynamic refresh logic handles hot-deployed grant types gracefully. While multiple threads could trigger a refresh simultaneously, the use of
ConcurrentHashMapand idempotent put operations ensures correctness without requiring explicit synchronization.
| public AccessTokenContext(SessionType sessionType, TokenType tokenType, String grantType, String rawTokenId) { | ||
| 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"); | ||
| this.sessionType = sessionType; | ||
| this.tokenType = tokenType; | ||
| this.grantType = grantType; | ||
| this.rawTokenId = rawTokenId; | ||
| } |
There was a problem hiding this comment.
Bug: rawTokenId is not validated for null.
Line 73 validates grantType a second time instead of rawTokenId. This allows null rawTokenId values to be stored, which would likely cause NPEs when getRawTokenId() is used downstream.
🐛 Proposed fix
public AccessTokenContext(SessionType sessionType, TokenType tokenType, String grantType, String rawTokenId) {
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");
+ Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed");
this.sessionType = sessionType;
this.tokenType = tokenType;
this.grantType = grantType;
this.rawTokenId = rawTokenId;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public AccessTokenContext(SessionType sessionType, TokenType tokenType, String grantType, String rawTokenId) { | |
| 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"); | |
| this.sessionType = sessionType; | |
| this.tokenType = tokenType; | |
| this.grantType = grantType; | |
| this.rawTokenId = rawTokenId; | |
| } | |
| public AccessTokenContext(SessionType sessionType, TokenType tokenType, String grantType, String rawTokenId) { | |
| Objects.requireNonNull(sessionType, "Null sessionType not allowed"); | |
| Objects.requireNonNull(tokenType, "Null tokenType not allowed"); | |
| Objects.requireNonNull(grantType, "Null grantType not allowed"); | |
| Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed"); | |
| this.sessionType = sessionType; | |
| this.tokenType = tokenType; | |
| this.grantType = grantType; | |
| this.rawTokenId = rawTokenId; | |
| } |
🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java
around lines 69 - 78, The constructor AccessTokenContext currently calls
Objects.requireNonNull(grantType) twice and fails to validate rawTokenId, so
replace the duplicate null-check with Objects.requireNonNull(rawTokenId, "Null
rawTokenId not allowed") to ensure rawTokenId cannot be null; update the
AccessTokenContext(SessionType, TokenType, String, String) constructor to
validate rawTokenId before assigning this.rawTokenId and leave the other checks
unchanged.
| @Override | ||
| public AccessTokenContext getTokenContextFromClientSessionContext(ClientSessionContext clientSessionContext, String rawTokenId) { | ||
| AccessTokenContext.SessionType sessionType; | ||
| UserSessionModel userSession = clientSessionContext.getClientSession().getUserSession(); | ||
| if (userSession.getPersistenceState() == UserSessionModel.SessionPersistenceState.TRANSIENT) { | ||
| sessionType = AccessTokenContext.SessionType.TRANSIENT; | ||
| } else { | ||
| sessionType = userSession.isOffline() ? AccessTokenContext.SessionType.OFFLINE : AccessTokenContext.SessionType.ONLINE; | ||
| } | ||
|
|
||
| boolean useLightweightToken = AbstractOIDCProtocolMapper.getShouldUseLightweightToken(session); | ||
| AccessTokenContext.TokenType tokenType = useLightweightToken ? AccessTokenContext.TokenType.LIGHTWEIGHT : AccessTokenContext.TokenType.REGULAR; | ||
|
|
||
| String grantType = clientSessionContext.getAttribute(Constants.GRANT_TYPE, String.class); | ||
| if (grantType == null) { | ||
| grantType = UNKNOWN; | ||
| } | ||
|
|
||
| return new AccessTokenContext(sessionType, tokenType, grantType, rawTokenId); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for calls to getTokenContextFromClientSessionContext
rg -n 'getTokenContextFromClientSessionContext' --type java -B 3 -A 3Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 3021
🏁 Script executed:
# Find the ClientSessionContext interface to understand the contract
fd -e java | xargs grep -l 'interface ClientSessionContext\|class ClientSessionContext' | head -5Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 154
🏁 Script executed:
# Check the UserSessionModel interface for getPersistenceState and isOffline nullability
rg -n 'getUserSession\(\)' --type java -B 2 -A 2 | head -40Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 6500
🏁 Script executed:
# Read the ClientSessionContext interface
cat services/src/main/java/org/keycloak/models/ClientSessionContext.java | head -60Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 184
🏁 Script executed:
# Check the context in TokenManager where clientSessionCtx is obtained (around line 1053)
sed -n '1030,1070p' services/src/main/java/org/keycloak/protocol/oidc/TokenManager.javaRepository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 2577
🏁 Script executed:
# Look for the method that creates or passes clientSessionCtx to understand null checks
rg -n 'clientSessionCtx' services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java -B 5 | head -50Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 2799
🏁 Script executed:
# Find ClientSessionContext interface
find . -name "*.java" -type f | xargs grep -l 'interface ClientSessionContext' 2>/dev/nullRepository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 158
🏁 Script executed:
# Find DefaultClientSessionContext implementation
find . -name "DefaultClientSessionContext.java" -type fRepository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 168
🏁 Script executed:
# Search for getClientSession method definition
rg -n 'getClientSession\(\)' --type java -A 2 | grep -E '(interface|class|ClientSession|getClientSession)' | head -30Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 4845
🏁 Script executed:
# Read the ClientSessionContext interface
cat ./server-spi/src/main/java/org/keycloak/models/ClientSessionContext.javaRepository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 2018
🏁 Script executed:
# Read the DefaultClientSessionContext implementation
cat ./services/src/main/java/org/keycloak/services/util/DefaultClientSessionContext.javaRepository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 12539
🏁 Script executed:
# Check if there are @Nullable or @NonNull annotations in the interfaces/classes
rg -n '@Nullable|@NonNull|@Nonnull' services/src/main/java/org/keycloak/protocol/oidc/encode/ -A 2 -B 2Repository: AI-Code-Review-Evals/coderabbit-keycloak
Length of output: 66
Add null checks for chained calls on potentially null objects.
The chained call clientSessionContext.getClientSession().getUserSession() at line 47 could throw NPE if getClientSession() returns null. Since getClientSession() has no nullability guarantees and other parts of the codebase explicitly check for null returns, add defensive null checks:
AuthenticatedClientSessionModel clientSession = clientSessionContext.getClientSession();
if (clientSession == null) {
throw new IllegalArgumentException("clientSessionContext must have a valid client session");
}
UserSessionModel userSession = clientSession.getUserSession();
if (userSession == null) {
throw new IllegalArgumentException("client session must have a user session");
}Alternatively, document the precondition that both clientSessionContext.getClientSession() and its user session must be non-null.
🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java
around lines 44 - 63, In getTokenContextFromClientSessionContext, avoid NPEs by
checking clientSessionContext.getClientSession() and the returned
AuthenticatedClientSessionModel.getUserSession() for null before dereferencing;
if clientSession is null throw an IllegalArgumentException stating
"clientSessionContext must have a valid client session", and if userSession is
null throw an IllegalArgumentException stating "client session must have a user
session", then proceed to compute sessionType, useLightweightToken (via
AbstractOIDCProtocolMapper.getShouldUseLightweightToken(session)), tokenType,
grantType and return the new AccessTokenContext as before.
| public static Matcher<String> isAccessTokenId(String expectedGrantShortcut) { | ||
| return new TypeSafeMatcher<String>() { | ||
| @Override | ||
| protected boolean matchesSafely(String item) { | ||
| 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; | ||
| return isUUID().matches(items[1]); | ||
| } | ||
|
|
||
| @Override | ||
| public void describeTo(Description description) { | ||
| description.appendText("Not a Token ID with expected grant: " + expectedGrantShortcut); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Critical bug: Inverted logic and incorrect substring indices.
The isAccessTokenId matcher has two issues:
-
Inverted condition: Line 483 returns
falsewhen the grant shortcut matches, but should returnfalsewhen it doesn't match. -
Wrong substring indices: Based on the token format (
trltcc:uuid= session(2) + tokenType(2) + grantType(2) + ":" + rawId), the grant type shortcut is at indices 4-5, not 3-4.substring(3, 5)extracts "tc" from "trltcc", not "cc".
These bugs will cause tests to pass incorrectly (false positives) since the matcher returns true when the grant shortcut does not match.
🐛 Proposed fix
public static Matcher<String> isAccessTokenId(String expectedGrantShortcut) {
return new TypeSafeMatcher<String>() {
@Override
protected boolean matchesSafely(String item) {
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;
+ // Grant type shortcut starts at index 4 and is 2-chars long
+ if (!items[0].substring(4, 6).equals(expectedGrantShortcut)) return false;
return isUUID().matches(items[1]);
}
@Override
public void describeTo(Description description) {
description.appendText("Not a Token ID with expected grant: " + expectedGrantShortcut);
}
};
}🤖 Prompt for AI Agents
In
@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java
around lines 476 - 492, The matcher isAccessTokenId has inverted logic and wrong
substring indices: in matchesSafely(String item) ensure you first split and
confirm items.length == 2, then extract the grant shortcut using
items[0].substring(4, 6) (chars at indices 4-5) and return false if that
substring does NOT equal expectedGrantShortcut (i.e., if (!items[0].substring(4,
6).equals(expectedGrantShortcut)) return false), finally return
isUUID().matches(items[1]); update the inline comment to reflect the correct
indices and grant position.
2 participants
Test 8
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#8