-
Notifications
You must be signed in to change notification settings - Fork 398
Revoke access tokens that are bound to an SSO session when that session is invalid. #2943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revoke access tokens that are bound to an SSO session when that session is invalid. #2943
Conversation
...ain/java/org/wso2/carbon/identity/oauth2/token/bindings/impl/SSOSessionBasedTokenBinder.java
Show resolved
Hide resolved
...ain/java/org/wso2/carbon/identity/oauth2/token/bindings/impl/SSOSessionBasedTokenBinder.java
Show resolved
Hide resolved
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2TokenUtil.java
Outdated
Show resolved
Hide resolved
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2TokenUtil.java
Outdated
Show resolved
Hide resolved
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2TokenUtil.java
Outdated
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Outdated
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Show resolved
Hide resolved
...y.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/TokenValidationHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
158414f
to
e15d74d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2943 +/- ##
============================================
- Coverage 57.28% 57.24% -0.04%
- Complexity 9251 9275 +24
============================================
Files 669 669
Lines 51972 52317 +345
Branches 11637 11669 +32
============================================
+ Hits 29770 29948 +178
- Misses 18024 18153 +129
- Partials 4178 4216 +38
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:
|
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
// Validate SSO session bound token. | ||
if (OAuth2Constants.TokenBinderType.SSO_SESSION_BASED_TOKEN_BINDER.equals(oAuthAppDO.getTokenBindingType())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check should comes as last logic, after checking for isPresent and isValidTokenBinding method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally placed the code block earlier because, within the isValidTokenBinding method [1], we cannot perform token revocation—at that point we don’t have access to the access token details.
By executing the session check before the token binding validation, we ensure that the session validity is verified regardless of whether the validateTokenBinding configuration is enabled. As a result, the subsequent binding reference validation will only be executed if the session is confirmed to be valid.
If we were to do it the other way around—relying solely on isValidTokenBinding [1]—then the session context would be checked there. If invalid, it would simply return false. At that point, before throwing the exception, we would still need to perform token revocation. But this would mean re-checking the session context, because we cannot conclusively say that the false result was due to session invalidation—it might have been triggered by some other reason.
That’s why I decided to proceed with the current approach. Please correct me if my understanding is off.
e15d74d
to
3e06aae
Compare
3e06aae
to
116b20b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request implements token revocation for access tokens bound to SSO sessions when those sessions become invalid. The changes add support for automatically revoking access tokens when session invalidation is detected during introspection API calls, resource access API calls, and refresh token grant flows.
Key changes include:
- Moved SSO session validation logic from
TokenValidationHandler
toOAuth2Util
for reusability - Added automatic token revocation when bound SSO sessions become invalid
- Enhanced refresh token grant flow to validate SSO session binding before issuing new tokens
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
TokenValidationHandlerTest.java | Updated test to use new OAuth2Util method for SSO session validation |
OAuth2UtilTest.java | Added comprehensive test coverage for new SSO session validation and token binding methods |
TokenValidationHandler.java | Refactored to use OAuth2Util.isTokenBoundToActiveSSOSession method |
OAuth2Util.java | Added utility methods for token binding validation, SSO session checks, and token revocation |
RefreshGrantHandler.java | Enhanced to validate SSO session binding during refresh token flow |
SSOSessionBasedTokenBinder.java | Refactored validation logic to use OAuth2Util helper method |
AbstractTokenBinder.java | Simplified token binding validation by using OAuth2Util helper method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
.../src/main/java/org/wso2/carbon/identity/oauth2/token/handlers/grant/RefreshGrantHandler.java
Outdated
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Outdated
Show resolved
Hide resolved
PR builder started |
PR builder completed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/18302578864
private static void revokeAccessToken(AccessTokenDO accessTokenDO) throws IdentityOAuth2Exception { | ||
|
||
if (log.isDebugEnabled()) { | ||
log.debug("Revoking token: " + accessTokenDO.getTokenId() + " for consumer key: " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we logging tokenId in other logs? can you check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes in several places. For an example refer
Line 56 in 57c49b3
"identifier: " + accessTokenDO.getTokenId()); |
* @param cookieName Cookie name | ||
* @return Token binding value | ||
*/ | ||
public static Optional<String> getTokenBindingValue(OAuth2AccessTokenReqDTO oAuth2AccessTokenReqDTO, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be a public util method or should go under session binder? Are we using this method in different places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is utilized in SSOSessionBasedTokenBinder
and AbstractTokenBinder
...y.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/TokenValidationHandler.java
Outdated
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Outdated
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Outdated
Show resolved
Hide resolved
...so2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2Util.java
Outdated
Show resolved
Hide resolved
...y.oauth/src/main/java/org/wso2/carbon/identity/oauth2/validators/TokenValidationHandler.java
Show resolved
Hide resolved
if (!OAuth2Util.isTokenBoundToActiveSSOSession(tokenBinding, validationDataDO.getAccessToken(), oAuthAppDO, | ||
validationDataDO.getAuthorizedUser())) { | ||
log.debug("Token is not bound to an active SSO session."); | ||
throw new IdentityOAuth2Exception("Token binding validation failed. Token is not bound to an " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a behavioural change.
00f584e
to
c25aeb2
Compare
PR builder started |
PR builder completed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/18360361622
.revokeAccessToken(revokeRequestDTO, accessTokenDO); | ||
} | ||
} catch (InvalidOAuthClientException | IdentityOAuth2Exception | UserIdNotFoundException e) { | ||
log.error("Error while revoking SSO session bound access token.", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we are logging it without throwing the error to upper level. Do we need to handle these errors gracefully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention was that a failure to revoke the token should not interrupt the introspection flow. To avoid interrupting the introspection flow, the exception for a failed token revocation is deliberately not thrown.
Proposed changes in this pull request
$Subject
Changes :
• Introspection API call
• Resource access API call
• Refresh token grant flow
Resolves,
Flows Tested
Opaque Token | Revoke token config enabled
Opaque Token | Revoke token config disabled
JWT Token | Revoke config enabled and disabled
Behavior Comparison
json { "active": false }
json { "schemas": [ "urn:ietf:params:scim:api:messages:2.0:Error" ], "detail": "Authorization failure. Authorization information was invalid or missing from your request.", "status": 401 }
json { "error_description": "Token binding validation failed. Token is not bound to an active SSO session.", "error": "invalid_grant" }
Developer Checklist (Mandatory)
product-is
issue to track any behavioral change or migration impact.Checklist (for reviewing)
General
Functionality
Code
Tests
Security
Documentation