Encoding context to access token IDs - #1
Open
linxia0415 wants to merge 2 commits into
Open
Conversation
closes #37118 Signed-off-by: mposolda <mposolda@gmail.com>
Signed-off-by: mposolda <mposolda@gmail.com>
📝 WalkthroughWalkthroughThis PR introduces a token context encoding system that compresses OAuth2 grant type, OIDC session type, and access token type metadata into encoded token IDs. All OAuth2 grant type factories define shortcut codes, the token flow tracks grant types through ClientSessionContext, and a new SPI-based provider handles bidirectional encoding and factory-based shortcut resolution. ChangesToken Context Encoding Infrastructure and Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java`:
- Around line 29-33: The Javadoc for OAuth2GrantTypeFactory#getShortcut() is
misleading — it says "3-letters" but token decoding assumes exactly three
2-character shortcuts; update the comment on getShortcut() to require exactly 2
characters (unique across grants) and mention that the value must be exactly 2
chars to match token-ID parsing and to avoid rejection by
DefaultTokenContextEncoderProviderFactory at startup.
In
`@services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java`:
- Around line 69-78: The constructor AccessTokenContext currently validates
grantType twice; replace the second Objects.requireNonNull(grantType, "Null
rawTokenId not allowed") with Objects.requireNonNull(rawTokenId, "Null
rawTokenId not allowed") so rawTokenId is actually checked; ensure the four null
checks refer respectively to sessionType, tokenType, grantType, and rawTokenId
before assigning fields.
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java`:
- Around line 476-492: In isAccessTokenId's anonymous TypeSafeMatcher
(matchesSafely), fix the substring indices and inverted logic: extract the
grant-type shortcut with items[0].substring(4, 6) (end index exclusive) and
return false when that extracted value does NOT equal expectedGrantShortcut
(i.e., if (!items[0].substring(4,6).equals(expectedGrantShortcut)) return
false), then continue to validate items[1] with isUUID().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec023784-7919-4083-88c3-397c181637d4
📒 Files selected for processing (28)
server-spi-private/src/main/java/org/keycloak/models/Constants.javaserver-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantType.javaserver-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/TokenManager.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderProvider.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderProviderFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/encode/TokenContextEncoderSpi.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/AuthorizationCodeGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/ClientCredentialsGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeBase.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/PermissionGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantType.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/RefreshTokenGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantType.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/TokenExchangeGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantTypeFactory.javaservices/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractOIDCProtocolMapper.javaservices/src/main/java/org/keycloak/protocol/oidc/tokenexchange/StandardTokenExchangeProvider.javaservices/src/main/resources/META-INF/services/org.keycloak.protocol.oidc.encode.TokenContextEncoderProviderFactoryservices/src/main/resources/META-INF/services/org.keycloak.provider.Spiservices/src/test/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderTest.javatestsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java
Comment on lines
+29
to
+33
| /** | ||
| * @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
# List all getShortcut() implementations and their returned literals
ast-grep --pattern $'public String getShortcut() {
return $X;
}'Repository: test-code-tool/keycloak__keycloak__CodeRabbit__PR37634__20260407
Length of output: 766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the SPI interface around the reported lines
SPI="server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java"
echo "---- $SPI ----"
nl -ba "$SPI" | sed -n '1,120p'
# 2) Find all in-tree occurrences of getShortcut() implementations
echo "---- getShortcut() implementations (in-tree) ----"
rg -n "String getShortcut\\(" --glob '*.java' server-spi-private services | head -n 200
# 3) Find implementations of the interface (directly)
echo "---- classes implementing OAuth2GrantTypeFactory ----"
rg -n "implements\\s+OAuth2GrantTypeFactory" --glob '*.java' . || true
# 4) Find usage of shortcut in token ID encoding/decoding (look for fixed-width/offsets)
echo "---- shortcut packing/decoding references ----"
rg -n "shortcut" --glob '*.java' server-spi-private services | head -n 200
# 5) Search for hardcoded substring lengths / fixed offsets near shortcut usage
echo "---- substring/offset patterns around shortcuts ----"
rg -n "substring\\(|offset|fixed|2-char|2\\s*\\*|char\\[\\]|length\\s*==\\s*2" --glob '*.java' services server-spi-private | head -n 200Repository: test-code-tool/keycloak__keycloak__CodeRabbit__PR37634__20260407
Length of output: 274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SPI="server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java"
echo "---- SPI snippet (29-80) ----"
sed -n '1,120p' "$SPI" | sed -n '29,80p' || true
echo
echo "---- Search Javadoc for '3-letters' ----"
rg -n "3-letters shortcut" --glob '*.java' "$SPI" server-spi-private services || true
echo
echo "---- All getShortcut() implementations across repo (first 200 matches) ----"
rg -n "String getShortcut\\(" --glob '*.java' server-spi-private services | head -n 200
echo
echo "---- All String getShortcut() methods specifically returning literals (first 200 matches) ----"
rg -n "String getShortcut\\([^{]*\\{[[:space:]]*return[[:space:]]*\"[^\"]+\";" --glob '*.java' server-spi-private services | head -n 200 || true
echo
echo "---- Where 'shortcut' is used in encoding/decoding logic ----"
rg -n "shortcut" --glob '*.java' server-spi-private services | head -n 200
echo
echo "---- Look for fixed offset / substring patterns involving shortcuts ----"
rg -n "(substring\\(|indexOf\\(|offset|fixed|len(gth)?\\s*==\\s*2|char\\[\\s*\\]|\\s*2\\s*characters)" --glob '*.java' server-spi-private services | head -n 200 || trueRepository: test-code-tool/keycloak__keycloak__CodeRabbit__PR37634__20260407
Length of output: 37001
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect SPI interface header/body
SPI="server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java"
echo "---- OAuth2GrantTypeFactory.java (1-120) ----"
sed -n '1,120p' "$SPI"
# Inspect shortcut encoding/decoding
ENC_FACTORY="services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProviderFactory.java"
echo
echo "---- DefaultTokenContextEncoderProviderFactory.java (60-170) ----"
sed -n '60,190p' "$ENC_FACTORY"
ENC_PROVIDER="services/src/main/java/org/keycloak/protocol/oidc/encode/DefaultTokenContextEncoderProvider.java"
echo
echo "---- DefaultTokenContextEncoderProvider.java (1-160) ----"
sed -n '1,170p' "$ENC_PROVIDER"
# Inspect each grant type factory's getShortcut() implementation
FILES=(
services/src/main/java/org/keycloak/protocol/oidc/grants/AuthorizationCodeGrantTypeFactory.java
services/src/main/java/org/keycloak/protocol/oidc/grants/PermissionGrantTypeFactory.java
services/src/main/java/org/keycloak/protocol/oidc/grants/PreAuthorizedCodeGrantTypeFactory.java
services/src/main/java/org/keycloak/protocol/oidc/grants/TokenExchangeGrantTypeFactory.java
services/src/main/java/org/keycloak/protocol/oidc/grants/ResourceOwnerPasswordCredentialsGrantTypeFactory.java
services/src/main/java/org/keycloak/protocol/oidc/grants/RefreshTokenGrantTypeFactory.java
services/src/main/java/org/keycloak/protocol/oidc/grants/ClientCredentialsGrantTypeFactory.java
services/src/main/java/org/keycloak/protocol/oidc/grants/device/DeviceGrantTypeFactory.java
services/src/main/java/org/keycloak/protocol/oidc/grants/ciba/CibaGrantTypeFactory.java
)
for f in "${FILES[@]}"; do
echo
echo "---- $(basename "$f") ----"
sed -n '1,120p' "$f" | rg -n "getShortcut\\(|Shortcut|SHORTCUT" || true
# Print the getShortcut method block (approx lines 1-120 includes it, but keep safe)
rg -n "getShortcut\\(" "$f"
# Print around the method for full context
line=$(rg -n "getShortcut\\(" "$f" | head -n1 | cut -d: -f1)
if [[ -n "${line:-}" ]]; then
start=$((line-5)); end=$((line+25));
sed -n "${start},${end}p" "$f"
fi
doneRepository: test-code-tool/keycloak__keycloak__CodeRabbit__PR37634__20260407
Length of output: 16934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java"
echo "---- AccessTokenContext.java (1-120) ----"
sed -n '1,140p' "$FILE"Repository: test-code-tool/keycloak__keycloak__CodeRabbit__PR37634__20260407
Length of output: 2722
Fix Javadoc for OAuth2GrantTypeFactory#getShortcut() to require exactly 2 characters
Token-ID parsing assumes a fixed 6-char prefix split into three 2-char shortcuts (substring(0, 2), substring(2, 4), substring(4, 6)), so “3-letters” is misleading and can break decoding for custom factories. In-tree grant factories use unique 2-char shortcuts, and DefaultTokenContextEncoderProviderFactory rejects duplicate shortcuts at startup.
📝 Proposed doc fix
/**
- * `@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.
+ * `@return` a fixed 2-character shortcut for this grant type. Useful where token length must be limited and the full grant name is not ideal.
+ * The shortcut must be exactly 2 characters and unique across all grant types, as it is parsed by fixed offset when decoding token IDs.
*/
String getShortcut();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@server-spi-private/src/main/java/org/keycloak/protocol/oidc/grants/OAuth2GrantTypeFactory.java`
around lines 29 - 33, The Javadoc for OAuth2GrantTypeFactory#getShortcut() is
misleading — it says "3-letters" but token decoding assumes exactly three
2-character shortcuts; update the comment on getShortcut() to require exactly 2
characters (unique across grants) and mention that the value must be exactly 2
chars to match token-ID parsing and to avoid rejection by
DefaultTokenContextEncoderProviderFactory at startup.
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.
Null check validates grantType twice; rawTokenId is never checked.
Line 73 calls Objects.requireNonNull(grantType, ...) again (with a rawTokenId message) instead of checking rawTokenId. A null rawTokenId will bypass validation, defeating the intended non-null invariant.
🐛 Proposed fix
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");📝 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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@services/src/main/java/org/keycloak/protocol/oidc/encode/AccessTokenContext.java`
around lines 69 - 78, The constructor AccessTokenContext currently validates
grantType twice; replace the second Objects.requireNonNull(grantType, "Null
rawTokenId not allowed") with Objects.requireNonNull(rawTokenId, "Null
rawTokenId not allowed") so rawTokenId is actually checked; ensure the four null
checks refer respectively to sessionType, tokenType, grantType, and rawTokenId
before assigning fields.
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 bug: incorrect substring indices and inverted match logic.
Line 483 has two bugs that will cause test failures or false positives:
- Wrong substring indices: The grant-type shortcut occupies indices 4–5 (encoded format is
sessionType(0-1) + tokenType(2-3) + grantType(4-5) + ':' + UUID), but the code usessubstring(3, 5), which extracts the last character of token type and the first character of grant type. - Inverted logic: The matcher should return
falsewhen the shortcut does not match, but the current code returnsfalsewhen it does match.
🐛 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;
+ // Grant type shortcut occupies indices 4-5 (format: sessionType + tokenType + grantType + ':' + UUID)
+ if (!items[0].substring(4, 6).equals(expectedGrantShortcut)) return false;
return isUUID().matches(items[1]);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/AssertEvents.java`
around lines 476 - 492, In isAccessTokenId's anonymous TypeSafeMatcher
(matchesSafely), fix the substring indices and inverted logic: extract the
grant-type shortcut with items[0].substring(4, 6) (end index exclusive) and
return false when that extracted value does NOT equal expectedGrantShortcut
(i.e., if (!items[0].substring(4,6).equals(expectedGrantShortcut)) return
false), then continue to validate items[1] with isUUID().
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
closes #37118
PR is adding some context information to be available in the
access-tokenwithout a need to introduce any additional custom claims. So far, it provides info about:These informations can be useful when token is later sent to some Keycloak endpoints (for example introspection, userInfo, admin/account REST API) to optimize something. For example:
session-type will allow to lookup only correct session type based on the type of the session, which token was created from. As for example today, we need to lookup "online" session and if it is not found, then fallback to "offline" session. So possible unnecessary lookup. Related follow-up issue for optimize that: #37662
Grant type knowledge can be useful for example for the token-exchange use-case: #37117
Lightweight access token: We will be able to recognize more reliably if access-token is lightweight or not and do something based on that (EG. lookup roles from model instead of lookup roles from the token)
In this PR, I've made token ID to look something like
onrtac:1f65761e-1c69-40a2-82da-2d8aa5e39a8a.onis shortcut for "online" sessionrtis shortcut for regular token (not lightweight token)acis shortcut for "authorization code" grantShortcuts are used, so the token ID contains important info, but at the same time, it is limited in size and does not introduce much new characters to the token.
The encoding into ID is used only for access-tokens. Not used for refresh tokens or ID tokens or any others.
Introduced dedicated SPI in the end. Wanted to "cache" some objects (EG. shortcuts for grant types etc) and it seems clearer to me to cache them at the level of factory, rather than in the static fields, which is not very pretty. Moreover, provider adds ability for someone to introduce his own provider (and eventually encode some more info or encode stuff in the different way etc).
Note
I've used 2 commits in this PR. Originally, in the 1st commit, I've made the encoding a bit longer with token ID to look something like
st.on_tt.rt_gt.ac:1f65761e-1c69-40a2-82da-2d8aa5e39a8a.stis shortcut for "session type" andonis shortcut for "online" sessionttis shortcut for "token type" andrtis shortcut for regular token (not lightweight token)gtis shortcut for grant type andacis shortcut for "authorization code" grantBut assuming that every "type" has always 2 characters long shortcut, I've rather updated it in the 2nd commit to have only 7 new characters instead of 18 characters. I planned to squash before merge (unless you think that we should rather use longer format for some reason, in which case the 2nd commit can be removed from the PR).
Summary by CodeRabbit
Release Notes