Set parent user's user store domain in shared token revoke flow #2664
Conversation
This file contains 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
SujanSanjula96
approved these changes
Jan 7, 2025
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2664 +/- ##
============================================
+ Coverage 55.48% 55.77% +0.28%
+ Complexity 8536 8416 -120
============================================
Files 632 632
Lines 49870 48515 -1355
Branches 9295 8965 -330
============================================
- Hits 27672 27058 -614
+ Misses 18254 17612 -642
+ Partials 3944 3845 -99
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
AnuradhaSK
reviewed
Jan 7, 2025
Comment on lines
+1357
to
+1358
| String parentUserId = OAuthComponentServiceHolder.getInstance().getOrganizationUserSharingService() | ||
| .getUserAssociation(userId, accessingOrgId) |
There was a problem hiding this comment.
Any chance of returning null from getUserAssociation(...) ? If yes, do a null check to avoid NPE
Comment on lines
+751
to
+753
| Optional<String> parentUserStoreDomain = getUserStoreDomainOfParentUser( | ||
| userId, accessingOrg, tenantDomain); | ||
| parentUserStoreDomain.ifPresent(authenticatedUser::setUserStoreDomain); |
There was a problem hiding this comment.
This part may need to move to a else block under below if block.
Shall we try the flow for sub-org and another level of sub-org. No need to add a user store.
AnuradhaSK
approved these changes
Jan 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed changes in this pull request
Purpose
When revoking an access token in a shared user flow, the authorized user's User Store Domain is set to the shared user's domain. Therefore, when retrieving the clientIDs from here, it will only retrieve the clientIDs associated with the shared user's User Store Domain.
However, when creating access tokens for shared users the User Store Domain of the parent user is used here and here. Hence these tokens are not getting revoked.
Approach
Hence the access token revoke logic for shared user flow has to be improved to handle the user's domain correctly which will eventually detect all the clientIDs issued.
This is an alternative approach to the fix wso2-extensions/identity-oauth2-grant-organization-switch#38. In which we amend the user store domain when the token is issued. Which would lead to user store mismatch between the token and the actual user. Here, we change the logic in the token revoke method.