From 7718dd855d7f680117268985a13e74f64f6a63d5 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Fri, 3 Oct 2025 12:31:55 +0200 Subject: [PATCH 1/3] small refactors on account manager --- .../com/cloud/user/AccountManagerImpl.java | 41 +++++++++---------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 53b88690654e..52677e6c8d74 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -86,7 +86,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.StringUtils; import org.jetbrains.annotations.NotNull; import org.springframework.beans.factory.NoSuchBeanDefinitionException; @@ -177,6 +176,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; @@ -592,10 +592,9 @@ public boolean isAdmin(Long accountId) { } if ((isRootAdmin(accountId)) || (isDomainAdmin(accountId)) || (isResourceDomainAdmin(accountId))) { return true; - } else if (acct.getType() == Account.Type.READ_ONLY_ADMIN) { - return true; + } else { + return acct.getType() == Account.Type.READ_ONLY_ADMIN; } - } return false; } @@ -649,10 +648,7 @@ public boolean isDomainAdmin(Long accountId) { @Override public boolean isNormalUser(long accountId) { AccountVO acct = _accountDao.findById(accountId); - if (acct != null && acct.getType() == Account.Type.NORMAL) { - return true; - } - return false; + return acct != null && acct.getType() == Account.Type.NORMAL; } @Override @@ -683,10 +679,7 @@ public boolean isInternalAccount(long accountId) { if (account == null) { return false; //account is deleted or does not exist } - if (isRootAdmin(accountId) || (account.getType() == Account.Type.ADMIN)) { - return true; - } - return false; + return isRootAdmin(accountId) || (account.getType() == Account.Type.ADMIN); } @Override @@ -736,12 +729,7 @@ public void checkAccess(Account caller, AccessType accessType, boolean sameOwner HashMap> 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 != null && (accessType == AccessType.UseEntry || accessType == AccessType.OperateEntry)) && !(entity instanceof AffinityGroup) && !(entity instanceof VirtualRouter)) { @@ -793,6 +781,17 @@ public void checkAccess(Account caller, AccessType accessType, boolean sameOwner } + private static 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(); @@ -2830,11 +2829,11 @@ public UserAccount authenticateUser(final String username, final String password final Boolean ApiSourceCidrChecksEnabled = ApiServiceConfiguration.ApiSourceCidrChecksEnabled.value(); if (ApiSourceCidrChecksEnabled) { - logger.debug("CIDRs from which account '" + account.toString() + "' is allowed to perform API calls: " + 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 '" + account.toString() + "' was denied since " + loginIpAddress.toString().replace("/", "") + " does not match " + 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"); } @@ -3007,7 +3006,7 @@ private UserAccount getUserAccountForSSO(String username, Long domainId, Map 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 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); From ef1e1d83e2b8f704d24a234441ae7cf45f9c18f0 Mon Sep 17 00:00:00 2001 From: Daan Hoogland Date: Mon, 17 Aug 2026 08:46:01 +0200 Subject: [PATCH 3/3] use proxy class for local method(s) --- server/src/main/java/com/cloud/user/AccountManagerImpl.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index c1c591998c20..72f6dfa98de5 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -117,7 +117,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;