check every rule belongs to the project and role when reordering project role permissions

Author: Damans227

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<ProjectRolePermission> 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);
         }

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)));
+    }
+}