Fix: SCIM error message for invalid request values in V1 and V2 Roles Remove-Add-Replace operations (fixes #20334)

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.
  */
@@ -771,21 +771,29 @@ private void updatePermissions(String roleId, List<PatchOperation> permissionOpe
 
     private void prepareAddedRemovedGroupLists(Set<String> addedGroupsIds, Set<String> removedGroupsIds,
                                                Set<String> replacedGroupsIds, PatchOperation groupOperation,
-                                               Map<String, String> groupObject, List<GroupBasicInfo> groupListOfRole) {
+                                               Map<String, String> groupObject, List<GroupBasicInfo> groupListOfRole)
+            throws BadRequestException {
+
+        String value = groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE);
+
+        if (StringUtils.isBlank(value)) {
+            throw new BadRequestException("Group id is required to update group of the role.",
+                    ResponseCodeConstants.INVALID_VALUE);
+        }
 
         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;
         }
     }

components/org.wso2.carbon.identity.scim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMRoleManagerV2.java

@@ -1214,19 +1214,26 @@ private List<String> getUserIDList(List<String> userList, String tenantDomain) t
 
     private void prepareInitialGroupLists(Set<String> givenAddedGroupsIds, Set<String> givenRemovedGroupsIds,
                                           Set<String> givenReplacedGroupsIds, PatchOperation groupOperation,
-                                          Map<String, String> groupObject) {
+                                          Map<String, String> groupObject) throws BadRequestException {
+
+        String value = groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE);
+
+        if (StringUtils.isBlank(value)) {
+            throw new BadRequestException("Group id is required to update group of the role.",
+                    ResponseCodeConstants.INVALID_VALUE);
+        }
 
         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;

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.Assert.assertEquals;
+
+/**
+ * Contains the unit test cases for SCIMRoleManagerV2.
+ */
+@PrepareForTest({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;
+
+    private SCIMRoleManagerV2 scimRoleManagerV2;
+
+    @BeforeClass
+    public void setUpClass() {
+
+        initMocks(this);
+    }
+
+    @BeforeMethod
+    public void setUpMethod() {
+
+        PowerMockito.mockStatic(IdentityUtil.class);
+        scimRoleManagerV2 = new SCIMRoleManagerV2(roleManagementService, SAMPLE_TENANT_DOMAIN);
+    }
+
+    @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<String, List<PatchOperation>> patchOperations = new HashMap<>();
+        Map<String, String> 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());
+        }
+    }
+}

components/org.wso2.carbon.identity.scim2.common/src/test/resources/testng.xml

@@ -34,6 +34,7 @@
             <class name="org.wso2.carbon.identity.scim2.common.impl.SCIMRoleManagerTest"/>
             <class name="org.wso2.carbon.identity.scim2.common.impl.IdentityResourceURLBuilderTest"/>
             <class name="org.wso2.carbon.identity.scim2.common.impl.DefaultSCIMUserStoreErrorResolverTest"/>
+            <class name="org.wso2.carbon.identity.scim2.common.impl.SCIMRoleManagerV2Test"/>
         </classes>
     </test>