[OB4] Removing session data key consent from the cache#915
Merged
[OB4] Removing session data key consent from the cache#915
Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| @@ -256,6 +256,13 @@ public Response persist(@Context HttpServletRequest request, @Context HttpServle | |||
| throws ConsentException, ConsentManagementException, URISyntaxException { | |||
|
|
|||
| ConsentData consentData = ConsentCache.getConsentDataFromCache(sessionDataKey); | |||
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
Suggested change
| ConsentData consentData = ConsentCache.getConsentDataFromCache(sessionDataKey); | |
| ConsentData consentData = ConsentCache.getConsentDataFromCache(sessionDataKey); | |
| if (consentData == null) { | |
| log.warn("No consent data found in cache for sessionDataKey: {}", sessionDataKey); | |
| } else { | |
| log.debug("Retrieved consent data from cache for sessionDataKey: {}", sessionDataKey); | |
| } |
| Remove consent data from the cache to prevent multiple calls to the persist endpoint. | ||
| This ensures the flow is completed and avoids further updates after a single persistence. | ||
| */ | ||
| ConsentCache.removeFromCache(sessionDataKey); |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 2
Suggested change
| ConsentCache.removeFromCache(sessionDataKey); | |
| ConsentCache.removeFromCache(sessionDataKey); | |
| log.info("Removed consent data from cache for sessionDataKey: {}", sessionDataKey); |
Comment on lines
+230
to
+234
| */ | ||
| public static void removeFromCache(String cacheKey) { | ||
|
|
||
| ConsentCache.getInstance().removeFromCache(ConsentCacheKey.of(cacheKey)); | ||
| } |
Contributor
There was a problem hiding this comment.
Log Improvement Suggestion No: 3
Suggested change
| */ | |
| public static void removeFromCache(String cacheKey) { | |
| ConsentCache.getInstance().removeFromCache(ConsentCacheKey.of(cacheKey)); | |
| } | |
| public static void removeFromCache(String cacheKey) { | |
| log.info("Removing consent from cache for key: " + cacheKey); | |
| ConsentCache.getInstance().removeFromCache(ConsentCacheKey.of(cacheKey)); | |
| } |
Contributor
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 | ||
| #### Log Improvement Suggestion No: 2 | ||
| #### Log Improvement Suggestion No: 3 |
WalkthroughThe PR adds cache cleanup functionality to the consent management flow by introducing a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Ashi1993
approved these changes
Mar 12, 2026
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.java (1)
268-292:⚠️ Potential issue | 🟠 MajorPre-existing NPE risk when consent data is unavailable.
This block is reached when
consentDataisnull, yet lines 273 and 289 accessconsentData.getRedirectURI()andconsentData.getState(), which will throwNullPointerException. This is pre-existing code, but your cache removal change may increase the likelihood of hitting this path (e.g., on retries or race conditions).Consider addressing this in a follow-up to avoid masking the root cause with an NPE:
🐛 Suggested fix for null safety
if (consentData == null) { if (ConsentConstants.STORE_CONSENT) { Map<String, String> consentDetailsMap = consentCoreService .getConsentAttributesByName(sessionDataKey); if (consentDetailsMap.isEmpty()) { - throw new ConsentException(consentData.getRedirectURI(), - AuthErrorCode.SERVER_ERROR, - "Unable to get consent data", consentData.getState()); + throw new ConsentException(null, + AuthErrorCode.SERVER_ERROR, + "Unable to get consent data", null); } // ... retrieval logic ... } else { - throw new ConsentException(consentData.getRedirectURI(), - AuthErrorCode.SERVER_ERROR, "Unable to get consent data", - consentData.getState()); + throw new ConsentException(null, + AuthErrorCode.SERVER_ERROR, "Unable to get consent data", + null); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.java` around lines 268 - 292, The null-pointer risk occurs inside ConsentAuthorizeEndpoint when consentData is null but the code still calls consentData.getRedirectURI() and consentData.getState() to construct ConsentException; change the null-unsafe exception creation to use safe fallbacks (e.g., extract redirectURI/state from consentDetailsMap or consentDetails/ sessionDataKey, or pass null/empty strings) before throwing, and ensure you only call ConsentUtils.getConsentDataFromAttributes and access consentData methods after verifying consentData != null (references: ConsentAuthorizeEndpoint, consentCoreService.getConsentAttributesByName, ConsentUtils.getConsentDataFromAttributes, ConsentException).
🧹 Nitpick comments (1)
financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/utils/ConsentCache.java (1)
227-234: Implementation looks good; consider null safety.The method follows the established patterns in this class. One minor consideration: if
cacheKeyisnull, the behavior depends onConsentCacheKey.of()implementation. If upstream callers can pass null, a guard clause would make failures more explicit.🛡️ Optional: Add null check
public static void removeFromCache(String cacheKey) { + if (cacheKey == null) { + log.warn("Attempted to remove null key from cache"); + return; + } ConsentCache.getInstance().removeFromCache(ConsentCacheKey.of(cacheKey)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/utils/ConsentCache.java` around lines 227 - 234, The removeFromCache method should guard against a null cacheKey to avoid delegating a null to ConsentCacheKey.of(); update ConsentCache.removeFromCache(String) to check if cacheKey is null (or add the check in the static wrapper) and return early or throw an IllegalArgumentException, e.g., validate cacheKey at the start of removeFromCache and handle null explicitly so ConsentCacheKey.of(cacheKey) is not called with null; refer to the static method removeFromCache and the ConsentCacheKey.of(...) call when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.java`:
- Around line 258-264: There is a race between
ConsentCache.getConsentDataFromCache(sessionDataKey) and
ConsentCache.removeFromCache(sessionDataKey) allowing two threads to both read
consentData; change the cache API to provide an atomic get-and-remove operation
(e.g., add and call ConsentCache.getAndRemove(sessionDataKey) or similar) and
update ConsentAuthorizeEndpoint to use that atomic method instead of separate
get and remove calls so a single thread obtains and clears the entry atomically.
---
Outside diff comments:
In
`@financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.java`:
- Around line 268-292: The null-pointer risk occurs inside
ConsentAuthorizeEndpoint when consentData is null but the code still calls
consentData.getRedirectURI() and consentData.getState() to construct
ConsentException; change the null-unsafe exception creation to use safe
fallbacks (e.g., extract redirectURI/state from consentDetailsMap or
consentDetails/ sessionDataKey, or pass null/empty strings) before throwing, and
ensure you only call ConsentUtils.getConsentDataFromAttributes and access
consentData methods after verifying consentData != null (references:
ConsentAuthorizeEndpoint, consentCoreService.getConsentAttributesByName,
ConsentUtils.getConsentDataFromAttributes, ConsentException).
---
Nitpick comments:
In
`@financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/utils/ConsentCache.java`:
- Around line 227-234: The removeFromCache method should guard against a null
cacheKey to avoid delegating a null to ConsentCacheKey.of(); update
ConsentCache.removeFromCache(String) to check if cacheKey is null (or add the
check in the static wrapper) and return early or throw an
IllegalArgumentException, e.g., validate cacheKey at the start of
removeFromCache and handle null explicitly so ConsentCacheKey.of(cacheKey) is
not called with null; refer to the static method removeFromCache and the
ConsentCacheKey.of(...) call when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f45192ea-76ba-457d-b07d-7e6e2d33223b
📒 Files selected for processing (2)
financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.javafinancial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/utils/ConsentCache.java
Comment on lines
258
to
+264
| ConsentData consentData = ConsentCache.getConsentDataFromCache(sessionDataKey); | ||
|
|
||
| /* | ||
| Remove consent data from the cache to prevent multiple calls to the persist endpoint. | ||
| This ensures the flow is completed and avoids further updates after a single persistence. | ||
| */ | ||
| ConsentCache.removeFromCache(sessionDataKey); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the base cache class has an atomic getAndRemove operation that could be used
ast-grep --pattern $'class FinancialServicesBaseCache {
$$$
}'
# Also check for any existing atomic cache operations in the codebase
rg -n "getAndRemove|removeAndGet|atomicRemove" --type javaRepository: wso2/financial-services-accelerator
Length of output: 61
🏁 Script executed:
# Find the ConsentCache class
fd -type f -name "*ConsentCache*" --type fRepository: wso2/financial-services-accelerator
Length of output: 249
🏁 Script executed:
# Search for ConsentCache class definition and methods
rg -n "class ConsentCache|public.*get.*Cache|public.*remove.*Cache" --type java -A 3Repository: wso2/financial-services-accelerator
Length of output: 26398
🏁 Script executed:
# Look at the complete flow in ConsentAuthorizeEndpoint to understand the race window
rg -n "getConsentDataFromCache|removeFromCache|persistConsent" --type java -B 2 -A 2Repository: wso2/financial-services-accelerator
Length of output: 14580
🏁 Script executed:
# Get the full implementation of FinancialServicesBaseCache to understand synchronization
cat financial-services-accelerator/components/org.wso2.financial.services.accelerator.common/src/main/java/org/wso2/financial/services/accelerator/common/caching/FinancialServicesBaseCache.java | head -200Repository: wso2/financial-services-accelerator
Length of output: 6279
🏁 Script executed:
# Check if there's any synchronization in the ConsentCache specifically
rg -n "synchronized" financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/utils/ConsentCache.javaRepository: wso2/financial-services-accelerator
Length of output: 129
🏁 Script executed:
# Look at the complete ConsentAuthorizeEndpoint flow around the cache removal to understand persistence logic
sed -n '250,380p' financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.javaRepository: wso2/financial-services-accelerator
Length of output: 7306
🏁 Script executed:
# Check the git history to see if lines 273, 284, 289 were modified in this PR
git log -p --follow -S "consentData.getRedirectURI()" financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.java | head -100Repository: wso2/financial-services-accelerator
Length of output: 61
🏁 Script executed:
# Show the exact context around line 273 and 284 to understand the NPE issue
sed -n '268,295p' financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.javaRepository: wso2/financial-services-accelerator
Length of output: 1655
Race condition window remains between retrieval and removal.
The cache removal improves security but two concurrent requests can both retrieve valid consentData before either executes the removal. For stronger guarantees, consider implementing an atomic get-and-remove operation in the cache layer.
If atomic operations aren't feasible, this fix still meaningfully narrows the attack window and is acceptable for merge. The database cleanup in the finally block (lines 367-380) provides an additional layer of protection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@financial-services-accelerator/internal-webapps/org.wso2.financial.services.accelerator.consent.mgt.endpoint/src/main/java/org/wso2/financial/services/accelerator/consent/mgt/endpoint/api/ConsentAuthorizeEndpoint.java`
around lines 258 - 264, There is a race between
ConsentCache.getConsentDataFromCache(sessionDataKey) and
ConsentCache.removeFromCache(sessionDataKey) allowing two threads to both read
consentData; change the cache API to provide an atomic get-and-remove operation
(e.g., add and call ConsentCache.getAndRemove(sessionDataKey) or similar) and
update ConsentAuthorizeEndpoint to use that atomic method instead of separate
get and remove calls so a single thread obtains and clears the entry atomically.
hasithakn
approved these changes
Mar 13, 2026
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
3 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.
[OB4] Removing session data key consent from the cache
Issue link: https://github.com/wso2-enterprise/wso2-ob-internal/issues/2382
Doc Issue: Optional, link issue from documentation repository
Applicable Labels: Spec, product, version, type (specify requested labels)
Development Checklist
Secure Development Checklist
Testing Checklist
Automation Test Details
Conformance Tests Details
Summary by CodeRabbit