Implement access token context encoding framework

[ShashankFC]

Test 8nn---nReplicated from ai-code-review-evaluation/keycloak-greptile#8

[GitHoobar]

@claude

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 2m 7s —— View job

---

Code Review Complete

Tasks

• Read repository guidelines (CLAUDE.md)
• Review core framework classes (SPI, providers, context)
• Review grant type factory changes
• Review test coverage
• Check for security and quality issues
• Post comprehensive review feedback

---

Summary

This PR implements a new token context encoding framework that encodes metadata about access tokens (session type, token type, grant type) directly into the token ID using a compact 6-character prefix format. The implementation follows Keycloak's SPI pattern well and integrates cleanly with the existing OAuth2 grant type system.

Critical Issues

1. Bug: Incorrect null check in AccessTokenContext constructor

services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java:73

The constructor checks grantType twice but never checks rawTokenId:

Objects.requireNonNull(grantType, "Null grantType not allowed");
Objects.requireNonNull(grantType, "Null rawTokenId not allowed");  // BUG: should be rawTokenId

This is a copy-paste error. The second line should be:

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

Fix this →

2. Typo: "aucienceClients" → "audienceClients"

services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java:867-869

Variable name has a typo that was fixed in this PR:

final ClientModel[] requestedAudienceClients = clientSessionCtx.getAttribute(Constants.REQUESTED_AUDIENCE_CLIENTS, ClientModel[].class);

Good fix! This improves code readability.

High Priority Issues

3. Missing package visibility modifier on fields

services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.java:41-42

Two fields have package-private visibility (no modifier) while others are private:

Map<String, String> grantsByShortcuts;
Map<String, String> grantsToShortcuts;

These should either be:

• Made private if the test doesn't need direct access
• Or documented why package visibility is needed (e.g., for testing)

The test at DefaultTokenContextEncoderProviderTest.java:42-48 directly accesses these fields. Consider adding protected getter methods or making them explicitly protected instead of package-private for clarity.

4. Inconsistent error handling patterns

services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java:66-97

The getTokenContextFromTokenId method:

• Returns UNKNOWN types for tokens without encoded context (backward compatibility)
• But throws IllegalArgumentException for malformed encoded contexts

Consider whether this is the desired behavior. Should malformed tokens also return UNKNOWN types for more graceful degradation? Document the rationale.

Medium Priority Issues

5. Magic number in length check

services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java:74-76

The hardcoded 6 should be a named constant:

private static final int ENCODED_CONTEXT_LENGTH = 6;  // 2 chars each for sessionType, tokenType, grantType

This improves maintainability and documents the format structure.

6. Concurrent modification risk during refresh

services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.java:106-119

The getShortcutByGrantType and getGrantTypeByShortcut methods refresh the ConcurrentHashMap when an entry is not found. However, they use a check-then-act pattern that could lead to redundant work under high concurrency:

String grantShortcut = grantsToShortcuts.get(grantType);
if (grantShortcut == null) {
    OAuth2GrantTypeFactory factory = ...;
    if (factory != null) {
        String shortcut = factory.getShortcut();
        grantsByShortcuts.put(shortcut, grantType);  // Could happen multiple times
        grantsToShortcuts.put(grantType, shortcut);
    }
    grantShortcut = grantsToShortcuts.get(grantType);
}

Consider using computeIfAbsent pattern or accepting that duplicate computation is acceptable (document it). Current approach is safe but could be more efficient.

7. Test coverage gaps

services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java

The test class only covers:

• Success cases for specific grant types (client credentials, password)
• Unknown grant type handling
• Old token format (no encoded context)
• Basic error case (incorrect grant type)

Missing test coverage for:

• All session types (only tests TRANSIENT, OFFLINE, ONLINE - not all enum values)
• Edge cases: empty token IDs, tokens with multiple colons
• Encoding validation (what happens with null context fields?)
• The concurrent refresh logic in the factory

Low Priority Issues

8. Documentation: Backward compatibility not explicitly documented

The code handles old tokens (without encoded context) by returning UNKNOWN types, but this backward compatibility strategy isn't documented in the class or interface Javadoc. Add a comment explaining how old tokens are handled.

9. Potential collision with token IDs containing colons

services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java:67

The encoding format uses : as a separator between the context prefix and the raw token ID:

int indexOf = encodedTokenId.indexOf(':');

If the rawTokenId itself contains a colon (unlikely but possible depending on UUID generation), this could cause parsing issues. Consider:

• Documenting that raw token IDs must not contain colons
• Using a different separator that's guaranteed not to appear in UUIDs
• Or parsing from the other direction (last 6 chars before first colon)

Actually, looking at line 117, the format is sessionType(2) + tokenType(2) + grantShort(2) + ':' + rawTokenId, so indexOf(':') finds the first colon which is correct. But rawTokenId should still not contain colons. Document this constraint.

Positive Observations

✅ Well-structured SPI implementation - Follows Keycloak's provider pattern correctly with proper factory, provider, and SPI classes.

✅ Good separation of concerns - The encoding logic is cleanly separated from token generation logic.

✅ Efficient encoding - The 6-character prefix is compact and doesn't significantly increase token size.

✅ Dynamic grant type support - The factory can discover new grant types at runtime, making the system extensible.

✅ Proper service registration - Both META-INF service files are correctly updated.

✅ Grant type propagation - The PR properly adds Constants.GRANT_TYPE to the client session context in multiple locations (TokenManager.java:246, OAuth2GrantTypeBase.java:109).

✅ Consistent shortcut implementation - All grant type factories now implement the getShortcut() method with 2-character codes.

Recommendations

1. Fix the critical null check bug immediately (issue Fixing Re-authentication with passkeys #1)
2. Add comprehensive integration tests - Test the full flow from token generation through encoding to decoding
3. Document the encoding format - Add detailed Javadoc explaining the format: [SS][TT][GG]:[UUID] where SS=session type, TT=token type, GG=grant type
4. Consider adding metrics - Track how often the dynamic grant type refresh occurs to monitor performance
5. Add validation for duplicate shortcuts - The factory already validates in postInit (line 82), but consider adding runtime checks if new providers are hot-deployed

---