Skip to content

server: optimize account creation by pre-loading the role permissions #11137

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
weizhouapache wants to merge 4 commits into from
Closed

server: optimize account creation by pre-loading the role permissions #11137

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5848ac
server: optimize account creation by pre-loading the role permissions
weizhouapache Jul 4, 2025
fc10728
APIChecker: add default implementation for methods
weizhouapache Aug 18, 2025
d395684
Update server/src/main/java/com/cloud/user/AccountManagerImpl.java
weizhouapache Feb 20, 2026
37fe152
Update server/src/main/java/com/cloud/user/AccountManagerImpl.java
weizhouapache Feb 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/src/main/java/org/apache/cloudstack/acl/APIChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
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;
Expand All @@ -43,4 +44,7 @@ public interface APIChecker extends Adapter {
*/
List<String> getApisAllowedToUser(Role role, User user, List<String> apiNames) throws PermissionDeniedException;
boolean isEnabled();

Pair<Role, List<RolePermission>> getRolePermissions(long roleId);
boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions);
}
24 changes: 22 additions & 2 deletions ...-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@
return accountService.getAccount(accountId);
}

protected Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
@Override
public Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
final Role accountRole = roleService.findRole(roleId);
if (accountRole == null || accountRole.getId() < 1L) {
return new Pair<>(null, null);
Expand Down Expand Up @@ -149,7 +150,7 @@
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<RolePermission> allPermissions = roleAndPermissions.second();
Expand Down Expand Up @@ -180,6 +181,25 @@
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<RolePermission> allPermissions) {

Check warning on line 185 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L185 was not covered by tests
if (accountRole == null) {
throw new PermissionDeniedException(String.format("The account [%s] has role null or unknown.", account));

Check warning on line 187 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L187 was not covered by tests
}

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));

Check warning on line 192 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L192 was not covered by tests
}
return true;

Check warning on line 194 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L194 was not covered by tests
}

if (checkApiPermissionByRole(accountRole, commandName, allPermissions)) {
return true;

Check warning on line 198 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L198 was not covered by tests
}
throw new UnavailableCommandException(String.format("The API [%s] does not exist or is not available for the account %s.", commandName, account));
}

Check warning on line 201 in plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L200 - L201 were not covered by tests
Comment on lines +184 to +201
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DynamicRoleBasedAPIAccessChecker adds a new checkAccess(Account, String, Role, List<RolePermission>) path, but the existing unit tests in this module only cover checkAccess(User, String) / checkAccess(Account, String). Adding tests for the new overload (allow/deny and root-admin cases) would help prevent regressions in the new preloaded-permissions flow used during account creation.

Copilot uses AI. Check for mistakes.

/**
* Only one strategy should be used between StaticRoleBasedAPIAccessChecker and DynamicRoleBasedAPIAccessChecker
* Default behavior is to use the Dynamic version. The StaticRoleBasedAPIAccessChecker is the legacy version.
Expand Down
13 changes: 12 additions & 1 deletion ...-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@

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;
Expand All @@ -33,6 +33,7 @@
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;

Expand Down Expand Up @@ -195,4 +196,14 @@
public void setServices(List<PluggableService> services) {
this.services = services;
}

@Override
public Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
return null;
}

Check warning on line 203 in plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java#L201-L203 

Added lines #L201 - L203 were not covered by tests

@Override
public boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) {
return false;
}

Check warning on line 208 in plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java#L206-L208 

Added lines #L206 - L208 were not covered by tests
}
10 changes: 10 additions & 0 deletions ...c-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
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;
Expand Down Expand Up @@ -200,4 +201,13 @@
this.commandPropertyFiles = commandPropertyFiles;
}

@Override
public Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
return null;

Check warning on line 206 in plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java#L206 

Added line #L206 was not covered by tests
}

@Override
public boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) {
return false;

Check warning on line 211 in plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java

View check run for this annotation

Codecov / codecov/patch

plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java#L211 

Added line #L211 was not covered by tests
}
}
12 changes: 12 additions & 0 deletions ...api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
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;

Expand All @@ -42,6 +43,7 @@
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
Expand Down Expand Up @@ -256,4 +258,14 @@
this.enabled = enabled;

}

@Override
public Pair<Role, List<RolePermission>> getRolePermissions(long roleId) {
return null;
}

Check warning on line 265 in plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java

View check run for this annotation

Codecov / codecov/patch

plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java#L263-L265 

Added lines #L263 - L265 were not covered by tests

@Override
public boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) {
return false;
}

Check warning on line 270 in plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java

View check run for this annotation

Codecov / codecov/patch

plugins/api/rate-limit/src/main/java/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java#L268-L270 

Added lines #L268 - L270 were not covered by tests
}
35 changes: 21 additions & 14 deletions server/src/main/java/com/cloud/user/AccountManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
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;
Expand Down Expand Up @@ -1431,29 +1432,35 @@
requested.getUuid(),
requested.getRoleId()));
}
if (caller.getRoleId().equals(requested.getRoleId())) {
return;

Check warning on line 1436 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/user/AccountManagerImpl.java#L1436 

Added line #L1436 was not covered by tests
}
List<APIChecker> apiCheckers = getEnabledApiCheckers();
for (APIChecker apiChecker : apiCheckers) {
checkApiAccess(apiChecker, caller, requested);
}
Comment on lines 1438 to +1441
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkRoleEscalation iterates over every enabled APIChecker. This includes non-ACL checkers like ApiRateLimitServiceImpl which increments per-account counters in checkAccess(Account, String) (and may throw RequestLimitException). That means creating an account can unexpectedly consume/trigger API rate limits and also interfere with the “requested can access” probing (exceptions are treated as “irrelevant”). Consider filtering the checkers used here to APIAclChecker only (or otherwise excluding rate limiting and other non-ACL checkers) so role escalation validation only evaluates permissions.

Copilot uses AI. Check for mistakes.
}

Check warning on line 1442 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/user/AccountManagerImpl.java#L1440-L1442 

Added lines #L1440 - L1442 were not covered by tests

private void checkApiAccess(APIChecker apiChecker, Account caller, Account requested) throws PermissionDeniedException {
Pair<Role, List<RolePermission>> roleAndPermissionsForCaller = apiChecker.getRolePermissions(caller.getRoleId());
Pair<Role, List<RolePermission>> roleAndPermissionsForRequested = apiChecker.getRolePermissions(requested.getRoleId());

Check warning on line 1446 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/user/AccountManagerImpl.java#L1444-L1446 

Added lines #L1444 - L1446 were not covered by tests
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);

Check warning on line 1450 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/user/AccountManagerImpl.java#L1450 

Added line #L1450 was not covered by tests
} else {
apiChecker.checkAccess(caller, command, roleAndPermissionsForRequested.first(), roleAndPermissionsForRequested.second());

Check warning on line 1452 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/user/AccountManagerImpl.java#L1452 

Added line #L1452 was not covered by tests
}
} catch (PermissionDeniedException pde) {

Check warning on line 1454 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/user/AccountManagerImpl.java#L1454 

Added line #L1454 was not covered by tests
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);

Check warning on line 1460 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/user/AccountManagerImpl.java#L1460 

Added line #L1460 was not covered by tests
} else {
apiChecker.checkAccess(caller, command, roleAndPermissionsForCaller.first(), roleAndPermissionsForCaller.second());

Check warning on line 1462 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/user/AccountManagerImpl.java#L1462 

Added line #L1462 was not covered by tests
}
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()));
Expand Down
Loading