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.

Sign up for GitHub

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

fix scim2 group & role issue #4824

Merged
merged 7 commits into from
Aug 3, 2023
Merged

fix scim2 group & role issue #4824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3759fe5
fix scim2 group & role issue
DilshanSenarath Jul 30, 2023
84964da
fix the seperator issue
DilshanSenarath Jul 31, 2023
0a74fb0
Merge branch 'wso2:master' into dilshan
DilshanSenarath Aug 3, 2023
a648f7a
add a comment
DilshanSenarath Aug 3, 2023
ebb5f73
update the comment
DilshanSenarath Aug 3, 2023
4a4d763
update the comment
DilshanSenarath Aug 3, 2023
a15fe68
update the comment according to the suggestion
DilshanSenarath Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion components/role-mgt/org.wso2.carbon.identity.role.mgt.core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@
<groupId>org.wso2.carbon.identity.framework</groupId>
<artifactId>org.wso2.carbon.identity.event</artifactId>
</dependency>
<dependency>
<groupId>org.wso2.carbon.identity.inbound.provisioning.scim2</groupId>
<artifactId>org.wso2.carbon.identity.scim2.common</artifactId>
</dependency>
</dependencies>

<build>
Expand Down Expand Up @@ -133,7 +137,8 @@
org.wso2.carbon.database.utils.*;version="${org.wso2.carbon.database.utils.version.range}",
org.wso2.carbon.identity.core.util;version="${carbon.identity.package.import.version.range}",
org.wso2.carbon.identity.event.*; version="${carbon.identity.package.import.version.range}",
org.wso2.carbon.identity.central.log.mgt.utils;version="${carbon.identity.package.import.version.range}"
org.wso2.carbon.identity.central.log.mgt.utils;version="${carbon.identity.package.import.version.range}",
org.wso2.carbon.identity.scim2.common.utils; version="${org.wso2.carbon.identity.scim2.common.version}"
</Import-Package>
<Export-Package>
!org.wso2.carbon.identity.role.mgt.core.internal,
Expand Down
4 changes: 3 additions & 1 deletion ...y.role.mgt.core/src/main/java/org/wso2/carbon/identity/role/mgt/core/dao/RoleDAOImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.wso2.carbon.identity.role.mgt.core.internal.RoleManagementServiceComponentHolder;
import org.wso2.carbon.identity.role.mgt.core.util.GroupIDResolver;
import org.wso2.carbon.identity.role.mgt.core.util.UserIDResolver;
import org.wso2.carbon.identity.scim2.common.utils.SCIMCommonUtils;
import org.wso2.carbon.user.api.RealmConfiguration;
import org.wso2.carbon.user.api.UserRealm;
import org.wso2.carbon.user.api.UserStoreException;
Expand Down Expand Up @@ -1479,7 +1480,8 @@ public String getRoleNameByID(String roleID, String tenantDomain) throws Identit
+ tenantDomain;
throw new IdentityRoleManagementServerException(UNEXPECTED_SERVER_ERROR.getCode(), errorMessage, e);
}
if (roleName == null) {
// Verify whether the roleName is either null or it's not contain any prefix Application/Internal
if (roleName == null || !SCIMCommonUtils.isHybridRole(roleName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is not very clear. You are trying to throw the error message "A role doesn't exist with id: " + roleID + " in the tenant domain: " + tenant domain, if the role name is null or the role name does not start with "Application" or "Internal". But from this method, we are trying to get the role name by passing the role id to the table IDN_SCIM_GROUP. So based on the logic if we get a role that does not contain the prefix "Application" or "Internal" it will throw an error. Is this expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the current implementation, IDN_SCIM_GROUP table stores both roles and groups. This group endpoint code (https://github.com/wso2-support/identity-inbound-provisioning-scim2/blob/645b50a897929cd5cb3d32c5a1af3b6c9c6b933c/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java#L2620-L2624) uses this method (https://github.com/wso2-support/identity-inbound-provisioning-scim2/blob/645b50a897929cd5cb3d32c5a1af3b6c9c6b933c/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java#L2744) to differentiate between roles and groups in the same table. Since we should only get roles name (Exclude group names), we have to do a filtering. That's why I have included this logic.

String errorMessage = "A role doesn't exist with id: " + roleID + " in the tenantDomain: " + tenantDomain;
throw new IdentityRoleManagementClientException(ROLE_NOT_FOUND.getCode(), errorMessage);
}
Expand Down
9 changes: 9 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1758,6 +1758,13 @@
<artifactId>xmlbeans</artifactId>
<version>${xmlbeans.version}</version>
</dependency>

<!-- Identity inbound provisioning scim2 component -->
<dependency>
<groupId>org.wso2.carbon.identity.inbound.provisioning.scim2</groupId>
<artifactId>org.wso2.carbon.identity.scim2.common</artifactId>
<version>${org.wso2.carbon.identity.scim2.common.version}</version>
</dependency>
</dependencies>

</dependencyManagement>
Expand Down Expand Up @@ -1811,6 +1818,8 @@
<org.wso2.carbon.identity.organization.management.core.version.range>[1.0.0, 2.0.0)
</org.wso2.carbon.identity.organization.management.core.version.range>

<org.wso2.carbon.identity.scim2.common.version>3.4.18</org.wso2.carbon.identity.scim2.common.version>

<!--Carbon registry version-->
<org.wso2.carbon.registry.version>4.8.12</org.wso2.carbon.registry.version>
<carbon.registry.common.imp.pkg.version.range>[0.0.0,1.0.0)</carbon.registry.common.imp.pkg.version.range>
Expand Down