diff --git a/api/src/main/java/com/cloud/user/Account.java b/api/src/main/java/com/cloud/user/Account.java index 6be4d0a48f6..1cc5eae64a6 100644 --- a/api/src/main/java/com/cloud/user/Account.java +++ b/api/src/main/java/com/cloud/user/Account.java @@ -71,6 +71,7 @@ public interface Account extends ControlledEntity, InternalIdentity, Identity { } public static final long ACCOUNT_ID_SYSTEM = 1; + public static final long ACCOUNT_ID_ADMIN = 2; public String getAccountName(); diff --git a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java index 12741e0996a..d30d7b2f74d 100644 --- a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java +++ b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java @@ -533,4 +533,8 @@ public class MockAccountManager extends ManagerBase implements AccountManager { public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user) { return null; } + + @Override + public void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount) { + } } diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index 85225fca6d7..c8ab6b4b069 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -50,6 +50,8 @@ import java.util.stream.Collectors; import javax.inject.Inject; import javax.naming.ConfigurationException; +import com.cloud.user.AccountManagerImpl; +import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.acl.SecurityChecker; import org.apache.cloudstack.affinity.AffinityGroup; import org.apache.cloudstack.affinity.AffinityGroupService; @@ -481,6 +483,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati private long _defaultPageSize = Long.parseLong(Config.DefaultPageSize.getDefaultValue()); private static final String DOMAIN_NAME_PATTERN = "^((?!-)[A-Za-z0-9-]{1,63}(? configValuesForValidation = new HashSet<>(); + private Set configKeysAllowedOnlyForDefaultAdmin = new HashSet<>(); private Set weightBasedParametersForValidation = new HashSet<>(); private Set overprovisioningFactorsForValidation = new HashSet<>(); @@ -545,6 +548,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati populateConfigValuesForValidationSet(); weightBasedParametersForValidation(); overProvisioningFactorsForValidation(); + populateConfigKeysAllowedOnlyForDefaultAdmin(); initMessageBusListener(); return true; } @@ -609,6 +613,11 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati overprovisioningFactorsForValidation.add(CapacityManager.StorageOverprovisioningFactor.key()); } + protected void populateConfigKeysAllowedOnlyForDefaultAdmin() { + configKeysAllowedOnlyForDefaultAdmin.add(AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key()); + configKeysAllowedOnlyForDefaultAdmin.add(AccountManagerImpl.allowOperationsOnUsersInSameAccount.key()); + } + private void initMessageBusListener() { messageBus.subscribe(EventTypes.EVENT_CONFIGURATION_VALUE_EDIT, new MessageSubscriber() { @Override @@ -1221,6 +1230,7 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati logger.error("Missing configuration variable " + name + " in configuration table"); return "Invalid configuration variable."; } + validateConfigurationAllowedOnlyForDefaultAdmin(name, value); String configScope = cfg.getScope(); if (scope != null) { @@ -1255,6 +1265,33 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati return validateValueRange(name, value, type, configuration); } + protected void validateConfigurationAllowedOnlyForDefaultAdmin(String configName, String value) { + if (configKeysAllowedOnlyForDefaultAdmin.contains(configName)) { + final Long userId = CallContext.current().getCallingUserId(); + if (userId != User.UID_ADMIN) { + throw new CloudRuntimeException("Only default admin is allowed to change this setting"); + } + + if (AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key().equals(configName)) { + if (value != null && !value.isBlank()) { + List validRoleTypes = Arrays.stream(RoleType.values()) + .map(Enum::name) + .collect(Collectors.toList()); + + boolean allValid = Arrays.stream(value.split(",")) + .map(String::trim) + .allMatch(validRoleTypes::contains); + + if (!allValid) { + throw new CloudRuntimeException("Invalid role types provided in value"); + } + } else { + throw new CloudRuntimeException("Value for role types must not be empty"); + } + } + } + } + /** * Returns whether a value is valid for a configuration of the provided type. * Valid configuration values are: diff --git a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java index b8c6e29c278..4a2702eb76d 100644 --- a/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java +++ b/server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java @@ -939,6 +939,7 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim } else { _accountMgr.checkAccess(caller, null, true, account); } + _accountMgr.verifyCallerPrivilegeForUserOrAccountOperations(account); ownerType = ResourceOwnerType.Account; ownerId = accountId; @@ -1090,6 +1091,11 @@ public class ResourceLimitManagerImpl extends ManagerBase implements ResourceLim throw new InvalidParameterValueException("Please specify a valid domain ID."); } _accountMgr.checkAccess(callerAccount, domain); + Account account = _entityMgr.findById(Account.class, accountId); + if (account == null) { + throw new InvalidParameterValueException("Unable to find account " + accountId); + } + _accountMgr.verifyCallerPrivilegeForUserOrAccountOperations(account); if (resourceType != null) { resourceTypes.add(resourceType); diff --git a/server/src/main/java/com/cloud/user/AccountManager.java b/server/src/main/java/com/cloud/user/AccountManager.java index 640eaa00076..c46ac78526b 100644 --- a/server/src/main/java/com/cloud/user/AccountManager.java +++ b/server/src/main/java/com/cloud/user/AccountManager.java @@ -206,4 +206,5 @@ public interface AccountManager extends AccountService, Configurable { UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user); + void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount); } diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 177f821d178..a0cd5113812 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -20,6 +20,7 @@ import java.net.InetAddress; import java.net.URLEncoder; import java.security.NoSuchAlgorithmException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; @@ -398,6 +399,22 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M true, ConfigKey.Scope.Domain); + public static ConfigKey listOfRoleTypesAllowedForOperationsOfSameRoleType = new ConfigKey<>("Hidden", + String.class, + "role.types.allowed.for.operations.on.accounts.of.same.role.type", + "Admin, ResourceAdmin, DomainAdmin", + "Comma separated list of role types that are allowed to do operations on accounts or users of the same role type within a domain.", + true, + ConfigKey.Scope.Domain); + + public static ConfigKey allowOperationsOnUsersInSameAccount = new ConfigKey<>("Hidden", + Boolean.class, + "allow.operations.on.users.in.same.account", + "true", + "Allow operations on users among them in the same account", + true, + ConfigKey.Scope.Domain); + protected AccountManagerImpl() { super(); } @@ -1404,7 +1421,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M /** * if there is any permission under the requested role that is not permitted for the caller, refuse */ - private void checkRoleEscalation(Account caller, Account requested) { + protected void checkRoleEscalation(Account caller, Account requested) { if (logger.isDebugEnabled()) { logger.debug(String.format("Checking if user of account %s [%s] with role-id [%d] can create an account of type %s [%s] with role-id [%d]", caller.getAccountName(), @@ -1510,6 +1527,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M assertUserNotAlreadyInAccount(duplicatedUser, account); } + verifyCallerPrivilegeForUserOrAccountOperations(account); UserVO user; user = createUser(account.getId(), userName, password, firstName, lastName, email, timeZone, userUUID, source); return user; @@ -1526,11 +1544,15 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M @ActionEvent(eventType = EventTypes.EVENT_USER_UPDATE, eventDescription = "Updating User") public UserAccount updateUser(UpdateUserCmd updateUserCmd) { UserVO user = retrieveAndValidateUser(updateUserCmd); + Account account = retrieveAndValidateAccount(user); + User caller = CallContext.current().getCallingUser(); + checkAccess(caller, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); + logger.debug("Updating user {}", user); validateAndUpdateApiAndSecretKeyIfNeeded(updateUserCmd, user); validateAndUpdateUserApiKeyAccess(updateUserCmd, user); - Account account = retrieveAndValidateAccount(user); validateAndUpdateFirstNameIfNeeded(updateUserCmd, user); validateAndUpdateLastNameIfNeeded(updateUserCmd, user); @@ -1553,6 +1575,85 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M return _userAccountDao.findById(user.getId()); } + @Override + public void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount) { + logger.debug(String.format("Verifying whether the caller has the correct privileges based on the user's role type and API permissions: %s", userAccount)); + + checkCallerRoleTypeAllowedForUserOrAccountOperations(userAccount, null); + checkCallerApiPermissionsForUserOrAccountOperations(userAccount); + } + + protected void verifyCallerPrivilegeForUserOrAccountOperations(User user) { + logger.debug(String.format("Verifying whether the caller has the correct privileges based on the user's role type and API permissions: %s", user)); + + Account userAccount = getAccount(user.getAccountId()); + checkCallerRoleTypeAllowedForUserOrAccountOperations(userAccount, user); + checkCallerApiPermissionsForUserOrAccountOperations(userAccount); + } + + protected void checkCallerRoleTypeAllowedForUserOrAccountOperations(Account userAccount, User user) { + Account callingAccount = getCurrentCallingAccount(); + RoleType callerRoleType = getRoleType(callingAccount); + RoleType userAccountRoleType = getRoleType(userAccount); + + if (RoleType.Unknown == callerRoleType || RoleType.Unknown == userAccountRoleType) { + String errMsg = String.format("The role type of account [%s, %s] or [%s, %s] is unknown", + callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid()); + throw new PermissionDeniedException(errMsg); + } + + boolean isCallerSystemOrDefaultAdmin = callingAccount.getId() == Account.ACCOUNT_ID_SYSTEM || callingAccount.getId() == Account.ACCOUNT_ID_ADMIN; + if (isCallerSystemOrDefaultAdmin) { + logger.trace(String.format("Admin account [%s, %s] performing this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + } else if (callerRoleType.getId() < userAccountRoleType.getId()) { + logger.trace(String.format("The calling account [%s, %s] has a higher role type than the user account [%s, %s]", + callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + } else if (callerRoleType.getId() == userAccountRoleType.getId()) { + if (callingAccount.getId() != userAccount.getId()) { + String allowedRoleTypes = listOfRoleTypesAllowedForOperationsOfSameRoleType.valueInDomain(callingAccount.getDomainId()); + boolean updateAllowed = allowedRoleTypes != null && + Arrays.stream(allowedRoleTypes.split(",")) + .map(String::trim) + .anyMatch(role -> role.equals(callerRoleType.toString())); + if (BooleanUtils.isFalse(updateAllowed)) { + String errMsg = String.format("The calling account [%s, %s] is not allowed to perform this operation on users from other accounts " + + "of the same role type within the domain", callingAccount.getName(), callingAccount.getUuid()); + logger.error(errMsg); + throw new PermissionDeniedException(errMsg); + } + } else if ((callingAccount.getId() == userAccount.getId()) && user != null) { + Boolean allowOperationOnUsersinSameAccount = allowOperationsOnUsersInSameAccount.valueInDomain(callingAccount.getDomainId()); + User callingUser = CallContext.current().getCallingUser(); + if (callingUser.getId() != user.getId() && BooleanUtils.isFalse(allowOperationOnUsersinSameAccount)) { + String errMsg = "The user operations are not allowed by the users in the same account"; + logger.error(errMsg); + throw new PermissionDeniedException(errMsg); + } + } + } else { + String errMsg = String.format("The calling account [%s, %s] has a lower role type than the user account [%s, %s]", + callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid()); + throw new PermissionDeniedException(errMsg); + } + } + + protected void checkCallerApiPermissionsForUserOrAccountOperations(Account userAccount) { + Account callingAccount = getCurrentCallingAccount(); + boolean isCallerRootAdmin = callingAccount.getId() == Account.ACCOUNT_ID_SYSTEM || isRootAdmin(callingAccount.getId()); + + if (isCallerRootAdmin) { + logger.trace(String.format("Admin account [%s, %s] performing this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + } else if (isRootAdmin(userAccount.getAccountId())) { + String errMsg = String.format("Account [%s, %s] cannot perform this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid()); + logger.error(errMsg); + throw new PermissionDeniedException(errMsg); + } else { + logger.debug(String.format("Checking calling account [%s, %s] permission to perform this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + checkRoleEscalation(callingAccount, userAccount); + logger.debug(String.format("Calling account [%s, %s] is allowed to perform this operation for user account [%s, %s] ", callingAccount.getName(), callingAccount.getUuid(), userAccount.getName(), userAccount.getUuid())); + } + } + /** * Updates the password in the user POJO if needed. If no password is provided, then the password is not updated. * The following validations are executed if 'password' is not null. Admins (root admins or domain admins) can execute password updates without entering the current password. @@ -1832,6 +1933,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); boolean success = doSetUserStatus(userId, State.DISABLED); if (success) { @@ -1873,6 +1975,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); boolean success = Transaction.execute(new TransactionCallback<>() { @Override @@ -1925,6 +2028,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); // make sure the account is enabled too // if the user is either locked already or disabled already, don't change state...only lock currently enabled @@ -1988,6 +2092,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkIfAccountManagesProjects(accountId); + verifyCallerPrivilegeForUserOrAccountOperations(account); CallContext.current().putContextParameter(Account.class, account.getUuid()); @@ -2050,6 +2155,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // Check if user performing the action is allowed to modify this account Account caller = getCurrentCallingAccount(); checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(account); boolean success = enableAccount(account.getId()); if (success) { @@ -2083,6 +2189,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(account); if (lockAccount(account.getId())) { CallContext.current().putContextParameter(Account.class, account.getUuid()); @@ -2113,6 +2220,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } checkAccess(caller, AccessType.OperateEntry, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(account); if (disableAccount(account.getId())) { CallContext.current().putContextParameter(Account.class, account.getUuid()); @@ -2158,6 +2266,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // Check if user performing the action is allowed to modify this account Account caller = getCurrentCallingAccount(); checkAccess(caller, _domainMgr.getDomain(account.getDomainId())); + verifyCallerPrivilegeForUserOrAccountOperations(account); validateAndUpdateAccountApiKeyAccess(cmd, acctForUpdate); @@ -2249,6 +2358,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M // don't allow to delete the user from the account of type Project checkAccountAndAccess(user, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); return _userDao.remove(id); } @@ -2259,6 +2369,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M UserVO user = getValidUserVO(id); Account oldAccount = _accountDao.findById(user.getAccountId()); checkAccountAndAccess(user, oldAccount); + verifyCallerPrivilegeForUserOrAccountOperations(user); long domainId = oldAccount.getDomainId(); long newAccountId = getNewAccountId(domainId, cmd.getAccountName(), cmd.getAccountId()); @@ -2596,6 +2707,11 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } + if (!Account.Type.PROJECT.equals(accountType)) { + AccountVO newAccount = new AccountVO(accountName, domainId, networkDomain, accountType, roleId, uuid); + verifyCallerPrivilegeForUserOrAccountOperations(newAccount); + } + // Create the account return Transaction.execute(new TransactionCallback<>() { @Override @@ -2939,8 +3055,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } final Account account = getAccount(getUserAccountById(userId).getAccountId()); //Extracting the Account from the userID of the requested user. User caller = CallContext.current().getCallingUser(); - preventRootDomainAdminAccessToRootAdminKeys(caller, account); checkAccess(caller, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); Map keys = new HashMap<>(); keys.put("apikey", user.getApiKey()); @@ -3004,8 +3120,8 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } Account account = _accountDao.findById(user.getAccountId()); - preventRootDomainAdminAccessToRootAdminKeys(user, account); checkAccess(caller, null, true, account); + verifyCallerPrivilegeForUserOrAccountOperations(user); // don't allow updating system user if (user.getId() == User.UID_SYSTEM) { @@ -3461,7 +3577,7 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M public ConfigKey[] getConfigKeys() { return new ConfigKey[] {UseSecretKeyInResponse, enableUserTwoFactorAuthentication, userTwoFactorAuthenticationDefaultProvider, mandateUserTwoFactorAuthentication, userTwoFactorAuthenticationIssuer, apiKeyAccess, - userAllowMultipleAccounts}; + userAllowMultipleAccounts, listOfRoleTypesAllowedForOperationsOfSameRoleType, allowOperationsOnUsersInSameAccount}; } public List getUserTwoFactorAuthenticationProviders() { diff --git a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java b/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java index 0a045296821..1309842b706 100644 --- a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java +++ b/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java @@ -45,15 +45,18 @@ import com.cloud.storage.StorageManager; import com.cloud.storage.dao.VMTemplateZoneDao; import com.cloud.storage.dao.VolumeDao; import com.cloud.user.Account; +import com.cloud.user.AccountManagerImpl; import com.cloud.user.User; import com.cloud.utils.db.EntityManager; import com.cloud.utils.db.SearchCriteria; +import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.net.NetUtils; import com.cloud.vm.dao.VMInstanceDao; import org.apache.cloudstack.annotation.dao.AnnotationDao; import org.apache.cloudstack.api.command.admin.network.CreateNetworkOfferingCmd; import org.apache.cloudstack.api.command.admin.offering.UpdateDiskOfferingCmd; import org.apache.cloudstack.api.command.admin.zone.DeleteZoneCmd; +import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.engine.subsystem.api.storage.ZoneScope; import org.apache.cloudstack.framework.config.ConfigDepot; import org.apache.cloudstack.framework.config.ConfigKey; @@ -182,6 +185,7 @@ public class ConfigurationManagerImplTest { configurationManagerImplSpy.populateConfigValuesForValidationSet(); configurationManagerImplSpy.weightBasedParametersForValidation(); configurationManagerImplSpy.overProvisioningFactorsForValidation(); + configurationManagerImplSpy.populateConfigKeysAllowedOnlyForDefaultAdmin(); ReflectionTestUtils.setField(configurationManagerImplSpy, "templateZoneDao", vmTemplateZoneDao); ReflectionTestUtils.setField(configurationManagerImplSpy, "annotationDao", annotationDao); @@ -851,4 +855,46 @@ public class ConfigurationManagerImplTest { boolean result = configurationManagerImplSpy.shouldValidateConfigRange(Config.ConsoleProxySessionMax.name(), "test", Config.ConsoleProxyUrlDomain); Assert.assertTrue(result); } + + @Test + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withAdminUser_shouldNotThrowException() { + CallContext callContext = mock(CallContext.class); + when(callContext.getCallingUserId()).thenReturn(User.UID_ADMIN); + try (MockedStatic ignored = Mockito.mockStatic(CallContext.class)) { + when(CallContext.current()).thenReturn(callContext); + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin(AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key(), "Admin"); + } + } + + @Test + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withNonAdminUser_shouldThrowException() { + CallContext callContext = mock(CallContext.class); + when(callContext.getCallingUserId()).thenReturn(123L); + try (MockedStatic ignored = Mockito.mockStatic(CallContext.class)) { + when(CallContext.current()).thenReturn(callContext); + Assert.assertThrows(CloudRuntimeException.class, () -> + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin(AccountManagerImpl.allowOperationsOnUsersInSameAccount.key(), "Admin") + ); + } + } + + @Test + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withNonRestrictedKey_shouldNotThrowException() { + CallContext callContext = mock(CallContext.class); + try (MockedStatic ignored = Mockito.mockStatic(CallContext.class)) { + when(CallContext.current()).thenReturn(callContext); + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin("some.other.config.key", "Admin"); + } + } + + @Test(expected = CloudRuntimeException.class) + public void testValidateConfigurationAllowedOnlyForDefaultAdmin_withValidConfigNameAndInvalidValue_shouldThrowException() { + CallContext callContext = mock(CallContext.class); + try (MockedStatic mockedCallContext = Mockito.mockStatic(CallContext.class)) { + mockedCallContext.when(CallContext::current).thenReturn(callContext); + when(callContext.getCallingUserId()).thenReturn(User.UID_ADMIN); + String invalidValue = "Admin, SuperUser"; + configurationManagerImplSpy.validateConfigurationAllowedOnlyForDefaultAdmin(AccountManagerImpl.listOfRoleTypesAllowedForOperationsOfSameRoleType.key(), invalidValue); + } + } } diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index eb401efa341..09a2d73080d 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -16,6 +16,7 @@ // under the License. package com.cloud.user; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.nullable; import java.net.InetAddress; @@ -124,6 +125,9 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { @Mock ConfigKey enableUserTwoFactorAuthenticationMock; + + @Mock + ConfigKey allowOperationsOnUsersInSameAccountMock; @Mock RoleService roleService; @@ -131,6 +135,9 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { public void setUp() throws Exception { enableUserTwoFactorAuthenticationMock = Mockito.mock(ConfigKey.class); accountManagerImpl.enableUserTwoFactorAuthentication = enableUserTwoFactorAuthenticationMock; + + allowOperationsOnUsersInSameAccountMock = Mockito.mock(ConfigKey.class); + accountManagerImpl.allowOperationsOnUsersInSameAccount = allowOperationsOnUsersInSameAccountMock; } @Before @@ -141,6 +148,8 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.doReturn(accountMockId).when(userVoMock).getAccountId(); Mockito.doReturn(userVoIdMock).when(userVoMock).getId(); + + Mockito.lenient().doNothing().when(accountManagerImpl).checkRoleEscalation(accountMock, accountMock); } @Test @@ -191,6 +200,7 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.when(_sshKeyPairDao.remove(Mockito.anyLong())).thenReturn(true); Mockito.when(userDataDao.removeByAccountId(Mockito.anyLong())).thenReturn(222); Mockito.doNothing().when(accountManagerImpl).deleteWebhooksForAccount(Mockito.anyLong()); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations((Account) any()); Assert.assertTrue(accountManagerImpl.deleteUserAccount(42l)); // assert that this was a clean delete @@ -211,6 +221,7 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.lenient().when(_domainMgr.getDomain(Mockito.anyLong())).thenReturn(domain); Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class), Mockito.any(Domain.class))).thenReturn(true); Mockito.doNothing().when(accountManagerImpl).deleteWebhooksForAccount(Mockito.anyLong()); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations((Account) any()); Assert.assertTrue(accountManagerImpl.deleteUserAccount(42l)); // assert that this was NOT a clean delete @@ -245,6 +256,7 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.doReturn(2L).when(accountVoMock).getId(); Mockito.doReturn(true).when(accountManagerImpl).isDeleteNeeded(Mockito.any(), Mockito.anyLong(), Mockito.any()); Mockito.doReturn(new ArrayList()).when(_projectAccountDao).listAdministratedProjectIds(Mockito.anyLong()); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations((Account) any()); accountManagerImpl.deleteUserAccount(accountId); } @@ -282,6 +294,7 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.doReturn(2L).when(userVoMock).getId(); Mockito.doNothing().when(accountManagerImpl).checkAccountAndAccess(Mockito.any(), Mockito.any()); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations(userVoMock); accountManagerImpl.deleteUser(cmd); } } @@ -348,7 +361,6 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { // Queried account - admin account AccountVO adminAccountMock = Mockito.mock(AccountVO.class); - Mockito.when(adminAccountMock.getAccountId()).thenReturn(2L); Mockito.when(_accountDao.findByIdIncludingRemoved(2L)).thenReturn(adminAccountMock); Mockito.lenient().when(accountService.isRootAdmin(2L)).thenReturn(true); Mockito.lenient().when(securityChecker.checkAccess(Mockito.any(Account.class), @@ -396,6 +408,12 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { @Test public void updateUserTestTimeZoneAndEmailNull() { + Mockito.when(userVoMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(accountMock).when(accountManagerImpl).getAccount(10L); + Mockito.when(accountMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(10L); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.User); + prepareMockAndExecuteUpdateUserTest(0); } @@ -403,6 +421,11 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { public void updateUserTestTimeZoneAndEmailNotNull() { Mockito.when(UpdateUserCmdMock.getEmail()).thenReturn("email"); Mockito.when(UpdateUserCmdMock.getTimezone()).thenReturn("timezone"); + Mockito.when(userVoMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(accountMock).when(accountManagerImpl).getAccount(10L); + Mockito.when(accountMock.getAccountId()).thenReturn(10L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(10L); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.User); prepareMockAndExecuteUpdateUserTest(1); } @@ -420,14 +443,17 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { Mockito.doReturn(true).when(userDaoMock).update(Mockito.anyLong(), Mockito.eq(userVoMock)); Mockito.doReturn(Mockito.mock(UserAccountVO.class)).when(userAccountDaoMock).findById(Mockito.anyLong()); + Mockito.doNothing().when(accountManagerImpl).checkAccess(nullable(User.class), nullable(Account.class)); accountManagerImpl.updateUser(UpdateUserCmdMock); + Mockito.lenient().doNothing().when(accountManagerImpl).checkRoleEscalation(accountMock, accountMock); + InOrder inOrder = Mockito.inOrder(userVoMock, accountManagerImpl, userDaoMock, userAccountDaoMock); inOrder.verify(accountManagerImpl).retrieveAndValidateUser(UpdateUserCmdMock); - inOrder.verify(accountManagerImpl).validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock); inOrder.verify(accountManagerImpl).retrieveAndValidateAccount(userVoMock); + inOrder.verify(accountManagerImpl).validateAndUpdateApiAndSecretKeyIfNeeded(UpdateUserCmdMock, userVoMock); inOrder.verify(accountManagerImpl).validateAndUpdateFirstNameIfNeeded(UpdateUserCmdMock, userVoMock); inOrder.verify(accountManagerImpl).validateAndUpdateLastNameIfNeeded(UpdateUserCmdMock, userVoMock); @@ -1339,8 +1365,8 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { accountManagerImpl.validateRoleChange(account, newRole, caller); } - @Test - public void checkIfAccountManagesProjectsTestNotThrowExceptionWhenTheAccountIsNotAProjectAdministrator() { + @Test + public void checkIfAccountManagesProjectsTestNotThrowExceptionWhenTheAccountIsNotAProjectAdministrator() { long accountId = 1L; List managedProjectIds = new ArrayList<>(); @@ -1477,4 +1503,74 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { accountManagerImpl.assertUserNotAlreadyInDomain(existingUser, originalAccount); } + + @Test + public void testCheckCallerRoleTypeAllowedToUpdateUserSameAccount() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(accountMock); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.DomainAdmin); + + accountManagerImpl.checkCallerRoleTypeAllowedForUserOrAccountOperations(accountMock, userVoMock); + } + + @Test(expected = PermissionDeniedException.class) + public void testCheckCallerRoleTypeAllowedToUpdateUserLowerAccountRoleType() { + Account callingAccount = Mockito.mock(Account.class); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(2L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(2L); + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(callingAccount))).thenReturn(RoleType.DomainAdmin); + Mockito.lenient().when(accountManagerImpl.getRoleType(Mockito.eq(accountMock))).thenReturn(RoleType.Admin); + accountManagerImpl.checkCallerRoleTypeAllowedForUserOrAccountOperations(accountMock, userVoMock); + } + + @Test + public void testcheckCallerApiPermissionsForUserOperationsRootAdminSameCaller() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(accountMock); + Mockito.when(accountMock.getId()).thenReturn(2L); + Mockito.doReturn(true).when(accountManagerImpl).isRootAdmin(2L); + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } + + @Test(expected = PermissionDeniedException.class) + public void testcheckCallerApiPermissionsForUserOperationsRootAdminDifferentAccount() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(3L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(3L); + Mockito.lenient().doReturn(false).when(accountManagerImpl).isRootAdmin(3L); + + Mockito.when(accountMock.getAccountId()).thenReturn(2L); + Mockito.doReturn(true).when(accountManagerImpl).isRootAdmin(2L); + + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } + + @Test + public void testcheckCallerApiPermissionsForUserOperationsAllowedApis() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(3L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(3L); + Mockito.lenient().doReturn(false).when(accountManagerImpl).isRootAdmin(3L); + + Mockito.when(accountMock.getAccountId()).thenReturn(2L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(2L); + + Mockito.lenient().doNothing().when(accountManagerImpl).checkRoleEscalation(callingAccount, accountMock); + + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } + + @Test(expected = PermissionDeniedException.class) + public void testcheckCallerApiPermissionsForUserOperationsNotAllowedApis() { + Mockito.lenient().when(accountManagerImpl.getCurrentCallingAccount()).thenReturn(callingAccount); + Mockito.lenient().when(callingAccount.getAccountId()).thenReturn(3L); + Mockito.lenient().doReturn(callingAccount).when(accountManagerImpl).getAccount(3L); + Mockito.lenient().doReturn(false).when(accountManagerImpl).isRootAdmin(3L); + + Mockito.when(accountMock.getAccountId()).thenReturn(2L); + Mockito.doReturn(false).when(accountManagerImpl).isRootAdmin(2L); + + Mockito.lenient().doThrow(PermissionDeniedException.class).when(accountManagerImpl).checkRoleEscalation(callingAccount, accountMock); + + accountManagerImpl.checkCallerApiPermissionsForUserOrAccountOperations(accountMock); + } } diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java index 4004321b8e9..6d69890c9a5 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplVolumeDeleteEventTest.java @@ -192,6 +192,7 @@ public class AccountManagerImplVolumeDeleteEventTest extends AccountManagetImplT public void destroyedVMRootVolumeUsageEvent() throws SecurityException, IllegalArgumentException, ReflectiveOperationException, AgentUnavailableException, ConcurrentOperationException, CloudException { lenient().doReturn(vm).when(_vmMgr).destroyVm(nullable(Long.class), nullable(Boolean.class)); + Mockito.doNothing().when(accountManagerImpl).verifyCallerPrivilegeForUserOrAccountOperations((Account) any()); List emittedEvents = deleteUserAccountRootVolumeUsageEvents(true); Assert.assertEquals(0, emittedEvents.size()); } diff --git a/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java b/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java index 1b75638b125..98f152088ed 100644 --- a/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java +++ b/server/src/test/java/com/cloud/user/AccountManagetImplTestBase.java @@ -210,6 +210,9 @@ public class AccountManagetImplTestBase { @Mock RoutedIpv4Manager routedIpv4Manager; + @Mock + Account accountMock; + @Before public void setup() { accountManagerImpl.setUserAuthenticators(Arrays.asList(userAuthenticator)); diff --git a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java index 62a95380a15..8c569fb3ec8 100644 --- a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java +++ b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java @@ -503,4 +503,8 @@ public class MockAccountManagerImpl extends ManagerBase implements Manager, Acco public UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user) { return null; } + + @Override + public void verifyCallerPrivilegeForUserOrAccountOperations(Account userAccount) { + } }