diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index db9c1d1dafde..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; @@ -204,6 +203,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; @@ -772,17 +772,12 @@ 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 == AccessType.UseEntry || accessType == AccessType.OperateEntry)) && !(entity instanceof AffinityGroup) && !(entity instanceof VirtualRouter) && !(entity instanceof DnsServer) && !(entity instanceof DnsZone)) { - List toBeChecked = domains.get(entity.getDomainId()); + List toBeChecked = domains.get(domainId); // for templates, we don't have to do cross domains check if (toBeChecked == null) { toBeChecked = new ArrayList<>(); @@ -830,6 +825,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(); @@ -2992,11 +2998,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); throw new CloudAuthenticationException("Failed to authenticate user '" + username + "' in domain '" + domain.getPath() + "' from ip " + loginIpAddress.toString().replace("/", "") + "; please provide valid credentials"); } @@ -3166,7 +3172,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);