Automated Test: feature-token-context-implementation #330
+722
−26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
closes #37118 Signed-off-by: mposolda <mposolda@gmail.com>
📝 WalkthroughWalkthroughThis pull request introduces a token context encoding system that encodes session type, token type, and grant type information directly into access token IDs. It adds grant type shortcuts to all OAuth2 grant type factories, implements a provider-based token ID encoder/decoder, and integrates this into token generation and validation flows. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant TokenManager
participant TokenContextEncoderProvider
participant OAuth2GrantType
participant AccessTokenContext
Client->>TokenManager: Request Token (with grant type)
TokenManager->>OAuth2GrantType: Process grant, extract context
OAuth2GrantType->>TokenManager: Return Context with grantType
TokenManager->>TokenContextEncoderProvider: getTokenContextFromClientSessionContext<br/>(sessionContext, rawTokenId)
TokenContextEncoderProvider->>AccessTokenContext: Create with session/token/grant types
TokenContextEncoderProvider->>TokenContextEncoderProvider: encodeTokenId(context)
TokenContextEncoderProvider-->>TokenManager: Encoded ID (e.g., "trltcc:1234")
TokenManager-->>Client: Return token with encoded ID
sequenceDiagram
actor Client
participant OAuth2Validation
participant TokenContextEncoderProvider
participant AccessTokenContext
Client->>OAuth2Validation: Present token ID
OAuth2Validation->>TokenContextEncoderProvider: getTokenContextFromTokenId("trltcc:1234")
TokenContextEncoderProvider->>TokenContextEncoderProvider: Parse and validate format
TokenContextEncoderProvider->>AccessTokenContext: Resolve session/token/grant types<br/>from shortcuts
TokenContextEncoderProvider-->>OAuth2Validation: Return AccessTokenContext
OAuth2Validation->>OAuth2Validation: Use context for validation logic
OAuth2Validation-->>Client: Token validation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ 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: 5
🤖 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(SessionType sessionType,
TokenType tokenType, String grantType, String rawTokenId) has a copy-paste null
check that validates grantType twice; replace the second
Objects.requireNonNull(grantType, ...) with Objects.requireNonNull(rawTokenId,
"Null rawTokenId not allowed") so rawTokenId is validated on construction (leave
the existing checks for sessionType, tokenType, grantType intact and assign
fields as before).
In
`@services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java`:
- Around line 66-98: getTokenContextFromTokenId currently calls
encodedTokenId.indexOf(':') without validating encodedTokenId, which can throw
NPE; update DefaultTokenContextEncoderProvider.getTokenContextFromTokenId to
explicitly check if encodedTokenId is null (or blank if you prefer) and throw an
IllegalArgumentException with a clear message (e.g., "Invalid token id: null or
empty") before calling indexOf, keeping the rest of the parsing logic
(encodedContext, rawId, stShortcut, ttShortcut, gtShortcut and factory lookups)
unchanged.
In
`@services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/StandardTokenExchangeProvider.java`:
- Line 50: Remove the unused import TokenExchangeGrantTypeFactory from
StandardTokenExchangeProvider: locate the import statement "import
org.keycloak.protocol.oidc.grants.TokenExchangeGrantTypeFactory;" at the top of
the file and delete it so the file no longer contains an unused reference to
TokenExchangeGrantTypeFactory.
In
`@services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java`:
- Around line 52-62: The test assertions in
DefaultTokenContextEncoderProviderTest use JUnit's Assert.assertEquals(actual,
expected) instead of the correct Assert.assertEquals(expected, actual); update
all calls (e.g., in testSuccessClientCredentialsToken where
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") and the final
Assert.assertEquals(tokenId, provider.encodeTokenId(ctx))) to swap the argument
order so the expected value is first and the actual value (method results like
ctx.getSessionType(), ctx.getTokenType(), ctx.getGrantType(),
ctx.getRawTokenId(), provider.encodeTokenId(ctx)) is second; apply the same fix
to all other assertEquals calls in this test class.
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java`:
- Around line 476-492: The matcher in isAccessTokenId (matchesSafely) currently
rejects when the grant shortcut matches and uses a hardcoded 2-char substring;
change the logic to return false when the grant shortcut does NOT match the
expected value and make the extraction length dynamic: compute the shortcut
length from expectedGrantShortcut (ensure items[0].length() >= 3 +
expectedGrantShortcut.length() before extracting) and compare
items[0].substring(3, 3 + expectedGrantShortcut.length()) to
expectedGrantShortcut, returning false on mismatch, then proceed to check
isUUID() on items[1].
🧹 Nitpick comments (5)
server-spi-private/src/main/java/org/keycloak/models/Constants.java (1)
206-207: Consider whether the alias adds value or confusion.
Constants.GRANT_TYPEis a pure alias forOAuth2Constants.GRANT_TYPE(same string"grant_type"). While the comment clarifies the intent (a session context attribute key), callers could inadvertently use either constant interchangeably. A distinct key like"token.grant_type"would eliminate any ambiguity with the OAuth2 form parameter name, and would make it clear this is a session-context attribute, not a request parameter.That said, if the Keycloak codebase already follows this aliasing pattern elsewhere, this is consistent.
services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java (1)
53-67:TokenType.REGULARshortcut"rt"may cause confusion with "refresh token".In the context of OAuth2/OIDC, "rt" is widely understood as "refresh token." Using it here for
REGULARtoken type could be misleading to developers reading encoded token IDs. Consider a less ambiguous shortcut like"rg".services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (2)
76-85: Overly broad exception type and typo in failure message.The catch block uses
RuntimeException, butgetTokenContextFromTokenIdthrowsIllegalArgumentException. Catching the more specific type ensures the test doesn't accidentally pass if a different runtime exception is thrown. Also, minor typo: "success" → "succeed".Proposed fix
`@Test` public void testIncorrectGrantType() { try { String tokenId = "ofrtac:5678"; AccessTokenContext ctx = provider.getTokenContextFromTokenId(tokenId); - Assert.fail("Not expected to success due incorrect grant type"); - } catch (RuntimeException iae) { + Assert.fail("Not expected to succeed due to incorrect grant type"); + } catch (IllegalArgumentException iae) { // ignored }Alternatively, use
@Test(expected = IllegalArgumentException.class)to simplify:`@Test`(expected = IllegalArgumentException.class) public void testIncorrectGrantType() { provider.getTokenContextFromTokenId("ofrtac:5678"); }
38-50: Test setup directly mutates package-private factory fields.The
@Beforemethod bypasses the normalpostInitlifecycle by directly assigninggrantsByShortcutsandgrantsToShortcuts. This couples the test tightly to the factory's internal field layout. Acceptable for a unit test, but worth noting that if fields become private or the initialization logic changes, these tests will break without compiler errors.services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.java (1)
64-86: Duplicate shortcut validation inpostInitis effective but the error message could be more diagnostic.The size-comparison check on lines 82–85 correctly detects duplicate grant shortcuts or grant IDs, but when it fires, the developer must manually inspect the maps to find the collision. Logging the actual map contents (or at least the differing keys) would significantly speed up debugging.
Also, minor: the lambda parameter
factory1(line 71) shadows the method parameterfactory— consider renaming for clarity (e.g.,providerFactoryorgrantFactory).Suggested improvement for diagnostics
if (grantsByShortcuts.size() != grantsToShortcuts.size()) { - throw new IllegalStateException("Different lengths of maps. grantsByShortcuts.size=" + grantsByShortcuts.size() + ", grantsToShortcuts.size=" + grantsToShortcuts.size() + - ". Make sure that there is no OAuth2GrantType implementation with same ID or shortcut like other grants"); + throw new IllegalStateException("Different lengths of maps. grantsByShortcuts=" + grantsByShortcuts + ", grantsToShortcuts=" + grantsToShortcuts + + ". Make sure that there is no OAuth2GrantType implementation with same ID or shortcut like other grants"); }
Comment on lines
+69
to
+78
| 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 null-check is missing — Line 73 validates grantType a second time instead.
Line 73 is a copy-paste error: it checks grantType again rather than rawTokenId. As a result, a null rawTokenId will silently pass construction and cause failures downstream when the token ID is encoded.
🐛 Fix the null check
Objects.requireNonNull(grantType, "Null grantType not allowed");
- Objects.requireNonNull(grantType, "Null rawTokenId not allowed");
+ Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed");📝 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.
Suggested change
| 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(SessionType
sessionType, TokenType tokenType, String grantType, String rawTokenId) has a
copy-paste null check that validates grantType twice; replace the second
Objects.requireNonNull(grantType, ...) with Objects.requireNonNull(rawTokenId,
"Null rawTokenId not allowed") so rawTokenId is validated on construction (leave
the existing checks for sessionType, tokenType, grantType intact and assign
fields as before).
Comment on lines
+66
to
+98
| public AccessTokenContext getTokenContextFromTokenId(String encodedTokenId) { | ||
| int indexOf = encodedTokenId.indexOf(':'); | ||
| if (indexOf == -1) { | ||
| return new AccessTokenContext(AccessTokenContext.SessionType.UNKNOWN, AccessTokenContext.TokenType.UNKNOWN, UNKNOWN, encodedTokenId); | ||
| } else { | ||
| String encodedContext = encodedTokenId.substring(0, indexOf); | ||
| String rawId = encodedTokenId.substring(indexOf + 1); | ||
|
|
||
| if (encodedContext.length() != 6) { | ||
| throw new IllegalArgumentException("Incorrect token id: '" + encodedTokenId + "'. Expected length of 6."); | ||
| } | ||
|
|
||
| // First 2 chars are "sessionType", next 2 chars "tokenType", last 2 chars "grantType" | ||
| String stShortcut = encodedContext.substring(0, 2); | ||
| String ttShortcut = encodedContext.substring(2, 4); | ||
| String gtShortcut = encodedContext.substring(4, 6); | ||
|
|
||
| AccessTokenContext.SessionType st = factory.getSessionTypeByShortcut(stShortcut); | ||
| if (st == null) { | ||
| throw new IllegalArgumentException("Incorrect token id: " + encodedTokenId + ". Unknown value '" + stShortcut + "' for session type"); | ||
| } | ||
| AccessTokenContext.TokenType tt = factory.getTokenTypeByShortcut(ttShortcut); | ||
| if (tt == null) { | ||
| throw new IllegalArgumentException("Incorrect token id: " + encodedTokenId + ". Unknown value '" + ttShortcut + "' for token type"); | ||
| } | ||
| String gt = factory.getGrantTypeByShortcut(gtShortcut); | ||
| if (gt == null) { | ||
| throw new IllegalArgumentException("Incorrect token id: " + encodedTokenId + ". Unknown value '" + gtShortcut + "' for grant type"); | ||
| } | ||
|
|
||
| return new AccessTokenContext(st, tt, gt, rawId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential NPE if encodedTokenId is null.
getTokenContextFromTokenId calls encodedTokenId.indexOf(':') without a null check. If a null token ID is ever passed (e.g., from a malformed token), this throws an unhelpful NullPointerException instead of a descriptive error.
Proposed fix
`@Override`
public AccessTokenContext getTokenContextFromTokenId(String encodedTokenId) {
+ if (encodedTokenId == null) {
+ throw new IllegalArgumentException("encodedTokenId must not be null");
+ }
int indexOf = encodedTokenId.indexOf(':');📝 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.
Suggested change
| public AccessTokenContext getTokenContextFromTokenId(String encodedTokenId) { | |
| int indexOf = encodedTokenId.indexOf(':'); | |
| if (indexOf == -1) { | |
| return new AccessTokenContext(AccessTokenContext.SessionType.UNKNOWN, AccessTokenContext.TokenType.UNKNOWN, UNKNOWN, encodedTokenId); | |
| } else { | |
| String encodedContext = encodedTokenId.substring(0, indexOf); | |
| String rawId = encodedTokenId.substring(indexOf + 1); | |
| if (encodedContext.length() != 6) { | |
| throw new IllegalArgumentException("Incorrect token id: '" + encodedTokenId + "'. Expected length of 6."); | |
| } | |
| // First 2 chars are "sessionType", next 2 chars "tokenType", last 2 chars "grantType" | |
| String stShortcut = encodedContext.substring(0, 2); | |
| String ttShortcut = encodedContext.substring(2, 4); | |
| String gtShortcut = encodedContext.substring(4, 6); | |
| AccessTokenContext.SessionType st = factory.getSessionTypeByShortcut(stShortcut); | |
| if (st == null) { | |
| throw new IllegalArgumentException("Incorrect token id: " + encodedTokenId + ". Unknown value '" + stShortcut + "' for session type"); | |
| } | |
| AccessTokenContext.TokenType tt = factory.getTokenTypeByShortcut(ttShortcut); | |
| if (tt == null) { | |
| throw new IllegalArgumentException("Incorrect token id: " + encodedTokenId + ". Unknown value '" + ttShortcut + "' for token type"); | |
| } | |
| String gt = factory.getGrantTypeByShortcut(gtShortcut); | |
| if (gt == null) { | |
| throw new IllegalArgumentException("Incorrect token id: " + encodedTokenId + ". Unknown value '" + gtShortcut + "' for grant type"); | |
| } | |
| return new AccessTokenContext(st, tt, gt, rawId); | |
| } | |
| } | |
| public AccessTokenContext getTokenContextFromTokenId(String encodedTokenId) { | |
| if (encodedTokenId == null) { | |
| throw new IllegalArgumentException("encodedTokenId must not be null"); | |
| } | |
| int indexOf = encodedTokenId.indexOf(':'); | |
| if (indexOf == -1) { | |
| return new AccessTokenContext(AccessTokenContext.SessionType.UNKNOWN, AccessTokenContext.TokenType.UNKNOWN, UNKNOWN, encodedTokenId); | |
| } else { | |
| String encodedContext = encodedTokenId.substring(0, indexOf); | |
| String rawId = encodedTokenId.substring(indexOf + 1); | |
| if (encodedContext.length() != 6) { | |
| throw new IllegalArgumentException("Incorrect token id: '" + encodedTokenId + "'. Expected length of 6."); | |
| } | |
| // First 2 chars are "sessionType", next 2 chars "tokenType", last 2 chars "grantType" | |
| String stShortcut = encodedContext.substring(0, 2); | |
| String ttShortcut = encodedContext.substring(2, 4); | |
| String gtShortcut = encodedContext.substring(4, 6); | |
| AccessTokenContext.SessionType st = factory.getSessionTypeByShortcut(stShortcut); | |
| if (st == null) { | |
| throw new IllegalArgumentException("Incorrect token id: " + encodedTokenId + ". Unknown value '" + stShortcut + "' for session type"); | |
| } | |
| AccessTokenContext.TokenType tt = factory.getTokenTypeByShortcut(ttShortcut); | |
| if (tt == null) { | |
| throw new IllegalArgumentException("Incorrect token id: " + encodedTokenId + ". Unknown value '" + ttShortcut + "' for token type"); | |
| } | |
| String gt = factory.getGrantTypeByShortcut(gtShortcut); | |
| if (gt == null) { | |
| throw new IllegalArgumentException("Incorrect token id: " + encodedTokenId + ". Unknown value '" + gtShortcut + "' for grant type"); | |
| } | |
| return new AccessTokenContext(st, tt, gt, rawId); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java`
around lines 66 - 98, getTokenContextFromTokenId currently calls
encodedTokenId.indexOf(':') without validating encodedTokenId, which can throw
NPE; update DefaultTokenContextEncoderProvider.getTokenContextFromTokenId to
explicitly check if encodedTokenId is null (or blank if you prefer) and throw an
IllegalArgumentException with a clear message (e.g., "Invalid token id: null or
empty") before calling indexOf, keeping the rest of the parsing logic
(encodedContext, rawId, stShortcut, ttShortcut, gtShortcut and factory lookups)
unchanged.
| @@ -47,6 +47,7 @@ | |||
| import org.keycloak.protocol.oidc.OIDCAdvancedConfigWrapper; | |||
| import org.keycloak.protocol.oidc.TokenExchangeContext; | |||
| import org.keycloak.protocol.oidc.TokenManager; | |||
| import org.keycloak.protocol.oidc.grants.TokenExchangeGrantTypeFactory; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the file and check for TokenExchangeGrantTypeFactory usage
fd 'StandardTokenExchangeProvider.java' --exec cat -n {} | head -100Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 5370
🏁 Script executed:
#!/bin/bash
# Search for TokenExchangeGrantTypeFactory references in StandardTokenExchangeProvider.java
fd 'StandardTokenExchangeProvider.java' --exec rg -n 'TokenExchangeGrantTypeFactory' {}Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Get the full file length and search for all references
fd 'StandardTokenExchangeProvider.java' --exec wc -l {}Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 191
🏁 Script executed:
#!/bin/bash
# Check the entire file for TokenExchangeGrantTypeFactory references
fd 'StandardTokenExchangeProvider.java' --exec rg 'TokenExchangeGrantTypeFactory' {} --countRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Read the entire file to verify TokenExchangeGrantTypeFactory is not used anywhere
fd 'StandardTokenExchangeProvider.java' --exec cat {} | grep -i 'TokenExchangeGrantTypeFactory'Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 158
Remove unused import TokenExchangeGrantTypeFactory.
This import is not referenced anywhere in the file and should be removed.
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/protocol/oidc/tokenexchange/StandardTokenExchangeProvider.java`
at line 50, Remove the unused import TokenExchangeGrantTypeFactory from
StandardTokenExchangeProvider: locate the import statement "import
org.keycloak.protocol.oidc.grants.TokenExchangeGrantTypeFactory;" at the top of
the file and delete it so the file no longer contains an unused reference to
TokenExchangeGrantTypeFactory.
Comment on lines
+52
to
+62
| @Test | ||
| public void testSuccessClientCredentialsToken() { | ||
| String tokenId = "trltcc:1234"; | ||
| AccessTokenContext ctx = provider.getTokenContextFromTokenId(tokenId); | ||
| 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(tokenId, provider.encodeTokenId(ctx)); | ||
| } |
There was a problem hiding this comment.
Reversed assertEquals argument order throughout the test class.
JUnit's Assert.assertEquals signature is assertEquals(expected, actual), but all assertions in this test class pass (actual, expected). When an assertion fails, the error message will swap expected/actual, making debugging confusing.
This applies to all assertEquals calls in lines 56–59, 68–71, 91–94, and 102–105.
Example fix for testSuccessClientCredentialsToken (apply same pattern elsewhere)
- 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());📝 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.
Suggested change
| @Test | |
| public void testSuccessClientCredentialsToken() { | |
| String tokenId = "trltcc:1234"; | |
| AccessTokenContext ctx = provider.getTokenContextFromTokenId(tokenId); | |
| 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(tokenId, provider.encodeTokenId(ctx)); | |
| } | |
| `@Test` | |
| public void testSuccessClientCredentialsToken() { | |
| String tokenId = "trltcc:1234"; | |
| AccessTokenContext ctx = provider.getTokenContextFromTokenId(tokenId); | |
| 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()); | |
| Assert.assertEquals(tokenId, provider.encodeTokenId(ctx)); | |
| } |
🤖 Prompt for AI Agents
In
`@services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java`
around lines 52 - 62, The test assertions in
DefaultTokenContextEncoderProviderTest use JUnit's Assert.assertEquals(actual,
expected) instead of the correct Assert.assertEquals(expected, actual); update
all calls (e.g., in testSuccessClientCredentialsToken where
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") and the final
Assert.assertEquals(tokenId, provider.encodeTokenId(ctx))) to swap the argument
order so the expected value is first and the actual value (method results like
ctx.getSessionType(), ctx.getTokenType(), ctx.getGrantType(),
ctx.getRawTokenId(), provider.encodeTokenId(ctx)) is second; apply the same fix
to all other assertEquals calls in this test class.
Comment on lines
+476
to
+492
| 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.
Bug: Inverted condition — matcher rejects valid token IDs and accepts invalid ones.
On line 483, the condition returns false when the grant shortcut matches the expected value, which is the opposite of the intended behavior. It should return false when the shortcut does not match.
Additionally, substring(3, 5) hardcodes a 2-character shortcut length. If any grant shortcut is 3 characters (as the interface Javadoc suggests is possible), this will silently extract the wrong substring.
Proposed fix
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;
+ if (!items[0].substring(3, 5).equals(expectedGrantShortcut)) return false;
return isUUID().matches(items[1]);
}🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java`
around lines 476 - 492, The matcher in isAccessTokenId (matchesSafely) currently
rejects when the grant shortcut matches and uses a hardcoded 2-char substring;
change the logic to return false when the grant shortcut does NOT match the
expected value and make the extraction length dynamic: compute the shortcut
length from expectedGrantShortcut (ensure items[0].length() >= 3 +
expectedGrantShortcut.length() before extracting) and compare
items[0].substring(3, 3 + expectedGrantShortcut.length()) to
expectedGrantShortcut, returning false on mismatch, then proceed to check
isUUID() on items[1].
Sign in
to join this conversation on GitHub.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Refactor