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..b50ec825aecc 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -17,16 +17,22 @@ package org.apache.cloudstack.acl; import com.cloud.exception.PermissionDeniedException; +import com.cloud.exception.RequestLimitException; import com.cloud.user.Account; import com.cloud.user.User; import com.cloud.utils.component.Adapter; +import java.util.ArrayList; import java.util.List; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + /** * APICheckers is designed to verify the ownership of resources and to control the access to APIs. */ public interface APIChecker extends Adapter { + Logger LOGGER = LogManager.getLogger(APIChecker.class); // Interface for checking access for a role using apiname // If true, apiChecker has checked the operation // If false, apiChecker is unable to handle the operation or not implemented @@ -42,5 +48,23 @@ public interface APIChecker extends Adapter { * @return the list of allowed apis for the given user */ List getApisAllowedToUser(Role role, User user, List apiNames) throws PermissionDeniedException; + + default List getApisAllowedToAccount(Account account, List apiNames) { + List allowedApis = new ArrayList<>(); + for (String apiName : apiNames) { + try { + checkAccess(account, apiName); + allowedApis.add(apiName); + } catch (RequestLimitException e) { + // Non-ACL failure (e.g. rate limiting) should not be treated as simple "not allowed". + // Propagate as unchecked so callers are aware of the failure. + throw new RuntimeException("Failed to check access for API [" + apiName + "] due to request limits", e); + } catch (PermissionDeniedException e) { + LOGGER.trace("Account [" + account + "] is not allowed to access API [" + apiName + "]"); + } + } + return allowedApis; + } + boolean isEnabled(); } diff --git a/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java b/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java index 030e0bcf0141..8fbe00d44d3a 100644 --- a/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java +++ b/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java @@ -76,6 +76,29 @@ public List getApisAllowedToUser(Role role, User user, List apiN return allowedApis; } + @Override + public List getApisAllowedToAccount(Account account, List apiNames) { + if (!isEnabled()) { + return apiNames; + } + Pair> roleAndPermissions = getRolePermissionsUsingCache(account.getRoleId()); + final Role accountRole = roleAndPermissions.first(); + if (accountRole == null) { + throw new PermissionDeniedException("The account [" + account + "] has role null or unknown."); + } + if (accountRole.getRoleType() == RoleType.Admin && accountRole.getId() == RoleType.Admin.getId()) { + return apiNames; + } + List allPermissions = roleAndPermissions.second(); + List allowedApis = new ArrayList<>(); + for (String api : apiNames) { + if (checkApiPermissionByRole(accountRole, api, allPermissions)) { + allowedApis.add(api); + } + } + return allowedApis; + } + /** * Checks if the given Role of an Account has the allowed permission for the given API. * @@ -173,7 +196,7 @@ public boolean checkAccess(Account account, String commandName) { return true; } - List allPermissions = roleService.findAllPermissionsBy(accountRole.getId()); + List allPermissions = roleAndPermissions.second(); if (checkApiPermissionByRole(accountRole, commandName, allPermissions)) { return true; } diff --git a/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java b/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java index e58be3a75e79..c89000e7ed1e 100644 --- a/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java +++ b/plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java @@ -40,6 +40,8 @@ import com.cloud.user.UserVO; import org.apache.cloudstack.acl.RolePermissionEntity.Permission; +import org.apache.cloudstack.utils.cache.LazyCache; +import com.cloud.utils.Pair; import junit.framework.TestCase; @@ -195,4 +197,134 @@ public void getApisAllowedToUserTestPermissionDenyForGivenApiShouldReturnEmptyLi List apisReceived = apiAccessCheckerSpy.getApisAllowedToUser(getTestRole(), getTestUser(), apiNames); Assert.assertEquals(0, apisReceived.size()); } + + // --- Tests for checkAccess(Account, String) --- + + @Test(expected = PermissionDeniedException.class) + public void testCheckAccessAccountNullRoleShouldThrow() { + Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(null); + apiAccessCheckerSpy.checkAccess(getTestAccount(), "someApi"); + } + + @Test + public void testCheckAccessAccountAdminShouldAllow() { + Account adminAccount = new AccountVO("root admin", 1L, null, Account.Type.ADMIN, "admin-uuid"); + Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(new RoleVO(1L, "Admin", RoleType.Admin, "default admin role")); + assertTrue(apiAccessCheckerSpy.checkAccess(adminAccount, "anyApi")); + } + + @Test + public void testCheckAccessAccountAllowedApi() { + final String allowedApiName = "someAllowedApi"; + final RolePermission permission = new RolePermissionVO(1L, allowedApiName, Permission.ALLOW, null); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); + assertTrue(apiAccessCheckerSpy.checkAccess(getTestAccount(), allowedApiName)); + } + + @Test(expected = PermissionDeniedException.class) + public void testCheckAccessAccountDeniedApi() { + final String deniedApiName = "someDeniedApi"; + final RolePermission permission = new RolePermissionVO(1L, deniedApiName, Permission.DENY, null); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); + apiAccessCheckerSpy.checkAccess(getTestAccount(), deniedApiName); + } + + @Test + public void testCheckAccessAccountUsesCachedPermissions() throws Exception { + // Enable caching by setting a positive cachePeriod + Field cachePeriodField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("cachePeriod"); + cachePeriodField.setAccessible(true); + cachePeriodField.set(apiAccessCheckerSpy, 1); + + Field rpCacheField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("rolePermissionsCache"); + rpCacheField.setAccessible(true); + rpCacheField.set(apiAccessCheckerSpy, new LazyCache>>(32, 1, apiAccessCheckerSpy::getRolePermissions)); + + final String allowedApiName = "someAllowedApi"; + final RolePermission permission = new RolePermissionVO(1L, allowedApiName, Permission.ALLOW, null); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); + + // First call should populate the cache + apiAccessCheckerSpy.checkAccess(getTestAccount(), allowedApiName); + // Second call should use cached permissions and not hit the DAO again + apiAccessCheckerSpy.checkAccess(getTestAccount(), allowedApiName); + + Mockito.verify(roleServiceMock, Mockito.times(1)).findAllPermissionsBy(Mockito.anyLong()); + } + + // --- Tests for getApisAllowedToAccount --- + + @Test + public void testGetApisAllowedToAccountDisabledShouldReturnAll() { + Mockito.doReturn(false).when(apiAccessCheckerSpy).isEnabled(); + List input = new ArrayList<>(Arrays.asList("api1", "api2", "api3")); + List result = apiAccessCheckerSpy.getApisAllowedToAccount(getTestAccount(), input); + Assert.assertEquals(3, result.size()); + } + + @Test(expected = PermissionDeniedException.class) + public void testGetApisAllowedToAccountNullRoleShouldThrow() { + Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(null); + apiAccessCheckerSpy.getApisAllowedToAccount(getTestAccount(), new ArrayList<>(Arrays.asList("api1"))); + } + + @Test + public void testGetApisAllowedToAccountAdminShouldReturnAll() { + Account adminAccount = new AccountVO("root admin", 1L, null, Account.Type.ADMIN, "admin-uuid"); + Mockito.when(roleServiceMock.findRole(Mockito.anyLong())).thenReturn(new RoleVO(1L, "Admin", RoleType.Admin, "default admin role")); + List input = new ArrayList<>(Arrays.asList("api1", "api2", "api3")); + List result = apiAccessCheckerSpy.getApisAllowedToAccount(adminAccount, input); + Assert.assertEquals(3, result.size()); + Assert.assertEquals(input, result); + } + + @Test + public void testGetApisAllowedToAccountFiltersCorrectly() { + final RolePermission allowPermission = new RolePermissionVO(1L, "allowedApi", Permission.ALLOW, null); + final RolePermission denyPermission = new RolePermissionVO(1L, "deniedApi", Permission.DENY, null); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Arrays.asList(allowPermission, denyPermission)); + List input = new ArrayList<>(Arrays.asList("allowedApi", "deniedApi", "unknownApi")); + List result = apiAccessCheckerSpy.getApisAllowedToAccount(getTestAccount(), input); + Assert.assertEquals(1, result.size()); + Assert.assertEquals("allowedApi", result.get(0)); + } + + @Test + public void testGetApisAllowedToAccountAnnotationFallback() { + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.emptyList()); + apiAccessCheckerSpy.addApiToRoleBasedAnnotationsMap(RoleType.User, "annotatedApi"); + List input = new ArrayList<>(Arrays.asList("annotatedApi", "unknownApi")); + List result = apiAccessCheckerSpy.getApisAllowedToAccount(getTestAccount(), input); + Assert.assertEquals(1, result.size()); + Assert.assertEquals("annotatedApi", result.get(0)); + } + + @Test + public void testGetApisAllowedToAccountUsesCachedPermissions() { + try { + // Ensure caching is enabled by setting a positive cachePeriod + Field cachePeriodField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("cachePeriod"); + cachePeriodField.setAccessible(true); + cachePeriodField.set(apiAccessCheckerSpy, 1); + + Field rpCacheField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("rolePermissionsCache"); + rpCacheField.setAccessible(true); + rpCacheField.set(apiAccessCheckerSpy, new LazyCache>>(32, 1, apiAccessCheckerSpy::getRolePermissions)); + + final RolePermission permission = new RolePermissionVO(1L, "api1", Permission.ALLOW, null); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); + + Account account = getTestAccount(); + List apis = new ArrayList<>(Arrays.asList("api1")); + + // First call should load permissions from the DAO and populate the cache + apiAccessCheckerSpy.getApisAllowedToAccount(account, apis); + // Second call should use cached permissions and not hit the DAO again + apiAccessCheckerSpy.getApisAllowedToAccount(account, apis); + + Mockito.verify(roleServiceMock, Mockito.times(1)).findAllPermissionsBy(Mockito.anyLong()); + } catch (NoSuchFieldException | IllegalAccessException e) { + Assert.fail("Failed to set cachePeriod for test: " + e.getMessage()); + } + } } 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 2e7ae23d6f1b..9dea1797f905 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 @@ -154,6 +154,11 @@ public boolean checkAccess(Account account, String apiCommandName) throws Permis return true; } + @Override + public List getApisAllowedToAccount(Account account, List apiNames) { + return apiNames; + } + public boolean isPermitted(Project project, ProjectAccount projectUser, String ... apiCommandNames) { ProjectRole projectRole = null; if(projectUser.getProjectRoleId() != null) { diff --git a/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java b/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java index 3444f967d784..b8dc516af7c4 100644 --- a/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java +++ b/plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java @@ -16,6 +16,7 @@ // under the License. package org.apache.cloudstack.acl; +import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -121,6 +122,21 @@ public boolean checkAccess(Account account, String commandName) { } } + @Override + public List getApisAllowedToAccount(Account account, List apiNames) { + if (!isEnabled()) { + return apiNames; + } + RoleType roleType = accountService.getRoleType(account); + List allowedApis = new ArrayList<>(); + for (String apiName : apiNames) { + if (isApiAllowed(apiName, roleType)) { + allowedApis.add(apiName); + } + } + return allowedApis; + } + /** * Verifies if the API is allowed for the given RoleType. * diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index bfe6a1b0a477..fbdc6bdc2ff3 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -43,6 +43,7 @@ import javax.naming.ConfigurationException; import org.apache.cloudstack.acl.APIChecker; +import org.apache.cloudstack.acl.APIAclChecker; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.InfrastructureEntity; import org.apache.cloudstack.acl.QuerySelector; @@ -1435,35 +1436,32 @@ protected void checkRoleEscalation(Account caller, Account requested) { requested.getRoleId())); } List apiCheckers = getEnabledApiCheckers(); - for (String command : apiNameList) { - try { - checkApiAccess(apiCheckers, requested, command); - } catch (PermissionDeniedException pde) { - if (logger.isTraceEnabled()) { - logger.trace(String.format( - "Checking for permission to \"%s\" is irrelevant as it is not requested for %s [%s]", - command, - requested.getAccountName(), - requested.getUuid() - ) - ); - } - continue; - } - // so requested can, now make sure caller can as well - try { - if (logger.isTraceEnabled()) { - logger.trace(String.format("permission to \"%s\" is requested", - command)); - } - checkApiAccess(apiCheckers, caller, command); - } catch (PermissionDeniedException pde) { - String msg = String.format("User of Account %s and domain %s can not create an account with access to more privileges they have themself.", - caller, _domainMgr.getDomain(caller.getDomainId())); - logger.warn(msg); - throw new PermissionDeniedException(msg,pde); + + // Only ACL checkers should influence the set of APIs allowed to an account. + List aclCheckers = new ArrayList<>(); + for (APIChecker apiChecker : apiCheckers) { + if (apiChecker instanceof APIAclChecker) { + aclCheckers.add((APIAclChecker) apiChecker); } } + + List allApis = new ArrayList<>(apiNameList); + List requestedAllowed = allApis; + for (final APIAclChecker apiChecker : aclCheckers) { + requestedAllowed = apiChecker.getApisAllowedToAccount(requested, requestedAllowed); + } + List callerAllowed = requestedAllowed; + for (final APIAclChecker apiChecker : aclCheckers) { + callerAllowed = apiChecker.getApisAllowedToAccount(caller, callerAllowed); + } + if (callerAllowed.size() < requestedAllowed.size()) { + List escalatedApis = new ArrayList<>(requestedAllowed); + escalatedApis.removeAll(callerAllowed); + String msg = String.format("User of Account %s and domain %s cannot create an account with access to more privileges than they have. Escalated APIs: %s", + caller, _domainMgr.getDomain(caller.getDomainId()), escalatedApis); + logger.warn(msg); + throw new PermissionDeniedException(msg); + } } private void checkApiAccess(List apiCheckers, Account caller, String command) { diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index b812f3bca938..2a86fe9004a5 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -24,9 +24,12 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; +import java.util.Collections; import java.util.List; import java.util.Map; +import org.apache.cloudstack.acl.APIAclChecker; +import org.apache.cloudstack.acl.APIChecker; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.Role; import org.apache.cloudstack.acl.RoleService; @@ -1584,4 +1587,119 @@ public void testcheckCallerApiPermissionsForUserOperationsNotAllowedApis() { accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); } + + // --- Tests for checkRoleEscalation --- + + private void setPrivateField(Object target, String fieldName, Object value) throws Exception { + Class clazz = target.getClass(); + while (clazz != null) { + try { + java.lang.reflect.Field field = clazz.getDeclaredField(fieldName); + field.setAccessible(true); + field.set(target, value); + return; + } catch (NoSuchFieldException e) { + clazz = clazz.getSuperclass(); + } + } + throw new NoSuchFieldException(fieldName); + } + + @Test + public void testCheckRoleEscalationSamePermissionsShouldPass() throws Exception { + APIChecker checker = Mockito.mock(APIChecker.class); + List apis = Arrays.asList("api1", "api2", "api3"); + Mockito.when(checker.isEnabled()).thenReturn(true); + + Account caller = Mockito.mock(Account.class); + Account requested = Mockito.mock(Account.class); + + accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker)); + setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>(apis)); + + accountManagerImpl.checkRoleEscalation(caller, requested); + } + + @Test + public void testCheckRoleEscalationCallerHasMorePermissionsShouldPass() throws Exception { + List allApis = Arrays.asList("api1", "api2", "api3"); + List requestedApis = Arrays.asList("api1", "api2"); + + APIAclChecker checker = Mockito.mock(APIAclChecker.class); + Mockito.when(checker.isEnabled()).thenReturn(true); + + Account caller = Mockito.mock(Account.class); + Account requested = Mockito.mock(Account.class); + + Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(requested), Mockito.anyList())).thenReturn(requestedApis); + Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(caller), Mockito.anyList())).thenReturn(allApis); + + accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker)); + setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>(allApis)); + + accountManagerImpl.checkRoleEscalation(caller, requested); + } + + @Test(expected = PermissionDeniedException.class) + public void testCheckRoleEscalationRequestedHasMorePermissionsShouldThrow() throws Exception { + List allApis = Arrays.asList("api1", "api2", "api3"); + List requestedApis = Arrays.asList("api1", "api2", "api3"); + List callerApis = Arrays.asList("api1"); + + APIAclChecker checker = Mockito.mock(APIAclChecker.class); + Mockito.when(checker.isEnabled()).thenReturn(true); + + Account caller = Mockito.mock(Account.class); + Account requested = Mockito.mock(Account.class); + + Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(requested), Mockito.anyList())).thenReturn(requestedApis); + Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(caller), Mockito.anyList())).thenReturn(callerApis); + + accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker)); + setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>(allApis)); + + accountManagerImpl.checkRoleEscalation(caller, requested); + } + + @Test + public void testCheckRoleEscalationEmptyApiListShouldPass() throws Exception { + APIAclChecker checker = Mockito.mock(APIAclChecker.class); + Mockito.when(checker.isEnabled()).thenReturn(true); + Mockito.when(checker.getApisAllowedToAccount(Mockito.any(Account.class), Mockito.anyList())).thenReturn(Collections.emptyList()); + + Account caller = Mockito.mock(Account.class); + Account requested = Mockito.mock(Account.class); + + accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker)); + setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>()); + + accountManagerImpl.checkRoleEscalation(caller, requested); + } + + @Test + public void testCheckRoleEscalationMultipleCheckersAppliedSequentially() throws Exception { + List allApis = Arrays.asList("api1", "api2", "api3"); + List afterChecker1 = Arrays.asList("api1", "api2"); + List afterChecker2 = Arrays.asList("api1"); + + APIAclChecker checker1 = Mockito.mock(APIAclChecker.class); + Mockito.when(checker1.isEnabled()).thenReturn(true); + APIAclChecker checker2 = Mockito.mock(APIAclChecker.class); + Mockito.when(checker2.isEnabled()).thenReturn(true); + + Account caller = Mockito.mock(Account.class); + Account requested = Mockito.mock(Account.class); + + // requested: checker1 filters to [api1, api2], checker2 further filters to [api1] + Mockito.when(checker1.getApisAllowedToAccount(Mockito.eq(requested), Mockito.eq(allApis))).thenReturn(afterChecker1); + Mockito.when(checker2.getApisAllowedToAccount(Mockito.eq(requested), Mockito.eq(afterChecker1))).thenReturn(afterChecker2); + // caller: same filtering, so no escalation + Mockito.when(checker1.getApisAllowedToAccount(Mockito.eq(caller), Mockito.eq(afterChecker2))).thenReturn(afterChecker2); + Mockito.when(checker2.getApisAllowedToAccount(Mockito.eq(caller), Mockito.eq(afterChecker2))).thenReturn(afterChecker2); + + accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker1, checker2)); + setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>(allApis)); + + accountManagerImpl.checkRoleEscalation(caller, requested); + } }