From e78115079b37e45c9f134329033d0da90e481ad4 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Wed, 5 Jun 2024 13:50:01 +0530 Subject: [PATCH 1/5] server: simplify role change validation Fixes #9015 Signed-off-by: Abhishek Kumar --- .../com/cloud/user/AccountManagerImpl.java | 37 ++++-- .../cloud/user/AccountManagerImplTest.java | 112 ++++++++++++++++++ 2 files changed, 139 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 3eed429ed215..41dc2e42c4c3 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1287,16 +1287,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 same or higher role type + - New role should not 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())) { // Same type caller + throw new PermissionDeniedException(String.format("%s as either current or new role has higher " + + "or same 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; } /** @@ -2031,7 +2048,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 9014af523fdc..db57e4bad709 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -33,6 +33,10 @@ import com.cloud.vm.UserVmVO; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.snapshot.VMSnapshotVO; + +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; @@ -101,6 +105,8 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase { @Mock ConfigKey enableUserTwoFactorAuthenticationMock; + @Mock + RoleService roleService; @Before public void setUp() throws Exception { @@ -988,4 +994,110 @@ public void testGetActiveUserAccountByEmail() { Assert.assertEquals(userAccountVOList.size(), userAccounts.size()); Assert.assertEquals(userAccountVOList.get(0), userAccounts.get(0)); } + + @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(expected = PermissionDeniedException.class) + public void testValidateRoleNewRoleSame() { + Account account = Mockito.mock(Account.class); + Mockito.when(account.getRoleId()).thenReturn(1L); + 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(expected = PermissionDeniedException.class) + public void testValidateRoleCurrentRoleSame() { + 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 testValidateRoleNewRoleHigher() { + 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 testValidateRoleNewRoleLower() { + 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); + Mockito.when(accountRole.getRoleType()).thenReturn(RoleType.DomainAdmin); + 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); + } } From 1064cb7697f5a438da117b7f202e263a1004cde7 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Wed, 5 Jun 2024 14:13:45 +0530 Subject: [PATCH 2/5] fix build failures Signed-off-by: Abhishek Kumar --- .../com/cloud/user/AccountManagerImpl.java | 4 +- .../cloud/user/AccountManagerImplTest.java | 53 +++++++++---------- 2 files changed, 28 insertions(+), 29 deletions(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 41dc2e42c4c3..733651db06a0 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -119,8 +119,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; @@ -134,6 +132,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; diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index db57e4bad709..6c2ef8bd552c 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -16,23 +16,15 @@ // under the License. package com.cloud.user; -import com.cloud.acl.DomainChecker; -import com.cloud.api.auth.SetupUserTwoFactorAuthenticationCmd; -import com.cloud.domain.Domain; -import com.cloud.domain.DomainVO; -import com.cloud.exception.ConcurrentOperationException; -import com.cloud.exception.InvalidParameterValueException; -import com.cloud.exception.PermissionDeniedException; -import com.cloud.exception.ResourceUnavailableException; -import com.cloud.projects.Project; -import com.cloud.projects.ProjectAccountVO; -import com.cloud.user.Account.State; -import com.cloud.utils.Pair; -import com.cloud.utils.exception.CloudRuntimeException; -import com.cloud.vm.UserVmManagerImpl; -import com.cloud.vm.UserVmVO; -import com.cloud.vm.VMInstanceVO; -import com.cloud.vm.snapshot.VMSnapshotVO; +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 org.apache.cloudstack.acl.Role; import org.apache.cloudstack.acl.RoleService; @@ -55,15 +47,23 @@ import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; -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 static org.mockito.ArgumentMatchers.nullable; +import com.cloud.acl.DomainChecker; +import com.cloud.api.auth.SetupUserTwoFactorAuthenticationCmd; +import com.cloud.domain.Domain; +import com.cloud.domain.DomainVO; +import com.cloud.exception.ConcurrentOperationException; +import com.cloud.exception.InvalidParameterValueException; +import com.cloud.exception.PermissionDeniedException; +import com.cloud.exception.ResourceUnavailableException; +import com.cloud.projects.Project; +import com.cloud.projects.ProjectAccountVO; +import com.cloud.user.Account.State; +import com.cloud.utils.Pair; +import com.cloud.utils.exception.CloudRuntimeException; +import com.cloud.vm.UserVmManagerImpl; +import com.cloud.vm.UserVmVO; +import com.cloud.vm.VMInstanceVO; +import com.cloud.vm.snapshot.VMSnapshotVO; @RunWith(MockitoJUnitRunner.class) public class AccountManagerImplTest extends AccountManagetImplTestBase { @@ -1091,7 +1091,6 @@ public void testValidateRoleAdminCannotEscalateAdminFromNonRootDomain() { Role newRole = Mockito.mock(Role.class); Mockito.when(newRole.getRoleType()).thenReturn(RoleType.Admin); Role accountRole = Mockito.mock(Role.class); - Mockito.when(accountRole.getRoleType()).thenReturn(RoleType.DomainAdmin); Role callerRole = Mockito.mock(Role.class); Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.Admin); Account caller = Mockito.mock(Account.class); From ccb30ca7c7ef2dbeca005fc716fdbd5001e9bd71 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Wed, 5 Jun 2024 16:41:02 +0530 Subject: [PATCH 3/5] refactor error message Co-authored-by: dahn --- server/src/main/java/com/cloud/user/AccountManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 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 733651db06a0..bd5768193f84 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1297,7 +1297,7 @@ public Pair doInTransaction(TransactionStatus status) { 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()); + 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)); } From 1bd976666ab45a3869018afc25ed6364373cf793 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Wed, 5 Jun 2024 17:32:02 +0530 Subject: [PATCH 4/5] Update server/src/main/java/com/cloud/user/AccountManagerImpl.java --- server/src/main/java/com/cloud/user/AccountManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 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 bd5768193f84..2fc80776f47b 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1292,7 +1292,7 @@ public Pair doInTransaction(TransactionStatus status) { - 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 same or higher role type - - New role should not of type Admin with domain other than ROOT domain + - 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()); From 3ccd65d981f98341973e85852aeabbb07dce0e24 Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Thu, 22 Aug 2024 17:18:37 +0530 Subject: [PATCH 5/5] changes Signed-off-by: Abhishek Kumar --- .../java/com/cloud/user/AccountManagerImpl.java | 8 ++++---- .../com/cloud/user/AccountManagerImplTest.java | 15 +++++++++------ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index 2fc80776f47b..a4485744c44f 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1291,7 +1291,7 @@ public Pair doInTransaction(TransactionStatus status) { 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 same or higher role type + - 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) { @@ -1305,10 +1305,10 @@ protected void validateRoleChange(Account account, Role role, Account caller) { 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())) { // Same type caller + (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 " + - "or same privileges than the caller", errorMsg)); + "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", diff --git a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java index 6c2ef8bd552c..17252f63fd81 100644 --- a/server/src/test/java/com/cloud/user/AccountManagerImplTest.java +++ b/server/src/test/java/com/cloud/user/AccountManagerImplTest.java @@ -1021,10 +1021,13 @@ public void testValidateRoleChangeUnknownNewRole() { accountManagerImpl.validateRoleChange(account, newRole, caller); } - @Test(expected = PermissionDeniedException.class) - public void testValidateRoleNewRoleSame() { + @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); @@ -1035,8 +1038,8 @@ public void testValidateRoleNewRoleSame() { accountManagerImpl.validateRoleChange(account, newRole, caller); } - @Test(expected = PermissionDeniedException.class) - public void testValidateRoleCurrentRoleSame() { + @Test + public void testValidateRoleCurrentRoleSameCaller() { Account account = Mockito.mock(Account.class); Mockito.when(account.getRoleId()).thenReturn(1L); Role accountRole = Mockito.mock(Role.class); @@ -1053,7 +1056,7 @@ public void testValidateRoleCurrentRoleSame() { } @Test(expected = PermissionDeniedException.class) - public void testValidateRoleNewRoleHigher() { + public void testValidateRoleNewRoleHigherCaller() { Account account = Mockito.mock(Account.class); Mockito.when(account.getRoleId()).thenReturn(1L); Role newRole = Mockito.mock(Role.class); @@ -1067,7 +1070,7 @@ public void testValidateRoleNewRoleHigher() { } @Test - public void testValidateRoleNewRoleLower() { + public void testValidateRoleNewRoleLowerCaller() { Account account = Mockito.mock(Account.class); Mockito.when(account.getRoleId()).thenReturn(1L); Role newRole = Mockito.mock(Role.class);