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,27 @@ 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();
+
+    default void refreshRoleCacheOnPermissionsChange(Role role) {
+        // Only applicable for dynamic role based checkers
+    }
 }

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

@@ -60,6 +60,11 @@ protected DynamicRoleBasedAPIAccessChecker() {
         }
     }
 
+    @Override
+    public void refreshRoleCacheOnPermissionsChange(Role role) {
+        invalidateRolePermissionsCache(role.getId());
+    }
+
     @Override
     public List<String> getApisAllowedToUser(Role role, User user, List<String> apiNames) throws PermissionDeniedException {
         if (!isEnabled()) {
@@ -76,6 +81,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.
      *
@@ -120,6 +148,12 @@ protected Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
         return new Pair<>(accountRole, roleService.findAllPermissionsBy(accountRole.getId()));
     }
 
+    protected void invalidateRolePermissionsCache(long roleId) {
+        if (cachePeriod > 0) {
+            rolePermissionsCache.invalidate(roleId);
+        }
+    }
+
     protected Pair<Role, List<RolePermission>> getRolePermissionsUsingCache(long roleId) {
         if (cachePeriod > 0) {
             return rolePermissionsCache.get(roleId);
@@ -173,7 +207,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.
      *

plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java

@@ -25,6 +25,7 @@
 import javax.naming.ConfigurationException;
 
 import com.cloud.api.auth.SetupUserTwoFactorAuthenticationCmd;
+import org.apache.cloudstack.acl.Role;
 import org.apache.cloudstack.api.command.admin.account.CreateAccountCmd;
 import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd;
 import org.apache.cloudstack.api.command.admin.user.MoveUserCmd;
@@ -543,4 +544,8 @@ public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccou
     @Override
     public void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount) {
     }
+
+    @Override
+    public void refreshRoleCheckersCacheOnPermissionsChange(Role role) {
+    }
 }

server/src/main/java/com/cloud/user/AccountManager.java

@@ -21,6 +21,7 @@
 import java.util.Map;
 
 import org.apache.cloudstack.acl.ControlledEntity;
+import org.apache.cloudstack.acl.Role;
 import org.apache.cloudstack.api.command.admin.account.UpdateAccountCmd;
 import org.apache.cloudstack.api.command.admin.user.DeleteUserCmd;
 import org.apache.cloudstack.api.command.admin.user.MoveUserCmd;
@@ -207,4 +208,6 @@ void buildACLViewSearchCriteria(SearchCriteria<? extends ControlledViewEntity> s
     UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user);
 
     void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount);
+
+    void refreshRoleCheckersCacheOnPermissionsChange(Role role);
 }