Keycloak / PR #28

Implement access token context encoding framework

keycloak/keycloak#37634
Golden recall: 100% Extra findings: 12

Golden Comments (4/4 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Wrong parameter in null check (grantType vs. rawTokenId) HIGH ✓ Found "The AccessTokenContext constructor contains a copy-paste bug: it calls Objects.requireNonNull(grantType, ...) twice, leaving rawTokenId unvalidated. A null raw token ID can therefore flow into the encoder" (oidc/encode/AccessTokenContext.java L68)
#2 In isAccessTokenId, the substring for the grant shortcut and the equality check look inverted: the grant shortcut occupies indices 4–5 (substring(4,6)), and a match should return true (combined with UUID check), not false. HIGH ✓ Found "The new isAccessTokenId matcher is logically broken: it extracts substring(3,5) instead of substring(4,6) (the grant-type segment) and inverts the equality check. The two bugs cancel each other out, so the grant-type field is never actually validated and wrong-grant tokens still pass" (AssertEvents.java L483)
#3 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. LOW ✓ Found "The Javadoc for getShortcut() still says 'usually like 3-letters shortcut', contradicting the fixed 6-char encoding format which requires exactly 2-char segments. Update the Javadoc to state the shortcut must be exactly 2 characters and document the uniqueness constraint" (grants/OAuth2GrantTypeFactory.java L33)
#4 Catching generic RuntimeException is too broad. The implementation throws IllegalArgumentException specifically — catch that instead for more precise testing. LOW ✓ Found "testIncorrectGrantType catches any RuntimeException and ignores it, passing regardless of which exception is thrown or why — the Assert.fail only fires if no exception at all is thrown. Use assertThrows with a precise exception type so regressions fail instead of being swallowed" (encode/DefaultTokenContextEncoderProviderTest.java L58)

Supplementary Findings (12 findings)

Finding File Severity Legitimate?
Access-token issuance now unconditionally calls session.getProvider(TokenContextEncoderProvider.class) and encoder.encodeTokenId(...) — a missing provider would NPE, and encodeTokenId throws IllegalStateException for unknown session/token/grant types, breaking token issuance entirely with no fallback to the raw ID TokenManager.java:1050 MEDIUM ✓ Real: issuance hard-depends on the encoder with no fallback; a bad provider breaks every access token
The bidirectional grant maps (grantsByShortcuts and grantsToShortcuts) are updated in two separate put calls without locking — concurrent lookups can observe a torn state, and the dynamic refresh path does not validate shortcut uniqueness, so a colliding shortcut can silently diverge the two maps DefaultTokenContextEncoderProviderFactory.java:105 MEDIUM ✓ Real: non-atomic pair update can break bidirectionality under concurrency
encodeTokenId concatenates the grant shortcut without enforcing the 2-char contract, while the decoder hard-codes a 6-char context — a custom grant factory returning a 3-char shortcut (as the OAuth2GrantTypeFactory Javadoc invites) would produce a token ID the same provider cannot decode DefaultTokenContextEncoderProvider.java:109 MEDIUM ✓ Real: encode/decode contract mismatch with a concrete failure mode
Constants.GRANT_TYPE is set only for grants routed through createTokenResponse — ClientCredentialsGrantType and PermissionGrantType build tokens directly and never set it, so real client-credentials/UMA tokens are encoded with the unknown na shortcut instead of the registered cc/pg OAuth2GrantTypeBase.java:109 MEDIUM ✓ Real: real tokens carry a meaningless shortcut, defeating the PR's purpose
Internal access-token creators that bypass the OAuth2 grant flows (KeycloakIdentity, PolicyEvaluationService) never set Constants.GRANT_TYPE, so their regenerated tokens get the unknown na grant shortcut TokenManager.java:1050 MEDIUM ✓ Real: same silent degradation on internal token regeneration
grantType is captured once in the constructor and never recomputed by setFormParams(...) — grants that mutate form params after construction or reuse a context from another grant type get a stale/wrong value written into the token OAuth2GrantType.java:103 MEDIUM ✓ Borderline: depends on post-construction form-param mutation, plausible in UMA flows
The unit test bypasses postInit and directly reassigns the package-private grantsByShortcuts/grantsToShortcuts fields with hand-built hash maps — it never exercises the production initialization, duplicate-shortcut validation, dynamic refresh or session-factory-backed lookups DefaultTokenContextEncoderProviderTest.java:38 MEDIUM ✓ Real: the thread-safety and bidirectionality behaviors the MR introduces are untested
getTokenContextFromTokenId returns an UNKNOWN context for any string without a colon — malformed/truncated new-format ids, empty strings and arbitrary garbage are silently accepted as legacy — and it NPEs on a null input DefaultTokenContextEncoderProvider.java:66 LOW ✓ Real: the legacy-path guard is much too lax
Decode and encode throw different unchecked exception types for analogous unknown-value failures: IllegalArgumentException on decode vs IllegalStateException on encode DefaultTokenContextEncoderProvider.java:110 LOW ✓ Borderline: stylistic, but confusing in a single round-trip API
The tests do not cover the enumerated edge cases: malformed token IDs with colon but context length != 6, unknown session-type and token-type shortcuts, the encode-side grant-not-found IllegalStateException, and getTokenContextFromClientSessionContext mapping DefaultTokenContextEncoderProviderTest.java:49 LOW ✓ Real: listed parse/encode branches are untested
The public Context(Context context) copy-constructor was removed when adding the grantType field — dead code in-repo, but deleting a public constructor from a server-spi-private interface is a minor API break for out-of-tree OAuth2GrantType implementations OAuth2GrantType.java:93 LOW ✓ Borderline: dead code in-repo, API break for out-of-tree implementations
getShouldUseLightweightToken is now a single source of truth, but the encoded lt shortcut can derive from the client/session policy attribute while AuthenticationManager.resolveLightweightAccessTokenRoles reads only the client attribute — a token can say "lightweight" while the server treats it as regular AbstractOIDCProtocolMapper.java:84 LOW ✓ Borderline: consumer-side inconsistency, latent rather than manifest

Honest Analysis

FriendlyReviewer found all four golden bugs. For the two HIGH-severity ones it goes beyond the reference comments: the null-check finding names the exact defect — Objects.requireNonNull(grantType, ...) is called twice, rawTokenId is never validated — and its consequence (a null raw token ID flowing into the encoder); the matcher finding shows that the wrong substring window and the inverted equality cancel each other out, which is the real impact: the grant-type assertion validates nothing and the test suite is silently weakened.

The two LOW-severity goldens are also found at their exact lines: the Javadoc's "3-letters shortcut" contradicts the fixed 6-char encoding format, and the over-broad catch (RuntimeException) in testIncorrectGrantType swallows the failure reason — the Assert.fail only fires when no exception at all is thrown, so the test can pass for the wrong reason.

The twelve supplementary findings circle one theme: the encoded grant information is unreliable for real traffic. Client-credentials and UMA grants, plus internal creators (KeycloakIdentity, PolicyEvaluationService), all produce the unknown na shortcut, and the matcher meant to verify the encoding is broken. The strongest extra is the hard dependency of token issuance on the encoder — a missing provider or a thrown exception breaks every access-token issuance with no fallback to the raw ID. The weakest are stylistic, like the encode/decode exception asymmetry, and the review itself buckets them accordingly.

The caveats are worth stating. The two canceling bugs in AssertEvents are established by reading the code rather than by running the matcher, and the stale-grantType finding (OAuth2GrantType.java L103) is the most speculative of the twelve, resting on flows that mutate form params after construction. The feature-level checklist stays consistent with the findings: "grant type is encoded" is marked partially covered because of the na gap, which is at odds with the PR description's framing but matches the review's own evidence.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech 🌤️ Feat
2 high, 7 medium, 7 low 3 ok, 4 mitigated

The MR introduces a token-context encoding SPI and encodes session type, token type and grant type into access-token IDs using a fixed 6-character prefix (st+tt+gt) followed by the raw UUID. The overall design is sound: the encoding is correctly limited to access tokens (refresh and ID tokens keep raw UUIDs), the SPI/factory registration follows Keycloak conventions, backward compatibility for legacy raw-UUID IDs is preserved, and the lightweight-token decision is consistently derived from a single shared source. However, several blocking defects must be resolved before merge. The AccessTokenContext constructor contains a copy-paste bug that never null-checks rawTokenId. The new AssertEvents.isAccessTokenId matcher uses the wrong substring window and inverted boolean logic, so the grant-type assertion effectively never validates anything, which also weakens the test suite. Token issuance now hard-depends on the encoder with no fallback (a missing provider or a thrown exception would break all access-token issuance). In addition, there are significant robustness gaps: the factory's bidirectional grant-map refresh is not thread-safe and can silently corrupt bidirectionality; encodeTokenId does not enforce the 2-char grant-shortcut contract; client-credentials and UMA grants, as well as internal token-creators (KeycloakIdentity, PolicyEvaluationService), do not propagate Constants.GRANT_TYPE, so those tokens are encoded with the unknown na shortcut; and the affected tests bypass postInit and cover only part of the branch/edge-case matrix. These issues should be fixed and the test coverage expanded before the MR is merged.

These might need a close look

  • 🔴 org/keycloak/protocol/oidc/encode/AccessTokenContext.java (L68)
    Copy-paste bug: the constructor calls Objects.requireNonNull(grantType, ...) twice, leaving rawTokenId unvalidated. Line 68 should validate rawTokenId instead of duplicating the grantType check, otherwise a null raw token ID can flow into the encoder.
  • 🔴 org/keycloak/testsuite/AssertEvents.java (L483)
    The new isAccessTokenId matcher is logically broken: it extracts substring(3,5) instead of substring(4,6) (grant-type segment) and inverts the equality check (if (X.equals(expected)) return false). The two bugs cancel each other out, so the grant-type field is never actually validated and wrong-grant tokens still pass. Fix the window and the negation (if (!items[0].substring(4,6).equals(expectedGrantShortcut)) return false;).

Worth checking

  • 🟡 org/keycloak/protocol/oidc/TokenManager.java (L1050)
    Access-token issuance now unconditionally calls session.getProvider(TokenContextEncoderProvider.class) and encoder.encodeTokenId(...). A missing provider would NPE at getTokenContextFromClientSessionContext, and encodeTokenId throws IllegalStateException for unknown session/token/grant types, breaking token issuance entirely. Since the encoding is informational, guard the encoder call and fall back to the raw ID, or at least add a null-check on the provider.
  • 🟡 org/keycloak/protocol/oidc/DefaultTokenContextEncoderProviderFactory.java (L105)
    The bidirectional grant maps (grantsByShortcuts and grantsToShortcuts) are updated in two separate put calls without locking/atomicity. Concurrent lookups can observe a torn state, and the dynamic refresh path does not validate shortcut uniqueness (unlike postInit), so a colliding shortcut can silently diverge the two maps and break the inverse mapping. Use putIfAbsent + verification or synchronize the pair and re-validate the invariant on every mutation.
  • 🟡 org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java (L109)
    encodeTokenId concatenates factory.getShortcutByGrantType(...) without enforcing the 2-char shortcut contract, while the decoder hard-codes a 6-char context (encodedContext.length() != 6 throws). A custom grant factory returning a 3-char shortcut (as the OAuth2GrantTypeFactory Javadoc invites) would produce a token ID the same provider cannot decode. Validate grantShort.length() == 2 at encode time and correct the Javadoc to "2-letters".
  • 🟡 org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.java (L109)
    Constants.GRANT_TYPE is set only for grants routed through createTokenResponse (authorization_code, device, CIBA). ClientCredentialsGrantType and PermissionGrantType build tokens directly via generateAccessToken() / AuthorizationTokenService and never set the attribute, so real client-credentials/UMA tokens are encoded with the unknown na shortcut instead of the registered cc/pg. Propagate the grant type in these flows so the encoded ID carries meaningful grant information.
  • 🟡 org/keycloak/protocol/oidc/TokenManager.java (L1050)
    Internal access-token creators that bypass the OAuth2 grant flows (KeycloakIdentity, PolicyEvaluationService) never set Constants.GRANT_TYPE, so their regenerated tokens get the unknown na grant shortcut. This silently degrades the info the PR aims to provide. Consider setting the attribute explicitly in these callers or documenting the expected na behavior.
  • 🟡 org/keycloak/protocol/oidc/grants/OAuth2GrantType.java (L103)
    grantType is captured once in the constructor from formParams.getFirst(OAuth2Constants.GRANT_TYPE) and is not recomputed by setFormParams(...). For grants that mutate form params after construction or reuse a context from another grant type (e.g., PermissionGrantType re-invoking client-credentials with the UMA context), context.getGrantType() can return a stale/wrong value that is written into the token. Derive the grant type from the grant factory's own getId() or recompute it in setFormParams.
  • 🟡 org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (L38)
    The unit test bypasses postInit and directly reassigns the package-private grantsByShortcuts/grantsToShortcuts fields with hand-built hash maps. It never exercises the production initialization (ConcurrentHashMap selection, factory-stream population, duplicate-shortcut validation, dynamic refresh, session-factory-backed lookups), so the thread-safety and bidirectionality behaviors the MR introduces are untested. Drive the factory through postInit with a real/embedded KeycloakSessionFactory and add a concurrency test for the bidirectional pair.
Small things (take or leave)
  • 🔵 org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java (L66)
    getTokenContextFromTokenId returns an UNKNOWN context for any string without a colon, including malformed/truncated new-format ids, empty strings, and arbitrary garbage, and it NPEs on a null input. Only colon-present malformed ids are rejected. Consider treating a no-colon string as legacy only when it plausibly matches the legacy UUID format, and add a null guard.
  • 🔵 org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java (L110)
    Decode and encode throw different unchecked exception types for analogous unknown-value failures: decoding unknown/unresolvable values throws IllegalArgumentException, while encoding unknown session/token/grant types throws IllegalStateException. For a single round-trip API, pick one exception type (or a shared dedicated exception) and use it consistently in both directions.
  • 🔵 org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (L49)
    The tests do not cover several edge cases enumerated in the task: malformed token IDs with colon but context length != 6, unknown session-type and token-type shortcuts, the encode-side grant-not-found IllegalStateException, token id with : and empty raw id, and getTokenContextFromClientSessionContext mapping. Add cases for each to exercise the parse/encode branches.
  • 🔵 org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (L58)
    testIncorrectGrantType catches any RuntimeException and ignores it, passing regardless of which exception is thrown or why (the Assert.fail only fires if no exception at all is thrown). Same for the catch (IllegalStateException expected) in testOldToken. Use assertThrows/ExpectedException with a type check and assert the message/field so regressions fail rather than being swallowed.
  • 🔵 org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java (L33)
    The Javadoc for getShortcut() still says "usually like 3-letters shortcut", contradicting the fixed 6-char encoding format used by DefaultTokenContextEncoderProvider which requires exactly 2-char segments. Update the Javadoc to state the shortcut must be exactly 2 characters and document the uniqueness constraint.
  • 🔵 org/keycloak/protocol/oidc/grants/OAuth2GrantType.java (L93)
    The public Context(Context context) copy-constructor was removed as part of adding the grantType field. Although no in-repo usage remains and it appears to have been dead code, deleting a public constructor from a server-spi-private interface is a minor API break for out-of-tree OAuth2GrantType implementations and is unrelated to the PR's purpose. Consider keeping it (and copying grantType) to avoid the break.
  • 🔵 org/keycloak/protocol/oidc/mappers/AbstractOIDCProtocolMapper.java (L84)
    The change to make getShouldUseLightweightToken public static is behavior-preserving and introduces a single source of truth, but note the encoded lt shortcut may be derived from the client/session policy attribute while AuthenticationManager.resolveLightweightAccessTokenRoles uses only the client attribute for role resolution. This consumer-side inconsistency can make a token say "lightweight" while the server treats it as regular. Worth reconciling so the encoded type and runtime behavior agree.

✅ Feature-level checklist

Looks good

  • ✅ Session type (online, offline, transient) is encoded in the access-token ID via a 2-char shortcut.
    Session types are covered in AccessTokenContext.SessionType and the encoder derives the shortcut from the ClientSessionContext. No blocking issues found for session type encoding/decoding.
  • ✅ Encoding is applied only to access tokens, not to refresh or ID tokens.
    Verified that refresh and ID tokens retain raw UUIDs (KeycloakModelUtils.generateId()), while only initToken (reached via createClientAccessToken) applies the encoded ID. The access-token-only contract is respected.
  • ✅ Backward compatibility for legacy (raw-UUID) access-token IDs is preserved.
    getTokenContextFromTokenId returns an UNKNOWN context for legacy colon-less IDs, and encodeTokenId refuses unknown types, preserving the ability to handle pre-existing tokens. The silent acceptance of arbitrary colon-less strings as legacy is a minor robustness gap, not a backward-compat break.

Partially covered

  • ⚠️ Token type (regular or lightweight) is encoded in the access-token ID.
    The token type is derived from the same getShouldUseLightweightToken used to decide whether claims are included, so the encoded lt/rt matches the claims present. However, there is a consumer-side inconsistency: a token marked lightweight via the session/policy attribute may still be treated as regular by role-resolution logic that only considers the client attribute. This weakens the reliability of the encoded token-type info.
  • ⚠️ Grant type is encoded in the access-token ID.
    Grant type is correctly encoded for authorization-code, device, CIBA, refresh, ROPC, token-exchange, and pre-authorized flows. However, client-credentials and UMA grants, plus internal creators such as KeycloakIdentity and PolicyEvaluationService, never set Constants.GRANT_TYPE, so their tokens are encoded with the unknown na shortcut. Additionally, the AssertEvents.isAccessTokenId matcher is broken, so the test suite does not actually validate the grant shortcut.
  • ⚠️ Shortcuts (2 chars each) are used to keep the token ID compact.
    The fixed 6-char prefix format is implemented and round-trips correctly for the in-tree 2-char shortcuts. However, encodeTokenId does not enforce the 2-char grant-shortcut contract, and the OAuth2GrantTypeFactory Javadoc still invites 3-char shortcuts, so a custom provider could produce malformed token IDs that cannot be decoded.
  • ⚠️ A dedicated SPI with provider factory is introduced.
    The TokenContextEncoderProvider, TokenContextEncoderProviderFactory, and TokenContextEncoderSpi are introduced, registered in META-INF/services, and isInternal() returns true without blocking external providers. However, the AccessTokenContext constructor has a copy-paste null-validation bug, and the default factory's bidirectional grant-map refresh is not thread-safe and can silently break bidirectionality, so the SPI implementation is not yet fully robust.