This is an automated email from the ASF dual-hosted git repository.
RosiKyu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/main by this push:
new 70860301eb1 small refactors on account manager (#11784)
70860301eb1 is described below
commit 70860301eb14cb0dc6af1c8546de9997e515d166
Author: dahn <[email protected]>
AuthorDate: Wed Nov 11 19:15:38 2026 +0100
small refactors on account manager (#11784)
* small refactors on account manager
* fix duplicate import + tests
* account manager: group entities by resolved domain id in checkAccess
---
.../java/com/cloud/user/AccountManagerImpl.java | 34 +++++-----
.../com/cloud/user/AccountManagerImplTest.java | 75 ++++++++++++++++++++++
2 files changed, 92 insertions(+), 17 deletions(-)
diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java
b/server/src/main/java/com/cloud/user/AccountManagerImpl.java
index 0fd46bc8440..e72700b8f82 100644
--- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java
+++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java
@@ -125,7 +125,6 @@ import org.apache.commons.codec.binary.Base64;
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;
@@ -212,6 +211,7 @@ import com.cloud.user.dao.UserDataDao;
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;
@@ -647,7 +647,6 @@ public class AccountManagerImpl extends ManagerBase
implements AccountManager, M
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;
}
@@ -782,23 +781,13 @@ public class AccountManagerImpl extends ManagerBase
implements AccountManager, M
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)
&& !(entity instanceof DnsServer) && !(entity instanceof
DnsZone)) {
- List<ControlledEntity> toBeChecked =
domains.get(entity.getDomainId());
// for templates, we don't have to do cross domains check
- if (toBeChecked == null) {
- toBeChecked = new ArrayList<>();
- domains.put(domainId, toBeChecked);
- }
- toBeChecked.add(entity);
+ domains.computeIfAbsent(domainId, k -> new
ArrayList<>()).add(entity);
}
boolean granted = false;
for (SecurityChecker checker : _securityCheckers) {
@@ -840,6 +829,17 @@ public class AccountManagerImpl extends ManagerBase
implements AccountManager, M
}
+ private long getDomainIdFor(ControlledEntity entity) {
+ 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();
@@ -3067,11 +3067,11 @@ public class AccountManagerImpl extends ManagerBase
implements AccountManager, M
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);
throw new CloudAuthenticationException("Failed to
authenticate user '" + username + "' in domain '" + domain.getPath() + "' from
ip "
+ loginIpAddress.toString().replace("/", "") + ";
please provide valid credentials");
}
@@ -3241,7 +3241,7 @@ public class AccountManagerImpl extends ManagerBase
implements AccountManager, M
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()));
}
}
diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java
b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java
index 955c939f4fa..a2cc9495405 100644
--- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java
+++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java
@@ -75,6 +75,7 @@ import org.mockito.Mockito;
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;
@@ -451,6 +452,80 @@ public class AccountManagerImplTest extends
AccountManagentImplTestBase {
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);
