Skip to content

small refactors on account manager #11784

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
DaanHoogland wants to merge 2 commits into
base: main
Choose a base branch
from
+91 −11
Open

small refactors on account manager #11784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd1d969
small refactors on account manager
Oct 3, 2025
55eae45
fix duplicate import + tests
Aug 16, 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
27 changes: 16 additions & 11 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 @@ -125,7 +125,6 @@
import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang3.BooleanUtils;
import org.apache.commons.lang3.ObjectUtils;
import org.apache.commons.lang3.StringUtils;
import org.jetbrains.annotations.NotNull;
import org.springframework.beans.factory.NoSuchBeanDefinitionException;

Expand Down Expand Up @@ -212,6 +211,7 @@
import com.cloud.utils.ConstantTimeComparator;
import com.cloud.utils.NumbersUtil;
import com.cloud.utils.Pair;
import com.cloud.utils.StringUtils;
import com.cloud.utils.Ternary;
import com.cloud.utils.UuidUtils;
import com.cloud.utils.component.ComponentContext;
Expand Down Expand Up @@ -647,7 +647,6 @@ public boolean isAdmin(Long accountId) {
return false; //account is deleted or does not exist
}
return (isRootAdmin(accountId)) || (isDomainAdmin(accountId)) || (isResourceDomainAdmin(accountId)) || (acct.getType() == Account.Type.READ_ONLY_ADMIN);

}
return false;
}
Expand Down Expand Up @@ -782,12 +781,7 @@ public void checkAccess(Account caller, AccessType accessType, boolean sameOwner
HashMap<Long, List<ControlledEntity>> domains = new HashMap<>();

for (ControlledEntity entity : entities) {
long domainId = entity.getDomainId();
if (entity.getAccountId() != -1 && domainId == -1) { // If account exists domainId should too so calculate
// it. This condition might be hit for templates or entities which miss domainId in their tables
Account account = ApiDBUtils.findAccountById(entity.getAccountId());
domainId = account != null ? account.getDomainId() : -1;
}
long domainId = getDomainIdFor(entity);
if (entity.getAccountId() != -1 && domainId != -1 && !(entity instanceof VirtualMachineTemplate)
&& !(entity instanceof Network && (accessType == AccessType.UseEntry || accessType == AccessType.OperateEntry))
&& !(entity instanceof AffinityGroup) && !(entity instanceof VirtualRouter)
Expand Down Expand Up @@ -840,6 +834,17 @@ public void checkAccess(Account caller, AccessType accessType, boolean sameOwner

}

private static long getDomainIdFor(ControlledEntity entity) {
Comment thread
DaanHoogland marked this conversation as resolved.
long domainId = entity.getDomainId();
if (entity.getAccountId() != -1 && domainId == -1) {
// If account exists domainId should too so calculate it.
// This condition might be hit for templates or entities which miss domainId in their tables
Account account = ApiDBUtils.findAccountById(entity.getAccountId());
domainId = account != null ? account.getDomainId() : -1;
}
return domainId;
}

@Override
public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) {
Class<?> resourceClass = resource.getClass();
Expand Down Expand Up @@ -3067,11 +3072,11 @@ public UserAccount authenticateUser(final String username, final String password
final Boolean ApiSourceCidrChecksEnabled = ApiServiceConfiguration.ApiSourceCidrChecksEnabled.value();

if (ApiSourceCidrChecksEnabled) {
logger.debug("CIDRs from which account '{}' is allowed to perform API calls: {}", account.toString(), accessAllowedCidrs);
logger.debug("CIDRs from which account '{}' is allowed to perform API calls: {}", account, accessAllowedCidrs);

// Block when is not in the list of allowed IPs
if (!NetUtils.isIpInCidrList(loginIpAddress, accessAllowedCidrs.split(","))) {
logger.warn("Request by account '{}' was denied since {} does not match {}", account.toString(), loginIpAddress.toString().replace("/", ""), accessAllowedCidrs);
logger.warn("Request by account '{}' was denied since {} does not match {}", account , loginIpAddress.toString().replace("/", ""), accessAllowedCidrs);

Copilot AI Dec 12, 2025

Copy link

Choose a reason for hiding this comment

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

There's an extra space before the comma after the first placeholder. The code has "account ," but it should be "account," for consistent formatting.

Suggested change
logger.warn("Request by account '{}' was denied since {} does not match {}", account , loginIpAddress.toString().replace("/", ""), accessAllowedCidrs);
logger.warn("Request by account '{}' was denied since {} does not match {}", account, loginIpAddress.toString().replace("/", ""), accessAllowedCidrs);

Copilot uses AI. Check for mistakes.
throw new CloudAuthenticationException("Failed to authenticate user '" + username + "' in domain '" + domain.getPath() + "' from ip "
+ loginIpAddress.toString().replace("/", "") + "; please provide valid credentials");
}
Expand Down Expand Up @@ -3241,7 +3246,7 @@ private UserAccount getUserAccountForSSO(String username, Long domainId, Map<Str
if (unsignedRequestBuffer.length() != 0) {
unsignedRequestBuffer.append("&");
}
unsignedRequestBuffer.append(paramName).append("=").append(URLEncoder.encode(paramValue, com.cloud.utils.StringUtils.getPreferredCharset()));
unsignedRequestBuffer.append(paramName).append("=").append(URLEncoder.encode(paramValue, StringUtils.getPreferredCharset()));
Comment thread
DaanHoogland marked this conversation as resolved.
}
}

Expand Down
75 changes: 75 additions & 0 deletions server/src/test/java/com/cloud/user/AccountManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
import org.springframework.beans.factory.NoSuchBeanDefinitionException;

import com.cloud.acl.DomainChecker;
import com.cloud.api.ApiDBUtils;
import com.cloud.api.auth.SetupUserTwoFactorAuthenticationCmd;
import com.cloud.domain.Domain;
import com.cloud.domain.DomainVO;
Expand Down Expand Up @@ -451,6 +452,80 @@ public void testPreventRootDomainAdminAccessToRootAdminKeysRootDomainAdminUser()
accountManagerImpl.preventRootDomainAdminAccessToRootAdminKeys(user, entity);
}

@Test
public void checkAccessResolvesDomainIdFromAccountWhenEntityDomainIdMissing() {
Account caller = Mockito.mock(Account.class);
Mockito.when(caller.getId()).thenReturn(999L);
Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(Mockito.anyLong());

ControlledEntity entity = Mockito.mock(ControlledEntity.class);
Mockito.when(entity.getDomainId()).thenReturn(-1L);
Mockito.when(entity.getAccountId()).thenReturn(10L);

Account resolvedAccount = Mockito.mock(Account.class);
Mockito.when(resolvedAccount.getDomainId()).thenReturn(7L);

Domain domain = Mockito.mock(Domain.class);
Mockito.when(_domainMgr.getDomain(7L)).thenReturn(domain);

Mockito.when(securityChecker.checkAccess(caller, entity, AccessType.ListEntry, "someApi")).thenReturn(true);
Mockito.when(securityChecker.checkAccess(caller, domain)).thenReturn(true);

try (MockedStatic<ApiDBUtils> apiDBUtilsMocked = Mockito.mockStatic(ApiDBUtils.class)) {
apiDBUtilsMocked.when(() -> ApiDBUtils.findAccountById(10L)).thenReturn(resolvedAccount);

accountManagerImpl.checkAccess(caller, AccessType.ListEntry, false, "someApi", entity);
}

// domainId for the entity had to be resolved via its account (entity.getDomainId() == -1),
// so the domain-level check must have run against the account's domain, not against -1.
Mockito.verify(_domainMgr).getDomain(7L);
Mockito.verify(_domainMgr, Mockito.never()).getDomain(-1L);
}

@Test
public void checkAccessKeepsAllEntitiesGroupedUnderResolvedDomainId() {
Account caller = Mockito.mock(Account.class);
Mockito.when(caller.getId()).thenReturn(999L);
Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(Mockito.anyLong());

// Both entities are missing their own domainId and resolve, via different accounts, to the same domain.
ControlledEntity entity1 = Mockito.mock(ControlledEntity.class);
Mockito.when(entity1.getDomainId()).thenReturn(-1L);
Mockito.when(entity1.getAccountId()).thenReturn(10L);

ControlledEntity entity2 = Mockito.mock(ControlledEntity.class);
Mockito.when(entity2.getDomainId()).thenReturn(-1L);
Mockito.when(entity2.getAccountId()).thenReturn(20L);

Account resolvedAccount1 = Mockito.mock(Account.class);
Mockito.when(resolvedAccount1.getDomainId()).thenReturn(7L);
Account resolvedAccount2 = Mockito.mock(Account.class);
Mockito.when(resolvedAccount2.getDomainId()).thenReturn(7L);

Domain domain = Mockito.mock(Domain.class);
Mockito.when(_domainMgr.getDomain(7L)).thenReturn(domain);

Mockito.when(securityChecker.checkAccess(caller, entity1, AccessType.ListEntry, "someApi")).thenReturn(true);
Mockito.when(securityChecker.checkAccess(caller, entity2, AccessType.ListEntry, "someApi")).thenReturn(true);
Mockito.when(securityChecker.checkAccess(caller, domain))
.thenThrow(new PermissionDeniedException("denied", caller, Collections.emptyList()));

PermissionDeniedException thrown;
try (MockedStatic<ApiDBUtils> apiDBUtilsMocked = Mockito.mockStatic(ApiDBUtils.class)) {
apiDBUtilsMocked.when(() -> ApiDBUtils.findAccountById(10L)).thenReturn(resolvedAccount1);
apiDBUtilsMocked.when(() -> ApiDBUtils.findAccountById(20L)).thenReturn(resolvedAccount2);

thrown = Assert.assertThrows(PermissionDeniedException.class,
() -> accountManagerImpl.checkAccess(caller, AccessType.ListEntry, false, "someApi", entity1, entity2));
}

// Both entities resolve to the same domain, so they must both be grouped under that single domain
// key and both show up as violations, instead of the second entity silently displacing the first.
Assert.assertEquals(2, thrown.getEntitiesInViolation().size());
Assert.assertTrue(thrown.getEntitiesInViolation().containsAll(Arrays.asList(entity1, entity2)));
}

@Test
public void updateUserTestTimeZoneAndEmailNull() {
Mockito.when(userVoMock.getAccountId()).thenReturn(10L);
Expand Down
Loading