Automated Test: feature-token-context-implementation #310
Conversation
closes #37118 Signed-off-by: mposolda <mposolda@gmail.com>
📝 WalkthroughWalkthroughIntroduces a token context encoding system for OIDC that encodes session type, token type, and grant type into compact token IDs. Adds provider interfaces, default implementation, SPI registration, and updates grant types with shortcut identifiers. TokenManager now uses the encoder to generate context-aware token IDs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (2 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/src/main/java/org/keycloak/protocol/oidc/grants/TokenExchangeGrantTypeFactory.java (1)
28-29:⚠️ Potential issue | 🟡 MinorStale Javadoc: says "Authorization Code Grant" but this is the Token Exchange factory.
This looks like a leftover from copy-paste.
Proposed fix
-/** - * Factory for OAuth 2.0 Authorization Code Grant +/** + * Factory for OAuth 2.0 Token Exchange Grantserver-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantType.java (1)
85-102:⚠️ Potential issue | 🟡 Minor
grantTypebecomes stale ifsetFormParams()is called later.
grantTypeis initialized fromformParamsin the constructor (line 102), butsetFormParams()(line 105) replacesformParamswithout updatinggrantType. AlthoughsetFormParams()is not currently called in the codebase, if invoked with new form params containing a differentgrant_typevalue,getGrantType()would silently return the original value.🛡️ Proposed fix — keep grantType in sync
public void setFormParams(MultivaluedHashMap<String, String> formParams) { this.formParams = formParams; + this.grantType = formParams.getFirst(OAuth2Constants.GRANT_TYPE); }
🤖 Fix all issues with AI agents
In
`@server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java`:
- Around line 29-33: Update the Javadoc on OAuth2GrantTypeFactory#getShortcut()
to state that shortcuts are 2-letter (not 3-letter) identifiers and must be
unique across grants; mention examples such as "cc", "te", "ci", etc., and keep
the contract that this method returns a short (2-character) shortcut distinct
from getId().
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 never null-checks rawTokenId;
replace the duplicate Objects.requireNonNull(grantType, "Null grantType not
allowed") with Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed")
so rawTokenId is validated before assignment to this.rawTokenId (ensure the four
requireNonNull calls validate sessionType, tokenType, grantType, and rawTokenId
respectively).
In
`@services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantTypeFactory.java`:
- Around line 58-61: Add a public static final String GRANT_SHORTCUT = "pc" to
PreAuthorizedCodeGrantTypeFactory and update getShortcut() to return that
constant instead of the hardcoded literal; specifically, declare GRANT_SHORTCUT
in the PreAuthorizedCodeGrantTypeFactory class and change the getShortcut()
implementation to return GRANT_SHORTCUT so it matches the pattern used by other
factories.
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java`:
- Around line 476-492: The matcher isAccessTokenId has two bugs: the
grant-shortcut comparison is inverted and uses wrong substring indices; update
the condition inside matchesSafely in isAccessTokenId to check
!items[0].substring(4, 6).equals(expectedGrantShortcut) (i.e., use
substring(4,6) for the grant shortcut and negate the equals), so it returns
false only when the shortcut does not match, then continue to validate the UUID
with isUUID().matches(items[1]).
🧹 Nitpick comments (6)
services/src/main/java/org/keycloak/protocol/oidc/grants/PermissionGrantTypeFactory.java (1)
38-41: Inconsistent: use aGRANT_SHORTCUTconstant for the shortcut value.Other factories in this PR (e.g.,
CibaGrantTypeFactory) definepublic static final String GRANT_SHORTCUT = "ci"and reference it fromgetShortcut(). This factory returns a string literal directly. Using a constant enables test code and encoding logic to reference the shortcut symbolically (e.g.,PermissionGrantTypeFactory.GRANT_SHORTCUT).Proposed fix
public class PermissionGrantTypeFactory implements OAuth2GrantTypeFactory { + public static final String GRANT_SHORTCUT = "pg"; + `@Override` public String getId() { return OAuth2Constants.UMA_GRANT_TYPE; } `@Override` public String getShortcut() { - return "pg"; + return GRANT_SHORTCUT; }services/src/main/java/org/keycloak/protocol/oidc/grants/TokenExchangeGrantTypeFactory.java (1)
40-43: Same as other factories: extract aGRANT_SHORTCUTconstant for consistency.See the comment on
PermissionGrantTypeFactory— the same pattern applies here.Proposed fix
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: Same pattern: extract aGRANT_SHORTCUTconstant.Consistent with
CibaGrantTypeFactoryand the recommendations on the other factories.Proposed fix
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"cc"to apublic static finalconstant for consistency.This factory currently returns a string literal while other similar factories (AuthorizationCodeGrantTypeFactory, RefreshTokenGrantTypeFactory, CibaGrantTypeFactory, DeviceGrantTypeFactory) define a
GRANT_SHORTCUTconstant. The constant pattern is particularly useful since tests reference these shortcuts (e.g., AssertEvents.java uses them for token assertions).♻️ 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 (1)
53-62:Assert.assertEqualsparameter order is swapped throughout.JUnit's
assertEqualsconvention is(expected, actual). All assertions in this class have them reversed (e.g.,Assert.assertEquals(ctx.getSessionType(), AccessTokenContext.SessionType.TRANSIENT)putsactualfirst). This won't break tests but produces confusing failure messages like "expected TRANSIENT but was ONLINE" instead of "expected ONLINE but was TRANSIENT".♻️ Example fix for one test (apply same pattern to all)
- 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)
41-42: Package-private fieldsgrantsByShortcutsandgrantsToShortcutsare directly mutated by tests.These fields are accessed and overwritten in
DefaultTokenContextEncoderProviderTest.before(). While functional for same-package tests, this tightly couples tests to internal state. Consider adding package-private setter methods or a test-only constructor to make the intent clearer.
| /** | ||
| * @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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find implementations of OAuth2GrantTypeFactory
rg "implements OAuth2GrantTypeFactory" -A 10 --max-count=20Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 12172
🏁 Script executed:
#!/bin/bash
# Find all shortcut implementations to verify 2-letter vs 3-letter claim
rg "getShortcut\(\)" -A 2 -B 2 | head -50Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 6162
🏁 Script executed:
#!/bin/bash
# Check the full interface definition to understand getId() method
cat -n server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 1599
🏁 Script executed:
#!/bin/bash
# Check PreAuthorizedCodeGrantTypeFactory for getShortcut implementation
rg "class PreAuthorizedCodeGrantTypeFactory" -A 30 | grep -A 5 "getShortcut"Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 435
🏁 Script executed:
#!/bin/bash
# Verify getId() implementation to understand why the proposed fix is wrong
rg "getId\(\)" services/src/main/java/org/keycloak/protocol/oidc/grants/ -A 1 | head -30Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 3350
🏁 Script executed:
#!/bin/bash
# Check if there are any comments about Private SPI backward compatibility
rg -i "private.*spi|spi.*private" server-spi-private/ | head -10Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 415
Update Javadoc to reflect 2-letter shortcut length, not 3-letter.
The documentation states "3-letters shortcut" but all 10 implementations use exactly 2-letter shortcuts (e.g., "cc", "te", "ci", "dg", "ro", "rt", "pc", "pg", "ac"). Update the Javadoc to say "2-letter shortcut" for accuracy.
Note: The concern about adding a method without a default is not applicable here—this is a Private SPI where backward compatibility for external implementations is not guaranteed, and all bundled implementations already include the getShortcut() method. The proposed default implementation of returning getId() is incorrect, as getId() returns full grant type names (not 2-letter shortcuts) and would violate the shortcut contract.
🤖 Prompt for AI Agents
In
`@server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java`
around lines 29 - 33, Update the Javadoc on OAuth2GrantTypeFactory#getShortcut()
to state that shortcuts are 2-letter (not 3-letter) identifiers and must be
unique across grants; mention examples such as "cc", "te", "ci", etc., and keep
the contract that this method returns a short (2-character) shortcut distinct
from getId().
| 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.
Copy-paste bug: rawTokenId is never null-checked.
Line 73 validates grantType a second time instead of rawTokenId. A null rawTokenId will be silently stored and cause NPEs later (e.g., when encoding the token ID).
🐛 Proposed fix
Objects.requireNonNull(grantType, "Null grantType not allowed");
- Objects.requireNonNull(grantType, "Null rawTokenId not allowed");
+ Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed");🤖 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 never null-checks rawTokenId;
replace the duplicate Objects.requireNonNull(grantType, "Null grantType not
allowed") with Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed")
so rawTokenId is validated before assignment to this.rawTokenId (ensure the four
requireNonNull calls validate sessionType, tokenType, grantType, and rawTokenId
respectively).
| @Override | ||
| public String getShortcut() { | ||
| return "pc"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'GRANT_SHORTCUT' --type=java -C2Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 8356
🏁 Script executed:
cat -n services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantTypeFactory.java | head -70Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2548
Add GRANT_SHORTCUT constant for consistency with other grant type factories.
All other grant type factories—AuthorizationCodeGrantTypeFactory, DeviceGrantTypeFactory, RefreshTokenGrantTypeFactory, and CibaGrantTypeFactory—expose their shortcuts via a public static final String GRANT_SHORTCUT constant. This factory returns a hardcoded string literal "pc" instead, breaking the pattern. Test code in AssertEvents.java directly references these constants, confirming that external code depends on them.
♻️ Proposed fix
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;
}📝 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.
| @Override | |
| public String getShortcut() { | |
| return "pc"; | |
| } | |
| 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 GRANT_SHORTCUT; | |
| } |
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantTypeFactory.java`
around lines 58 - 61, Add a public static final String GRANT_SHORTCUT = "pc" to
PreAuthorizedCodeGrantTypeFactory and update getShortcut() to return that
constant instead of the hardcoded literal; specifically, declare GRANT_SHORTCUT
in the PreAuthorizedCodeGrantTypeFactory class and change the getShortcut()
implementation to return GRANT_SHORTCUT so it matches the pattern used by other
factories.
| 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.
Two bugs in isAccessTokenId: inverted condition and wrong substring indices.
-
Inverted logic (Line 483):
if (items[0].substring(3, 5).equals(expectedGrantShortcut)) return false;returnsfalsewhen the shortcut matches. Should be!equals. -
Wrong substring range (Line 483): The encoded context prefix is
<session:2><token:2><grant:2>, so the grant type shortcut is at indices 4–5, not 3–4. Should besubstring(4, 6).
As written, this matcher will always fail on correctly encoded token IDs (the equals check incorrectly rejects matches), and the wrong offset means it's comparing the wrong characters entirely.
🐛 Proposed fix
- // 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;🤖 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 two bugs: the
grant-shortcut comparison is inverted and uses wrong substring indices; update
the condition inside matchesSafely in isAccessTokenId to check
!items[0].substring(4, 6).equals(expectedGrantShortcut) (i.e., use
substring(4,6) for the grant shortcut and negate the equals), so it returns
false only when the shortcut does not match, then continue to validate the UUID
with isUUID().matches(items[1]).
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Chores