From 429c9454b1018baa5957403b0f4b25892212da6b Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Thu, 5 Mar 2026 15:15:07 -0800 Subject: [PATCH 01/15] Fix checkRoleEscalation performance and bugs in access checking Replace per-API-command loop (~1,484 DB calls) with batch getApisAllowedToAccount calls using cached role permissions, reducing to ~2 DB calls (or 0 with warm cache). Fix NPE on pde.getAccount(), format string mismatch (3 %s but 4 args), and use cached permissions in DynamicRoleBasedAPIAccessChecker.checkAccess(Account, String). --- .../org/apache/cloudstack/acl/APIChecker.java | 15 ++++++ .../acl/DynamicRoleBasedAPIAccessChecker.java | 25 +++++++++- .../acl/ProjectRoleBasedApiAccessChecker.java | 5 ++ .../acl/StaticRoleBasedAPIAccessChecker.java | 16 +++++++ .../com/cloud/user/AccountManagerImpl.java | 46 ++++++------------- 5 files changed, 74 insertions(+), 33 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 660f64f43ef2..fe3bbb676e72 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -21,6 +21,7 @@ import com.cloud.user.User; import com.cloud.utils.component.Adapter; +import java.util.ArrayList; import java.util.List; /** @@ -42,5 +43,19 @@ 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 (PermissionDeniedException e) { + // not allowed, skip + } + } + 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/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..4dbc91040fe7 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1435,40 +1435,22 @@ 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); - } + List allApis = new ArrayList<>(apiNameList); + List requestedAllowed = allApis; + for (final APIChecker apiChecker : apiCheckers) { + requestedAllowed = apiChecker.getApisAllowedToAccount(requested, requestedAllowed); } - } - - private void checkApiAccess(List apiCheckers, Account caller, String command) { + List callerAllowed = requestedAllowed; for (final APIChecker apiChecker : apiCheckers) { - apiChecker.checkAccess(caller, command); + 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); + s_logger.warn(msg); + throw new PermissionDeniedException(msg); } } From 7055bf211a2ca3b57e5e94a696cd2866a3afe3ed Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Thu, 5 Mar 2026 15:55:53 -0800 Subject: [PATCH 02/15] Add tests for checkRoleEscalation, checkAccess, and getApisAllowedToAccount Add tests for DynamicRoleBasedAPIAccessChecker covering checkAccess(Account, String) and getApisAllowedToAccount including cache verification, admin pass-through, allow/deny filtering, and annotation fallback. Add tests for AccountManagerImpl.checkRoleEscalation covering same permissions, caller superset, escalation detection, empty API list, and multi-checker chaining. --- .../DynamicRoleBasedAPIAccessCheckerTest.java | 95 +++++++++++ .../cloud/user/AccountManagerImplTest.java | 154 ++++++++++++++++++ 2 files changed, 249 insertions(+) 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..ed0ac1d46091 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 @@ -195,4 +195,99 @@ 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() { + final String allowedApiName = "someAllowedApi"; + final RolePermission permission = new RolePermissionVO(1L, allowedApiName, Permission.ALLOW, null); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); + 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() { + final RolePermission permission = new RolePermissionVO(1L, "api1", Permission.ALLOW, null); + Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); + apiAccessCheckerSpy.getApisAllowedToAccount(getTestAccount(), new ArrayList<>(Arrays.asList("api1"))); + Mockito.verify(roleServiceMock, Mockito.times(1)).findAllPermissionsBy(Mockito.anyLong()); + } } diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index b812f3bca938..34578cfb9455 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -24,9 +24,11 @@ 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.APIChecker; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.Role; import org.apache.cloudstack.acl.RoleService; @@ -1584,4 +1586,156 @@ 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); + Mockito.when(checker.getApisAllowedToAccount(Mockito.any(Account.class), Mockito.anyList())).thenReturn(apis); + + Account caller = Mockito.mock(Account.class); + Mockito.when(caller.getAccountName()).thenReturn("caller"); + Mockito.when(caller.getUuid()).thenReturn("caller-uuid"); + Mockito.when(caller.getRoleId()).thenReturn(4L); + + Account requested = Mockito.mock(Account.class); + Mockito.when(requested.getAccountName()).thenReturn("requested"); + Mockito.when(requested.getUuid()).thenReturn("requested-uuid"); + Mockito.when(requested.getRoleId()).thenReturn(4L); + + 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"); + + APIChecker checker = Mockito.mock(APIChecker.class); + Mockito.when(checker.isEnabled()).thenReturn(true); + + Account caller = Mockito.mock(Account.class); + Mockito.when(caller.getAccountName()).thenReturn("caller"); + Mockito.when(caller.getUuid()).thenReturn("caller-uuid"); + Mockito.when(caller.getRoleId()).thenReturn(4L); + + Account requested = Mockito.mock(Account.class); + Mockito.when(requested.getAccountName()).thenReturn("requested"); + Mockito.when(requested.getUuid()).thenReturn("requested-uuid"); + Mockito.when(requested.getRoleId()).thenReturn(5L); + + Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(requested), Mockito.anyList())).thenReturn(requestedApis); + Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(caller), Mockito.anyList())).thenReturn(requestedApis); + + 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"); + + APIChecker checker = Mockito.mock(APIChecker.class); + Mockito.when(checker.isEnabled()).thenReturn(true); + + Account caller = Mockito.mock(Account.class); + Mockito.when(caller.getAccountName()).thenReturn("caller"); + Mockito.when(caller.getUuid()).thenReturn("caller-uuid"); + Mockito.when(caller.getRoleId()).thenReturn(4L); + Mockito.when(caller.getDomainId()).thenReturn(1L); + + Account requested = Mockito.mock(Account.class); + Mockito.when(requested.getAccountName()).thenReturn("requested"); + Mockito.when(requested.getUuid()).thenReturn("requested-uuid"); + Mockito.when(requested.getRoleId()).thenReturn(5L); + + 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 { + APIChecker checker = Mockito.mock(APIChecker.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); + Mockito.when(caller.getAccountName()).thenReturn("caller"); + Mockito.when(caller.getUuid()).thenReturn("caller-uuid"); + Mockito.when(caller.getRoleId()).thenReturn(4L); + + Account requested = Mockito.mock(Account.class); + Mockito.when(requested.getAccountName()).thenReturn("requested"); + Mockito.when(requested.getUuid()).thenReturn("requested-uuid"); + Mockito.when(requested.getRoleId()).thenReturn(5L); + + 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"); + + APIChecker checker1 = Mockito.mock(APIChecker.class); + Mockito.when(checker1.isEnabled()).thenReturn(true); + APIChecker checker2 = Mockito.mock(APIChecker.class); + Mockito.when(checker2.isEnabled()).thenReturn(true); + + Account caller = Mockito.mock(Account.class); + Mockito.when(caller.getAccountName()).thenReturn("caller"); + Mockito.when(caller.getUuid()).thenReturn("caller-uuid"); + Mockito.when(caller.getRoleId()).thenReturn(4L); + + Account requested = Mockito.mock(Account.class); + Mockito.when(requested.getAccountName()).thenReturn("requested"); + Mockito.when(requested.getUuid()).thenReturn("requested-uuid"); + Mockito.when(requested.getRoleId()).thenReturn(5L); + + // 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); + } } From 098ba06e62b8defdd9b1820a5c7bdd0ad48e49d5 Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Tue, 10 Mar 2026 16:46:14 -0700 Subject: [PATCH 03/15] Add debug logging exception --- api/src/main/java/org/apache/cloudstack/acl/APIChecker.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 fe3bbb676e72..d23ed0a3c350 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -24,10 +24,13 @@ import java.util.ArrayList; import java.util.List; +import org.apache.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 = Logger.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 @@ -51,7 +54,7 @@ default List getApisAllowedToAccount(Account account, List apiNa checkAccess(account, apiName); allowedApis.add(apiName); } catch (PermissionDeniedException e) { - // not allowed, skip + LOGGER.debug("Account [" + account + "] is not allowed to access API [" + apiName + "]", e); } } return allowedApis; From f84ed2aadf350191c1cee89373353a192729a3eb Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Wed, 11 Mar 2026 14:56:45 -0700 Subject: [PATCH 04/15] make logging vars consistent to other files --- api/src/main/java/org/apache/cloudstack/acl/APIChecker.java | 4 ++-- 1 file changed, 2 insertions(+), 2 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 d23ed0a3c350..52fda20437c7 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -30,7 +30,7 @@ * APICheckers is designed to verify the ownership of resources and to control the access to APIs. */ public interface APIChecker extends Adapter { - Logger LOGGER = Logger.getLogger(APIChecker.class); + Logger s_logger = Logger.getLogger(APIChecker.class.getName()); // 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 @@ -54,7 +54,7 @@ default List getApisAllowedToAccount(Account account, List apiNa checkAccess(account, apiName); allowedApis.add(apiName); } catch (PermissionDeniedException e) { - LOGGER.debug("Account [" + account + "] is not allowed to access API [" + apiName + "]", e); + s_logger.debug("Account [" + account + "] is not allowed to access API [" + apiName + "]", e); } } return allowedApis; From 5996ee16ecf51efaad423d012366cd86d70c84d4 Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Thu, 19 Mar 2026 09:56:25 +0900 Subject: [PATCH 05/15] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../java/com/cloud/user/AccountManagerImpl.java | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 4dbc91040fe7..1039ddde1534 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,13 +1436,22 @@ protected void checkRoleEscalation(Account caller, Account requested) { requested.getRoleId())); } List apiCheckers = getEnabledApiCheckers(); + + // 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 APIChecker apiChecker : apiCheckers) { + for (final APIAclChecker apiChecker : aclCheckers) { requestedAllowed = apiChecker.getApisAllowedToAccount(requested, requestedAllowed); } List callerAllowed = requestedAllowed; - for (final APIChecker apiChecker : apiCheckers) { + for (final APIAclChecker apiChecker : aclCheckers) { callerAllowed = apiChecker.getApisAllowedToAccount(caller, callerAllowed); } if (callerAllowed.size() < requestedAllowed.size()) { From d6289b648ab05bc813ae8ddfad0446b180783244 Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Thu, 19 Mar 2026 09:56:49 +0900 Subject: [PATCH 06/15] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- api/src/main/java/org/apache/cloudstack/acl/APIChecker.java | 5 +++++ 1 file changed, 5 insertions(+) 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 52fda20437c7..84889627f255 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -17,6 +17,7 @@ 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; @@ -53,6 +54,10 @@ default List getApisAllowedToAccount(Account account, List apiNa 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) { s_logger.debug("Account [" + account + "] is not allowed to access API [" + apiName + "]", e); } From 5dce6fc7d7f0af44daaa385abc506a696c41783d Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Thu, 19 Mar 2026 09:57:18 +0900 Subject: [PATCH 07/15] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- api/src/main/java/org/apache/cloudstack/acl/APIChecker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 84889627f255..05700bc690ed 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -59,7 +59,7 @@ default List getApisAllowedToAccount(Account account, List apiNa // 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) { - s_logger.debug("Account [" + account + "] is not allowed to access API [" + apiName + "]", e); + s_logger.trace("Account [" + account + "] is not allowed to access API [" + apiName + "]"); } } return allowedApis; From 3b5c9195f887a1bb305ad1b7e6acfea89c4e2328 Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Thu, 19 Mar 2026 09:59:03 +0900 Subject: [PATCH 08/15] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../acl/DynamicRoleBasedAPIAccessCheckerTest.java | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) 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 ed0ac1d46091..1c45789191d8 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 @@ -228,11 +228,21 @@ public void testCheckAccessAccountDeniedApi() { } @Test - public void testCheckAccessAccountUsesCachedPermissions() { + public void testCheckAccessAccountUsesCachedPermissions() throws Exception { + // Enable caching by setting a positive cachePeriod + Field cachePeriodField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("cachePeriod"); + cachePeriodField.setAccessible(true); + cachePeriodField.set(apiAccessCheckerSpy, 1L); + 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()); } From d0cad25a5b5bc9c4ca3c8f83a99060c40d5ae719 Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Thu, 19 Mar 2026 10:22:15 +0900 Subject: [PATCH 09/15] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../DynamicRoleBasedAPIAccessCheckerTest.java | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) 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 1c45789191d8..2915b475577a 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 @@ -295,9 +295,26 @@ public void testGetApisAllowedToAccountAnnotationFallback() { @Test public void testGetApisAllowedToAccountUsesCachedPermissions() { - final RolePermission permission = new RolePermissionVO(1L, "api1", Permission.ALLOW, null); - Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); - apiAccessCheckerSpy.getApisAllowedToAccount(getTestAccount(), new ArrayList<>(Arrays.asList("api1"))); - Mockito.verify(roleServiceMock, Mockito.times(1)).findAllPermissionsBy(Mockito.anyLong()); + try { + // Ensure caching is enabled by setting a positive cachePeriod + Field cachePeriodField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("cachePeriod"); + cachePeriodField.setAccessible(true); + cachePeriodField.set(apiAccessCheckerSpy, 1L); + + 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()); + } } } From 987b3b3448f6d8b17b288d7d1b1b876a43684af5 Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Thu, 19 Mar 2026 10:30:20 +0900 Subject: [PATCH 10/15] fix complation and test --- .../java/com/cloud/user/AccountManagerImplTest.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 34578cfb9455..a43800f0e675 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -28,6 +28,7 @@ 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; @@ -1632,7 +1633,7 @@ public void testCheckRoleEscalationCallerHasMorePermissionsShouldPass() throws E List allApis = Arrays.asList("api1", "api2", "api3"); List requestedApis = Arrays.asList("api1", "api2"); - APIChecker checker = Mockito.mock(APIChecker.class); + APIAclChecker checker = Mockito.mock(APIAclChecker.class); Mockito.when(checker.isEnabled()).thenReturn(true); Account caller = Mockito.mock(Account.class); @@ -1646,7 +1647,7 @@ public void testCheckRoleEscalationCallerHasMorePermissionsShouldPass() throws E Mockito.when(requested.getRoleId()).thenReturn(5L); Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(requested), Mockito.anyList())).thenReturn(requestedApis); - Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(caller), 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)); @@ -1660,7 +1661,7 @@ public void testCheckRoleEscalationRequestedHasMorePermissionsShouldThrow() thro List requestedApis = Arrays.asList("api1", "api2", "api3"); List callerApis = Arrays.asList("api1"); - APIChecker checker = Mockito.mock(APIChecker.class); + APIAclChecker checker = Mockito.mock(APIAclChecker.class); Mockito.when(checker.isEnabled()).thenReturn(true); Account caller = Mockito.mock(Account.class); @@ -1685,7 +1686,7 @@ public void testCheckRoleEscalationRequestedHasMorePermissionsShouldThrow() thro @Test public void testCheckRoleEscalationEmptyApiListShouldPass() throws Exception { - APIChecker checker = Mockito.mock(APIChecker.class); + 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()); @@ -1711,9 +1712,9 @@ public void testCheckRoleEscalationMultipleCheckersAppliedSequentially() throws List afterChecker1 = Arrays.asList("api1", "api2"); List afterChecker2 = Arrays.asList("api1"); - APIChecker checker1 = Mockito.mock(APIChecker.class); + APIAclChecker checker1 = Mockito.mock(APIAclChecker.class); Mockito.when(checker1.isEnabled()).thenReturn(true); - APIChecker checker2 = Mockito.mock(APIChecker.class); + APIAclChecker checker2 = Mockito.mock(APIAclChecker.class); Mockito.when(checker2.isEnabled()).thenReturn(true); Account caller = Mockito.mock(Account.class); From 8e04b154a5bb598d473c91a3cc0f632a7338e2a8 Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Fri, 20 Mar 2026 10:56:01 +0900 Subject: [PATCH 11/15] fix mokito UnnecessaryStubbing --- server/src/test/java/com/cloud/user/AccountManagerImplTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index a43800f0e675..1a01cc0ee46d 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -1610,7 +1610,6 @@ public void testCheckRoleEscalationSamePermissionsShouldPass() throws Exception APIChecker checker = Mockito.mock(APIChecker.class); List apis = Arrays.asList("api1", "api2", "api3"); Mockito.when(checker.isEnabled()).thenReturn(true); - Mockito.when(checker.getApisAllowedToAccount(Mockito.any(Account.class), Mockito.anyList())).thenReturn(apis); Account caller = Mockito.mock(Account.class); Mockito.when(caller.getAccountName()).thenReturn("caller"); From 63ae895269101486e7be90b308db67050c731ed0 Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Fri, 20 Mar 2026 11:59:37 +0900 Subject: [PATCH 12/15] fix cachePeriodField illegal arg --- .../cloudstack/acl/DynamicRoleBasedAPIAccessCheckerTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 2915b475577a..eb5f68c3921a 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 @@ -232,7 +232,7 @@ public void testCheckAccessAccountUsesCachedPermissions() throws Exception { // Enable caching by setting a positive cachePeriod Field cachePeriodField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("cachePeriod"); cachePeriodField.setAccessible(true); - cachePeriodField.set(apiAccessCheckerSpy, 1L); + cachePeriodField.set(apiAccessCheckerSpy, 1); final String allowedApiName = "someAllowedApi"; final RolePermission permission = new RolePermissionVO(1L, allowedApiName, Permission.ALLOW, null); @@ -299,7 +299,7 @@ public void testGetApisAllowedToAccountUsesCachedPermissions() { // Ensure caching is enabled by setting a positive cachePeriod Field cachePeriodField = DynamicRoleBasedAPIAccessChecker.class.getDeclaredField("cachePeriod"); cachePeriodField.setAccessible(true); - cachePeriodField.set(apiAccessCheckerSpy, 1L); + cachePeriodField.set(apiAccessCheckerSpy, 1); final RolePermission permission = new RolePermissionVO(1L, "api1", Permission.ALLOW, null); Mockito.when(roleServiceMock.findAllPermissionsBy(Mockito.anyLong())).thenReturn(Collections.singletonList(permission)); From 4eda50efa8b713aead2ffa58fd34832717c2a9ab Mon Sep 17 00:00:00 2001 From: Aaron Chung Date: Fri, 20 Mar 2026 12:24:16 +0900 Subject: [PATCH 13/15] fix null test --- .../acl/DynamicRoleBasedAPIAccessCheckerTest.java | 10 ++++++++++ 1 file changed, 10 insertions(+) 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 eb5f68c3921a..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; @@ -234,6 +236,10 @@ public void testCheckAccessAccountUsesCachedPermissions() throws Exception { 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)); @@ -301,6 +307,10 @@ public void testGetApisAllowedToAccountUsesCachedPermissions() { 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)); From 5b1aca4b4d4daa57cf1e81e0c9022954356660eb Mon Sep 17 00:00:00 2001 From: nvazquez Date: Tue, 7 Apr 2026 10:15:38 -0300 Subject: [PATCH 14/15] Fix build --- .../main/java/org/apache/cloudstack/acl/APIChecker.java | 7 ++++--- .../src/main/java/com/cloud/user/AccountManagerImpl.java | 8 +++++++- 2 files changed, 11 insertions(+), 4 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 05700bc690ed..b50ec825aecc 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -25,13 +25,14 @@ import java.util.ArrayList; import java.util.List; -import org.apache.log4j.Logger; +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 s_logger = Logger.getLogger(APIChecker.class.getName()); + 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 @@ -59,7 +60,7 @@ default List getApisAllowedToAccount(Account account, List apiNa // 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) { - s_logger.trace("Account [" + account + "] is not allowed to access API [" + apiName + "]"); + LOGGER.trace("Account [" + account + "] is not allowed to access API [" + apiName + "]"); } } return allowedApis; diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 1039ddde1534..fbdc6bdc2ff3 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1459,11 +1459,17 @@ protected void checkRoleEscalation(Account caller, Account requested) { 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); - s_logger.warn(msg); + logger.warn(msg); throw new PermissionDeniedException(msg); } } + private void checkApiAccess(List apiCheckers, Account caller, String command) { + for (final APIChecker apiChecker : apiCheckers) { + apiChecker.checkAccess(caller, command); + } + } + @Override public void checkApiAccess(Account caller, String command) { List apiCheckers = getEnabledApiCheckers(); From 3a3df829a9afe23cf944ae9ca7bce0ff065e5db0 Mon Sep 17 00:00:00 2001 From: nvazquez Date: Tue, 7 Apr 2026 11:08:05 -0300 Subject: [PATCH 15/15] Remove unnecessary stubbings --- .../cloud/user/AccountManagerImplTest.java | 36 ------------------- 1 file changed, 36 deletions(-) diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 1a01cc0ee46d..2a86fe9004a5 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -1612,14 +1612,7 @@ public void testCheckRoleEscalationSamePermissionsShouldPass() throws Exception Mockito.when(checker.isEnabled()).thenReturn(true); Account caller = Mockito.mock(Account.class); - Mockito.when(caller.getAccountName()).thenReturn("caller"); - Mockito.when(caller.getUuid()).thenReturn("caller-uuid"); - Mockito.when(caller.getRoleId()).thenReturn(4L); - Account requested = Mockito.mock(Account.class); - Mockito.when(requested.getAccountName()).thenReturn("requested"); - Mockito.when(requested.getUuid()).thenReturn("requested-uuid"); - Mockito.when(requested.getRoleId()).thenReturn(4L); accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker)); setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>(apis)); @@ -1636,14 +1629,7 @@ public void testCheckRoleEscalationCallerHasMorePermissionsShouldPass() throws E Mockito.when(checker.isEnabled()).thenReturn(true); Account caller = Mockito.mock(Account.class); - Mockito.when(caller.getAccountName()).thenReturn("caller"); - Mockito.when(caller.getUuid()).thenReturn("caller-uuid"); - Mockito.when(caller.getRoleId()).thenReturn(4L); - Account requested = Mockito.mock(Account.class); - Mockito.when(requested.getAccountName()).thenReturn("requested"); - Mockito.when(requested.getUuid()).thenReturn("requested-uuid"); - Mockito.when(requested.getRoleId()).thenReturn(5L); Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(requested), Mockito.anyList())).thenReturn(requestedApis); Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(caller), Mockito.anyList())).thenReturn(allApis); @@ -1664,15 +1650,7 @@ public void testCheckRoleEscalationRequestedHasMorePermissionsShouldThrow() thro Mockito.when(checker.isEnabled()).thenReturn(true); Account caller = Mockito.mock(Account.class); - Mockito.when(caller.getAccountName()).thenReturn("caller"); - Mockito.when(caller.getUuid()).thenReturn("caller-uuid"); - Mockito.when(caller.getRoleId()).thenReturn(4L); - Mockito.when(caller.getDomainId()).thenReturn(1L); - Account requested = Mockito.mock(Account.class); - Mockito.when(requested.getAccountName()).thenReturn("requested"); - Mockito.when(requested.getUuid()).thenReturn("requested-uuid"); - Mockito.when(requested.getRoleId()).thenReturn(5L); Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(requested), Mockito.anyList())).thenReturn(requestedApis); Mockito.when(checker.getApisAllowedToAccount(Mockito.eq(caller), Mockito.anyList())).thenReturn(callerApis); @@ -1690,14 +1668,7 @@ public void testCheckRoleEscalationEmptyApiListShouldPass() throws Exception { Mockito.when(checker.getApisAllowedToAccount(Mockito.any(Account.class), Mockito.anyList())).thenReturn(Collections.emptyList()); Account caller = Mockito.mock(Account.class); - Mockito.when(caller.getAccountName()).thenReturn("caller"); - Mockito.when(caller.getUuid()).thenReturn("caller-uuid"); - Mockito.when(caller.getRoleId()).thenReturn(4L); - Account requested = Mockito.mock(Account.class); - Mockito.when(requested.getAccountName()).thenReturn("requested"); - Mockito.when(requested.getUuid()).thenReturn("requested-uuid"); - Mockito.when(requested.getRoleId()).thenReturn(5L); accountManagerImpl.setApiAccessCheckers(Arrays.asList(checker)); setPrivateField(accountManagerImpl, "apiNameList", new ArrayList<>()); @@ -1717,14 +1688,7 @@ public void testCheckRoleEscalationMultipleCheckersAppliedSequentially() throws Mockito.when(checker2.isEnabled()).thenReturn(true); Account caller = Mockito.mock(Account.class); - Mockito.when(caller.getAccountName()).thenReturn("caller"); - Mockito.when(caller.getUuid()).thenReturn("caller-uuid"); - Mockito.when(caller.getRoleId()).thenReturn(4L); - Account requested = Mockito.mock(Account.class); - Mockito.when(requested.getAccountName()).thenReturn("requested"); - Mockito.when(requested.getUuid()).thenReturn("requested-uuid"); - Mockito.when(requested.getRoleId()).thenReturn(5L); // requested: checker1 filters to [api1, api2], checker2 further filters to [api1] Mockito.when(checker1.getApisAllowedToAccount(Mockito.eq(requested), Mockito.eq(allApis))).thenReturn(afterChecker1);