Skip to content
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

Inherit parent user domain in shared token revoke flow #2664

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

HasiniSama
Copy link
Contributor

@HasiniSama HasiniSama commented Jan 7, 2025

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.

Tested Flows (Unit test added to each)

Scenario User Domain Authz User Tenant Authorized Organization Grant Type IDP_ID
Initial login with no switch Primary Username -1234 NONE authorization_code 1
Switch to OrgID_01 Primary Username -1234 OrgID_01 organization_switch 1
Switch to OrgID_02 Primary Username -1234 OrgID_02 organization_switch 1
Initial login with no switch Secondary Username -1234 NONE authorization_code 1
Switch to OrgID_01 Secondary Username -1234 OrgID_01 organization_switch 1
Switch to OrgID_02 Secondary Username -1234 OrgID_02 organization_switch 1
SSO login to OrgID_01 Primary* Username 33 NONE authorization_code 33
SSO login to OrgID_01 Federated User ID 33 OrgID_01 authorization_code 3
Switch to OrgID_02 after SSO Federated User ID 33 OrgID_02 organization_switch 3
Federated login without JIT Federated Federated Username -1234 NONE authorization_code 38
JIT provisioned Federated login Secondary JIT given Username -1234 NONE authorization_code 1

*The access token issued by the shared application to the parent application. This doesn't get deleted from the database when the user logs out.

SujanSanjula96
SujanSanjula96 previously approved these changes Jan 7, 2025
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.

Project coverage is 55.69%. Comparing base (32cb723) to head (23ff236).

Files with missing lines Patch % Lines
...java/org/wso2/carbon/identity/oauth/OAuthUtil.java 77.77% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2664      +/-   ##
============================================
+ Coverage     55.66%   55.69%   +0.02%     
+ Complexity     8435     8317     -118     
============================================
  Files           632      632              
  Lines         48587    48605      +18     
  Branches       9300     9299       -1     
============================================
+ Hits          27048    27072      +24     
+ Misses        17670    17658      -12     
- Partials       3869     3875       +6     
Flag Coverage Δ
unit 39.03% <77.77%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AnuradhaSK
AnuradhaSK previously approved these changes Jan 7, 2025
@HasiniSama HasiniSama dismissed stale reviews from AnuradhaSK and SujanSanjula96 via c534d4f January 9, 2025 11:17
@HasiniSama HasiniSama force-pushed the fix-user-store-domain-issue branch from 87b1244 to c534d4f Compare January 9, 2025 11:17
@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/12688880386

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/12688880386
Status: failure

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/12703591457

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/12703591457
Status: failure

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/12802533205

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/12802533205
Status: failure

@HasiniSama HasiniSama changed the title Set parent user's user store domain in shared token revoke flow Inherit parent user domain in shared token revoke flow Jan 17, 2025
@HasiniSama HasiniSama force-pushed the fix-user-store-domain-issue branch 5 times, most recently from e534b83 to c81096d Compare January 17, 2025 08:41
@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/12826300018

@HasiniSama HasiniSama force-pushed the fix-user-store-domain-issue branch from c81096d to 23ff236 Compare January 17, 2025 11:16
@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/12826300018
Status: success

Copy link

@jenkins-is-staging jenkins-is-staging left a 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/12826300018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants