Skip to content

Commit 92ff36d

Browse files
shwstpprDaanHoogland
authored and
dhslove
committed
server: simplify role change validation (apache#9173)
Signed-off-by: Abhishek Kumar <[email protected]> Co-authored-by: dahn <[email protected]>
1 parent 4ad57c9 commit 92ff36d

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
@@ -124,8 +124,6 @@
124124
import com.cloud.network.IpAddressManager;
125125
import com.cloud.network.Network;
126126
import com.cloud.network.NetworkModel;
127-
import com.cloud.network.security.SecurityGroupService;
128-
import com.cloud.network.security.SecurityGroupVO;
129127
import com.cloud.network.VpnUserVO;
130128
import com.cloud.network.as.AutoScaleManager;
131129
import com.cloud.network.dao.AccountGuestVlanMapDao;
@@ -139,6 +137,8 @@
139137
import com.cloud.network.dao.VpnUserDao;
140138
import com.cloud.network.router.VirtualRouter;
141139
import com.cloud.network.security.SecurityGroupManager;
140+
import com.cloud.network.security.SecurityGroupService;
141+
import com.cloud.network.security.SecurityGroupVO;
142142
import com.cloud.network.security.dao.SecurityGroupDao;
143143
import com.cloud.network.vpc.Vpc;
144144
import com.cloud.network.vpc.VpcManager;
@@ -1345,16 +1345,33 @@ public Pair<Long, Account> doInTransaction(TransactionStatus status) {
13451345
return _userAccountDao.findById(userId);
13461346
}
13471347

1348-
private boolean isValidRoleChange(Account account, Role role) {
1349-
Long currentAccRoleId = account.getRoleId();
1350-
Role currentRole = roleService.findRole(currentAccRoleId);
1351-
1352-
if (role.getRoleType().ordinal() < currentRole.getRoleType().ordinal() && ((account.getType() == Account.Type.NORMAL && role.getRoleType().getAccountType().ordinal() > Account.Type.NORMAL.ordinal()) ||
1353-
account.getType().ordinal() > Account.Type.NORMAL.ordinal() && role.getRoleType().getAccountType().ordinal() < account.getType().ordinal() && role.getRoleType().getAccountType().ordinal() > 0)) {
1354-
throw new PermissionDeniedException(String.format("Unable to update account role to %s as you are " +
1355-
"attempting to escalate the account %s to account type %s which has higher privileges", role.getName(), account.getAccountName(), role.getRoleType().getAccountType().name()));
1348+
/*
1349+
Role change should follow the below conditions:
1350+
- Caller should not be of Unknown role type
1351+
- New role's type should not be Unknown
1352+
- Caller should not be able to escalate or de-escalate an account's role which is of higher role type
1353+
- New role should not be of type Admin with domain other than ROOT domain
1354+
*/
1355+
protected void validateRoleChange(Account account, Role role, Account caller) {
1356+
Role currentRole = roleService.findRole(account.getRoleId());
1357+
Role callerRole = roleService.findRole(caller.getRoleId());
1358+
String errorMsg = String.format("Unable to update account role to %s, ", role.getName());
1359+
if (RoleType.Unknown.equals(callerRole.getRoleType())) {
1360+
throw new PermissionDeniedException(String.format("%s as the caller privileges are unknown", errorMsg));
1361+
}
1362+
if (RoleType.Unknown.equals(role.getRoleType())) {
1363+
throw new PermissionDeniedException(String.format("%s as the new role privileges are unknown", errorMsg));
1364+
}
1365+
if (!callerRole.getRoleType().equals(RoleType.Admin) &&
1366+
(role.getRoleType().ordinal() < callerRole.getRoleType().ordinal() ||
1367+
currentRole.getRoleType().ordinal() < callerRole.getRoleType().ordinal())) {
1368+
throw new PermissionDeniedException(String.format("%s as either current or new role has higher " +
1369+
"privileges than the caller", errorMsg));
1370+
}
1371+
if (role.getRoleType().equals(RoleType.Admin) && account.getDomainId() != Domain.ROOT_DOMAIN) {
1372+
throw new PermissionDeniedException(String.format("%s as the user does not belong to the ROOT domain",
1373+
errorMsg));
13561374
}
1357-
return true;
13581375
}
13591376

13601377
/**
@@ -2166,7 +2183,7 @@ public AccountVO updateAccount(UpdateAccountCmd cmd) {
21662183
"in the domain '" + domainId + "'.");
21672184
}
21682185
Role role = roleService.findRole(roleId);
2169-
isValidRoleChange(account, role);
2186+
validateRoleChange(account, role, caller);
21702187
acctForUpdate.setRoleId(roleId);
21712188
acctForUpdate.setType(role.getRoleType().getAccountType());
21722189
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
@@ -27,11 +27,14 @@
2727
import java.util.Map;
2828

2929
import com.cloud.event.ActionEventUtils;
30+
31+
import org.apache.cloudstack.acl.ControlledEntity;
32+
import org.apache.cloudstack.acl.Role;
33+
import org.apache.cloudstack.acl.RoleService;
34+
import org.apache.cloudstack.acl.RoleType;
3035
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
3136
import org.apache.cloudstack.api.command.admin.account.UpdateAccountCmd;
3237
import org.apache.cloudstack.api.command.admin.user.DeleteUserCmd;
33-
34-
import org.apache.cloudstack.acl.ControlledEntity;
3538
import org.apache.cloudstack.api.command.admin.user.GetUserKeysCmd;
3639
import org.apache.cloudstack.api.command.admin.user.UpdateUserCmd;
3740
import org.apache.cloudstack.api.response.UserTwoFactorAuthenticationSetupResponse;
@@ -50,6 +53,7 @@
5053
import org.mockito.MockedStatic;
5154
import org.mockito.Mockito;
5255
import org.mockito.junit.MockitoJUnitRunner;
56+
5357
import org.springframework.beans.factory.NoSuchBeanDefinitionException;
5458

5559
import com.cloud.acl.DomainChecker;
@@ -120,6 +124,8 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase {
120124

121125
@Mock
122126
ConfigKey<Boolean> enableUserTwoFactorAuthenticationMock;
127+
@Mock
128+
RoleService roleService;
123129

124130
@Before
125131
public void setUp() throws Exception {
@@ -1225,4 +1231,112 @@ public void testDeleteWebhooksForAccountNoBean() {
12251231
accountManagerImpl.deleteWebhooksForAccount(1L);
12261232
}
12271233
}
1234+
1235+
@Test(expected = PermissionDeniedException.class)
1236+
public void testValidateRoleChangeUnknownCaller() {
1237+
Account account = Mockito.mock(Account.class);
1238+
Mockito.when(account.getRoleId()).thenReturn(1L);
1239+
Role role = Mockito.mock(Role.class);
1240+
Mockito.when(role.getRoleType()).thenReturn(RoleType.Unknown);
1241+
Account caller = Mockito.mock(Account.class);
1242+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1243+
Mockito.when(roleService.findRole(2L)).thenReturn(role);
1244+
accountManagerImpl.validateRoleChange(account, Mockito.mock(Role.class), caller);
1245+
}
1246+
1247+
@Test(expected = PermissionDeniedException.class)
1248+
public void testValidateRoleChangeUnknownNewRole() {
1249+
Account account = Mockito.mock(Account.class);
1250+
Mockito.when(account.getRoleId()).thenReturn(1L);
1251+
Role newRole = Mockito.mock(Role.class);
1252+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.Unknown);
1253+
Role callerRole = Mockito.mock(Role.class);
1254+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1255+
Account caller = Mockito.mock(Account.class);
1256+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1257+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1258+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1259+
}
1260+
1261+
@Test
1262+
public void testValidateRoleNewRoleSameCaller() {
1263+
Account account = Mockito.mock(Account.class);
1264+
Mockito.when(account.getRoleId()).thenReturn(1L);
1265+
Role currentRole = Mockito.mock(Role.class);
1266+
Mockito.when(currentRole.getRoleType()).thenReturn(RoleType.User);
1267+
Mockito.when(roleService.findRole(1L)).thenReturn(currentRole);
1268+
Role newRole = Mockito.mock(Role.class);
1269+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1270+
Role callerRole = Mockito.mock(Role.class);
1271+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1272+
Account caller = Mockito.mock(Account.class);
1273+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1274+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1275+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1276+
}
1277+
1278+
@Test
1279+
public void testValidateRoleCurrentRoleSameCaller() {
1280+
Account account = Mockito.mock(Account.class);
1281+
Mockito.when(account.getRoleId()).thenReturn(1L);
1282+
Role accountRole = Mockito.mock(Role.class);
1283+
Mockito.when(accountRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1284+
Role newRole = Mockito.mock(Role.class);
1285+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.User);
1286+
Role callerRole = Mockito.mock(Role.class);
1287+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1288+
Account caller = Mockito.mock(Account.class);
1289+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1290+
Mockito.when(roleService.findRole(1L)).thenReturn(accountRole);
1291+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1292+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1293+
}
1294+
1295+
@Test(expected = PermissionDeniedException.class)
1296+
public void testValidateRoleNewRoleHigherCaller() {
1297+
Account account = Mockito.mock(Account.class);
1298+
Mockito.when(account.getRoleId()).thenReturn(1L);
1299+
Role newRole = Mockito.mock(Role.class);
1300+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.Admin);
1301+
Role callerRole = Mockito.mock(Role.class);
1302+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1303+
Account caller = Mockito.mock(Account.class);
1304+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1305+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1306+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1307+
}
1308+
1309+
@Test
1310+
public void testValidateRoleNewRoleLowerCaller() {
1311+
Account account = Mockito.mock(Account.class);
1312+
Mockito.when(account.getRoleId()).thenReturn(1L);
1313+
Role newRole = Mockito.mock(Role.class);
1314+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.User);
1315+
Role accountRole = Mockito.mock(Role.class);
1316+
Mockito.when(accountRole.getRoleType()).thenReturn(RoleType.User);
1317+
Role callerRole = Mockito.mock(Role.class);
1318+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.DomainAdmin);
1319+
Account caller = Mockito.mock(Account.class);
1320+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1321+
Mockito.when(roleService.findRole(1L)).thenReturn(accountRole);
1322+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1323+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1324+
}
1325+
1326+
@Test(expected = PermissionDeniedException.class)
1327+
public void testValidateRoleAdminCannotEscalateAdminFromNonRootDomain() {
1328+
Account account = Mockito.mock(Account.class);
1329+
Mockito.when(account.getRoleId()).thenReturn(1L);
1330+
Mockito.when(account.getDomainId()).thenReturn(2L);
1331+
Role newRole = Mockito.mock(Role.class);
1332+
Mockito.when(newRole.getRoleType()).thenReturn(RoleType.Admin);
1333+
Role accountRole = Mockito.mock(Role.class);
1334+
Role callerRole = Mockito.mock(Role.class);
1335+
Mockito.when(callerRole.getRoleType()).thenReturn(RoleType.Admin);
1336+
Account caller = Mockito.mock(Account.class);
1337+
Mockito.when(caller.getRoleId()).thenReturn(2L);
1338+
Mockito.when(roleService.findRole(1L)).thenReturn(accountRole);
1339+
Mockito.when(roleService.findRole(2L)).thenReturn(callerRole);
1340+
accountManagerImpl.validateRoleChange(account, newRole, caller);
1341+
}
12281342
}

0 commit comments

Comments
 (0)