Implement cache clearance for original token scopes during revocation#3071
Open
KD23243 wants to merge 7 commits intowso2-extensions:masterfrom
Open
Implement cache clearance for original token scopes during revocation#3071KD23243 wants to merge 7 commits intowso2-extensions:masterfrom
KD23243 wants to merge 7 commits intowso2-extensions:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new OAuthUtil method that reads persisted token scopes and clears cache entries using those original scopes; OAuth2Service invokes this during token revocation for token-bound access tokens. Unit tests for many edge cases and related utilities were added. Changes
Sequence DiagramsequenceDiagram
actor Revocation as OAuth2Service
participant Util as OAuthUtil
participant Config as OAuthServerConfiguration
participant DAO as AccessTokenDAO
participant Cache as OAuthCache
Revocation->>Util: clearOAuthCacheUsingPersistedScopes(bindingRef, accessTokenDO, revokeRequestDTO)
Util->>Util: validate allowed scopes & token string
Util->>Config: get token persistence factory / DAO
Util->>DAO: getAccessToken(accessTokenString)
DAO-->>Util: persisted AccessTokenDO (original scopes)
Util->>Util: build scope string from persisted scopes
Util->>Cache: clear cache (scopes + bindingRef, consumerKey, authzUser)
Cache-->>Util: ack
Util->>Cache: clear cache (scopes, consumerKey, authzUser)
Cache-->>Util: ack
Util-->>Revocation: return
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
...s/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthUtil.java
Show resolved
Hide resolved
...s/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthUtil.java
Show resolved
Hide resolved
....wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/OAuth2Service.java
Show resolved
Hide resolved
....wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/OAuth2Service.java
Show resolved
Hide resolved
...g.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java
Show resolved
Hide resolved
...g.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java
Show resolved
Hide resolved
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java (1)
813-835: Test structure observation.In this test,
mockAccessTokenDAOis created but never wired tomockFactory. The verification at line 833 passes because the method exits early when allowed scopes are empty, so no DAO call occurs. While functionally correct, the mock setup could be misleading to future readers.Consider adding a brief comment explaining the mock is intentionally unconnected since we're verifying early exit behavior, or wire up the mock properly to make the test more self-documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java` around lines 813 - 835, Wire the unused mockAccessTokenDAO to the mocked OAuthTokenPersistenceFactory to make the test intent explicit: after creating mockFactory and mockAccessTokenDAO, stub mockFactory.getAccessTokenDAO() to return mockAccessTokenDAO (i.e., when(mockFactory.getAccessTokenDAO()).thenReturn(mockAccessTokenDAO)) before calling OAuthUtil.clearOAuthCacheUsingPersistedScopes; alternatively, if you intentionally want it unconnected to assert early exit, add a brief comment above the mock creation explaining that the DAO is deliberately not wired because allowed scopes are empty and the method should return early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java`:
- Around line 813-835: Wire the unused mockAccessTokenDAO to the mocked
OAuthTokenPersistenceFactory to make the test intent explicit: after creating
mockFactory and mockAccessTokenDAO, stub mockFactory.getAccessTokenDAO() to
return mockAccessTokenDAO (i.e.,
when(mockFactory.getAccessTokenDAO()).thenReturn(mockAccessTokenDAO)) before
calling OAuthUtil.clearOAuthCacheUsingPersistedScopes; alternatively, if you
intentionally want it unconnected to assert early exit, add a brief comment
above the mock creation explaining that the DAO is deliberately not wired
because allowed scopes are empty and the method should return early.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthUtil.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/OAuth2Service.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java
KD23243
force-pushed
the
cacheClearRevoke
branch
from
a3ea428 to
cf8b7a8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3071 +/- ##
============================================
- Coverage 57.76% 57.35% -0.42%
+ Complexity 10229 10194 -35
============================================
Files 707 707
Lines 56817 58635 +1818
Branches 13861 13299 -562
============================================
+ Hits 32820 33628 +808
- Misses 19599 20527 +928
- Partials 4398 4480 +82
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:
|
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
approved these changes
Feb 27, 2026
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/22480094039
| // The in-memory scopes may be mutated during validation. To avoid cache-key | ||
| // mismatches, retrieve the original scopes from the database before clearing | ||
| // the OAuth cache. | ||
| AccessTokenDO dbTokenDO = OAuthTokenPersistenceFactory.getInstance() |
Contributor
There was a problem hiding this comment.
Accessing dao layer from util method may not be the correct way. Don't we have service methods to fetch the access token.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java (2)
846-848: Same mock wiring suggestion applies here.Same pattern as the previous test — consider wiring
mockAccessTokenDAOtomockFactoryfor a stronger assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java` around lines 846 - 848, Test creates mockFactory and mockAccessTokenDAO but never wires the DAO to the factory; update the test to stub OAuthTokenPersistenceFactory.getInstance() to return mockFactory and also stub mockFactory.getAccessTokenDAO() (or the factory method that returns an AccessTokenDAO) to return mockAccessTokenDAO so callers in OAuthUtilTest use the mocked DAO; reference OAuthTokenPersistenceFactory, getInstance, mockFactory, AccessTokenDAO, and mockAccessTokenDAO when adding the when(...).thenReturn(...) wiring.
822-824: Consider wiring the mock DAO to strengthen the test.The
mockAccessTokenDAOis created but not connected tomockFactory. The verification passes trivially because the mock is never used. For a stronger test, wire the mock:OAuthTokenPersistenceFactory mockFactory = mock(OAuthTokenPersistenceFactory.class); oAuthTokenPersistenceFactory.when(OAuthTokenPersistenceFactory::getInstance).thenReturn(mockFactory); AccessTokenDAO mockAccessTokenDAO = mock(AccessTokenDAO.class); +when(mockFactory.getAccessTokenDAO()).thenReturn(mockAccessTokenDAO);This ensures the verification confirms the early return actually prevents DAO access, rather than passing because the mock isn't wired.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java` around lines 822 - 824, The test creates mockFactory (OAuthTokenPersistenceFactory) and mockAccessTokenDAO (AccessTokenDAO) but never wires them together, so the DAO is never used; update the test to stub mockFactory to return mockAccessTokenDAO from the factory method (e.g., when(mockFactory.getAccessTokenDAO()).thenReturn(mockAccessTokenDAO) or equivalent) and ensure OAuthTokenPersistenceFactory.getInstance() still returns mockFactory so verifications against mockAccessTokenDAO actually exercise the mock rather than passing vacuously.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java`:
- Around line 846-848: Test creates mockFactory and mockAccessTokenDAO but never
wires the DAO to the factory; update the test to stub
OAuthTokenPersistenceFactory.getInstance() to return mockFactory and also stub
mockFactory.getAccessTokenDAO() (or the factory method that returns an
AccessTokenDAO) to return mockAccessTokenDAO so callers in OAuthUtilTest use the
mocked DAO; reference OAuthTokenPersistenceFactory, getInstance, mockFactory,
AccessTokenDAO, and mockAccessTokenDAO when adding the when(...).thenReturn(...)
wiring.
- Around line 822-824: The test creates mockFactory
(OAuthTokenPersistenceFactory) and mockAccessTokenDAO (AccessTokenDAO) but never
wires them together, so the DAO is never used; update the test to stub
mockFactory to return mockAccessTokenDAO from the factory method (e.g.,
when(mockFactory.getAccessTokenDAO()).thenReturn(mockAccessTokenDAO) or
equivalent) and ensure OAuthTokenPersistenceFactory.getInstance() still returns
mockFactory so verifications against mockAccessTokenDAO actually exercise the
mock rather than passing vacuously.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62906e8a-19a1-4c27-a4b6-f6b704d00d1b
📒 Files selected for processing (3)
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthUtil.javacomponents/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/OAuth2Service.javacomponents/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth/OAuthUtil.java
- components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/OAuth2Service.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java (1)
1222-1266: Consider using try-finally for interceptor cleanup to improve test isolation.The interceptor cleanup (
addOauthEventInterceptorProxy(null)) is placed at the end of each test method. If a test fails before reaching this line (e.g., due to an assertion failure or unexpected exception), the interceptor remains set, potentially affecting subsequent tests.Consider wrapping the test body in try-finally:
`@Test` public void testInvokePreRevocationBySystemListeners_AccessTokenDO_WhenInterceptorEnabled() throws Exception { OAuthEventInterceptor interceptor = mock(OAuthEventInterceptor.class); when(interceptor.isEnabled()).thenReturn(true); OAuthComponentServiceHolder.getInstance().addOauthEventInterceptorProxy(interceptor); try { AccessTokenDO accessTokenDO = new AccessTokenDO(); Map<String, Object> params = new HashMap<>(); OAuthUtil.invokePreRevocationBySystemListeners(accessTokenDO, params); verify(interceptor, times(1)).onPreTokenRevocationBySystem(accessTokenDO, params); } finally { OAuthComponentServiceHolder.getInstance().addOauthEventInterceptorProxy(null); } }Alternatively, centralize interceptor cleanup in
@AfterMethodif multiple tests use this pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java` around lines 1222 - 1266, Wrap each test that sets a global interceptor (e.g., in testInvokePreRevocationBySystemListeners_AccessTokenDO_WhenInterceptorEnabled, ...WhenInterceptorThrowsException) in a try-finally or move cleanup to an `@AfterMethod` so OAuthComponentServiceHolder.getInstance().addOauthEventInterceptorProxy(null) always runs; specifically ensure after adding the interceptor (via addOauthEventInterceptorProxy(interceptor)) the test body runs in try { ... } and the finally block calls addOauthEventInterceptorProxy(null) to guarantee cleanup and avoid cross-test interference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java`:
- Around line 1222-1266: Wrap each test that sets a global interceptor (e.g., in
testInvokePreRevocationBySystemListeners_AccessTokenDO_WhenInterceptorEnabled,
...WhenInterceptorThrowsException) in a try-finally or move cleanup to an
`@AfterMethod` so
OAuthComponentServiceHolder.getInstance().addOauthEventInterceptorProxy(null)
always runs; specifically ensure after adding the interceptor (via
addOauthEventInterceptorProxy(interceptor)) the test body runs in try { ... }
and the finally block calls addOauthEventInterceptorProxy(null) to guarantee
cleanup and avoid cross-test interference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e37b2c3-f6a0-4bc3-97fc-389f7bcba389
📒 Files selected for processing (1)
components/org.wso2.carbon.identity.oauth/src/test/java/org/wso2/carbon/identity/oauth/OAuthUtilTest.java
sadilchamishka
previously approved these changes
Mar 12, 2026
Thumimku
requested changes
Mar 12, 2026
| // The in-memory scopes may be mutated during validation. To avoid cache-key | ||
| // mismatches, retrieve the original scopes from the database before clearing | ||
| // the OAuth cache. | ||
| AccessTokenDO dbTokenDO = OAuth2Util.findAccessToken(accessToken, true); |
Contributor
There was a problem hiding this comment.
We already have AccessTokenDO object in the method, why we need to get the string and get the DO again?
and don't use OAuth2Util.findAccessToken, use the default Token Provider
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
4 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.
Implementation
This pull request introduces an enhancement to the OAuth token revocation process by ensuring that OAuth cache entries are cleared using the original, persisted scopes from the database rather than potentially mutated scopes from the in-memory token object. This helps prevent cache inconsistencies during token revocation. The key changes are as follows:
Enhancement to cache clearing logic:
clearOAuthCacheUsingPersistedScopestoOAuthUtil.java, which retrieves the original scopes from the database and uses them to clear OAuth cache entries, addressing cases where scopes may have been mutated during request processing.OAuth2Service.javato call the newclearOAuthCacheUsingPersistedScopesmethod during token revocation, ensuring cache consistency.Dependency and import updates:
OAuthServerConfigurationandOAuthRevocationRequestDTOinOAuthUtil.javato support the new cache clearing logic. [1] [2]Testing
Screen.Recording.2026-02-24.at.6.40.10.PM.mov
Summary by CodeRabbit
Bug Fixes
Tests