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
+47 −16
Closed

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

Show file tree
Hide file tree
Changes from all commits
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();

default Pair<Role, List<RolePermission>> getRolePermissions(long roleId) { return null; }
default boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) { return false; }
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.

APIChecker.checkAccess(Account, String, Role, List<RolePermission>) has a default implementation that returns false. In AccountManagerImpl.checkApiAccess(...) the return value is ignored and only exceptions deny access, so a false default will effectively behave like “allowed” if any implementation ever returns a non-null value from getRolePermissions(...) but forgets to override this new checkAccess(...) overload. Consider changing the default implementation to delegate to the existing checkAccess(Account, String) (or throw an UnsupportedOperationException) so the safe/legacy behavior is preserved by default.

Suggested change
default boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) { return false; }
default boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) {
return checkAccess(account, commandName);
}

Copilot uses AI. Check for mistakes.
Comment on lines +48 to +49
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.

The new checkAccess(Account, String, Role, List<RolePermission>) overload and getRolePermissions(long) are role/ACL-specific, but they were added to the base APIChecker interface (which is also used by non-ACL checkers like rate limiting). To avoid coupling unrelated checkers to role permission concepts, consider moving these new methods to APIAclChecker (or another ACL-specific interface) instead of APIChecker.

Suggested change
default Pair<Role, List<RolePermission>> getRolePermissions(long roleId) { return null; }
default boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) { return false; }
/**
* ACL-specific extension for API checkers that work with roles and role permissions.
*/
interface APIAclChecker extends APIChecker {
default Pair<Role, List<RolePermission>> getRolePermissions(long roleId) { return null; }
default boolean checkAccess(Account account, String commandName, Role accountRole, List<RolePermission> allPermissions) { return false; }
}

Copilot uses AI. Check for mistakes.
}
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 @@ protected Account getAccountFromId(long accountId) {
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 @@ public boolean checkAccess(User user, String commandName) throws PermissionDenie
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 @@ public boolean checkAccess(Account account, String commandName) {
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) {
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));
}
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
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 @@ protected void checkRoleEscalation(Account caller, Account requested) {
requested.getUuid(),
requested.getRoleId()));
}
if (caller.getRoleId().equals(requested.getRoleId())) {
return;
}
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.
}

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());
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(requested, command);
} else {
apiChecker.checkAccess(requested, 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()));
Expand Down
Loading