-
Notifications
You must be signed in to change notification settings - Fork 85
Provide service for selective role sharing with application sharing #525
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
Conversation
- Provide the support to get application with filtering option. - Add new error message for application sharing - Moved application share operations models to a new package
# Conflicts: # components/org.wso2.carbon.identity.organization.management.application/src/main/java/org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.java # components/org.wso2.carbon.identity.organization.management.application/src/main/java/org/wso2/carbon/identity/organization/management/application/constant/OrgApplicationMgtConstants.java # components/org.wso2.carbon.identity.organization.management.application/src/main/java/org/wso2/carbon/identity/organization/management/application/internal/OrgApplicationMgtDataHolder.java # components/org.wso2.carbon.identity.organization.management.application/src/main/java/org/wso2/carbon/identity/organization/management/application/internal/OrgApplicationMgtServiceComponent.java
- Convert logs to debug - Convert the ShareRoleMgt behaviour to previous behavior
- Improve logging - Add unit tests - Reduce using start tenant flows - Optimize to execute less DB queries
…itiationMode for future-impacting policy-holder organizations
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 PR adds support for selective role sharing within application-sharing flows, ports existing logic from the organization-management extension, and extends resource sharing to include applications.
- Introduce
getResourceSharingPolicyByInitiatingOrgIdmethods in DAO, service interface, and service impl - Extend
ResourceTypeandPolicyEnumto includeAPPLICATIONresources - Add new SQL constant and update POM version for organization-management core
- Implement comprehensive application share handlers and utility classes (e.g., hierarchy processing, SCIM filter parsing, filter builders)
Reviewed Changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Bump <identity.organization.management.core.version> to 1.1.33 |
| ResourceSharingPolicyHandlerDAOImpl.java | Implement getResourceSharingPolicyByInitiatingOrgId |
| ResourceSharingSQLConstants.java | Add GET_RESOURCE_SHARING_POLICIES_WITH_INITIATING_ORG_ID SQL template |
| ResourceType.java & PolicyEnum.java | Add APPLICATION as a valid resource type and include in all policies |
| FilterQueriesUtil.java | Introduce generic SCIM filter-to-query builder with various operations |
| SharedRoleMgtHandler.java / SharedRoleMgtListener.java / SharedRoleMgtHandlerTest.java | Enhance event handling for application share and selective role updates |
| OrgApplicationShareProcessor.java | Add BFS-based priority sorting for selective organization shares |
Comments suppressed due to low confidence (2)
components/org.wso2.carbon.identity.organization.resource.sharing.policy.management/src/main/java/org/wso2/carbon/identity/organization/resource/sharing/policy/management/dao/ResourceSharingPolicyHandlerDAOImpl.java:476
- The new
getResourceSharingPolicyByInitiatingOrgIdmethod is not covered by unit tests. Please add tests for this DAO method to verify grouping logic and empty-case behavior.
return result.stream().collect(Collectors.groupingBy(
components/org.wso2.carbon.identity.organization.resource.sharing.policy.management/src/main/java/org/wso2/carbon/identity/organization/resource/sharing/policy/management/constant/ResourceSharingSQLConstants.java:73
- The SQL string includes semicolons before the AND clauses, which will break the query. Remove the semicolons so that it reads
... = :INITIATING_ORG_ID AND ... = :RESOURCE_TYPE AND ... = :RESOURCE_ID.
"WHERE rsp.UM_INITIATING_ORG_ID = :" + SQLPlaceholders.DB_SCHEMA_COLUMN_NAME_INITIATING_ORG_ID
...ava/org/wso2/carbon/identity/organization/management/application/util/FilterQueriesUtil.java
Show resolved
Hide resolved
...main/java/org/wso2/carbon/identity/organization/management/handler/SharedRoleMgtHandler.java
Show resolved
Hide resolved
.../org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.java
Show resolved
Hide resolved
.../org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.java
Outdated
Show resolved
Hide resolved
.../org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.java
Outdated
Show resolved
Hide resolved
|
|
||
| String filter = RoleConstants.AUDIENCE_ID + " " + RoleConstants.EQ + " " + appId; | ||
| return getRoleManagementServiceV2().getRoles(filter, null, 0, null, null, tenantDomain); | ||
| } |
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.
Refactor the code to the role handler.
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.
Check the role amount returned.
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.
Add pagination here. Same as the listener.
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.
Check whether the roles get unshared.
...2/carbon/identity/organization/management/application/util/OrgApplicationShareProcessor.java
Show resolved
Hide resolved
...ity/organization/resource/sharing/policy/management/dao/ResourceSharingPolicyHandlerDAO.java
Outdated
Show resolved
Hide resolved
.../org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.java
Outdated
Show resolved
Hide resolved
.../org/wso2/carbon/identity/organization/management/application/OrgApplicationManagerImpl.java
Outdated
Show resolved
Hide resolved
...main/java/org/wso2/carbon/identity/organization/management/handler/SharedRoleMgtHandler.java
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (24.71%) 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 @@
## main #525 +/- ##
============================================
- Coverage 56.18% 52.90% -3.28%
- Complexity 1988 2002 +14
============================================
Files 186 199 +13
Lines 10977 12505 +1528
Branches 1640 1947 +307
============================================
+ Hits 6167 6616 +449
- Misses 4269 5255 +986
- Partials 541 634 +93
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:
|
|
Unit test to improve coverage will be tracked via: wso2/product-is#24757 |
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
|
PR builder started |
|
PR builder completed |
jenkins-is-staging
left a comment
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/16434336417
ef355ce
ef355ce to
4f6a010
Compare
Purpose
Issue: wso2/product-is#21100
Ported the implementation from: #512
Additional changes introduced:
Related PR(s)
wso2/identity-api-server#929