diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 412a5b58282c..7e86413409fa 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -120,8 +120,6 @@ import com.cloud.network.IpAddressManager; import com.cloud.network.Network; import com.cloud.network.NetworkModel; -import com.cloud.network.security.SecurityGroupService; -import com.cloud.network.security.SecurityGroupVO; import com.cloud.network.VpnUserVO; import com.cloud.network.as.AutoScaleManager; import com.cloud.network.dao.AccountGuestVlanMapDao; @@ -135,6 +133,8 @@ import com.cloud.network.dao.VpnUserDao; import com.cloud.network.router.VirtualRouter; import com.cloud.network.security.SecurityGroupManager; +import com.cloud.network.security.SecurityGroupService; +import com.cloud.network.security.SecurityGroupVO; import com.cloud.network.security.dao.SecurityGroupDao; import com.cloud.network.vpc.Vpc; import com.cloud.network.vpc.VpcManager; @@ -1301,16 +1301,33 @@ public Pair doInTransaction(TransactionStatus status) { return _userAccountDao.findById(userId); } - private boolean isValidRoleChange(Account account, Role role) { - Long currentAccRoleId = account.getRoleId(); - Role currentRole = roleService.findRole(currentAccRoleId); - - if (role.getRoleType().ordinal() < currentRole.getRoleType().ordinal() && ((account.getType() == Account.Type.NORMAL && role.getRoleType().getAccountType().ordinal() > Account.Type.NORMAL.ordinal()) || - account.getType().ordinal() > Account.Type.NORMAL.ordinal() && role.getRoleType().getAccountType().ordinal() < account.getType().ordinal() && role.getRoleType().getAccountType().ordinal() > 0)) { - throw new PermissionDeniedException(String.format("Unable to update account role to %s as you are " + - "attempting to escalate the account %s to account type %s which has higher privileges", role.getName(), account.getAccountName(), role.getRoleType().getAccountType().name())); + /* + Role change should follow the below conditions: + - Caller should not be of Unknown role type + - New role's type should not be Unknown + - Caller should not be able to escalate or de-escalate an account's role which is of higher role type + - New role should not be of type Admin with domain other than ROOT domain + */ + protected void validateRoleChange(Account account, Role role, Account caller) { + Role currentRole = roleService.findRole(account.getRoleId()); + Role callerRole = roleService.findRole(caller.getRoleId()); + String errorMsg = String.format("Unable to update account role to %s, ", role.getName()); + if (RoleType.Unknown.equals(callerRole.getRoleType())) { + throw new PermissionDeniedException(String.format("%s as the caller privileges are unknown", errorMsg)); + } + if (RoleType.Unknown.equals(role.getRoleType())) { + throw new PermissionDeniedException(String.format("%s as the new role privileges are unknown", errorMsg)); + } + if (!callerRole.getRoleType().equals(RoleType.Admin) && + (role.getRoleType().ordinal() < callerRole.getRoleType().ordinal() || + currentRole.getRoleType().ordinal() < callerRole.getRoleType().ordinal())) { + throw new PermissionDeniedException(String.format("%s as either current or new role has higher " + + "privileges than the caller", errorMsg)); + } + if (role.getRoleType().equals(RoleType.Admin) && account.getDomainId() != Domain.ROOT_DOMAIN) { + throw new PermissionDeniedException(String.format("%s as the user does not belong to the ROOT domain", + errorMsg)); } - return true; } /** @@ -2045,7 +2062,7 @@ public AccountVO updateAccount(UpdateAccountCmd cmd) { } Role role = roleService.findRole(roleId); - isValidRoleChange(account, role); + validateRoleChange(account, role, caller); acctForUpdate.setRoleId(roleId); acctForUpdate.setType(role.getRoleType().getAccountType()); checkRoleEscalation(getCurrentCallingAccount(), acctForUpdate); diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 3ecb4f2f1cf1..a50779ef61ae 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -16,15 +16,20 @@ // under the License. package com.cloud.user; +import static org.mockito.ArgumentMatchers.nullable; + import java.net.InetAddress; import java.net.UnknownHostException; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.HashMap; import org.apache.cloudstack.acl.ControlledEntity; +import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.acl.RoleService; +import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd; import org.apache.cloudstack.api.command.admin.user.UpdateUserCmd; @@ -42,7 +47,6 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; -import static org.mockito.ArgumentMatchers.nullable; import com.cloud.acl.DomainChecker; import com.cloud.api.auth.SetupUserTwoFactorAuthenticationCmd; @@ -102,6 +106,8 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { @Mock ConfigKey enableUserTwoFactorAuthenticationMock; + @Mock + RoleService roleService; @Before public void setUp() throws Exception { @@ -1056,4 +1062,112 @@ public void deleteAndCleanupUserTestRemovesUserFromProjects() { Mockito.verify(_projectAccountDao).removeUserFromProjects(userId); } + + @Test(expected = PermissionDeniedException.class) + public void testValidateRoleChangeUnknownCaller() { + Account account = Mockito.mock(Account.class); + Mockito.when(account.getRoleId()).thenReturn(1L); + Role role = Mockito.mock(Role.class); + Mockito.when(role.getRoleType()).thenReturn(RoleType.Unknown); + Account caller = Mockito.mock(Account.class); + Mockito.when(caller.getRoleId()).thenReturn(2L); + Mockito.when(roleService.findRole(2L)).thenReturn(role); + accountManagerImpl.validateRoleChange(account, Mockito.mock(Role.class), caller); + } + + @Test(expected = PermissionDeniedException.class) + public void testValidateRoleChangeUnknownNewRole() { + Account account = Mockito.mock(Account.class); + Mockito.when(account.getRoleId()).thenReturn(1L); + Role newRole = Mockito.mock(Role.class); + Mockito.when(newRole.getRoleType()).thenReturn(RoleType.Unknown); + Role callerRole = Mockito.mock(Role.class); + Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin); + Account caller = Mockito.mock(Account.class); + Mockito.when(caller.getRoleId()).thenReturn(2L); + Mockito.when(roleService.findRole(2L)).thenReturn(callerRole); + accountManagerImpl.validateRoleChange(account, newRole, caller); + } + + @Test + public void testValidateRoleNewRoleSameCaller() { + Account account = Mockito.mock(Account.class); + Mockito.when(account.getRoleId()).thenReturn(1L); + Role currentRole = Mockito.mock(Role.class); + Mockito.when(currentRole.getRoleType()).thenReturn(RoleType.User); + Mockito.when(roleService.findRole(1L)).thenReturn(currentRole); + Role newRole = Mockito.mock(Role.class); + Mockito.when(newRole.getRoleType()).thenReturn(RoleType.DomainAdmin); + Role callerRole = Mockito.mock(Role.class); + Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin); + Account caller = Mockito.mock(Account.class); + Mockito.when(caller.getRoleId()).thenReturn(2L); + Mockito.when(roleService.findRole(2L)).thenReturn(callerRole); + accountManagerImpl.validateRoleChange(account, newRole, caller); + } + + @Test + public void testValidateRoleCurrentRoleSameCaller() { + Account account = Mockito.mock(Account.class); + Mockito.when(account.getRoleId()).thenReturn(1L); + Role accountRole = Mockito.mock(Role.class); + Mockito.when(accountRole.getRoleType()).thenReturn(RoleType.DomainAdmin); + Role newRole = Mockito.mock(Role.class); + Mockito.when(newRole.getRoleType()).thenReturn(RoleType.User); + Role callerRole = Mockito.mock(Role.class); + Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin); + Account caller = Mockito.mock(Account.class); + Mockito.when(caller.getRoleId()).thenReturn(2L); + Mockito.when(roleService.findRole(1L)).thenReturn(accountRole); + Mockito.when(roleService.findRole(2L)).thenReturn(callerRole); + accountManagerImpl.validateRoleChange(account, newRole, caller); + } + + @Test(expected = PermissionDeniedException.class) + public void testValidateRoleNewRoleHigherCaller() { + Account account = Mockito.mock(Account.class); + Mockito.when(account.getRoleId()).thenReturn(1L); + Role newRole = Mockito.mock(Role.class); + Mockito.when(newRole.getRoleType()).thenReturn(RoleType.Admin); + Role callerRole = Mockito.mock(Role.class); + Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin); + Account caller = Mockito.mock(Account.class); + Mockito.when(caller.getRoleId()).thenReturn(2L); + Mockito.when(roleService.findRole(2L)).thenReturn(callerRole); + accountManagerImpl.validateRoleChange(account, newRole, caller); + } + + @Test + public void testValidateRoleNewRoleLowerCaller() { + Account account = Mockito.mock(Account.class); + Mockito.when(account.getRoleId()).thenReturn(1L); + Role newRole = Mockito.mock(Role.class); + Mockito.when(newRole.getRoleType()).thenReturn(RoleType.User); + Role accountRole = Mockito.mock(Role.class); + Mockito.when(accountRole.getRoleType()).thenReturn(RoleType.User); + Role callerRole = Mockito.mock(Role.class); + Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin); + Account caller = Mockito.mock(Account.class); + Mockito.when(caller.getRoleId()).thenReturn(2L); + Mockito.when(roleService.findRole(1L)).thenReturn(accountRole); + Mockito.when(roleService.findRole(2L)).thenReturn(callerRole); + accountManagerImpl.validateRoleChange(account, newRole, caller); + } + + @Test(expected = PermissionDeniedException.class) + public void testValidateRoleAdminCannotEscalateAdminFromNonRootDomain() { + Account account = Mockito.mock(Account.class); + Mockito.when(account.getRoleId()).thenReturn(1L); + Mockito.when(account.getDomainId()).thenReturn(2L); + Role newRole = Mockito.mock(Role.class); + Mockito.when(newRole.getRoleType()).thenReturn(RoleType.Admin); + Role accountRole = Mockito.mock(Role.class); + Role callerRole = Mockito.mock(Role.class); + Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.Admin); + Account caller = Mockito.mock(Account.class); + Mockito.when(caller.getRoleId()).thenReturn(2L); + Mockito.when(roleService.findRole(1L)).thenReturn(accountRole); + Mockito.when(roleService.findRole(2L)).thenReturn(callerRole); + accountManagerImpl.validateRoleChange(account, newRole, caller); + } }