Skip to content

Commit 8639ba8

Browse files
shwstpprDaanHoogland
shwstppr
and
DaanHoogland
authored
server: simplify role change validation (#9173)
Signed-off-by: Abhishek Kumar <[email protected]> Co-authored-by: dahn <[email protected]>
1 parent a278849 commit 8639ba8

File tree

2 files changed

+145
-14
lines changed

2 files changed

+145
-14
lines changed

‎server/src/main/java/com/cloud/user/AccountManagerImpl.java‎

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,6 @@
120120
import com.cloud.network.IpAddressManager;
121121
import com.cloud.network.Network;
122122
import com.cloud.network.NetworkModel;
123-
import com.cloud.network.security.SecurityGroupService;
124-
import com.cloud.network.security.SecurityGroupVO;
125123
import com.cloud.network.VpnUserVO;
126124
import com.cloud.network.as.AutoScaleManager;
127125
import com.cloud.network.dao.AccountGuestVlanMapDao;
@@ -135,6 +133,8 @@
135133
import com.cloud.network.dao.VpnUserDao;
136134
import com.cloud.network.router.VirtualRouter;
137135
import com.cloud.network.security.SecurityGroupManager;
136+
import com.cloud.network.security.SecurityGroupService;
137+
import com.cloud.network.security.SecurityGroupVO;
138138
import com.cloud.network.security.dao.SecurityGroupDao;
139139
import com.cloud.network.vpc.Vpc;
140140
import com.cloud.network.vpc.VpcManager;
@@ -1301,16 +1301,33 @@ public Pair<Long, Account> doInTransaction(TransactionStatus status) {
13011301
return _userAccountDao.findById(userId);
13021302
}
13031303

1304-
private boolean isValidRoleChange(Account account, Role role) {
1305-
Long currentAccRoleId = account.getRoleId();
1306-
Role currentRole = roleService.findRole(currentAccRoleId);
1307-
1308-
if (role.getRoleType().ordinal() < currentRole.getRoleType().ordinal() && ((account.getType() == Account.Type.NORMAL && role.getRoleType().getAccountType().ordinal() > Account.Type.NORMAL.ordinal()) ||
1309-
account.getType().ordinal() > Account.Type.NORMAL.ordinal() && role.getRoleType().getAccountType().ordinal() < account.getType().ordinal() && role.getRoleType().getAccountType().ordinal() > 0)) {
1310-
throw new PermissionDeniedException(String.format("Unable to update account role to %s as you are " +
1311-
"attempting to escalate the account %s to account type %s which has higher privileges", role.getName(), account.getAccountName(), role.getRoleType().getAccountType().name()));
1304+
/*
1305+
Role change should follow the below conditions:
1306+
- Caller should not be of Unknown role type
1307+
- New role's type should not be Unknown
1308+
- Caller should not be able to escalate or de-escalate an account's role which is of higher role type
1309+
- New role should not be of type Admin with domain other than ROOT domain
1310+
*/
1311+
protected void validateRoleChange(Account account, Role role, Account caller) {
1312+
Role currentRole = roleService.findRole(account.getRoleId());
1313+
Role callerRole = roleService.findRole(caller.getRoleId());
1314+
String errorMsg = String.format("Unable to update account role to %s, ", role.getName());
1315+
if (RoleType.Unknown.equals(callerRole.getRoleType())) {
1316+
throw new PermissionDeniedException(String.format("%s as the caller privileges are unknown", errorMsg));
1317+
}
1318+
if (RoleType.Unknown.equals(role.getRoleType())) {
1319+
throw new PermissionDeniedException(String.format("%s as the new role privileges are unknown", errorMsg));
1320+
}
1321+
if (!callerRole.getRoleType().equals(RoleType.Admin) &&
1322+
(role.getRoleType().ordinal() < callerRole.getRoleType().ordinal() ||
1323+
currentRole.getRoleType().ordinal() < callerRole.getRoleType().ordinal())) {
1324+
throw new PermissionDeniedException(String.format("%s as either current or new role has higher " +
1325+
"privileges than the caller", errorMsg));
1326+
}
1327+
if (role.getRoleType().equals(RoleType.Admin) && account.getDomainId() != Domain.ROOT_DOMAIN) {
1328+
throw new PermissionDeniedException(String.format("%s as the user does not belong to the ROOT domain",
1329+
errorMsg));
13121330
}
1313-
return true;
13141331
}
13151332

13161333
/**
@@ -2053,7 +2070,7 @@ public AccountVO updateAccount(UpdateAccountCmd cmd) {
20532070
}
20542071

20552072
Role role = roleService.findRole(roleId);
2056-
isValidRoleChange(account, role);
2073+
validateRoleChange(account, role, caller);
20572074
acctForUpdate.setRoleId(roleId);
20582075
acctForUpdate.setType(role.getRoleType().getAccountType());
20592076
checkRoleEscalation(getCurrentCallingAccount(), acctForUpdate);

‎server/src/test/java/com/cloud/user/AccountManagerImplTest.java‎

Lines changed: 116 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,20 @@
1616
// under the License.
1717
package com.cloud.user;
1818

19+
import static org.mockito.ArgumentMatchers.nullable;
20+
1921
import java.net.InetAddress;
2022
import java.net.UnknownHostException;
2123
import java.util.ArrayList;
2224
import java.util.Arrays;
25+
import java.util.HashMap;
2326
import java.util.List;
2427
import java.util.Map;
25-
import java.util.HashMap;
2628

2729
import org.apache.cloudstack.acl.ControlledEntity;
30+
import org.apache.cloudstack.acl.Role;
31+
import org.apache.cloudstack.acl.RoleService;
32+
import org.apache.cloudstack.acl.RoleType;
2833
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
2934
import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd;
3035
import org.apache.cloudstack.api.command.admin.user.UpdateUserCmd;
@@ -42,7 +47,6 @@
4247
import org.mockito.Mock;
4348
import org.mockito.Mockito;
4449
import org.mockito.junit.MockitoJUnitRunner;
45-
import static org.mockito.ArgumentMatchers.nullable;
4650

4751
import com.cloud.acl.DomainChecker;
4852
import com.cloud.api.auth.SetupUserTwoFactorAuthenticationCmd;
@@ -102,6 +106,8 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase {
102106

103107
@Mock
104108
ConfigKey<Boolean> enableUserTwoFactorAuthenticationMock;
109+
@Mock
110+
RoleService roleService;
105111

106112
@Before
107113
public void setUp() throws Exception {
@@ -1086,4 +1092,112 @@ public void deleteAndCleanupUserTestRemovesUserFromProjects() {
10861092

10871093
Mockito.verify(_projectAccountDao).removeUserFromProjects(userId);
10881094
}
1095+
1096+
@Test(expected = PermissionDeniedException.class)
1097+
public void testValidateRoleChangeUnknownCaller() {
1098+
Account account = Mockito.mock(Account.class);
1099+
Mockito.when(account.getRoleId()).thenReturn(1L);
1100+
Role role = Mockito.mock(Role.class);
1101+
Mockito.when(role.getRoleType()).thenReturn(RoleType.Unknown);
1102+
Account caller = Mockito.mock(Account.class);
1103+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1104+
Mockito.when(roleService.findRole(2L)).thenReturn(role);
1105+
accountManagerImpl.validateRoleChange(account, Mockito.mock(Role.class), caller);
1106+
}
1107+
1108+
@Test(expected = PermissionDeniedException.class)
1109+
public void testValidateRoleChangeUnknownNewRole() {
1110+
Account account = Mockito.mock(Account.class);
1111+
Mockito.when(account.getRoleId()).thenReturn(1L);
1112+
Role newRole = Mockito.mock(Role.class);
1113+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.Unknown);
1114+
Role callerRole = Mockito.mock(Role.class);
1115+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1116+
Account caller = Mockito.mock(Account.class);
1117+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1118+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1119+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1120+
}
1121+
1122+
@Test
1123+
public void testValidateRoleNewRoleSameCaller() {
1124+
Account account = Mockito.mock(Account.class);
1125+
Mockito.when(account.getRoleId()).thenReturn(1L);
1126+
Role currentRole = Mockito.mock(Role.class);
1127+
Mockito.when(currentRole.getRoleType()).thenReturn(RoleType.User);
1128+
Mockito.when(roleService.findRole(1L)).thenReturn(currentRole);
1129+
Role newRole = Mockito.mock(Role.class);
1130+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1131+
Role callerRole = Mockito.mock(Role.class);
1132+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1133+
Account caller = Mockito.mock(Account.class);
1134+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1135+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1136+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1137+
}
1138+
1139+
@Test
1140+
public void testValidateRoleCurrentRoleSameCaller() {
1141+
Account account = Mockito.mock(Account.class);
1142+
Mockito.when(account.getRoleId()).thenReturn(1L);
1143+
Role accountRole = Mockito.mock(Role.class);
1144+
Mockito.when(accountRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1145+
Role newRole = Mockito.mock(Role.class);
1146+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.User);
1147+
Role callerRole = Mockito.mock(Role.class);
1148+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1149+
Account caller = Mockito.mock(Account.class);
1150+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1151+
Mockito.when(roleService.findRole(1L)).thenReturn(accountRole);
1152+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1153+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1154+
}
1155+
1156+
@Test(expected = PermissionDeniedException.class)
1157+
public void testValidateRoleNewRoleHigherCaller() {
1158+
Account account = Mockito.mock(Account.class);
1159+
Mockito.when(account.getRoleId()).thenReturn(1L);
1160+
Role newRole = Mockito.mock(Role.class);
1161+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.Admin);
1162+
Role callerRole = Mockito.mock(Role.class);
1163+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1164+
Account caller = Mockito.mock(Account.class);
1165+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1166+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1167+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1168+
}
1169+
1170+
@Test
1171+
public void testValidateRoleNewRoleLowerCaller() {
1172+
Account account = Mockito.mock(Account.class);
1173+
Mockito.when(account.getRoleId()).thenReturn(1L);
1174+
Role newRole = Mockito.mock(Role.class);
1175+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.User);
1176+
Role accountRole = Mockito.mock(Role.class);
1177+
Mockito.when(accountRole.getRoleType()).thenReturn(RoleType.User);
1178+
Role callerRole = Mockito.mock(Role.class);
1179+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1180+
Account caller = Mockito.mock(Account.class);
1181+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1182+
Mockito.when(roleService.findRole(1L)).thenReturn(accountRole);
1183+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1184+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1185+
}
1186+
1187+
@Test(expected = PermissionDeniedException.class)
1188+
public void testValidateRoleAdminCannotEscalateAdminFromNonRootDomain() {
1189+
Account account = Mockito.mock(Account.class);
1190+
Mockito.when(account.getRoleId()).thenReturn(1L);
1191+
Mockito.when(account.getDomainId()).thenReturn(2L);
1192+
Role newRole = Mockito.mock(Role.class);
1193+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.Admin);
1194+
Role accountRole = Mockito.mock(Role.class);
1195+
Role callerRole = Mockito.mock(Role.class);
1196+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.Admin);
1197+
Account caller = Mockito.mock(Account.class);
1198+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1199+
Mockito.when(roleService.findRole(1L)).thenReturn(accountRole);
1200+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1201+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1202+
}
10891203
}

0 commit comments

Comments
 (0)