Add config for non persistent access token#7730
Conversation
WalkthroughThree new TokenPersistence configuration options were added to identity configuration files: PersistAccessToken, PersistRefreshToken, and KeepRevokedAccessTokens. The template variant includes conditional logic for the revoked tokens setting, defaulting to true when environment variables are not defined. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🤖 Fix all issues with AI agents
In
`@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2`:
- Around line 1168-1184: The three XML config elements PersistAccessToken,
PersistRefreshToken, and KeepRevokedAccessTokens are defined in the template but
not wired to runtime; either remove them or wire them into the OAuth
configuration and token-handling code. Fix by updating the configuration loader
that processes oauth.token_persistence (the same place other
oauth.token_persistence values are read) to read PersistAccessToken,
PersistRefreshToken and KeepRevokedAccessTokens and expose them via the runtime
config object used by the token issuance/refresh/revocation paths so the OAuth
token handler checks these flags (persist-access logic in the token
issuance/refresh code and revocation cleanup logic) and honors the configured
behavior; if you choose not to support them, remove the three XML elements and
related toml mappings to avoid dead configuration.
🧹 Nitpick comments (1)
features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 (1)
1175-1184: Clarify precedence when both revoked-token variables are defined.Right now
retain_revoked_tokenoverridesretain_revoked_access_tokensilently. A short inline note would prevent confusion during migrations.📝 Suggested comment tweak
<!-- Applicable when non-persistent access tokens are enabled. Determines whether revoked access tokens should still be retained. - Default: true. --> + Default: true. + If both retain_revoked_token and retain_revoked_access_token are set, + retain_revoked_token takes precedence. -->
| <!-- Determines whether access tokens should be persisted in the database. | ||
| Default: true. Set to false to enable the non-persistent access token feature. --> | ||
| <PersistAccessToken>{{oauth.token_persistence.persist_access_token | default(true)}}</PersistAccessToken> | ||
| <!-- Determines whether refresh tokens should be persisted in the database. | ||
| Default: true. Set to false to enable the non-persistent refresh token feature. | ||
| Note: This feature is only subject to use with non-persistent access token feature. --> | ||
| <PersistRefreshToken>{{oauth.token_persistence.persist_refresh_token | default(true)}}</PersistRefreshToken> | ||
| <!-- Applicable when non-persistent access tokens are enabled. | ||
| Determines whether revoked access tokens should still be retained. | ||
| Default: true. --> | ||
| {% if oauth.token_persistence.retain_revoked_token is defined %} | ||
| <KeepRevokedAccessTokens>{{ oauth.token_persistence.retain_revoked_token }}</KeepRevokedAccessTokens> | ||
| {% elif oauth.token_persistence.retain_revoked_access_token is defined %} | ||
| <KeepRevokedAccessTokens>{{ oauth.token_persistence.retain_revoked_access_token }}</KeepRevokedAccessTokens> | ||
| {% else %} | ||
| <KeepRevokedAccessTokens>true</KeepRevokedAccessTokens> | ||
| {% endif %} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for references to the new token persistence keys in config and code
echo "=== Searching for XML element names ==="
rg -n "PersistAccessToken|PersistRefreshToken|KeepRevokedAccessTokens" \
--type-list | grep -E '\.(java|kt|xml|j2|toml|yaml)' || true
rg -n "PersistAccessToken|PersistRefreshToken|KeepRevokedAccessTokens" \
-g '*.java' -g '*.kt' -g '*.xml' -g '*.j2' -g '*.toml' -g '*.yaml'
echo ""
echo "=== Searching for deployment.toml template keys ==="
rg -n "persist_access_token|persist_refresh_token|retain_revoked_token|retain_revoked_access_token" \
-g '*.java' -g '*.kt' -g '*.toml' -g '*.yaml' -g '*.j2'Repository: wso2/carbon-identity-framework
Length of output: 5819
🏁 Script executed:
#!/bin/bash
# Search for Java/Kotlin code that references these XML element names
echo "=== Searching for code reading PersistAccessToken, PersistRefreshToken, KeepRevokedAccessTokens ==="
rg -n "PersistAccessToken|PersistRefreshToken|KeepRevokedAccessTokens" \
-g '*.java' -g '*.kt'
echo ""
echo "=== Searching for TokenPersistence config parsing ==="
rg -n "TokenPersistence|token.persistence" -g '*.java' -g '*.kt' -A 3 -B 1 | head -100
echo ""
echo "=== Searching for config factory/reader classes ==="
rg -n "class.*Config|XMLConfig" -g '*.java' -g '*.kt' | grep -i token | head -20Repository: wso2/carbon-identity-framework
Length of output: 281
🏁 Script executed:
#!/bin/bash
# Search more broadly for OAuth and token configuration handling
echo "=== Searching for OAuth config classes ==="
find . -type f -name "*.java" -o -name "*.kt" | xargs grep -l "oauth" -i | grep -i config | head -20
echo ""
echo "=== Searching for 'persist' in token/oauth context ==="
rg -n "persist" -i -g '*.java' -g '*.kt' | grep -i "token\|oauth" | head -30
echo ""
echo "=== Searching for token-related configuration classes ==="
rg -n "class.*Token.*Config" -g '*.java' -g '*.kt'
echo ""
echo "=== Searching for persist_access_token in deployment files ==="
find . -name "*.toml" -o -name "*.yaml" -o -name "*.yml" | xargs grep -l "persist" 2>/dev/null | head -10Repository: wso2/carbon-identity-framework
Length of output: 1637
🏁 Script executed:
#!/bin/bash
# Search for OAuth configuration and identity.xml parsing
echo "=== Searching for OAuth/Token configuration classes ==="
rg -n "OAuthServer|OAuth2Server|TokenPersistence" -g '*.java' -g '*.kt'
echo ""
echo "=== Searching for how identity.xml is loaded/parsed ==="
rg -n "identity\.xml|parseXML|readConfiguration" -g '*.java' -g '*.kt' | grep -v test | head -40
echo ""
echo "=== Checking for any config related to token persistence or retention ==="
rg -n "revok|token.*persist|persist.*token" -i -g '*.java' -g '*.kt' | head -30
echo ""
echo "=== List files in identity core oauth module ==="
find . -path "*/oauth/*" -name "*.java" | grep -v test | head -20Repository: wso2/carbon-identity-framework
Length of output: 14647
🏁 Script executed:
#!/bin/bash
# Find OAuth-related modules and config handling
echo "=== Find all oauth modules ==="
find . -type d -name "*oauth*" | grep -E "src/(main|test)" | head -20
echo ""
echo "=== Search for token-related config in oauth modules ==="
find . -path "*/oauth*" -name "*.java" -not -path "*/test/*" | xargs grep -l "persist\|token.*config" -i 2>/dev/null | head -20
echo ""
echo "=== Check IdentityConfigParser for how config is loaded ==="
grep -n "getProperty\|TokenPersistence" ./components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/util/IdentityConfigParser.java 2>/dev/null | head -20
echo ""
echo "=== Find where identity.xml properties are accessed ==="
rg -n "getProperty.*Token|Token.*getProperty" -g '*.java' | head -20Repository: wso2/carbon-identity-framework
Length of output: 4022
🏁 Script executed:
#!/bin/bash
# Get full context around the token persistence section in identity.xml.j2
echo "=== Full context of TokenPersistence section in identity.xml.j2 ==="
sed -n '1160,1195p' ./features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2
echo ""
echo "=== Check for any TODOs/FIXMEs related to token persistence ==="
rg -n "TODO|FIXME" -g '*.j2' -g '*.java' | grep -i "token\|persist" | head -20
echo ""
echo "=== Check static identity.xml token persistence section ==="
sed -n '680,700p' ./features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml
echo ""
echo "=== Search for any usage of property names with 'TokenPersistence' pattern ==="
rg -n "getProperty.*Token|TokenPersistence" -g '*.java' -g '*.kt' -B 2 -A 2 | head -40Repository: wso2/carbon-identity-framework
Length of output: 8272
🏁 Script executed:
#!/bin/bash
# Search more carefully for how TokenPersistence section is parsed
echo "=== Search for TokenPersistence parsing in any Java code ==="
rg -n "TokenPersistence" -g '*.java' -g '*.kt' -B 3 -A 5
echo ""
echo "=== Look for OAuth configuration classes that might handle token persistence ==="
find . -path "*/oauth*" -name "*.java" -not -path "*/test/*" 2>/dev/null | head -30
echo ""
echo "=== Search in oauth modules for configuration-related classes ==="
find . -path "*/oauth*" -name "*Config*.java" -not -path "*/test/*" 2>/dev/null | head -30Repository: wso2/carbon-identity-framework
Length of output: 283
Confirm these configuration elements are wired to runtime code—they do not appear to be read by any Java code in this repository.
The XML elements PersistAccessToken, PersistRefreshToken, and KeepRevokedAccessTokens are defined in the template and static config, but no Java code in this repository reads them. Users setting these properties in deployment.toml will generate valid XML, but the runtime will not honor those settings. Either these must be integrated with the OAuth token handling code, or they should be removed as dead configuration.
🤖 Prompt for AI Agents
In
`@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2`
around lines 1168 - 1184, The three XML config elements PersistAccessToken,
PersistRefreshToken, and KeepRevokedAccessTokens are defined in the template but
not wired to runtime; either remove them or wire them into the OAuth
configuration and token-handling code. Fix by updating the configuration loader
that processes oauth.token_persistence (the same place other
oauth.token_persistence values are read) to read PersistAccessToken,
PersistRefreshToken and KeepRevokedAccessTokens and expose them via the runtime
config object used by the token issuance/refresh/revocation paths so the OAuth
token handler checks these flags (persist-access logic in the token
issuance/refresh code and revocation cleanup logic) and honors the configured
behavior; if you choose not to support them, remove the three XML elements and
related toml mappings to avoid dead configuration.
There was a problem hiding this comment.
Pull request overview
Adds new OAuth2 token persistence configuration knobs to the default identity.xml and the identity.xml.j2 template to support non-persistent access/refresh tokens and configurable retention of revoked access tokens.
Changes:
- Introduced
<PersistAccessToken>and<PersistRefreshToken>under<TokenPersistence>with defaults. - Added
<KeepRevokedAccessTokens>with template logic supporting multiple variable names and a default oftrue. - Documented the new options inline in
identity.xml.j2.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 |
Adds template-driven token persistence flags plus inline documentation and backward-compatible variable handling for revoked-token retention. |
features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml |
Adds the new token persistence elements with default true values to the shipped default configuration. |
| <!-- Determines whether refresh tokens should be persisted in the database. | ||
| Default: true. Set to false to enable the non-persistent refresh token feature. | ||
| Note: This feature is only subject to use with non-persistent access token feature. --> |
There was a problem hiding this comment.
Grammar in the inline documentation is awkward: "This feature is only subject to use with non-persistent access token feature." Consider rephrasing (e.g., "only intended for use with the non-persistent access token feature") to make the constraint clear to config authors.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7730 +/- ##
============================================
+ Coverage 50.71% 50.79% +0.08%
+ Complexity 20174 19735 -439
============================================
Files 2172 2172
Lines 129040 126694 -2346
Branches 26473 25741 -732
============================================
- Hits 65438 64356 -1082
+ Misses 55248 54081 -1167
+ Partials 8354 8257 -97
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Proposed changes in this pull request
Public Issue: wso2/product-is#24066
This pull request introduces new configuration options to the token persistence settings, allowing for more granular control over how access and refresh tokens are stored and managed. The changes add support for persisting access tokens, persisting refresh tokens, and retaining revoked access tokens, with clear documentation and default values.
Token persistence configuration enhancements:
<PersistAccessToken>and<PersistRefreshToken>elements to bothidentity.xmlandidentity.xml.j2, enabling control over whether access and refresh tokens are persisted in the database. [1] [2]<KeepRevokedAccessTokens>element, allowing configuration of whether revoked access tokens are retained, with logic to handle multiple possible variable names and a default value oftrue. [1] [2]Documentation improvements:
identity.xml.j2to clarify the purpose and usage of each new configuration option, including their defaults and relationships.Summary by CodeRabbit