Implement access token context encoding framework#3
Implement access token context encoding framework#3akshayutture-augment wants to merge 1 commit into
Conversation
closes #37118 Signed-off-by: mposolda <mposolda@gmail.com>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request introduces a token context encoding system for OAuth2 tokens. It adds new SPI provider interfaces and implementations to encode contextual metadata (session type, token type, grant type) within token IDs using a compact 6-character prefix scheme. Grant type factories are updated with shortcut identifiers, and token generation flows modified to use the new encoder. Changes
Sequence Diagram(s)sequenceDiagram
participant TokenManager as TokenManager<br/>(Token Generation)
participant Encoder as TokenContextEncoderProvider<br/>(Encoding Service)
participant Factory as DefaultTokenContextEncoderProviderFactory<br/>(Shortcut Mappings)
participant TokenCtx as AccessTokenContext<br/>(Encoded Metadata)
TokenManager->>Encoder: encodeTokenId(AccessTokenContext)
Encoder->>Factory: getSessionTypeShortcut(sessionType)
Factory-->>Encoder: shortcut (e.g., "O")
Encoder->>Factory: getTokenTypeShortcut(tokenType)
Factory-->>Encoder: shortcut (e.g., "R")
Encoder->>Factory: getGrantShortcut(grantType)
Factory-->>Encoder: shortcut (e.g., "ac")
Encoder->>Encoder: compose prefix = "ORac" + rawTokenId
Encoder-->>TokenManager: encoded token ID
Note over Encoder,Factory: Decode Path (Reverse)
TokenManager->>Encoder: getTokenContextFromTokenId(encodedId)
Encoder->>Encoder: parse 6-char prefix
Encoder->>Factory: lookupByShortcuts(sessionShortcut,<br/>tokenShortcut, grantShortcut)
Factory-->>Encoder: AccessTokenContext
Encoder-->>TokenManager: AccessTokenContext with metadata
sequenceDiagram
participant Flow as Grant Flow<br/>(e.g., AuthCode)
participant GrantTypeBase as OAuth2GrantTypeBase<br/>(Grant Handler)
participant SessionCtx as ClientSessionContext<br/>(Session State)
participant TokenMgr as TokenManager<br/>(Token Creation)
Flow->>GrantTypeBase: createTokenResponse()
GrantTypeBase->>SessionCtx: setAttribute(GRANT_TYPE, context.getGrantType())
Note over SessionCtx: Grant type now in context<br/>for downstream processing
GrantTypeBase->>TokenMgr: initToken(token, ...)
TokenMgr->>TokenMgr: derive session/token type<br/>from SessionCtx
TokenMgr->>TokenMgr: use encoder to generate<br/>contextual token ID
TokenMgr-->>Flow: token with encoded ID
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantType.java (1)
85-103: EnsuregrantTypestays consistent withformParams
grantTypeis initialized fromformParamsin the constructor butsetFormParams(...)can later replaceformParamswithout updatinggrantType, sogetGrantType()may return a stale value after callers adjust parameters.Consider recalculating
grantTypeinsetFormParams(and handling null safely), for example:public void setFormParams(MultivaluedHashMap<String, String> formParams) { this.formParams = formParams; this.grantType = formParams != null ? formParams.getFirst(OAuth2Constants.GRANT_TYPE) : null; }Alternatively, derive
grantTypeon demand fromformParamsinstead of caching it.Also applies to: 105-107, 173-175
🧹 Nitpick comments (14)
services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantTypeFactory.java (2)
37-37: LGTM! Consider adding JavaDoc for clarity.The constant declaration is correct and "ci" is an appropriate 2-character shortcut for the CIBA grant type. It follows the pattern being applied across other grant type factories in this PR.
Optionally, consider adding JavaDoc to explain the constant's role in the token context encoding framework:
+ /** + * Short identifier for CIBA grant type, used in token context encoding. + */ public static final String GRANT_SHORTCUT = "ci";
44-47: LGTM! Method implementation is clean and correct.The
getShortcut()method correctly implements the interface contract and returns the appropriate shortcut identifier. The implementation is straightforward and follows best practices.Optionally, consider adding JavaDoc to document the method's purpose:
+ /** + * Returns the short identifier for this grant type, used in token context encoding. + * + * @return the grant type shortcut identifier + */ @Override public String getShortcut() { return GRANT_SHORTCUT; }services/src/main/java/org/keycloak/protocol/oidc/grants/TokenExchangeGrantTypeFactory.java (2)
29-32: Fix the incorrect JavaDoc (pre-existing issue).The JavaDoc incorrectly describes this as the "Factory for OAuth 2.0 Authorization Code Grant," but this is actually the Token Exchange grant type factory. This appears to be a copy-paste error from another factory class.
Apply this diff to correct the documentation:
/** - * Factory for OAuth 2.0 Authorization Code Grant + * Factory for OAuth 2.0 Token Exchange Grant * * @author <a href="mailto:demetrio@carretti.pro">Dmitry Telegin</a> */
40-43: Consider extracting the shortcut to a constant for consistency with newer grant type factories.The implementation is correct and appropriately returns a 2-character shortcut for the Token Exchange grant type. However, verification shows that recent grant type factories (CibaGrantTypeFactory, DeviceGrantTypeFactory, RefreshTokenGrantTypeFactory, and AuthorizationCodeGrantTypeFactory) define
GRANT_SHORTCUTconstants and return them ingetShortcut(). Extracting the hardcoded value would improve consistency with this established pattern in the codebase.Suggested refactor:
} + public static final String GRANT_SHORTCUT = "te"; + @Override public String getShortcut() { - return "te"; + return GRANT_SHORTCUT; }services/src/main/java/org/keycloak/protocol/oidc/grants/ClientCredentialsGrantTypeFactory.java (1)
38-41: Consider using a constant for consistency.While the implementation is functionally correct, some grant factories (e.g.,
AuthorizationCodeGrantTypeFactory,RefreshTokenGrantTypeFactory) define aGRANT_SHORTCUTconstant instead of using a string literal. Using a constant improves discoverability and makes future refactoring easier.Apply this diff to align with the pattern used in other factories:
+ public static final String GRANT_SHORTCUT = "cc"; + @Override public String getShortcut() { - return "cc"; + return GRANT_SHORTCUT; }services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantTypeFactory.java (1)
58-61: Consider using a constant for consistency.Similar to other grant factories, consider defining a
GRANT_SHORTCUTconstant instead of using a string literal. This improves consistency withAuthorizationCodeGrantTypeFactoryandRefreshTokenGrantTypeFactory.Apply this diff:
+ 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/ResourceOwnerPasswordCredentialsGrantTypeFactory.java (1)
38-41: Consider using a constant for consistency.For consistency with
AuthorizationCodeGrantTypeFactoryandRefreshTokenGrantTypeFactory, consider defining aGRANT_SHORTCUTconstant.Apply this diff:
+ public static final String GRANT_SHORTCUT = "ro"; + @Override public String getShortcut() { - return "ro"; + return GRANT_SHORTCUT; }services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java (1)
1052-1055: Consider defensive fallback ifTokenContextEncoderProvideris unavailable
initTokennow unconditionally calls:TokenContextEncoderProvider encoder = session.getProvider(TokenContextEncoderProvider.class); AccessTokenContext tokenCtx = encoder.getTokenContextFromClientSessionContext(clientSessionCtx, KeycloakModelUtils.generateId()); token.id(encoder.encodeTokenId(tokenCtx));If for any reason the SPI is misconfigured and no provider is registered, this will throw a
NullPointerExceptionat runtime. In core deployments this should never happen, but hard failure on missing provider may complicate extensions/tests.Consider a minimal fallback, e.g.:
TokenContextEncoderProvider encoder = session.getProvider(TokenContextEncoderProvider.class); String rawId = KeycloakModelUtils.generateId(); if (encoder == null) { token.id(rawId); } else { AccessTokenContext tokenCtx = encoder.getTokenContextFromClientSessionContext(clientSessionCtx, rawId); token.id(encoder.encodeTokenId(tokenCtx)); }so token issuance still works even if the encoder SPI is not wired.
services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java (1)
107-115: Guard against nullgrant_typewhen populating client session context
createTokenResponsenow does:clientSessionCtx.setAttribute(Constants.GRANT_TYPE, context.getGrantType());If
context.getGrantType()can ever benull(e.g., non-standard flows or tests constructingContextmanually), this will store a null value and may confuse consumers like the token-context encoder.If null is possible, prefer:
String grantType = context.getGrantType(); if (grantType != null) { clientSessionCtx.setAttribute(Constants.GRANT_TYPE, grantType); }or enforce non-nullability in
Contextso callers must always provide a grant type.services/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderProvider.java (1)
25-38: Public SPI is clear; consider minimal method-level docsThe interface shape is good and keeps the responsibilities tight (derive context, encode, decode). For external implementers it would help to document at method level:
- Whether
getTokenContextFromTokenIdmay throw on malformed IDs vs. returningnull.- Any expectations about immutability of
AccessTokenContextpassed toencodeTokenId.Not strictly required, but improves SPI usability.
services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (1)
76-85: Refactor exception assertions usingassertThrowsfor clarityJUnit 4.13.2 is configured in this project, which supports
Assert.assertThrows. The manualtry/fail/catchblocks at lines 76-85 and 107-112 can be refactored to useassertThrowswith the specific expected exception type for improved readability and clearer intent.Example:
// Instead of: try { String tokenId = "ofrtac:5678"; AccessTokenContext ctx = provider.getTokenContextFromTokenId(tokenId); Assert.fail("Not expected to success due incorrect grant type"); } catch (RuntimeException iae) { // ignored } // Use: Assert.assertThrows(IllegalArgumentException.class, () -> { provider.getTokenContextFromTokenId("ofrtac:5678"); });This is a readability and test-maintenance improvement; existing tests function correctly.
services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.java (3)
38-43: Encapsulate grant maps unless package access is required
grantsByShortcutsandgrantsToShortcutsare currently package‑visible, while all access goes through the protected helpers. If there’s no intentional external use in this package, consider making these fieldsprivateto avoid accidental mutation from other classes and keep the concurrency assumptions local to this factory.
49-62: Strengthen duplicate‑shortcut detection for session/token types and grantsRight now:
init()silently overwrites entries if twoSessionTypeorTokenTypeenums share the same shortcut.postInit()detects issues only via a size comparison betweengrantsByShortcutsandgrantsToShortcuts, which makes debugging harder and may miss some edge patterns.To fail fast and provide clearer diagnostics, you could:
- In
init(), detect duplicate shortcuts when inserting and throw with the conflicting enum names.- In
postInit(), useputIfAbsent(or manual checks) and throw immediately with the conflicting grant ID/shortcut instead of relying solely on the final size check.Example sketch inside the
forEachinpostInit():- factory.getProviderFactoriesStream(OAuth2GrantType.class) - .forEach((factory1) -> { - OAuth2GrantTypeFactory gtf = (OAuth2GrantTypeFactory) factory1; - String grantName = gtf.getId(); - String grantShortcut = gtf.getShortcut(); - grantsByShortcuts.put(grantShortcut, grantName); - grantsToShortcuts.put(grantName, grantShortcut); - }); + factory.getProviderFactoriesStream(OAuth2GrantType.class) + .forEach((factory1) -> { + OAuth2GrantTypeFactory gtf = (OAuth2GrantTypeFactory) factory1; + String grantName = gtf.getId(); + String grantShortcut = gtf.getShortcut(); + + String prevGrantForShortcut = grantsByShortcuts.putIfAbsent(grantShortcut, grantName); + if (prevGrantForShortcut != null && !prevGrantForShortcut.equals(grantName)) { + throw new IllegalStateException("Duplicate shortcut '" + grantShortcut + + "' for grants '" + prevGrantForShortcut + "' and '" + grantName + "'"); + } + + String prevShortcutForGrant = grantsToShortcuts.putIfAbsent(grantName, grantShortcut); + if (prevShortcutForGrant != null && !prevShortcutForGrant.equals(grantShortcut)) { + throw new IllegalStateException("Duplicate grant id '" + grantName + + "' with shortcuts '" + prevShortcutForGrant + "' and '" + grantShortcut + "'"); + } + });A similar pattern could be applied in
init()for enum shortcuts.Also applies to: 64-86
106-119: Clarify behavior for unknown grant types / shortcuts and consider optional negative cachingBoth
getShortcutByGrantTypeandgetGrantTypeByShortcut:
- Attempt a one‑time refresh from the session factory when a mapping is missing.
- Return
nullif the grant cannot be resolved.Two optional improvements to consider:
- If the desired behavior for unknowns is to fall back to the
UNKNOWNsentinel, you could map unresolved values directly toDefaultTokenContextEncoderProvider.UNKNOWNinstead of returningnull, which would keep the encoding/decoding pipeline more consistent.- For unresolved values, you currently repeat the stream lookup on every call; if dynamic provider registration after first use is not a requirement, caching “not found” results (e.g., mapping to
UNKNOWN) would avoid repeated scans.If tests already assert the current
nullsemantics for invalid/unknown grants, you may choose to leave this as-is and just rely on that coverage.Also applies to: 121-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
server-spi-private/src/main/java/org/keycloak/models/Constants.java(1 hunks)server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantType.java(4 hunks)server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java(4 hunks)services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderProvider.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderProviderFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderSpi.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/AuthorizationCodeGrantTypeFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/ClientCredentialsGrantTypeFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java(2 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/PermissionGrantTypeFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantType.java(2 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantTypeFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/RefreshTokenGrantTypeFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantType.java(2 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantTypeFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/TokenExchangeGrantTypeFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantTypeFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantTypeFactory.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractOIDCProtocolMapper.java(1 hunks)services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/StandardTokenExchangeProvider.java(2 hunks)services/src/main/resources/META-INF/services/org.keycloak.protocol.oidc.encode.TokenContextEncoderProviderFactory(1 hunks)services/src/main/resources/META-INF/services/org.keycloak.provider.Spi(1 hunks)services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java(1 hunks)testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java(6 hunks)
🔇 Additional comments (21)
services/src/main/java/org/keycloak/protocol/oidc/grants/PermissionGrantTypeFactory.java (1)
38-41: LGTM! Shortcut is unique.Verification confirms "pg" doesn't conflict with existing grant type shortcuts (te, ro, pc, cc). The implementation correctly follows the established pattern and is semantically appropriate for "permission grant".
services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractOIDCProtocolMapper.java (1)
84-87: Change is safe and compatible with all callers.The conversion from package-private instance method to public static is properly executed. All four existing callers are compatible:
- Internal callers in AbstractOIDCProtocolMapper and subclasses use unqualified calls, which work correctly with static methods
- The new external caller in DefaultTokenContextEncoderProvider correctly uses the qualified static call pattern
services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantTypeFactory.java (2)
37-38: Shortcut uniqueness verified—no issues found.The "dg" shortcut is unique across all grant type factories ("ac" for authorization code, "rt" for refresh token, "ci" for CIBA). The implementation follows the established pattern and is safe to merge.
44-48: Test coverage confirmed via integration tests and token encoding verification.The
getShortcut()method is properly tested indirectly through the device authorization grant integration tests. The constantGRANT_SHORTCUTis actively used in token encoding verification withinAssertEvents.java(line 173), which validates the method works correctly within the token context framework. This testing pattern is consistent across other grant type factories likeAuthorizationCodeGrantTypeFactoryandTokenExchangeGrantTypeFactory, which follow the same design: direct integration testing of the grant flow rather than isolated unit tests of simple getter methods.services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/StandardTokenExchangeProvider.java (2)
50-50: Import addition has no behavioral impactThe added
TokenExchangeGrantTypeFactoryimport doesn’t affect runtime behavior in this class as-is. No concerns here.
236-240: Review comment verified with findings on cross-flow consistencyThe reviewed code correctly sets
Constants.GRANT_TYPEtoOAuth2Constants.TOKEN_EXCHANGE_GRANT_TYPEat line 237 before the token response builder is invoked. This timing is appropriate.However, verification found that other token exchange provider implementations do not consistently set the grant type:
- StandardTokenExchangeProvider (reviewed): Sets TOKEN_EXCHANGE_GRANT_TYPE ✓
- FederatedTokenExchangeProvider, SubjectImpersonationTokenExchangeProvider, V1TokenExchangeProvider: Do not override
exchangeClientToOIDCClient()and rely onAbstractTokenExchangeProvider's implementation, which does not set any GRANT_TYPE attribute.The inconsistency across token exchange flows suggests these other providers may also need explicit grant type handling, but determining whether they should set TOKEN_EXCHANGE_GRANT_TYPE or use a different grant type value requires manual verification of their intended behavior and specifications.
services/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantType.java (1)
34-34: LGTM! Grant type propagation implemented correctly.The addition of the Constants import and setting the grant type in the client session context follows the established pattern across grant types in this PR. This enables downstream token encoding to access grant type metadata.
Also applies to: 134-134
services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantType.java (1)
32-32: LGTM! Consistent grant type propagation.The import and session context attribute setting align with the broader pattern established across grant types in this PR.
Also applies to: 82-82
services/src/main/java/org/keycloak/protocol/oidc/grants/AuthorizationCodeGrantTypeFactory.java (1)
32-32: LGTM! Good use of constant for the shortcut.Defining
GRANT_SHORTCUTas a constant improves maintainability and makes the shortcut value easily discoverable. This pattern is preferable to inline string literals.Also applies to: 39-42
services/src/main/java/org/keycloak/protocol/oidc/grants/RefreshTokenGrantTypeFactory.java (1)
33-33: LGTM! Consistent constant pattern.The use of
GRANT_SHORTCUTconstant aligns with best practices and maintains consistency withAuthorizationCodeGrantTypeFactory.Also applies to: 40-43
services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java (2)
245-253: Refresh-token flow now tags client session with GRANT_TYPESetting
clientSessionCtx.setAttribute(Constants.GRANT_TYPE, OAuth2Constants.REFRESH_TOKEN);ensures the encoder and any downstream logic can distinguish tokens minted via refresh. This matches the new context-encoding design and looks correct.
867-872: Audience restriction uses client IDs derived from session attributeUsing
Constants.REQUESTED_AUDIENCE_CLIENTSto retrieveClientModel[]and passing theirclientIds intorestrictRequestedAudiencealigns with how the same attribute is populated when building refresh tokens. Semantics and null-guard are fine here.services/src/main/resources/META-INF/services/org.keycloak.provider.Spi (1)
24-31: SPI registration forTokenContextEncoderSpilooks correctAdding
org.keycloak.protocol.oidc.encode.TokenContextEncoderSpito the SPI service list is consistent with how other SPIs are exposed and is required for the new encoder to be discoverable.server-spi-private/src/main/java/org/keycloak/models/Constants.java (1)
202-208: NewGRANT_TYPEconstant is consistent with existing constantsExposing
GRANT_TYPEviaConstantsand mapping it toOAuth2Constants.GRANT_TYPEmatches how other cross-cutting keys are handled (e.g.,REQUESTED_AUDIENCE,REUSE_ID) and keeps session-note keys centralized.services/src/main/resources/META-INF/services/org.keycloak.protocol.oidc.encode.TokenContextEncoderProviderFactory (1)
1-20: ServiceLoader registration for encoder factory is correctThe META-INF/services entry for
org.keycloak.protocol.oidc.encode.TokenContextEncoderProviderFactorycorrectly points atDefaultTokenContextEncoderProviderFactory, enabling the SPI to be instantiated at runtime.testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java (3)
33-37: Grant-type factory imports for shortcut-based token ID assertionsPulling in the various
*GrantTypeFactoryclasses to access theirGRANT_SHORTCUTconstants is a good way to keep the test expectations aligned with the encoding scheme instead of hard-coding shortcuts.
142-188: Event expectations correctly updated to validate grant-aware token IDsUsing
isAccessTokenId(…GRANT_SHORTCUT)inexpectCodeToToken,expectDeviceCodeToToken, andexpectRefreshensures tests assert the new prefixed token ID format rather than plain UUIDs. This keeps tests coupled to the encoding behavior in a controlled way.
241-248: CIBA authreqid-to-token expectation aligned with new token ID formatThe
expectAuthReqIdToTokenexpectation now checksDetails.TOKEN_IDviaisAccessTokenId(CibaGrantTypeFactory.GRANT_SHORTCUT), matching how CIBA-issued tokens should be encoded. This is consistent with other grant tests.services/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderProviderFactory.java (1)
27-28: Factory interface shape looks correctInterface cleanly extends
ProviderFactory<TokenContextEncoderProvider>and fits the SPI pattern; no changes needed.services/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderSpi.java (1)
31-49: SPI wiring looks consistent
isInternal(),getName(),getProviderClass(), andgetProviderFactoryClass()are all set appropriately for this SPI; no issues spotted.services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java (1)
44-118: Encoding/decoding logic is coherent and defensiveSession type, token type, and grant type are derived consistently, unknown/legacy tokens fall back to
UNKNOWNtypes, and encoded IDs are validated for prefix length and known shortcuts before decoding.encodeTokenIdalso guards against unknown session/token types and unmapped grants, which should help surface misconfigurations early.No changes needed here from a logic perspective.
| /** | ||
| * @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. | ||
| */ | ||
| String getShortcut(); |
There was a problem hiding this comment.
Fix JavaDoc to reflect actual shortcut length.
The JavaDoc states "usually like 3-letters shortcut," but all implementations in this PR use 2-letter shortcuts ("cc", "pc", "ro", "ac", "rt", etc.). Update the documentation to accurately reflect the actual usage pattern.
Apply this diff to correct the JavaDoc:
/**
- * @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 a short (typically 2-letter) identifier for the grant type. Useful when character count must be limited (e.g., in tokens) and the full grant name
* is not ideal. Shortcut should be unique across grants.
*/
String getShortcut();🤖 Prompt for AI Agents
In
server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java
around lines 29 to 33, the JavaDoc incorrectly states "usually like 3-letters
shortcut" while implementations use 2-letter shortcuts; update the JavaDoc to
state that shortcuts are usually 2 letters (or a short 1–3 character code) and
keep the note that the shortcut should be unique across grants and suitable for
token size constraints.
| 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.
Fix null-check bug for rawTokenId in constructor
The constructor currently checks grantType twice:
Objects.requireNonNull(grantType, "Null grantType not allowed");
Objects.requireNonNull(grantType, "Null rawTokenId not allowed");so rawTokenId is never validated and the second error message is misleading. This can allow a null rawTokenId to slip through.
Suggested fix:
- Objects.requireNonNull(grantType, "Null grantType not allowed");
- Objects.requireNonNull(grantType, "Null rawTokenId not allowed");
+ Objects.requireNonNull(grantType, "Null grantType not allowed");
+ Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed");This aligns the checks with the field semantics.
🤖 Prompt for AI Agents
In
services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java
around lines 69 to 77, the constructor performs
Objects.requireNonNull(grantType, ...) twice so rawTokenId is never validated
and the error message is wrong; replace the second requireNonNull call to
validate rawTokenId (Objects.requireNonNull(rawTokenId, "Null rawTokenId not
allowed")) so each parameter is checked with the correct variable and message.
| 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.
isAccessTokenId has inverted logic and can never match valid IDs
The helper currently reads:
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]);
}
...
};
}Issues:
- The
ifcondition is inverted: it returnsfalsewhen the prefix matches the expected shortcut, so valid token IDs will always fail. - There is no length guard before
substring(3, 5), which can throwStringIndexOutOfBoundsExceptionfor malformed or too-short IDs (e.g., in debugging or future format changes).
Suggested 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;
- return isUUID().matches(items[1]);
- }
-
- @Override
- public void describeTo(Description description) {
- description.appendText("Not a Token ID with expected grant: " + expectedGrantShortcut);
- }
- };
- }
+ 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 4th char (index 3) and is 2 chars long
+ String prefix = items[0];
+ if (prefix.length() < 5) {
+ return false;
+ }
+ if (!prefix.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);
+ }
+ };
+ }This makes the matcher accept only IDs with the expected shortcut at the correct position and a UUID-like suffix, and fail gracefully on malformed prefixes.
🤖 Prompt for AI Agents
In
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java
around lines 476-492, the matcher currently returns false when the grant
shortcut matches and also calls substring(3,5) without checking length; change
the logic so that it only returns true when the prefix contains at least 5
characters and the substring at indices 3-5 equals expectedGrantShortcut,
otherwise return false, then validate the suffix with isUUID(); in short, add a
length guard for items[0] (e.g. items[0].length() >= 5), invert the equality
check so a match proceeds, and keep failing gracefully on malformed input before
calling substring.
2 participants
Test 8
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/keycloak-coderabbit#8