Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: simplify role change validation #9173

Merged
merged 9 commits into from
Dec 14, 2024
Merged

server: simplify role change validation #9173

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e781150
server: simplify role change validation
shwstppr Jun 5, 2024
1064cb7
fix build failures
shwstppr Jun 5, 2024
ccb30ca
refactor error message
shwstppr Jun 5, 2024
1bd9766
Update server/src/main/java/com/cloud/user/AccountManagerImpl.java
shwstppr Jun 5, 2024
de8a1c2
Merge branch '4.19' into simplify-role-change-check
shwstppr Jul 22, 2024
3ccd65d
changes
shwstppr Aug 22, 2024
140445c
Merge remote-tracking branch 'apache/4.19' into simplify-role-change-…
shwstppr Aug 22, 2024
bc1b2eb
Merge remote-tracking branch 'apache/4.19' into simplify-role-change-…
shwstppr Dec 4, 2024
690188c
Merge remote-tracking branch 'apache/4.19' into simplify-role-change-…
shwstppr Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 29 additions & 12 deletions server/src/main/java/com/cloud/user/AccountManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -1287,16 +1287,33 @@
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
shwstppr marked this conversation as resolved.
Show resolved Hide resolved
*/
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());
shwstppr marked this conversation as resolved.
Show resolved Hide resolved
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;
}

/**
Expand Down Expand Up @@ -2031,7 +2048,7 @@
}

Role role = roleService.findRole(roleId);
isValidRoleChange(account, role);
validateRoleChange(account, role, caller);

Check warning on line 2051 in server/src/main/java/com/cloud/user/AccountManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/user/AccountManagerImpl.java#L2051 

Added line #L2051 was not covered by tests
acctForUpdate.setRoleId(roleId);
acctForUpdate.setType(role.getRoleType().getAccountType());
checkRoleEscalation(getCurrentCallingAccount(), acctForUpdate);
Expand Down
163 changes: 137 additions & 26 deletions server/src/test/java/com/cloud/user/AccountManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,19 @@
// 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;
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;
Expand All @@ -51,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 {
Expand Down Expand Up @@ -101,6 +105,8 @@ public class AccountManagerImplTest extends AccountManagetImplTestBase {

@Mock
ConfigKey<Boolean> enableUserTwoFactorAuthenticationMock;
@Mock
RoleService roleService;

@Before
public void setUp() throws Exception {
Expand Down Expand Up @@ -988,4 +994,109 @@ 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);
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);
}
}
Loading