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.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<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>