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

Improve cleaning issued tokens of an organization user when user is deleting #2680

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ShanChathusanda93
Copy link
Contributor

Proposed changes in this pull request

  • $subject

Copy link

codecov bot commented Jan 20, 2025

Codecov Report

Attention: Patch coverage is 29.23077% with 46 lines in your changes missing coverage. Please review.

Project coverage is 56.55%. Comparing base (4e516a5) to head (34e46de).

Files with missing lines Patch % Lines
...java/org/wso2/carbon/identity/oauth/OAuthUtil.java 29.23% 42 Missing and 4 partials ⚠️

❌ Your patch check has failed because the patch coverage (29.23%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2680      +/-   ##
============================================
+ Coverage     56.17%   56.55%   +0.37%     
+ Complexity     8633     8500     -133     
============================================
  Files           654      654              
  Lines         49129    48268     -861     
  Branches       9817     9648     -169     
============================================
- Hits          27598    27297     -301     
+ Misses        17586    17095     -491     
+ Partials       3945     3876      -69     
Flag Coverage Δ
unit 39.49% <29.23%> (-0.01%) ⬇️

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.

@ShanChathusanda93 ShanChathusanda93 force-pushed the org-token-clean-branch branch 2 times, most recently from 7bb48f6 to 59463df Compare January 25, 2025 06:37
@@ -763,8 +763,11 @@ private static AuthenticatedUser buildAuthenticatedUser(UserStoreManager userSto
return authenticatedUser;
}

// Organization SSO user flow
authenticatedUser.setUserName(userId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure whether removing this code will not affect existing flows by chance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added to cater the SSO flow token revokation. Before calling the toke revoke method the user id will be added as the username from the following code from the OAuthUtil -> revokeTokens method.

if (authenticatedUser.getUserResidentOrganization() != null) {
            try {
                userResidentTenant = OAuthComponentServiceHolder.getInstance().getOrganizationManager()
                        .resolveTenantDomain(authenticatedUser.getUserResidentOrganization());
                if (OrganizationManagementUtil.isOrganization(userResidentTenant)) {
                    authenticatedUserName = authenticatedUser.getUserName();
                    authenticatedUser.setUserName(authenticatedUser.getUserId());
                }
            } catch (OrganizationManagementException | UserIdNotFoundException e) {
                throw new UserStoreException("Error occurred while constructing the authenticated user.", e);
            }
        }

isErrorOnRevokingTokens = processTokenRevocation(clientIds, authenticatedUser, userStoreDomain, username);
if (authenticatedUser.getUserResidentOrganization() != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why authenticatedUser.getUserResidentOrganization() != null logic is separately handled in 1098 and 1081 lines?
Can't we handled these in a single if block?

Comment on lines 1109 to 1126
if (StringUtils.isNotEmpty(userResidentTenant) &&
OrganizationManagementUtil.isOrganization(userResidentTenant)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Login a sub org user into a B2B app and swicthing to another below sub org can also set a sub org as userResidentTenant
Have you tested that as well?

If the flow I mentioned is handling from here, the added comment is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the logic

@ShanChathusanda93 ShanChathusanda93 force-pushed the org-token-clean-branch branch 4 times, most recently from 1b39ab0 to efee736 Compare January 29, 2025 20:42
@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

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.

4 participants