Fixing JWT claim cache enable configuration#13834
Conversation
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR fixes the JWT claim cache enablement check: ChangesJWT Claim Cache Configuration Property Fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 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 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.java (1)
3267-3267: ⚡ Quick winRename variable to reflect the property being read.
The variable name
jwtClaimCacheExpiryEnabledStringreferences "Expiry" but now reads the enable flag (ENABLED_JWT_CLAIM_CACHE) rather than the expiry duration. Consider renaming tojwtClaimCacheEnabledStringfor clarity.♻️ Suggested variable rename
- String jwtClaimCacheExpiryEnabledString = getFirstProperty(APIConstants.ENABLED_JWT_CLAIM_CACHE); + String jwtClaimCacheEnabledString = getFirstProperty(APIConstants.ENABLED_JWT_CLAIM_CACHE); - if (StringUtils.isNotEmpty(jwtClaimCacheExpiryEnabledString)){ - return Boolean.parseBoolean(jwtClaimCacheExpiryEnabledString); + if (StringUtils.isNotEmpty(jwtClaimCacheEnabledString)){ + return Boolean.parseBoolean(jwtClaimCacheEnabledString); }🤖 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 `@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.java` at line 3267, Rename the misleading variable jwtClaimCacheExpiryEnabledString to jwtClaimCacheEnabledString in APIManagerConfiguration where you call getFirstProperty(APIConstants.ENABLED_JWT_CLAIM_CACHE); update all local usages of jwtClaimCacheExpiryEnabledString (assignments, conditionals, parsing) to the new name so the identifier accurately reflects that it holds the enable flag rather than an expiry duration.
🤖 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.
Nitpick comments:
In
`@components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.java`:
- Line 3267: Rename the misleading variable jwtClaimCacheExpiryEnabledString to
jwtClaimCacheEnabledString in APIManagerConfiguration where you call
getFirstProperty(APIConstants.ENABLED_JWT_CLAIM_CACHE); update all local usages
of jwtClaimCacheExpiryEnabledString (assignments, conditionals, parsing) to the
new name so the identifier accurately reflects that it holds the enable flag
rather than an expiry duration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d43a3d7-f96e-4bed-a442-1457b0659b19
📒 Files selected for processing (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/APIManagerConfiguration.java
e87f551
3770bda to
0467bd5
Compare
Purpose
This PR fixes an issue where configuration
[apim.cache.jwt_claim]does not work as expected even when it is set to true.JWT claim caching is intended to be enabled by default and allow user claims to be retrieved from the cache instead of repeatedly calling the Key Manager
APIManagerConfiguration.isJWTClaimCacheEnabled()incorrectly reads the propertyJWT_CLAIM_CACHE_EXPIRY(e.g., "15m") and attempts to interpret it as a boolean. Since non-boolean values cannot be parsed correctly, this evaluation always resulted in false. As a result, JWT user-claim caching in the Key Manager JWT generation path remained disabledApproach
This fix updates the logic to correctly read the
ENABLED_JWT_CLAIM_CACHEproperty instead of the expiry value.Tests
Lowered the JWT token cache (Upper caching layer before claim cache) and verified the behaviour for in JWTGenerator.getClaims() method
Related issues