Skip to content

refactor: avoid redundant DB calls when checking for root admin account #11390

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

refactor: avoid redundant DB calls when checking for root admin account #11390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0576578
refactor: avoid redundant DB calls when checking for root admin account
shwstppr Aug 3, 2025
61dbcca
Merge remote-tracking branch 'apache/main' into refactor-isrootadmin
shwstppr Aug 4, 2025
9ff1440
Merge remote-tracking branch 'apache/main' into refactor-isrootadmin
shwstppr Aug 4, 2025
9ec24e3
fix noredist
shwstppr Aug 4, 2025
3be274c
fix no bean error
shwstppr Aug 5, 2025
e14ffd4
build fix
shwstppr Aug 5, 2025
b2d4fb6
fix and changes
shwstppr Aug 5, 2025
ef5cf76
add tests
shwstppr Aug 5, 2025
ffddb7f
more test
shwstppr Aug 5, 2025
1235a7d
Merge remote-tracking branch 'apache/main' into refactor-isrootadmin
shwstppr Aug 6, 2025
c826221
add javadoc
shwstppr Aug 7, 2025
02b4e38
Merge branch 'main' into refactor-isrootadmin
shwstppr Nov 6, 2025
b98f54b
Merge remote-tracking branch 'apache/main' into refactor-isrootadmin
shwstppr Nov 27, 2025
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
8 changes: 8 additions & 0 deletions api/src/main/java/com/cloud/user/AccountService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,14 @@ User createUser(String userName, String password, String firstName, String lastN

boolean isRootAdmin(Long accountId);

/**
* Checks if the given account has ROOT admin privileges.
*
* @param account the account to check
* @return true if the account is a ROOT admin, false otherwise
*/
boolean isRootAdmin(Account account);

boolean isDomainAdmin(Long accountId);

boolean isResourceDomainAdmin(Long accountId);
Expand Down
3 changes: 2 additions & 1 deletion ...ain/java/org/apache/cloudstack/api/command/user/affinitygroup/CreateAffinityGroupCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ public long getEntityOwnerId() {
Account caller = CallContext.current().getCallingAccount();

//For domain wide affinity groups (if the affinity group processor type allows it)
if(projectId == null && domainId != null && accountName == null && _accountService.isRootAdmin(caller.getId())){
if(projectId == null && domainId != null && accountName == null &&
CallContext.current().isCallingAccountRootAdmin()) {
return Account.ACCOUNT_ID_SYSTEM;
}
Account owner = _accountService.finalizeOwner(caller, accountName, domainId, projectId);
Expand Down
3 changes: 2 additions & 1 deletion ...ain/java/org/apache/cloudstack/api/command/user/affinitygroup/DeleteAffinityGroupCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ public long getEntityOwnerId() {
Account caller = CallContext.current().getCallingAccount();

//For domain wide affinity groups (if the affinity group processor type allows it)
if(projectId == null && domainId != null && accountName == null && _accountService.isRootAdmin(caller.getId())){
if(projectId == null && domainId != null && accountName == null &&
CallContext.current().isCallingAccountRootAdmin()) {
return Account.ACCOUNT_ID_SYSTEM;
}
Account owner = _accountService.finalizeOwner(caller, accountName, domainId, projectId);
Expand Down
4 changes: 1 addition & 3 deletions ...rg/apache/cloudstack/api/command/user/storage/sharedfs/ChangeSharedFSDiskOfferingCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@

import com.cloud.event.EventTypes;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.user.Account;

@APICommand(name = "changeSharedFileSystemDiskOffering",
responseObject= SharedFSResponse.class,
Expand Down Expand Up @@ -130,8 +129,7 @@ public void execute() throws ResourceAllocationException {
SharedFS sharedFS = sharedFSService.changeSharedFSDiskOffering(this);
if (sharedFS != null) {
ResponseObject.ResponseView respView = getResponseView();
Account caller = CallContext.current().getCallingAccount();
if (_accountService.isRootAdmin(caller.getId())) {
if (CallContext.current().isCallingAccountRootAdmin()) {
respView = ResponseObject.ResponseView.Full;
}
SharedFSResponse response = _responseGenerator.createSharedFSResponse(respView, sharedFS);
Expand Down
6 changes: 2 additions & 4 deletions ...apache/cloudstack/api/command/user/storage/sharedfs/ChangeSharedFSServiceOfferingCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
import org.apache.cloudstack.api.ResponseObject;
import org.apache.cloudstack.api.ServerApiException;
import org.apache.cloudstack.api.command.user.UserCmd;
import org.apache.cloudstack.api.response.SharedFSResponse;
import org.apache.cloudstack.api.response.ServiceOfferingResponse;
import org.apache.cloudstack.api.response.SharedFSResponse;
import org.apache.cloudstack.context.CallContext;
import org.apache.cloudstack.storage.sharedfs.SharedFS;
import org.apache.cloudstack.storage.sharedfs.SharedFSService;
Expand All @@ -39,7 +39,6 @@
import com.cloud.exception.OperationTimedoutException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.exception.VirtualMachineMigrationException;
import com.cloud.user.Account;
import com.cloud.utils.exception.CloudRuntimeException;

@APICommand(name = "changeSharedFileSystemServiceOffering",
Expand Down Expand Up @@ -132,8 +131,7 @@ public void execute() {

if (sharedFS != null) {
ResponseObject.ResponseView respView = getResponseView();
Account caller = CallContext.current().getCallingAccount();
if (_accountService.isRootAdmin(caller.getId())) {
if (CallContext.current().isCallingAccountRootAdmin()) {
respView = ResponseObject.ResponseView.Full;
}
SharedFSResponse response = _responseGenerator.createSharedFSResponse(respView, sharedFS);
Expand Down
22 changes: 10 additions & 12 deletions .../main/java/org/apache/cloudstack/api/command/user/storage/sharedfs/CreateSharedFSCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,6 @@

import javax.inject.Inject;

import com.cloud.event.EventTypes;
import com.cloud.exception.ConcurrentOperationException;
import com.cloud.exception.InsufficientCapacityException;
import com.cloud.exception.OperationTimedoutException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.user.Account;
import com.cloud.utils.exception.CloudRuntimeException;

import org.apache.cloudstack.acl.RoleType;
import org.apache.cloudstack.api.APICommand;
import org.apache.cloudstack.api.ApiCommandResourceType;
Expand All @@ -40,16 +31,24 @@
import org.apache.cloudstack.api.command.user.UserCmd;
import org.apache.cloudstack.api.response.DiskOfferingResponse;
import org.apache.cloudstack.api.response.DomainResponse;
import org.apache.cloudstack.api.response.SharedFSResponse;
import org.apache.cloudstack.api.response.NetworkResponse;
import org.apache.cloudstack.api.response.ProjectResponse;
import org.apache.cloudstack.api.response.ServiceOfferingResponse;
import org.apache.cloudstack.api.response.SharedFSResponse;
import org.apache.cloudstack.api.response.ZoneResponse;
import org.apache.cloudstack.context.CallContext;
import org.apache.cloudstack.storage.sharedfs.SharedFS;
import org.apache.cloudstack.storage.sharedfs.SharedFSProvider;
import org.apache.cloudstack.storage.sharedfs.SharedFSService;

import com.cloud.event.EventTypes;
import com.cloud.exception.ConcurrentOperationException;
import com.cloud.exception.InsufficientCapacityException;
import com.cloud.exception.OperationTimedoutException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.utils.exception.CloudRuntimeException;

@APICommand(name = "createSharedFileSystem",
responseObject= SharedFSResponse.class,
description = "Create a new Shared File System of specified size and disk offering, attached to the given network",
Expand Down Expand Up @@ -289,8 +288,7 @@ public void execute() {

if (sharedFS != null) {
ResponseObject.ResponseView respView = getResponseView();
Account caller = CallContext.current().getCallingAccount();
if (_accountService.isRootAdmin(caller.getId())) {
if (CallContext.current().isCallingAccountRootAdmin()) {
respView = ResponseObject.ResponseView.Full;
}
SharedFSResponse response = _responseGenerator.createSharedFSResponse(respView, sharedFS);
Expand Down
4 changes: 1 addition & 3 deletions ...main/java/org/apache/cloudstack/api/command/user/storage/sharedfs/RestartSharedFSCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import com.cloud.exception.OperationTimedoutException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.user.Account;
import com.cloud.utils.exception.CloudRuntimeException;

@APICommand(name = "restartSharedFileSystem",
Expand Down Expand Up @@ -130,8 +129,7 @@ public void execute() {

if (sharedFS != null) {
ResponseObject.ResponseView respView = getResponseView();
Account caller = CallContext.current().getCallingAccount();
if (_accountService.isRootAdmin(caller.getId())) {
if (CallContext.current().isCallingAccountRootAdmin()) {
respView = ResponseObject.ResponseView.Full;
}
SharedFSResponse response = _responseGenerator.createSharedFSResponse(respView, sharedFS);
Expand Down
4 changes: 1 addition & 3 deletions ...c/main/java/org/apache/cloudstack/api/command/user/storage/sharedfs/StartSharedFSCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import com.cloud.exception.OperationTimedoutException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.user.Account;
import com.cloud.utils.exception.CloudRuntimeException;

@APICommand(name = "startSharedFileSystem",
Expand Down Expand Up @@ -120,8 +119,7 @@ public void execute() {

if (sharedFS != null) {
ResponseObject.ResponseView respView = getResponseView();
Account caller = CallContext.current().getCallingAccount();
if (_accountService.isRootAdmin(caller.getId())) {
if (CallContext.current().isCallingAccountRootAdmin()) {
respView = ResponseObject.ResponseView.Full;
}
SharedFSResponse response = _responseGenerator.createSharedFSResponse(respView, sharedFS);
Expand Down
4 changes: 1 addition & 3 deletions ...rc/main/java/org/apache/cloudstack/api/command/user/storage/sharedfs/StopSharedFSCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import org.apache.cloudstack.storage.sharedfs.SharedFSService;

import com.cloud.event.EventTypes;
import com.cloud.user.Account;

@APICommand(name = "stopSharedFileSystem",
responseObject= SharedFSResponse.class,
Expand Down Expand Up @@ -100,8 +99,7 @@ public void execute() {
SharedFS sharedFS = sharedFSService.stopSharedFS(this.getId(), this.isForced());
if (sharedFS != null) {
ResponseObject.ResponseView respView = getResponseView();
Account caller = CallContext.current().getCallingAccount();
if (_accountService.isRootAdmin(caller.getId())) {
if (CallContext.current().isCallingAccountRootAdmin()) {
respView = ResponseObject.ResponseView.Full;
}
SharedFSResponse response = _responseGenerator.createSharedFSResponse(respView, sharedFS);
Expand Down
9 changes: 3 additions & 6 deletions .../main/java/org/apache/cloudstack/api/command/user/storage/sharedfs/UpdateSharedFSCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
// under the License.
package org.apache.cloudstack.api.command.user.storage.sharedfs;

import javax.inject.Inject;

import org.apache.cloudstack.acl.RoleType;
import org.apache.cloudstack.api.APICommand;
import org.apache.cloudstack.api.ApiConstants;
Expand All @@ -30,10 +32,6 @@
import org.apache.cloudstack.storage.sharedfs.SharedFS;
import org.apache.cloudstack.storage.sharedfs.SharedFSService;

import javax.inject.Inject;

import com.cloud.user.Account;

@APICommand(name = "updateSharedFileSystem",
responseObject= SharedFSResponse.class,
description = "Update a Shared FileSystem",
Expand Down Expand Up @@ -98,8 +96,7 @@ public void execute() {
SharedFS sharedFS = sharedFSService.updateSharedFS(this);
if (sharedFS != null) {
ResponseObject.ResponseView respView = getResponseView();
Account caller = CallContext.current().getCallingAccount();
if (_accountService.isRootAdmin(caller.getId())) {
if (CallContext.current().isCallingAccountRootAdmin()) {
respView = ResponseObject.ResponseView.Full;
}
SharedFSResponse response = _responseGenerator.createSharedFSResponse(respView, sharedFS);
Expand Down
23 changes: 21 additions & 2 deletions api/src/main/java/org/apache/cloudstack/context/CallContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,20 @@

import org.apache.cloudstack.api.ApiCommandResourceType;
import org.apache.cloudstack.managed.threadlocal.ManagedThreadLocal;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.ThreadContext;
import org.springframework.beans.factory.NoSuchBeanDefinitionException;

import com.cloud.exception.CloudAuthenticationException;
import com.cloud.projects.Project;
import com.cloud.user.Account;
import com.cloud.user.AccountService;
import com.cloud.user.User;
import com.cloud.utils.UuidUtils;
import com.cloud.utils.component.ComponentContext;
import com.cloud.utils.db.EntityManager;
import com.cloud.utils.exception.CloudRuntimeException;
import org.apache.logging.log4j.ThreadContext;

/**
* CallContext records information about the environment the call is made. This
Expand All @@ -53,6 +56,7 @@ protected Stack<CallContext> initialValue() {
private String contextId;
private Account account;
private long accountId;
private Boolean isAccountRootAdmin = null;
private long startEventId = 0;
private String eventDescription;
private String eventDetails;
Expand Down Expand Up @@ -134,6 +138,21 @@ public Account getCallingAccount() {
return account;
}

public boolean isCallingAccountRootAdmin() {
if (isAccountRootAdmin == null) {
AccountService accountService;
try {
accountService = ComponentContext.getDelegateComponentOfType(AccountService.class);
} catch (NoSuchBeanDefinitionException e) {
LOGGER.warn("Falling back to account type check for isRootAdmin for account ID: {} as no AccountService bean found: {}", accountId, e.getMessage());
Account caller = getCallingAccount();
return caller != null && caller.getType() == Account.Type.ADMIN;
Comment on lines +147 to +149
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The fallback check caller.getType() == Account.Type.ADMIN is inconsistent with the actual root admin check performed by AccountService, which uses security checkers. This could lead to different behavior when AccountService is unavailable. Consider removing the fallback entirely or throwing an exception to fail fast when AccountService is not available.

Suggested change
LOGGER.warn("Falling back to account type check for isRootAdmin for account ID: {} as no AccountService bean found: {}", accountId, e.getMessage());
Account caller = getCallingAccount();
return caller != null && caller.getType() == Account.Type.ADMIN;
throw new CloudRuntimeException("AccountService bean not found, cannot determine if calling account is root admin for account ID: " + accountId, e);

Copilot uses AI. Check for mistakes.
}
isAccountRootAdmin = accountService.isRootAdmin(getCallingAccount());
}
return Boolean.TRUE.equals(isAccountRootAdmin);
}
Comment on lines +141 to +154
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The fallback logic at line 149 returns the result directly without caching it in isAccountRootAdmin. This means if AccountService is unavailable, subsequent calls will repeat the check instead of using the cached value. Either cache the result before returning, or document this as intentional behavior.

Copilot uses AI. Check for mistakes.

public static CallContext current() {
CallContext context = s_currentContext.get();

Expand Down
55 changes: 54 additions & 1 deletion api/src/test/java/org/apache/cloudstack/context/CallContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,30 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
import org.mockito.junit.MockitoJUnitRunner;

import com.cloud.user.Account;
import com.cloud.user.AccountService;
import com.cloud.user.User;
import com.cloud.utils.component.ComponentContext;
import com.cloud.utils.db.EntityManager;

@RunWith(MockitoJUnitRunner.class)
public class CallContextTest {

@Mock
EntityManager entityMgr;
@Mock
User user;
@Mock
Account account;

@Before
public void setUp() {
CallContext.init(entityMgr);
CallContext.register(Mockito.mock(User.class), Mockito.mock(Account.class));
CallContext.register(user, account);
}

@After
Expand Down Expand Up @@ -80,4 +87,50 @@ public void testGetContextParameter() {
Assert.assertEquals("current context map should have exactly three entries", 3, currentContext.getContextParameters().size());
}


@Test
public void isCallingAccountRootAdminReturnsTrueWhenAccountIsRootAdminAccountServiceNotAvailable() {
Mockito.when(account.getType()).thenReturn(Account.Type.ADMIN);

CallContext context = CallContext.current();
Assert.assertTrue(context.isCallingAccountRootAdmin());
}

@Test
public void isCallingAccountRootAdminReturnsFalseWhenAccountIsNotRootAdminAccountServiceNotAvailable() {
Mockito.when(account.getType()).thenReturn(Account.Type.NORMAL);

CallContext context = CallContext.current();
Assert.assertFalse(context.isCallingAccountRootAdmin());
Assert.assertFalse(context.isCallingAccountRootAdmin());
}

@Test
public void isCallingAccountRootAdminTrueWhenAccountServiceAvailable() {
try (MockedStatic<ComponentContext> componentContextMockedStatic = Mockito.mockStatic(ComponentContext.class)) {
AccountService accountService = Mockito.mock(AccountService.class);
Mockito.when(accountService.isRootAdmin(account)).thenReturn(true);
componentContextMockedStatic.when(() -> ComponentContext.getDelegateComponentOfType(AccountService.class)).thenReturn(accountService);
CallContext context = CallContext.current();
Assert.assertTrue(context.isCallingAccountRootAdmin());
// Verify isRootAdmin was called only once
Assert.assertTrue(context.isCallingAccountRootAdmin());
componentContextMockedStatic.verify(() -> ComponentContext.getDelegateComponentOfType(AccountService.class));
}
}

@Test
public void isCallingAccountRootAdminFalseWhenAccountServiceAvailable() {
try (MockedStatic<ComponentContext> componentContextMockedStatic = Mockito.mockStatic(ComponentContext.class)) {
AccountService accountService = Mockito.mock(AccountService.class);
Mockito.when(accountService.isRootAdmin(account)).thenReturn(false);
componentContextMockedStatic.when(() -> ComponentContext.getDelegateComponentOfType(AccountService.class)).thenReturn(accountService);
CallContext context = CallContext.current();
Assert.assertFalse(context.isCallingAccountRootAdmin());
// Verify isRootAdmin was called only once
Assert.assertFalse(context.isCallingAccountRootAdmin());
componentContextMockedStatic.verify(() -> ComponentContext.getDelegateComponentOfType(AccountService.class));
}
}

}
2 changes: 1 addition & 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 @@ -120,7 +120,7 @@ public boolean checkAccess(User user, String apiCommandName) throws PermissionDe
}

Account userAccount = accountService.getAccount(user.getAccountId());
if (accountService.isRootAdmin(userAccount.getId()) || accountService.isDomainAdmin(userAccount.getAccountId())) {
if (accountService.isRootAdmin(userAccount) || accountService.isDomainAdmin(userAccount.getAccountId())) {
logger.info(String.format("Account [%s] is Root Admin or Domain Admin, all APIs are allowed.", userAccount.getAccountName()));
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion ...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 @@ -173,7 +173,7 @@ public boolean checkAccess(User user, String apiCommandName) throws PermissionDe
@Override
public boolean checkAccess(Account account, String commandName) {
Long accountId = account.getAccountId();
if (_accountService.isRootAdmin(accountId)) {
if (_accountService.isRootAdmin(account)) {
logger.info(String.format("Account [%s] is Root Admin, in this case, API limit does not apply.",
ReflectionToStringBuilderUtils.reflectOnlySelectedFields(account, "accountName", "uuid")));
return true;
Expand Down
4 changes: 2 additions & 2 deletions ...etes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -856,7 +856,7 @@ public KubernetesClusterResponse createKubernetesClusterResponse(long kubernetes
List<KubernetesClusterVmMapVO> vmList = kubernetesClusterVmMapDao.listByClusterId(kubernetesCluster.getId());
ResponseView respView = ResponseView.Restricted;
Account caller = CallContext.current().getCallingAccount();
if (accountService.isRootAdmin(caller.getId())) {
if (CallContext.current().isCallingAccountRootAdmin()) {
respView = ResponseView.Full;
}
final String responseName = "virtualmachine";
Expand Down Expand Up @@ -893,7 +893,7 @@ public KubernetesClusterResponse createKubernetesClusterResponse(long kubernetes
response.setEtcdIps(etcdIps);
}
response.setHasAnnotation(annotationDao.hasAnnotations(kubernetesCluster.getUuid(),
AnnotationService.EntityType.KUBERNETES_CLUSTER.name(), accountService.isRootAdmin(caller.getId())));
AnnotationService.EntityType.KUBERNETES_CLUSTER.name(), CallContext.current().isCallingAccountRootAdmin()));
response.setVirtualMachines(vmResponses);
response.setAutoscalingEnabled(kubernetesCluster.getAutoscalingEnabled());
response.setMinSize(kubernetesCluster.getMinSize());
Expand Down
Loading
Loading