From f8dc3cb7ad4e658e8c6fe6104d7f5cddb5fc9446 Mon Sep 17 00:00:00 2001 From: Bimsara Bodaragama Date: Tue, 2 Jul 2024 13:00:20 +0530 Subject: [PATCH 1/7] Fix SCIM error message for invalid request values in V1 and V2 Roles Remove-Add-Replace operations (fixes #20334) --- .../scim2/common/impl/SCIMRoleManager.java | 23 +++++++++++++------ .../scim2/common/impl/SCIMRoleManagerV2.java | 20 +++++++++++----- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java b/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java index 5b792e3f1..889fbb6ac 100644 --- a/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java +++ b/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java @@ -771,21 +771,30 @@ private void updatePermissions(String roleId, List permissionOpe private void prepareAddedRemovedGroupLists(Set addedGroupsIds, Set removedGroupsIds, Set replacedGroupsIds, PatchOperation groupOperation, - Map groupObject, List groupListOfRole) { + Map groupObject, List groupListOfRole) + throws BadRequestException { + + String value = groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE); + + if (StringUtils.isBlank(value)) { + throw new BadRequestException( + "Updating groups of the role by display name is not supported. Update using group id instead.", + ResponseCodeConstants.INVALID_SYNTAX); + } switch (groupOperation.getOperation()) { case (SCIMConstants.OperationalConstants.ADD): - removedGroupsIds.remove(groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE)); - if (!isGroupExist(groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE), groupListOfRole)) { - addedGroupsIds.add(groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE)); + removedGroupsIds.remove(value); + if (!isGroupExist(value, groupListOfRole)) { + addedGroupsIds.add(value); } break; case (SCIMConstants.OperationalConstants.REMOVE): - addedGroupsIds.remove(groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE)); - removedGroupsIds.add(groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE)); + addedGroupsIds.remove(value); + removedGroupsIds.add(value); break; case (SCIMConstants.OperationalConstants.REPLACE): - replacedGroupsIds.add(groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE)); + replacedGroupsIds.add(value); break; } } diff --git a/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2.java b/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2.java index 63cfab4b9..47bd3b2b2 100644 --- a/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2.java +++ b/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2.java @@ -1214,19 +1214,27 @@ private List getUserIDList(List userList, String tenantDomain) t private void prepareInitialGroupLists(Set givenAddedGroupsIds, Set givenRemovedGroupsIds, Set givenReplacedGroupsIds, PatchOperation groupOperation, - Map groupObject) { + Map groupObject) throws BadRequestException { + + String value = groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE); + + if (StringUtils.isBlank(value)) { + throw new BadRequestException( + "Updating groups of the role by display name is not supported. Update using group id instead.", + ResponseCodeConstants.INVALID_SYNTAX); + } switch (groupOperation.getOperation()) { case (SCIMConstants.OperationalConstants.ADD): - givenRemovedGroupsIds.remove(groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE)); - givenAddedGroupsIds.add(groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE)); + givenRemovedGroupsIds.remove(value); + givenAddedGroupsIds.add(value); break; case (SCIMConstants.OperationalConstants.REMOVE): - givenAddedGroupsIds.remove(groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE)); - givenRemovedGroupsIds.add(groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE)); + givenAddedGroupsIds.remove(value); + givenRemovedGroupsIds.add(value); break; case (SCIMConstants.OperationalConstants.REPLACE): - givenReplacedGroupsIds.add(groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE)); + givenReplacedGroupsIds.add(value); break; default: break; From 04d7b00759483fbc11eaa0f126b2f74727adb734 Mon Sep 17 00:00:00 2001 From: Bimsara Bodaragama Date: Tue, 2 Jul 2024 15:17:14 +0530 Subject: [PATCH 2/7] Update the licenses of SCIMRoleManagerV1 with the new header format for 2020-2024 --- .../carbon/identity/scim2/common/impl/SCIMRoleManager.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java b/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java index 889fbb6ac..d821069ed 100644 --- a/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java +++ b/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java @@ -1,7 +1,7 @@ /* - * Copyright (c) 2020, WSO2 Inc. (http://www.wso2.org) All Rights Reserved. + * Copyright (c) 2020-2024, WSO2 LLC. (http://www.wso2.com). * - * WSO2 Inc. licenses this file to you under the Apache License, + * WSO2 LLC. licenses this file to you under the Apache License, * Version 2.0 (the "License"); you may not use this file except * in compliance with the License. * You may obtain a copy of the License at @@ -11,7 +11,7 @@ * Unless required by applicable law or agreed to in writing, * software distributed under the License is distributed on an * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the + * KIND, either express or implied. See the License for the * specific language governing permissions and limitations * under the License. */ From b10c8833f244e7001a416957709676263049bb65 Mon Sep 17 00:00:00 2001 From: Bimsara Bodaragama Date: Thu, 4 Jul 2024 09:52:49 +0530 Subject: [PATCH 3/7] Fix invalidValue code and update to generic error message for group ID requirement --- .../carbon/identity/scim2/common/impl/SCIMRoleManager.java | 5 ++--- .../carbon/identity/scim2/common/impl/SCIMRoleManagerV2.java | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java b/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java index d821069ed..b0402b85d 100644 --- a/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java +++ b/components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManager.java @@ -777,9 +777,8 @@ private void prepareAddedRemovedGroupLists(Set addedGroupsIds, Set givenAddedGroupsIds, Set Date: Fri, 5 Jul 2024 11:16:54 +0530 Subject: [PATCH 4/7] Test: Add testPatchRoleWithGroupDisplayNameInsteadOfGroupIdThrowingErrors for SCIM error handling in V2 Roles operations --- .../common/impl/SCIMRoleManagerV2Test.java | 117 ++++++++++++++++++ .../src/test/resources/testng.xml | 1 + 2 files changed, 118 insertions(+) create mode 100644 components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java diff --git a/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java b/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java new file mode 100644 index 000000000..e1381443b --- /dev/null +++ b/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java @@ -0,0 +1,117 @@ +/* + * Copyright (c) 2024, WSO2 LLC. (http://www.wso2.com). + * + * WSO2 LLC. licenses this file to you under the Apache License, + * Version 2.0 (the "License"); you may not use this file except + * in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.wso2.carbon.identity.scim2.common.impl; + +import org.mockito.Mock; +import org.powermock.api.mockito.PowerMockito; +import org.powermock.core.classloader.annotations.PrepareForTest; +import org.powermock.modules.testng.PowerMockTestCase; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; +import org.wso2.carbon.identity.core.util.IdentityUtil; +import org.wso2.carbon.identity.role.v2.mgt.core.RoleManagementService; +import org.wso2.carbon.identity.role.v2.mgt.core.exception.IdentityRoleManagementException; +import org.wso2.charon3.core.exceptions.BadRequestException; +import org.wso2.charon3.core.exceptions.CharonException; +import org.wso2.charon3.core.exceptions.ConflictException; +import org.wso2.charon3.core.exceptions.ForbiddenException; +import org.wso2.charon3.core.exceptions.NotFoundException; +import org.wso2.charon3.core.protocol.ResponseCodeConstants; +import org.wso2.charon3.core.schema.SCIMConstants; +import org.wso2.charon3.core.utils.codeutils.PatchOperation; + +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.mockito.Mockito.when; +import static org.mockito.MockitoAnnotations.initMocks; +import static org.testng.AssertJUnit.assertEquals; + +/** + * Contains the unit test cases for SCIMRoleManagerV2. + */ +@PrepareForTest({SCIMRoleManagerV2.class, IdentityUtil.class}) +public class SCIMRoleManagerV2Test extends PowerMockTestCase { + + private static final String SAMPLE_TENANT_DOMAIN = "carbon.super"; + private static final String SAMPLE_VALID_ROLE_ID = "595f5508-f286-446a-86c4-5071e07b98fc"; + private static final String SAMPLE_GROUP_NAME = "testGroup"; + private static final String SAMPLE_VALID_ROLE_NAME = "admin"; + private static final int BAD_REQUEST = 400; + + @Mock + private RoleManagementService roleManagementService; + @Mock + private SCIMRoleManagerV2 scimRoleManagerV2; + + @BeforeClass + public void setUpClass() { + + initMocks(this); + } + + @BeforeMethod + public void setUpMethod() { + + scimRoleManagerV2 = PowerMockito.spy(new SCIMRoleManagerV2(roleManagementService, SAMPLE_TENANT_DOMAIN)); + PowerMockito.mockStatic(IdentityUtil.class); + } + + @DataProvider(name = "scimOperations") + public Object[][] provideScimOperations() { + + return new Object[][]{ + {SCIMConstants.OperationalConstants.ADD}, + {SCIMConstants.OperationalConstants.REMOVE}, + {SCIMConstants.OperationalConstants.REPLACE} + }; + } + + @Test(dataProvider = "scimOperations") + public void testPatchRoleWithGroupDisplayNameInsteadOfGroupIdThrowingErrors(String operation) + throws IdentityRoleManagementException, ForbiddenException, ConflictException, NotFoundException, + CharonException { + + Map> patchOperations = new HashMap<>(); + Map valueMap = new HashMap<>(); + valueMap.put(SCIMConstants.CommonSchemaConstants.DISPLAY, SAMPLE_GROUP_NAME); + valueMap.put(SCIMConstants.CommonSchemaConstants.VALUE, null); + + PatchOperation patchOperation = new PatchOperation(); + patchOperation.setOperation(operation); + patchOperation.setAttributeName(SCIMConstants.RoleSchemaConstants.GROUPS); + patchOperation.setValues(valueMap); + patchOperations.put(SCIMConstants.RoleSchemaConstants.GROUPS, Collections.singletonList(patchOperation)); + + when(roleManagementService.getRoleNameByRoleId(SAMPLE_VALID_ROLE_ID, SAMPLE_TENANT_DOMAIN)) + .thenReturn(SAMPLE_VALID_ROLE_NAME); + + try { + scimRoleManagerV2.patchRole(SAMPLE_VALID_ROLE_ID, patchOperations); + } catch (BadRequestException e) { + assertEquals(BAD_REQUEST, e.getStatus()); + assertEquals(ResponseCodeConstants.INVALID_VALUE, e.getScimType()); + assertEquals("Group id is required to update group of the role.", e.getDetail()); + } + } +} diff --git a/components/org.wso2.carbon.identity.scim2.common/src/test/resources/testng.xml b/components/org.wso2.carbon.identity.scim2.common/src/test/resources/testng.xml index 83c5f989d..e9b0cdcfd 100644 --- a/components/org.wso2.carbon.identity.scim2.common/src/test/resources/testng.xml +++ b/components/org.wso2.carbon.identity.scim2.common/src/test/resources/testng.xml @@ -34,6 +34,7 @@ + From e15eeb8cbecd24f16671dd1c8787b128013b42db Mon Sep 17 00:00:00 2001 From: Bimsara Bodaragama Date: Fri, 5 Jul 2024 14:12:56 +0530 Subject: [PATCH 5/7] refactor: instantiate SCIMRoleManagerV2 in tests --- .../identity/scim2/common/impl/SCIMRoleManagerV2Test.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java b/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java index e1381443b..e407e0eb2 100644 --- a/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java +++ b/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java @@ -61,7 +61,7 @@ public class SCIMRoleManagerV2Test extends PowerMockTestCase { @Mock private RoleManagementService roleManagementService; - @Mock + private SCIMRoleManagerV2 scimRoleManagerV2; @BeforeClass @@ -73,8 +73,8 @@ public void setUpClass() { @BeforeMethod public void setUpMethod() { - scimRoleManagerV2 = PowerMockito.spy(new SCIMRoleManagerV2(roleManagementService, SAMPLE_TENANT_DOMAIN)); PowerMockito.mockStatic(IdentityUtil.class); + scimRoleManagerV2 = new SCIMRoleManagerV2(roleManagementService, SAMPLE_TENANT_DOMAIN); } @DataProvider(name = "scimOperations") From c8cf26b7535e056067f1a2e42d11ae66f55e7c77 Mon Sep 17 00:00:00 2001 From: Bimsara Bodaragama Date: Fri, 5 Jul 2024 14:30:18 +0530 Subject: [PATCH 6/7] refactor: remove SCIMRoleManagerV2 from @PrepareForTest --- .../identity/scim2/common/impl/SCIMRoleManagerV2Test.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java b/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java index e407e0eb2..8a5727e8b 100644 --- a/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java +++ b/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java @@ -50,7 +50,7 @@ /** * Contains the unit test cases for SCIMRoleManagerV2. */ -@PrepareForTest({SCIMRoleManagerV2.class, IdentityUtil.class}) +@PrepareForTest({IdentityUtil.class}) public class SCIMRoleManagerV2Test extends PowerMockTestCase { private static final String SAMPLE_TENANT_DOMAIN = "carbon.super"; From e97c090d1bf26f394511fb6fe8c8dac6afec2900 Mon Sep 17 00:00:00 2001 From: Bimsara Bodaragama Date: Fri, 5 Jul 2024 14:59:41 +0530 Subject: [PATCH 7/7] fix: replace JUnit assertEquals with TestNG assertEquals --- .../identity/scim2/common/impl/SCIMRoleManagerV2Test.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java b/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java index 8a5727e8b..96eca05c0 100644 --- a/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java +++ b/components/org.wso2.carbon.identity.scim2.common/src/test/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2Test.java @@ -45,7 +45,7 @@ import static org.mockito.Mockito.when; import static org.mockito.MockitoAnnotations.initMocks; -import static org.testng.AssertJUnit.assertEquals; +import static org.testng.Assert.assertEquals; /** * Contains the unit test cases for SCIMRoleManagerV2.