This is an automated email from the ASF dual-hosted git repository. RosiKyu pushed a commit to branch revert-12973-422-fix-checkroleescalation-performance in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit 56c4e1697152dfd1a3fcbc9f9a18e9881e67858e Author: Rositsa Kyuchukova <[email protected]> AuthorDate: Sat Aug 15 01:05:06 2026 +0300 Revert "Fix checkRoleEscalation performance and bugs in access checking (#12973)" This reverts commit 5dcb8abaf34a3e483dc132df6724ef61e0cddcb4. --- .../java/org/apache/cloudstack/acl/APIChecker.java | 28 ----- .../acl/DynamicRoleBasedAPIAccessChecker.java | 36 +----- .../acl/DynamicRoleBasedAPIAccessCheckerTest.java | 132 --------------------- .../acl/ProjectRoleBasedApiAccessChecker.java | 5 - .../acl/StaticRoleBasedAPIAccessChecker.java | 16 --- .../contrail/management/MockAccountManager.java | 5 - .../main/java/com/cloud/user/AccountManager.java | 3 - .../java/com/cloud/user/AccountManagerImpl.java | 75 +++++------- .../org/apache/cloudstack/acl/RoleManagerImpl.java | 3 - .../com/cloud/user/AccountManagerImplTest.java | 118 ------------------ 10 files changed, 28 insertions(+), 393 deletions(-) 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 c9a8d384945..660f64f43ef 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/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 - } } 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 a8b9042ccf6..030e0bcf014 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 @@ -60,11 +60,6 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API } } - @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 class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API 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 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API 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 class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API return true; } - List<RolePermission> allPermissions = roleAndPermissions.second(); + List<RolePermission> allPermissions = roleService.findAllPermissionsBy(accountRole.getId()); 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 c89000e7ed1..e58be3a75e7 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,8 +40,6 @@ import com.cloud.user.User; 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 class DynamicRoleBasedAPIAccessCheckerTest extends TestCase { 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()); - } - } } 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 9dea1797f90..2e7ae23d6f1 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,11 +154,6 @@ public class ProjectRoleBasedApiAccessChecker extends AdapterBase implements AP 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) { 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 b8dc516af7c..3444f967d78 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,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 class StaticRoleBasedAPIAccessChecker extends AdapterBase implements APIA } } - @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. * diff --git a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java index 860674b329f..2ff68b4836f 100644 --- a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java +++ b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java @@ -25,7 +25,6 @@ import javax.inject.Inject; 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 class MockAccountManager extends ManagerBase implements AccountManager { @Override public void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount) { } - - @Override - public void refreshRoleCheckersCacheOnPermissionsChange(Role role) { - } } diff --git a/server/src/main/java/com/cloud/user/AccountManager.java b/server/src/main/java/com/cloud/user/AccountManager.java index f12a1b68b03..c46ac78526b 100644 --- a/server/src/main/java/com/cloud/user/AccountManager.java +++ b/server/src/main/java/com/cloud/user/AccountManager.java @@ -21,7 +21,6 @@ import java.util.List; 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 @@ public interface AccountManager extends AccountService, Configurable { UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user); void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount); - - void refreshRoleCheckersCacheOnPermissionsChange(Role role); } diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 32bf0e9c507..c485aae87ec 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -43,7 +43,6 @@ import javax.inject.Inject; 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; @@ -1434,34 +1433,35 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M requested.getUuid(), requested.getRoleId())); } - - List<APIAclChecker> aclCheckers = getApiACLCheckers(); - - List<String> allApis = new ArrayList<>(apiNameList); - List<String> requestedAllowed = allApis; - List<String> callerAllowed = new ArrayList<>(); - try { - for (final APIAclChecker apiChecker : aclCheckers) { - requestedAllowed = apiChecker.getApisAllowedToAccount(requested, requestedAllowed); + 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; } - callerAllowed = requestedAllowed; - for (final APIAclChecker apiChecker : aclCheckers) { - callerAllowed = apiChecker.getApisAllowedToAccount(caller, callerAllowed); + // 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); } - } catch (PermissionDeniedException e) { - String msg = String.format("User of account: %s cannot assign this role on the requested account: %s", caller.getAccountName(), requested.getAccountName()); - String logMsg = String.format("%s: %s", msg, e.getMessage()); - logger.error(logMsg, e); - throw new PermissionDeniedException(msg); - } - - 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()), CollectionUtils.isNotEmpty(escalatedApis) ? escalatedApis.size() : "None"); - logger.warn(msg); - throw new PermissionDeniedException(msg); } } @@ -1477,19 +1477,6 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M checkApiAccess(apiCheckers, caller, command); } - protected List<APIAclChecker> getApiACLCheckers() { - List<APIChecker> apiCheckers = getEnabledApiCheckers(); - - // 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); - } - } - return aclCheckers; - } - @NotNull private List<APIChecker> getEnabledApiCheckers() { // we are really only interested in the dynamic access checker @@ -1600,14 +1587,6 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } - @Override - public void refreshRoleCheckersCacheOnPermissionsChange(Role role) { - List<APIAclChecker> aclCheckers = getApiACLCheckers(); - for (final APIAclChecker aclChecker : aclCheckers) { - aclChecker.refreshRoleCacheOnPermissionsChange(role); - } - } - protected void verifyCallerPrivilegeForUserOrAccountOperations(User user) { logger.debug(String.format("Verifying whether the caller has the correct privileges based on the user's role type and API permissions: %s", user)); diff --git a/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java b/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java index 4c7c86baeb7..d1ae1b44a51 100644 --- a/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java @@ -393,8 +393,6 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu throw new PermissionDeniedException("Rule already exists for the role: " + role.getName()); } - accountManager.refreshRoleCheckersCacheOnPermissionsChange(role); - return Transaction.execute(new TransactionCallback<RolePermissionVO>() { @Override public RolePermissionVO doInTransaction(TransactionStatus status) { @@ -430,7 +428,6 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu if (role.isDefault()) { throw new PermissionDeniedException("Role permission cannot be deleted for Default roles"); } - accountManager.refreshRoleCheckersCacheOnPermissionsChange(role); return rolePermission != null && rolePermissionsDao.remove(rolePermission.getId()); } diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 2a86fe9004a..b812f3bca93 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -24,12 +24,9 @@ import java.net.UnknownHostException; 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; @@ -1587,119 +1584,4 @@ public class AccountManagerImplTest extends AccountManagentImplTestBase { 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<String> 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<String> allApis = Arrays.asList("api1", "api2", "api3"); - List<String> 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<String> allApis = Arrays.asList("api1", "api2", "api3"); - List<String> requestedApis = Arrays.asList("api1", "api2", "api3"); - List<String> 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<String> allApis = Arrays.asList("api1", "api2", "api3"); - List<String> afterChecker1 = Arrays.asList("api1", "api2"); - List<String> 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); - } }
