Skip to content

Commit 7086030

Browse files
DaanHooglandDaanHoogland
authored
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
1 parent 1881dc6 commit 7086030

2 files changed

Lines changed: 92 additions & 17 deletions

File tree

‎server/src/main/java/com/cloud/user/AccountManagerImpl.java‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,6 @@
125125
import org.apache.commons.collections.CollectionUtils;
126126
import org.apache.commons.lang3.BooleanUtils;
127127
import org.apache.commons.lang3.ObjectUtils;
128-
import org.apache.commons.lang3.StringUtils;
129128
import org.jetbrains.annotations.NotNull;
130129
import org.springframework.beans.factory.NoSuchBeanDefinitionException;
131130

@@ -212,6 +211,7 @@
212211
import com.cloud.utils.ConstantTimeComparator;
213212
import com.cloud.utils.NumbersUtil;
214213
import com.cloud.utils.Pair;
214+
import com.cloud.utils.StringUtils;
215215
import com.cloud.utils.Ternary;
216216
import com.cloud.utils.UuidUtils;
217217
import com.cloud.utils.component.ComponentContext;
@@ -647,7 +647,6 @@ public boolean isAdmin(Long accountId) {
647647
return false; //account is deleted or does not exist
648648
}
649649
return (isRootAdmin(accountId)) || (isDomainAdmin(accountId)) || (isResourceDomainAdmin(accountId)) || (acct.getType() == Account.Type.READ_ONLY_ADMIN);
650-
651650
}
652651
return false;
653652
}
@@ -782,23 +781,13 @@ public void checkAccess(Account caller, AccessType accessType, boolean sameOwner
782781
HashMap<Long, List<ControlledEntity>> domains = new HashMap<>();
783782

784783
for (ControlledEntity entity : entities) {
785-
long domainId = entity.getDomainId();
786-
if (entity.getAccountId() != -1 && domainId == -1) { // If account exists domainId should too so calculate
787-
// it. This condition might be hit for templates or entities which miss domainId in their tables
788-
Account account = ApiDBUtils.findAccountById(entity.getAccountId());
789-
domainId = account != null ? account.getDomainId() : -1;
790-
}
784+
long domainId = getDomainIdFor(entity);
791785
if (entity.getAccountId() != -1 && domainId != -1 && !(entity instanceof VirtualMachineTemplate)
792786
&& !(entity instanceof Network && (accessType == AccessType.UseEntry || accessType == AccessType.OperateEntry))
793787
&& !(entity instanceof AffinityGroup) && !(entity instanceof VirtualRouter)
794788
&& !(entity instanceof DnsServer) && !(entity instanceof DnsZone)) {
795-
List<ControlledEntity> toBeChecked = domains.get(entity.getDomainId());
796789
// for templates, we don't have to do cross domains check
797-
if (toBeChecked == null) {
798-
toBeChecked = new ArrayList<>();
799-
domains.put(domainId, toBeChecked);
800-
}
801-
toBeChecked.add(entity);
790+
domains.computeIfAbsent(domainId, k -> new ArrayList<>()).add(entity);
802791
}
803792
boolean granted = false;
804793
for (SecurityChecker checker : _securityCheckers) {
@@ -840,6 +829,17 @@ public void checkAccess(Account caller, AccessType accessType, boolean sameOwner
840829

841830
}
842831

832+
private long getDomainIdFor(ControlledEntity entity) {
833+
long domainId = entity.getDomainId();
834+
if (entity.getAccountId() != -1 && domainId == -1) {
835+
// If account exists domainId should too so calculate it.
836+
// This condition might be hit for templates or entities which miss domainId in their tables
837+
Account account = ApiDBUtils.findAccountById(entity.getAccountId());
838+
domainId = account != null ? account.getDomainId() : -1;
839+
}
840+
return domainId;
841+
}
842+
843843
@Override
844844
public void validateAccountHasAccessToResource(Account account, AccessType accessType, Object resource) {
845845
Class<?> resourceClass = resource.getClass();
@@ -3067,11 +3067,11 @@ public UserAccount authenticateUser(final String username, final String password
30673067
final Boolean ApiSourceCidrChecksEnabled = ApiServiceConfiguration.ApiSourceCidrChecksEnabled.value();
30683068

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

30723072
// Block when is not in the list of allowed IPs
30733073
if (!NetUtils.isIpInCidrList(loginIpAddress, accessAllowedCidrs.split(","))) {
3074-
logger.warn("Request by account '{}' was denied since {} does not match {}", account.toString(), loginIpAddress.toString().replace("/", ""), accessAllowedCidrs);
3074+
logger.warn("Request by account '{}' was denied since {} does not match {}", account, loginIpAddress.toString().replace("/", ""), accessAllowedCidrs);
30753075
throw new CloudAuthenticationException("Failed to authenticate user '" + username + "' in domain '" + domain.getPath() + "' from ip "
30763076
+ loginIpAddress.toString().replace("/", "") + "; please provide valid credentials");
30773077
}
@@ -3241,7 +3241,7 @@ private UserAccount getUserAccountForSSO(String username, Long domainId, Map<Str
32413241
if (unsignedRequestBuffer.length() != 0) {
32423242
unsignedRequestBuffer.append("&");
32433243
}
3244-
unsignedRequestBuffer.append(paramName).append("=").append(URLEncoder.encode(paramValue, com.cloud.utils.StringUtils.getPreferredCharset()));
3244+
unsignedRequestBuffer.append(paramName).append("=").append(URLEncoder.encode(paramValue, StringUtils.getPreferredCharset()));
32453245
}
32463246
}
32473247

‎server/src/test/java/com/cloud/user/AccountManagerImplTest.java‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@
7575
import org.springframework.beans.factory.NoSuchBeanDefinitionException;
7676

7777
import com.cloud.acl.DomainChecker;
78+
import com.cloud.api.ApiDBUtils;
7879
import com.cloud.api.auth.SetupUserTwoFactorAuthenticationCmd;
7980
import com.cloud.domain.Domain;
8081
import com.cloud.domain.DomainVO;
@@ -451,6 +452,80 @@ public void testPreventRootDomainAdminAccessToRootAdminKeysRootDomainAdminUser()
451452
accountManagerImpl.preventRootDomainAdminAccessToRootAdminKeys(user, entity);
452453
}
453454

455+
@Test
456+
public void checkAccessResolvesDomainIdFromAccountWhenEntityDomainIdMissing() {
457+
Account caller = Mockito.mock(Account.class);
458+
Mockito.when(caller.getId()).thenReturn(999L);
459+
Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(Mockito.anyLong());
460+
461+
ControlledEntity entity = Mockito.mock(ControlledEntity.class);
462+
Mockito.when(entity.getDomainId()).thenReturn(-1L);
463+
Mockito.when(entity.getAccountId()).thenReturn(10L);
464+
465+
Account resolvedAccount = Mockito.mock(Account.class);
466+
Mockito.when(resolvedAccount.getDomainId()).thenReturn(7L);
467+
468+
Domain domain = Mockito.mock(Domain.class);
469+
Mockito.when(_domainMgr.getDomain(7L)).thenReturn(domain);
470+
471+
Mockito.when(securityChecker.checkAccess(caller, entity, AccessType.ListEntry, "someApi")).thenReturn(true);
472+
Mockito.when(securityChecker.checkAccess(caller, domain)).thenReturn(true);
473+
474+
try (MockedStatic<ApiDBUtils> apiDBUtilsMocked = Mockito.mockStatic(ApiDBUtils.class)) {
475+
apiDBUtilsMocked.when(() -> ApiDBUtils.findAccountById(10L)).thenReturn(resolvedAccount);
476+
477+
accountManagerImpl.checkAccess(caller, AccessType.ListEntry, false, "someApi", entity);
478+
}
479+
480+
// domainId for the entity had to be resolved via its account (entity.getDomainId() == -1),
481+
// so the domain-level check must have run against the account's domain, not against -1.
482+
Mockito.verify(_domainMgr).getDomain(7L);
483+
Mockito.verify(_domainMgr, Mockito.never()).getDomain(-1L);
484+
}
485+
486+
@Test
487+
public void checkAccessKeepsAllEntitiesGroupedUnderResolvedDomainId() {
488+
Account caller = Mockito.mock(Account.class);
489+
Mockito.when(caller.getId()).thenReturn(999L);
490+
Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(Mockito.anyLong());
491+
492+
// Both entities are missing their own domainId and resolve, via different accounts, to the same domain.
493+
ControlledEntity entity1 = Mockito.mock(ControlledEntity.class);
494+
Mockito.when(entity1.getDomainId()).thenReturn(-1L);
495+
Mockito.when(entity1.getAccountId()).thenReturn(10L);
496+
497+
ControlledEntity entity2 = Mockito.mock(ControlledEntity.class);
498+
Mockito.when(entity2.getDomainId()).thenReturn(-1L);
499+
Mockito.when(entity2.getAccountId()).thenReturn(20L);
500+
501+
Account resolvedAccount1 = Mockito.mock(Account.class);
502+
Mockito.when(resolvedAccount1.getDomainId()).thenReturn(7L);
503+
Account resolvedAccount2 = Mockito.mock(Account.class);
504+
Mockito.when(resolvedAccount2.getDomainId()).thenReturn(7L);
505+
506+
Domain domain = Mockito.mock(Domain.class);
507+
Mockito.when(_domainMgr.getDomain(7L)).thenReturn(domain);
508+
509+
Mockito.when(securityChecker.checkAccess(caller, entity1, AccessType.ListEntry, "someApi")).thenReturn(true);
510+
Mockito.when(securityChecker.checkAccess(caller, entity2, AccessType.ListEntry, "someApi")).thenReturn(true);
511+
Mockito.when(securityChecker.checkAccess(caller, domain))
512+
.thenThrow(new PermissionDeniedException("denied", caller, Collections.emptyList()));
513+
514+
PermissionDeniedException thrown;
515+
try (MockedStatic<ApiDBUtils> apiDBUtilsMocked = Mockito.mockStatic(ApiDBUtils.class)) {
516+
apiDBUtilsMocked.when(() -> ApiDBUtils.findAccountById(10L)).thenReturn(resolvedAccount1);
517+
apiDBUtilsMocked.when(() -> ApiDBUtils.findAccountById(20L)).thenReturn(resolvedAccount2);
518+
519+
thrown = Assert.assertThrows(PermissionDeniedException.class,
520+
() -> accountManagerImpl.checkAccess(caller, AccessType.ListEntry, false, "someApi", entity1, entity2));
521+
}
522+
523+
// Both entities resolve to the same domain, so they must both be grouped under that single domain
524+
// key and both show up as violations, instead of the second entity silently displacing the first.
525+
Assert.assertEquals(2, thrown.getEntitiesInViolation().size());
526+
Assert.assertTrue(thrown.getEntitiesInViolation().containsAll(Arrays.asList(entity1, entity2)));
527+
}
528+
454529
@Test
455530
public void updateUserTestTimeZoneAndEmailNull() {
456531
Mockito.when(userVoMock.getAccountId()).thenReturn(10L);

0 commit comments

Comments
 (0)