From e5848acdd073492cfcc4b74e2d9bf081791218df Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Fri, 4 Jul 2025 14:32:45 +0200 Subject: [PATCH 1/2] server: optimize account creation by pre-loading the role permissions --- .../org/apache/cloudstack/acl/APIChecker.java | 4 +++ .../acl/DynamicRoleBasedAPIAccessChecker.java | 24 +++++++++++-- .../acl/ProjectRoleBasedApiAccessChecker.java | 13 ++++++- .../acl/StaticRoleBasedAPIAccessChecker.java | 10 ++++++ .../ratelimit/ApiRateLimitServiceImpl.java | 12 +++++++ .../com/cloud/user/AccountManagerImpl.java | 35 +++++++++++-------- 6 files changed, 81 insertions(+), 17 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 660f64f43ef..08a80a52d5e 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -19,6 +19,7 @@ package org.apache.cloudstack.acl; import com.cloud.exception.PermissionDeniedException; import com.cloud.user.Account; import com.cloud.user.User; +import com.cloud.utils.Pair; import com.cloud.utils.component.Adapter; import java.util.List; @@ -43,4 +44,7 @@ public interface APIChecker extends Adapter { */ List getApisAllowedToUser(Role role, User user, List apiNames) throws PermissionDeniedException; boolean isEnabled(); + + Pair> getRolePermissions(long roleId); + boolean checkAccess(Account account, String commandName, Role accountRole, List allPermissions); } 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 030e0bcf014..8ae1ee19b24 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 @@ -107,7 +107,8 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API return accountService.getAccount(accountId); } - protected Pair> getRolePermissions(long roleId) { + @Override + public Pair> getRolePermissions(long roleId) { final Role accountRole = roleService.findRole(roleId); if (accountRole == null || accountRole.getId() < 1L) { return new Pair<>(null, null); @@ -149,7 +150,7 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API throw new PermissionDeniedException(String.format("Account role for user id [%s] cannot be found.", user.getUuid())); } if (accountRole.getRoleType() == RoleType.Admin && accountRole.getId() == RoleType.Admin.getId()) { - logger.info("Account for user id {} is Root Admin or Domain Admin, all APIs are allowed.", user.getUuid()); + logger.info("Account for user id {} is Root Admin, all APIs are allowed.", user.getUuid()); return true; } List allPermissions = roleAndPermissions.second(); @@ -180,6 +181,25 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for the account %s.", commandName, account)); } + @Override + public boolean checkAccess(Account account, String commandName, Role accountRole, List allPermissions) { + if (accountRole == null) { + throw new PermissionDeniedException(String.format("The account [%s] has role null or unknown.", account)); + } + + if (accountRole.getRoleType() == RoleType.Admin && accountRole.getId() == RoleType.Admin.getId()) { + if (logger.isTraceEnabled()) { + logger.trace(String.format("Account [%s] is Root Admin, all APIs are allowed.", account)); + } + return true; + } + + if (checkApiPermissionByRole(accountRole, commandName, allPermissions)) { + return true; + } + throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for the account %s.", commandName, account)); + } + /** * Only one strategy should be used between StaticRoleBasedAPIAccessChecker and DynamicRoleBasedAPIAccessChecker * Default behavior is to use the Dynamic version. The StaticRoleBasedAPIAccessChecker is the legacy version. 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 2e7ae23d6f1..3b98e736ec4 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 @@ -21,8 +21,8 @@ import java.util.Map; import javax.inject.Inject; import javax.naming.ConfigurationException; -import org.apache.cloudstack.acl.RolePermissionEntity.Permission; +import org.apache.cloudstack.acl.RolePermissionEntity.Permission; import org.apache.cloudstack.context.CallContext; import com.cloud.exception.PermissionDeniedException; @@ -33,6 +33,7 @@ import com.cloud.projects.dao.ProjectAccountDao; import com.cloud.user.Account; import com.cloud.user.AccountService; import com.cloud.user.User; +import com.cloud.utils.Pair; import com.cloud.utils.component.AdapterBase; import com.cloud.utils.component.PluggableService; @@ -195,4 +196,14 @@ public class ProjectRoleBasedApiAccessChecker extends AdapterBase implements AP public void setServices(List services) { this.services = services; } + + @Override + public Pair> getRolePermissions(long roleId) { + return null; + } + + @Override + public boolean checkAccess(Account account, String commandName, Role accountRole, List allPermissions) { + return false; + } } 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 3444f967d78..cb335a948e2 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 @@ -33,6 +33,7 @@ import com.cloud.exception.PermissionDeniedException; import com.cloud.user.Account; import com.cloud.user.AccountService; import com.cloud.user.User; +import com.cloud.utils.Pair; import com.cloud.utils.PropertiesUtil; import com.cloud.utils.component.AdapterBase; import com.cloud.utils.component.PluggableService; @@ -200,4 +201,13 @@ public class StaticRoleBasedAPIAccessChecker extends AdapterBase implements APIA this.commandPropertyFiles = commandPropertyFiles; } + @Override + public Pair> getRolePermissions(long roleId) { + return null; + } + + @Override + public boolean checkAccess(Account account, String commandName, Role accountRole, List allPermissions) { + return false; + } } diff --git a/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java b/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java index 917cd7bb2b4..901159a1de7 100644 --- a/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java +++ b/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java @@ -27,6 +27,7 @@ import net.sf.ehcache.Cache; import net.sf.ehcache.CacheManager; import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.acl.RolePermission; import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; import org.springframework.stereotype.Component; @@ -42,6 +43,7 @@ import com.cloud.exception.RequestLimitException; import com.cloud.user.Account; import com.cloud.user.AccountService; import com.cloud.user.User; +import com.cloud.utils.Pair; import com.cloud.utils.component.AdapterBase; @Component @@ -256,4 +258,14 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APIChecker, this.enabled = enabled; } + + @Override + public Pair> getRolePermissions(long roleId) { + return null; + } + + @Override + public boolean checkAccess(Account account, String commandName, Role accountRole, List allPermissions) { + return false; + } } diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index ecd761bb7d9..b4b8be61b15 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -47,6 +47,7 @@ import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.InfrastructureEntity; import org.apache.cloudstack.acl.QuerySelector; import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.acl.RolePermission; import org.apache.cloudstack.acl.RoleService; import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.acl.SecurityChecker; @@ -1431,29 +1432,35 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M requested.getUuid(), requested.getRoleId())); } + if (caller.getRoleId().equals(requested.getRoleId())) { + return; + } List apiCheckers = getEnabledApiCheckers(); + for (APIChecker apiChecker : apiCheckers) { + checkApiAccess(apiChecker, caller, requested); + } + } + + private void checkApiAccess(APIChecker apiChecker, Account caller, Account requested) throws PermissionDeniedException { + Pair> roleAndPermissionsForCaller = apiChecker.getRolePermissions(caller.getRoleId()); + Pair> roleAndPermissionsForRequested = apiChecker.getRolePermissions(requested.getRoleId()); 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() - ) - ); + if (roleAndPermissionsForRequested == null) { + apiChecker.checkAccess(caller, command); + } else { + apiChecker.checkAccess(caller, command, roleAndPermissionsForRequested.first(), roleAndPermissionsForRequested.second()); } + } catch (PermissionDeniedException pde) { 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)); + if (roleAndPermissionsForCaller == null) { + apiChecker.checkAccess(caller, command); + } else { + apiChecker.checkAccess(caller, command, roleAndPermissionsForCaller.first(), roleAndPermissionsForCaller.second()); } - 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())); From fc10728f10f3bb06e3f412eaf60e9379e6ff892e Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Mon, 18 Aug 2025 17:17:31 +0200 Subject: [PATCH 2/2] APIChecker: add default implementation for methods --- .../java/org/apache/cloudstack/acl/APIChecker.java | 4 ++-- .../acl/ProjectRoleBasedApiAccessChecker.java | 13 +------------ .../acl/StaticRoleBasedAPIAccessChecker.java | 10 ---------- .../ratelimit/ApiRateLimitServiceImpl.java | 12 ------------ 4 files changed, 3 insertions(+), 36 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 08a80a52d5e..ce06911f67a 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java +++ b/api/src/main/java/org/apache/cloudstack/acl/APIChecker.java @@ -45,6 +45,6 @@ public interface APIChecker extends Adapter { List getApisAllowedToUser(Role role, User user, List apiNames) throws PermissionDeniedException; boolean isEnabled(); - Pair> getRolePermissions(long roleId); - boolean checkAccess(Account account, String commandName, Role accountRole, List allPermissions); + default Pair> getRolePermissions(long roleId) { return null; } + default boolean checkAccess(Account account, String commandName, Role accountRole, List allPermissions) { return false; } } 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 3b98e736ec4..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 @@ -21,8 +21,8 @@ import java.util.Map; import javax.inject.Inject; import javax.naming.ConfigurationException; - import org.apache.cloudstack.acl.RolePermissionEntity.Permission; + import org.apache.cloudstack.context.CallContext; import com.cloud.exception.PermissionDeniedException; @@ -33,7 +33,6 @@ import com.cloud.projects.dao.ProjectAccountDao; import com.cloud.user.Account; import com.cloud.user.AccountService; import com.cloud.user.User; -import com.cloud.utils.Pair; import com.cloud.utils.component.AdapterBase; import com.cloud.utils.component.PluggableService; @@ -196,14 +195,4 @@ public class ProjectRoleBasedApiAccessChecker extends AdapterBase implements AP public void setServices(List services) { this.services = services; } - - @Override - public Pair> getRolePermissions(long roleId) { - return null; - } - - @Override - public boolean checkAccess(Account account, String commandName, Role accountRole, List allPermissions) { - return false; - } } 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 cb335a948e2..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 @@ -33,7 +33,6 @@ import com.cloud.exception.PermissionDeniedException; import com.cloud.user.Account; import com.cloud.user.AccountService; import com.cloud.user.User; -import com.cloud.utils.Pair; import com.cloud.utils.PropertiesUtil; import com.cloud.utils.component.AdapterBase; import com.cloud.utils.component.PluggableService; @@ -201,13 +200,4 @@ public class StaticRoleBasedAPIAccessChecker extends AdapterBase implements APIA this.commandPropertyFiles = commandPropertyFiles; } - @Override - public Pair> getRolePermissions(long roleId) { - return null; - } - - @Override - public boolean checkAccess(Account account, String commandName, Role accountRole, List allPermissions) { - return false; - } } diff --git a/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java b/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java index 901159a1de7..917cd7bb2b4 100644 --- a/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java +++ b/plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java @@ -27,7 +27,6 @@ import net.sf.ehcache.Cache; import net.sf.ehcache.CacheManager; import org.apache.cloudstack.acl.Role; -import org.apache.cloudstack.acl.RolePermission; import org.apache.cloudstack.utils.reflectiontostringbuilderutils.ReflectionToStringBuilderUtils; import org.springframework.stereotype.Component; @@ -43,7 +42,6 @@ import com.cloud.exception.RequestLimitException; import com.cloud.user.Account; import com.cloud.user.AccountService; import com.cloud.user.User; -import com.cloud.utils.Pair; import com.cloud.utils.component.AdapterBase; @Component @@ -258,14 +256,4 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APIChecker, this.enabled = enabled; } - - @Override - public Pair> getRolePermissions(long roleId) { - return null; - } - - @Override - public boolean checkAccess(Account account, String commandName, Role accountRole, List allPermissions) { - return false; - } }