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.

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: SCIM error message for invalid request values in V1 and V2 Roles Remove-Add-Replace operations (fixes #20334) #559

Merged
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Contributor

@AnuradhaSK AnuradhaSK Jul 5, 2024

Choose a reason for hiding this comment

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

Suggested change
import static org.testng.AssertJUnit.assertEquals;
import static org.testng.Assert;

Let's import org.testng.Assert and use its assertEquals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by e97c090


/**
* 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;

private SCIMRoleManagerV2 scimRoleManagerV2;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not mock the class which the method we are testing

Copy link
Contributor Author

@BimsaraBodaragama BimsaraBodaragama Jul 5, 2024

Choose a reason for hiding this comment

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

Fixed by e15eeb8 and c8cf26b


@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());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand Down
Loading