Fix checkRoleEscalation performance and bugs in access checking

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<String> getApisAllowedToUser(Role role, User user, List<String> apiNames) throws PermissionDeniedException;
+
+    default List<String> getApisAllowedToAccount(Account account, List<String> apiNames) {
+        List<String> 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();
 }

plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

@@ -76,6 +76,29 @@ public List<String> getApisAllowedToUser(Role role, User user, List<String> apiN
         return allowedApis;
     }
 
+    @Override
+    public List<String> getApisAllowedToAccount(Account account, List<String> apiNames) {
+        if (!isEnabled()) {
+            return apiNames;
+        }
+        Pair<Role, List<RolePermission>> 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<RolePermission> allPermissions = roleAndPermissions.second();
+        List<String> 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<RolePermission> allPermissions = roleService.findAllPermissionsBy(accountRole.getId());
+        List<RolePermission> allPermissions = roleAndPermissions.second();
         if (checkApiPermissionByRole(accountRole, commandName, allPermissions)) {
             return true;
         }

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<String> 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<Long, Pair<Role, List<RolePermission>>>(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<String> input = new ArrayList<>(Arrays.asList("api1", "api2", "api3"));
+        List<String> 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<String> input = new ArrayList<>(Arrays.asList("api1", "api2", "api3"));
+        List<String> 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<String> input = new ArrayList<>(Arrays.asList("allowedApi", "deniedApi", "unknownApi"));
+        List<String> 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<String> input = new ArrayList<>(Arrays.asList("annotatedApi", "unknownApi"));
+        List<String> 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<Long, Pair<Role, List<RolePermission>>>(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<String> 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());
+        }
+    }
 }

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<String> getApisAllowedToAccount(Account account, List<String> apiNames) {
+        return apiNames;
+    }
+
     public boolean isPermitted(Project project, ProjectAccount projectUser, String ... apiCommandNames) {
         ProjectRole projectRole = null;
         if(projectUser.getProjectRoleId() != null) {

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<String> getApisAllowedToAccount(Account account, List<String> apiNames) {
+        if (!isEnabled()) {
+            return apiNames;
+        }
+        RoleType roleType = accountService.getRoleType(account);
+        List<String> 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.
      *

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<APIChecker> 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<APIAclChecker> aclCheckers = new ArrayList<>();
+        for (APIChecker apiChecker : apiCheckers) {
+            if (apiChecker instanceof APIAclChecker) {
+                aclCheckers.add((APIAclChecker) apiChecker);
             }
         }
+
+        List<String> allApis = new ArrayList<>(apiNameList);
+        List<String> requestedAllowed = allApis;
+        for (final APIAclChecker apiChecker : aclCheckers) {
+            requestedAllowed = apiChecker.getApisAllowedToAccount(requested, requestedAllowed);
+        }
+        List<String> callerAllowed = requestedAllowed;
+        for (final APIAclChecker apiChecker : aclCheckers) {
+            callerAllowed = apiChecker.getApisAllowedToAccount(caller, callerAllowed);
+        }
+        if (callerAllowed.size() < requestedAllowed.size()) {
+            List<String> 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<APIChecker> apiCheckers, Account caller, String command) {