Automated Test: feature-token-context-implementation #320
+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 PR introduces a token context encoding mechanism for OIDC tokens. It adds infrastructure to encode session type, token type, and grant type information into token identifiers, and modifies grant type factories to provide short-form identifiers. Grant type classes now record grant types in client session contexts, and the TokenManager integrates the new encoder provider. Changes
Sequence DiagramsequenceDiagram
participant Client
participant TokenManager
participant Encoder as TokenContextEncoder<br/>Provider
participant Factory as TokenContextEncoder<br/>Factory
participant Context as AccessTokenContext
participant Token as Token
Client->>TokenManager: initializeToken(clientSessionCtx)
TokenManager->>Factory: getTokenContextEncoderProvider()
Factory-->>TokenManager: encoder instance
TokenManager->>Encoder: getTokenContextFromClientSessionContext(clientSessionCtx, rawTokenId)
Encoder->>Factory: getSessionTypeByShortcut()
Encoder->>Factory: getTokenTypeByShortcut()
Encoder->>Factory: getShortcutByGrantType()
Factory-->>Encoder: mappings resolved
Encoder->>Context: new AccessTokenContext(sessionType, tokenType, grantType, rawTokenId)
Context-->>Encoder: context created
TokenManager->>Encoder: encodeTokenId(context)
Encoder->>Encoder: concatenate shortcodes
Encoder-->>TokenManager: encoded token id
TokenManager->>Token: setId(encodedTokenId)
TokenManager-->>Client: token with encoded context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 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: 3
🤖 Fix all issues with AI agents
In
`@services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java`:
- Around line 69-77: The null-check for rawTokenId is incorrect: in the
AccessTokenContext constructor replace the duplicate
Objects.requireNonNull(grantType, "Null rawTokenId not allowed") with a check
against rawTokenId so rawTokenId is validated (i.e., call
Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed")); keep the
other checks for sessionType, tokenType and grantType unchanged.
In `@services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java`:
- Around line 1051-1054: The code assumes TokenContextEncoderProvider is
present; add a null-check after
session.getProvider(TokenContextEncoderProvider.class) to handle a missing
provider (either throw a descriptive IllegalStateException like
"TokenContextEncoderProvider not configured" or log and abort), so that
subsequent calls to encoder.getTokenContextFromClientSessionContext(...) and
encoder.encodeTokenId(...) and token.id(...) do not cause an NPE; update the
block around the encoder variable accordingly.
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java`:
- Around line 476-492: The isAccessTokenId matcher has two bugs: it extracts the
grant shortcut from the wrong indices and inverts the match logic. Update the
substring call in isAccessTokenId to use substring(4, 6) to read the 2-char
grantType, and change the conditional so it returns false when that extracted
grant does NOT equal expectedGrantShortcut (i.e., if
(!extracted.equals(expectedGrantShortcut)) return false), leaving the UUID check
via isUUID().matches(items[1]) as-is; adjust within the anonymous
TypeSafeMatcher implementation.
🧹 Nitpick comments (5)
services/src/main/java/org/keycloak/protocol/oidc/grants/ClientCredentialsGrantTypeFactory.java (1)
38-41: Consider defining a constant for the shortcut.Other factories like
DeviceGrantTypeFactoryexpose apublic static final String GRANT_SHORTCUTconstant, allowing tests and other components to reference the shortcut without hardcoding strings. For consistency, consider:♻️ Suggested refactor
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; }server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java (1)
29-33: Consider adding a default implementation for backward compatibility.Adding
getShortcut()to an existing interface without a default implementation is a breaking change for any third-party or customOAuth2GrantTypeFactoryimplementations. Consider providing a fallback default:♻️ Suggested default implementation
/** * `@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(); + default String getShortcut() { + // Fallback to first 3 characters of grant ID, or full ID if shorter + String id = getId(); + return id != null && id.length() > 3 ? id.substring(0, 3) : id; + } }services/src/main/java/org/keycloak/protocol/oidc/grants/TokenExchangeGrantTypeFactory.java (1)
28-32: Minor: Incorrect class-level Javadoc.The Javadoc states "Factory for OAuth 2.0 Authorization Code Grant" but this class is
TokenExchangeGrantTypeFactory. This appears to be a pre-existing copy-paste issue but could be corrected while making changes to this file.📝 Suggested fix
/** - * 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> */services/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderProvider.java (1)
31-37: LGTM with suggestion: Consider adding method-level Javadoc.The interface design is clean and follows Keycloak's provider pattern. For better API clarity, consider documenting the expected behavior for edge cases:
📝 Suggested documentation enhancement
public interface TokenContextEncoderProvider extends Provider { + /** + * Creates an AccessTokenContext from the given client session context. + * + * `@param` clientSessionContext the client session context + * `@param` rawTokenId the raw token identifier to include in the context + * `@return` the token context containing session, token, and grant type information + */ AccessTokenContext getTokenContextFromClientSessionContext(ClientSessionContext clientSessionContext, String rawTokenId); + /** + * Decodes an AccessTokenContext from an encoded token ID. + * + * `@param` encodedTokenId the encoded token ID + * `@return` the decoded token context, or a context with unknown values if decoding fails + */ AccessTokenContext getTokenContextFromTokenId(String encodedTokenId); + /** + * Encodes the token context into a token ID string. + * + * `@param` tokenContext the token context to encode + * `@return` the encoded token ID containing the context information + */ String encodeTokenId(AccessTokenContext tokenContext); }services/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.java (1)
76-85: Consider usingassertThrowsfor cleaner exception testing.The current try-catch pattern works but is more verbose than modern JUnit patterns.
♻️ Suggested refactor
`@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) { - // ignored - } + String tokenId = "ofrtac:5678"; + Assert.assertThrows(IllegalArgumentException.class, + () -> provider.getTokenContextFromTokenId(tokenId)); }
Comment on lines
+69
to
+77
| 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 for rawTokenId (currently checks grantType twice).
Line 73 validates grantType again, so rawTokenId can be null and fail later.
🛠️ Proposed fix
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");
+ Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed");
this.sessionType = sessionType;
this.tokenType = tokenType;
this.grantType = grantType;
this.rawTokenId = rawTokenId;
}📝 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 - 77, The null-check for rawTokenId is incorrect: in the
AccessTokenContext constructor replace the duplicate
Objects.requireNonNull(grantType, "Null rawTokenId not allowed") with a check
against rawTokenId so rawTokenId is validated (i.e., call
Objects.requireNonNull(rawTokenId, "Null rawTokenId not allowed")); keep the
other checks for sessionType, tokenType and grantType unchanged.
Comment on lines
+1051
to
+1054
|
|
||
| TokenContextEncoderProvider encoder = session.getProvider(TokenContextEncoderProvider.class); | ||
| AccessTokenContext tokenCtx = encoder.getTokenContextFromClientSessionContext(clientSessionCtx, KeycloakModelUtils.generateId()); | ||
| token.id(encoder.encodeTokenId(tokenCtx)); |
There was a problem hiding this comment.
Consider adding null check for the encoder provider.
If TokenContextEncoderProvider is not registered or the SPI lookup fails, session.getProvider() may return null, causing an NPE on the subsequent method calls.
🛡️ Suggested defensive check
TokenContextEncoderProvider encoder = session.getProvider(TokenContextEncoderProvider.class);
+ if (encoder == null) {
+ // Fallback to simple ID generation if encoder not available
+ token.id(KeycloakModelUtils.generateId());
+ } else {
- AccessTokenContext tokenCtx = encoder.getTokenContextFromClientSessionContext(clientSessionCtx, KeycloakModelUtils.generateId());
- token.id(encoder.encodeTokenId(tokenCtx));
+ AccessTokenContext tokenCtx = encoder.getTokenContextFromClientSessionContext(clientSessionCtx, KeycloakModelUtils.generateId());
+ token.id(encoder.encodeTokenId(tokenCtx));
+ }Alternatively, if the provider is mandatory, throw a descriptive exception:
TokenContextEncoderProvider encoder = session.getProvider(TokenContextEncoderProvider.class);
if (encoder == null) {
throw new IllegalStateException("TokenContextEncoderProvider not configured");
}🤖 Prompt for AI Agents
In `@services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java` around
lines 1051 - 1054, The code assumes TokenContextEncoderProvider is present; add
a null-check after session.getProvider(TokenContextEncoderProvider.class) to
handle a missing provider (either throw a descriptive IllegalStateException like
"TokenContextEncoderProvider not configured" or log and abort), so that
subsequent calls to encoder.getTokenContextFromClientSessionContext(...) and
encoder.encodeTokenId(...) and token.id(...) do not cause an NPE; update the
block around the encoder variable accordingly.
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.
Critical logic bugs in isAccessTokenId matcher.
Two issues in this method:
-
Inverted condition logic (Line 483): The condition returns
falsewhen the grant shortcut matches, but it should returnfalsewhen it does not match. This causes the matcher to accept mismatched grants and reject correct ones. -
Incorrect substring indices: Based on the encoding format from
DefaultTokenContextEncoderProvider.encodeTokenId()—sessionType(2 chars) + tokenType(2 chars) + grantType(2 chars) + ":" + rawTokenId— the grant type is at indices 4-5 (0-indexed), so it should besubstring(4, 6), notsubstring(3, 5).
🐛 Proposed 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;
+ // Grant type shortcut starts at index 4 and is 2-chars long
+ if (!items[0].substring(4, 6).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);
}
};
}🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java`
around lines 476 - 492, The isAccessTokenId matcher has two bugs: it extracts
the grant shortcut from the wrong indices and inverts the match logic. Update
the substring call in isAccessTokenId to use substring(4, 6) to read the 2-char
grantType, and change the conditional so it returns false when that extracted
grant does NOT equal expectedGrantShortcut (i.e., if
(!extracted.equals(expectedGrantShortcut)) return false), leaving the UUID check
via isUUID().matches(items[1]) as-is; adjust within the anonymous
TypeSafeMatcher implementation.
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
Tests