Revert "Fix checkRoleEscalation performance and bugs in access checking (#12973)"

This reverts commit 5dcb8ab.

Author: RosiKyu

api/src/main/java/org/apache/cloudstack/acl/APIChecker.java

@@ -17,22 +17,16 @@
 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
@@ -48,27 +42,5 @@ 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,11 +60,6 @@ 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()) {
@@ -81,29 +76,6 @@ 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.
      *
@@ -148,12 +120,6 @@ 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);
@@ -207,7 +173,7 @@ public boolean checkAccess(Account account, String commandName) {
             return true;
         }
 
-        List<RolePermission> allPermissions = roleAndPermissions.second();
+        List<RolePermission> allPermissions = roleService.findAllPermissionsBy(accountRole.getId());
         if (checkApiPermissionByRole(accountRole, commandName, allPermissions)) {
             return true;
         }

plugins/acl/dynamic-role-based/src/test/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java

@@ -40,8 +40,6 @@
 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;
 
@@ -197,134 +195,4 @@ 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,11 +154,6 @@ 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,7 +16,6 @@
 // under the License.
 package org.apache.cloudstack.acl;
 
-import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -122,21 +121,6 @@ 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,7 +25,6 @@
 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;
@@ -544,8 +543,4 @@ 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,7 +21,6 @@
 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;
@@ -208,6 +207,4 @@ void buildACLViewSearchCriteria(SearchCriteria<? extends ControlledViewEntity> s
     UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user);
 
     void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount);
-
-    void refreshRoleCheckersCacheOnPermissionsChange(Role role);
 }