diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/project/UpdateProjectRolePermissionCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/project/UpdateProjectRolePermissionCmd.java index b273b9f58493..e32b2c162c71 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/project/UpdateProjectRolePermissionCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/project/UpdateProjectRolePermissionCmd.java @@ -122,7 +122,7 @@ public void execute() { if (getProjectRulePermissionOrder() != null) { throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Parameters permission and ruleid must be mutually exclusive with ruleorder"); } - ProjectRolePermission rolePermission = getValidProjectRolePermission(); + ProjectRolePermission rolePermission = findProjectRolePermissionInProject(getProjectRuleId()); CallContext.current().setEventDetails("Updating project role permission for rule id: " + getProjectRuleId() + " to: " + getProjectRolePermission().toString()); result = projRoleService.updateProjectRolePermission(projectId, projectRole, rolePermission, getProjectRolePermission()); } @@ -131,20 +131,24 @@ public void execute() { setResponseObject(response); } - private ProjectRolePermission getValidProjectRolePermission() { - ProjectRolePermission rolePermission = projRoleService.findProjectRolePermission(getProjectRuleId()); + /** + * Resolves a caller supplied rule id and rejects it when it belongs to another project, so that + * no code path can reach permissions outside the project the command was called with. + */ + private ProjectRolePermission findProjectRolePermissionInProject(Long rolePermissionId) { + final ProjectRolePermission rolePermission = projRoleService.findProjectRolePermission(rolePermissionId); if (rolePermission == null || rolePermission.getProjectId() != getProjectId()) { throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Role permission doesn't exist in the project, probably because of invalid rule id"); } return rolePermission; } - private boolean updateProjectRolePermissionOrder(ProjectRole projectRole) { + protected boolean updateProjectRolePermissionOrder(ProjectRole projectRole) { final List rolePermissionsOrder = new ArrayList<>(); for (Long rolePermissionId : getProjectRulePermissionOrder()) { - final ProjectRolePermission rolePermission = projRoleService.findProjectRolePermission(rolePermissionId); - if (rolePermission == null) { - throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Provided project role permission(s) do not exist"); + final ProjectRolePermission rolePermission = findProjectRolePermissionInProject(rolePermissionId); + if (rolePermission.getProjectRoleId() != projectRole.getId()) { + throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Provided project role permission(s) do not belong to the given project role"); } rolePermissionsOrder.add(rolePermission); } diff --git a/api/src/test/java/org/apache/cloudstack/api/command/admin/acl/project/UpdateProjectRolePermissionCmdTest.java b/api/src/test/java/org/apache/cloudstack/api/command/admin/acl/project/UpdateProjectRolePermissionCmdTest.java new file mode 100644 index 000000000000..ae883204c9ca --- /dev/null +++ b/api/src/test/java/org/apache/cloudstack/api/command/admin/acl/project/UpdateProjectRolePermissionCmdTest.java @@ -0,0 +1,88 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF 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.apache.cloudstack.api.command.admin.acl.project; + +import org.apache.cloudstack.acl.ProjectRole; +import org.apache.cloudstack.acl.ProjectRolePermission; +import org.apache.cloudstack.acl.ProjectRoleService; +import org.apache.cloudstack.api.ServerApiException; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; +import org.springframework.test.util.ReflectionTestUtils; + +import java.util.Arrays; + +public class UpdateProjectRolePermissionCmdTest { + + @Mock + private ProjectRoleService projectRoleService; + @Mock + private ProjectRole projectRole; + @Mock + private ProjectRolePermission projectRolePermissionOne; + @Mock + private ProjectRolePermission projectRolePermissionTwo; + + private final UpdateProjectRolePermissionCmd updateProjectRolePermissionCmd = new UpdateProjectRolePermissionCmd(); + + private static final Long projectId = 9L; + private static final Long ruleOneId = 111L; + private static final Long ruleTwoId = 222L; + private static final Long projectRoleId = 20L; + + @Before + public void setUp() throws Exception { + MockitoAnnotations.initMocks(this); + updateProjectRolePermissionCmd.projRoleService = projectRoleService; + + Mockito.when(projectRolePermissionOne.getProjectId()).thenReturn(projectId); + Mockito.when(projectRolePermissionTwo.getProjectId()).thenReturn(projectId); + Mockito.when(projectRolePermissionOne.getProjectRoleId()).thenReturn(projectRoleId); + Mockito.when(projectRolePermissionTwo.getProjectRoleId()).thenReturn(projectRoleId); + Mockito.when(projectRole.getId()).thenReturn(projectRoleId); + + ReflectionTestUtils.setField(updateProjectRolePermissionCmd, "projectRulePermissionOrder", Arrays.asList(ruleOneId, ruleTwoId)); + ReflectionTestUtils.setField(updateProjectRolePermissionCmd, "projectId", projectId); + + Mockito.when(projectRoleService.findProjectRolePermission(ruleOneId)).thenReturn(projectRolePermissionOne); + Mockito.when(projectRoleService.findProjectRolePermission(ruleTwoId)).thenReturn(projectRolePermissionTwo); + } + + @Test(expected = ServerApiException.class) + public void testUpdateProjectRolePermissionOrderRuleIdFromAnotherProject() { + Long differentProjectId = 2L; + Mockito.when(projectRolePermissionTwo.getProjectId()).thenReturn(differentProjectId); + updateProjectRolePermissionCmd.updateProjectRolePermissionOrder(projectRole); + } + + @Test(expected = ServerApiException.class) + public void testUpdateProjectRolePermissionOrderDifferentRuleRoleInSameProject() { + Long differentRoleId = 3L; + Mockito.when(projectRolePermissionTwo.getProjectRoleId()).thenReturn(differentRoleId); + updateProjectRolePermissionCmd.updateProjectRolePermissionOrder(projectRole); + } + + @Test + public void testUpdateProjectRolePermissionOrder() { + updateProjectRolePermissionCmd.updateProjectRolePermissionOrder(projectRole); + Mockito.verify(projectRoleService).updateProjectRolePermission(Mockito.eq(projectId), Mockito.eq(projectRole), + Mockito.eq(Arrays.asList(projectRolePermissionOne, projectRolePermissionTwo))); + } +}