From cd1b5c338b76e6ddbd19d1cbf414b3fd2cfbb57c Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 20 May 2024 17:13:16 +0530 Subject: [PATCH 1/3] api,server: allow project role-based api access Fixes #9071 Signed-off-by: Abhishek Kumar --- .../org/apache/cloudstack/acl/APIChecker.java | 8 +- .../acl/ProjectRoleBasedApiAccessChecker.java | 7 +- .../main/java/com/cloud/api/ApiServer.java | 28 ++++- .../java/com/cloud/api/ApiServerTest.java | 115 ++++++++++++++++++ 4 files changed, 150 insertions(+), 8 deletions(-) create mode 100644 server/src/test/java/com/cloud/api/ApiServerTest.java diff --git a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java index 660f64f43ef2..e03b1c536ef8 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -16,13 +16,13 @@ // under the License. package org.apache.cloudstack.acl; +import java.util.List; + import com.cloud.exception.PermissionDeniedException; import com.cloud.user.Account; import com.cloud.user.User; import com.cloud.utils.component.Adapter; -import java.util.List; - /** * APICheckers is designed to verify the ownership of resources and to control the access to APIs. */ @@ -43,4 +43,8 @@ public interface APIChecker extends Adapter { */ List getApisAllowedToUser(Role role, User user, List apiNames) throws PermissionDeniedException; boolean isEnabled(); + + default boolean isProjectRoleBasedChecker() { + return false; + } } diff --git a/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java b/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java index 0306a062df98..8bdd57bd2ab3 100644 --- a/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java +++ b/plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java @@ -21,8 +21,8 @@ import javax.inject.Inject; import javax.naming.ConfigurationException; -import org.apache.cloudstack.acl.RolePermissionEntity.Permission; +import org.apache.cloudstack.acl.RolePermissionEntity.Permission; import org.apache.cloudstack.context.CallContext; import org.apache.log4j.Logger; @@ -197,4 +197,9 @@ public List getServices() { public void setServices(List services) { this.services = services; } + + @Override + public boolean isProjectRoleBasedChecker() { + return true; + } } diff --git a/server/src/main/java/com/cloud/api/ApiServer.java b/server/src/main/java/com/cloud/api/ApiServer.java index 76a436f74d42..c94b02ccb782 100644 --- a/server/src/main/java/com/cloud/api/ApiServer.java +++ b/server/src/main/java/com/cloud/api/ApiServer.java @@ -47,6 +47,7 @@ import java.util.concurrent.TimeUnit; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; @@ -1223,6 +1224,27 @@ public boolean verifyUser(final Long userId) { return true; } + protected boolean checkCommandAccessWithCheckers(final User user, final String commandName) throws + PermissionDeniedException { + List accessCheckers = apiAccessCheckers; + if (CallContext.current().getProject() != null) { + APIChecker projectRoleChecker = apiAccessCheckers.stream() + .filter(APIChecker::isProjectRoleBasedChecker).findFirst().orElse(null); + if (projectRoleChecker != null) { + if (projectRoleChecker.checkAccess(user, commandName)) { + return true; + } + accessCheckers = apiAccessCheckers.stream() + .filter(c -> !c.isProjectRoleBasedChecker()) + .collect(Collectors.toList()); + } + } + for (final APIChecker apiChecker : accessCheckers) { + apiChecker.checkAccess(user, commandName); + } + return true; + } + private void checkCommandAvailable(final User user, final String commandName, final InetAddress remoteAddress) throws PermissionDeniedException { if (user == null) { throw new PermissionDeniedException("User is null for role based API access check for command" + commandName); @@ -1239,11 +1261,7 @@ private void checkCommandAvailable(final User user, final String commandName, fi throw new OriginDeniedException("Calls from disallowed origin", account, remoteAddress); } } - - - for (final APIChecker apiChecker : apiAccessCheckers) { - apiChecker.checkAccess(user, commandName); - } + checkCommandAccessWithCheckers(user, commandName); } @Override diff --git a/server/src/test/java/com/cloud/api/ApiServerTest.java b/server/src/test/java/com/cloud/api/ApiServerTest.java new file mode 100644 index 000000000000..64aa840df5a8 --- /dev/null +++ b/server/src/test/java/com/cloud/api/ApiServerTest.java @@ -0,0 +1,115 @@ +// 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 com.cloud.api; + +import java.util.ArrayList; +import java.util.List; + +import org.apache.cloudstack.acl.APIChecker; +import org.apache.cloudstack.context.CallContext; +import org.junit.After; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.stubbing.Answer; +import org.springframework.test.util.ReflectionTestUtils; + +import com.cloud.exception.PermissionDeniedException; +import com.cloud.projects.Project; +import com.cloud.user.User; + +@RunWith(MockitoJUnitRunner.class) +public class ApiServerTest { + + @InjectMocks + ApiServer apiServer = new ApiServer(); + + private final String PROJECT_ACCESSIBLE_CMD_NAME = "projectcommand"; + private final String PROJECT_INACCESSIBLE_CMD_NAME = "noprojectcommand"; + private final String ACCESSIBLE_CMD_NAME = "command"; + private final String INACCESSIBLE_CMD_NAME = "nocommand"; + private User user = Mockito.mock(User.class); + private CallContext callContext = Mockito.mock(CallContext.class); + MockedStatic mockedCallContext; + + @Before + public void setup() throws Exception { + mockedCallContext = Mockito.mockStatic(CallContext.class); + mockedCallContext.when(CallContext::current).thenReturn(callContext); + List apiCheckers = new ArrayList<>(); + APIChecker checker1 = Mockito.mock(APIChecker.class); + apiCheckers.add(checker1); + APIChecker checker2 = Mockito.mock(APIChecker.class); + Mockito.when(checker2.isProjectRoleBasedChecker()) + .thenReturn(true); + apiCheckers.add(checker2); + Mockito.doAnswer((Answer) invocation -> { + String cmd = (String) invocation.getArguments()[1]; + if (!cmd.equals(ACCESSIBLE_CMD_NAME)) { + throw new PermissionDeniedException("Denied"); + } + return true; + }).when(checker1).checkAccess(Mockito.any(User.class), Mockito.anyString()); + Mockito.doAnswer((Answer) invocation -> { + String cmd = (String) invocation.getArguments()[1]; + if (List.of(PROJECT_ACCESSIBLE_CMD_NAME, ACCESSIBLE_CMD_NAME).contains(cmd)) { + return true; + } else if (cmd.equals(PROJECT_INACCESSIBLE_CMD_NAME)) { + return false; + } + throw new PermissionDeniedException("Denied"); + }).when(checker2).checkAccess(Mockito.any(User.class), Mockito.anyString()); + ReflectionTestUtils.setField(apiServer, "apiAccessCheckers", apiCheckers); + } + + @After + public void tearDown() throws Exception { + mockedCallContext.close(); + } + + @Test + public void testCheckCommandAccessWithCheckersProjectCommand() { + Mockito.when(callContext.getProject()).thenReturn(Mockito.mock(Project.class)); + Assert.assertTrue(apiServer.checkCommandAccessWithCheckers(user, PROJECT_ACCESSIBLE_CMD_NAME)); + } + + @Test(expected = PermissionDeniedException.class) + public void testCheckCommandAccessWithCheckersInaccessibleProjectCommand() { + Mockito.when(callContext.getProject()).thenReturn(Mockito.mock(Project.class)); + apiServer.checkCommandAccessWithCheckers(user, PROJECT_INACCESSIBLE_CMD_NAME); + } + + @Test(expected = PermissionDeniedException.class) + public void testCheckCommandAccessWithCheckersProjectCommandWithoutProject() { + apiServer.checkCommandAccessWithCheckers(user, PROJECT_INACCESSIBLE_CMD_NAME); + } + + @Test + public void testCheckCommandAccessWithCheckersAccessibleCommand() { + Assert.assertTrue(apiServer.checkCommandAccessWithCheckers(user, ACCESSIBLE_CMD_NAME)); + } + + @Test(expected = PermissionDeniedException.class) + public void testCheckCommandAccessWithCheckersInaccessibleCommand() { + apiServer.checkCommandAccessWithCheckers(user, INACCESSIBLE_CMD_NAME); + } +} \ No newline at end of file From 763a9ad662f0411b3c49c3b1cd5158192b58b149 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 20 May 2024 17:18:19 +0530 Subject: [PATCH 2/3] fix eof --- server/src/test/java/com/cloud/api/ApiServerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/com/cloud/api/ApiServerTest.java b/server/src/test/java/com/cloud/api/ApiServerTest.java index 64aa840df5a8..be7e6b295f25 100644 --- a/server/src/test/java/com/cloud/api/ApiServerTest.java +++ b/server/src/test/java/com/cloud/api/ApiServerTest.java @@ -112,4 +112,4 @@ public void testCheckCommandAccessWithCheckersAccessibleCommand() { public void testCheckCommandAccessWithCheckersInaccessibleCommand() { apiServer.checkCommandAccessWithCheckers(user, INACCESSIBLE_CMD_NAME); } -} \ No newline at end of file +} From 91baba145bb27f157f94fb57827fc2ba70ac0e16 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Mon, 20 May 2024 17:43:29 +0530 Subject: [PATCH 3/3] fix --- server/src/test/java/com/cloud/api/ApiServerTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/com/cloud/api/ApiServerTest.java b/server/src/test/java/com/cloud/api/ApiServerTest.java index be7e6b295f25..c0d65e315ebc 100644 --- a/server/src/test/java/com/cloud/api/ApiServerTest.java +++ b/server/src/test/java/com/cloud/api/ApiServerTest.java @@ -43,10 +43,10 @@ public class ApiServerTest { @InjectMocks ApiServer apiServer = new ApiServer(); - private final String PROJECT_ACCESSIBLE_CMD_NAME = "projectcommand"; - private final String PROJECT_INACCESSIBLE_CMD_NAME = "noprojectcommand"; - private final String ACCESSIBLE_CMD_NAME = "command"; - private final String INACCESSIBLE_CMD_NAME = "nocommand"; + private static final String PROJECT_ACCESSIBLE_CMD_NAME = "projectcommand"; + private static final String PROJECT_INACCESSIBLE_CMD_NAME = "noprojectcommand"; + private static final String ACCESSIBLE_CMD_NAME = "command"; + private static final String INACCESSIBLE_CMD_NAME = "nocommand"; private User user = Mockito.mock(User.class); private CallContext callContext = Mockito.mock(CallContext.class); MockedStatic mockedCallContext;