This is an automated email from the ASF dual-hosted git repository.
dahn pushed a commit to branch 4.19
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.19 by this push:
new 8639ba8b013 server: simplify role change validation (#9173)
8639ba8b013 is described below
commit 8639ba8b013213faff6451a61f54467615f51389
Author: Abhishek Kumar <[email protected]>
AuthorDate: Sun Dec 15 00:56:32 2024 +0530
server: simplify role change validation (#9173)
Signed-off-by: Abhishek Kumar <[email protected]>
Co-authored-by: dahn <[email protected]>
---
.../java/com/cloud/user/AccountManagerImpl.java | 41 ++++---
.../com/cloud/user/AccountManagerImplTest.java | 118 ++++++++++++++++++++-
2 files changed, 145 insertions(+), 14 deletions(-)
diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java
b/server/src/main/java/com/cloud/user/AccountManagerImpl.java
index c21d8b830dd..2d7ebf595fd 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.IpAddress;
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.RemoteAccessVpnVO;
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 class AccountManagerImpl extends ManagerBase
implements AccountManager, M
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;
}
/**
@@ -2053,7 +2070,7 @@ public class AccountManagerImpl extends ManagerBase
implements AccountManager, M
}
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 a98d187b5a9..ed0a123d4a3 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.InOrder;
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<Boolean> enableUserTwoFactorAuthenticationMock;
+ @Mock
+ RoleService roleService;
@Before
public void setUp() throws Exception {
@@ -1086,4 +1092,112 @@ public class AccountManagerImplTest extends
AccountManagetImplTestBase {
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);
+ }
}
