Skip to content

Commit a5c1c71

Browse files
bernardodemarcobernardodemarco
committed
consider key pairs when expunging VMs
1 parent 212d881 commit a5c1c71

File tree

5 files changed

+37
-17
lines changed

5 files changed

+37
-17
lines changed

‎plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -589,7 +589,7 @@ public void validateUserPasswordAndUpdateIfNeeded(String newPassword, UserVO use
589589
}
590590

591591
@Override
592-
public void checkApiAccess(Account account, String command) throws PermissionDeniedException {
592+
public void checkApiAccess(Account account, String command, String apiKey) throws PermissionDeniedException {
593593
}
594594

595595
@Override

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ void buildACLViewSearchCriteria(SearchCriteria<? extends ControlledViewEntity> s
202202

203203
void validateUserPasswordAndUpdateIfNeeded(String newPassword, UserVO user, String currentPassword, boolean skipCurrentPassValidation);
204204

205-
void checkApiAccess(Account caller, String command);
205+
void checkApiAccess(Account caller, String command, String apiKey);
206206

207207
UserAccount clearUserTwoFactorAuthenticationInSetupStateOnLogin(UserAccount user);
208208

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

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1512,16 +1512,25 @@ protected void checkRoleEscalation(Account caller, Account requested) {
15121512
}
15131513
}
15141514

1515-
private void checkApiAccess(List<APIChecker> apiCheckers, Account caller, String command) {
1515+
private void checkApiAccess(List<APIChecker> apiCheckers, Account caller, String command, ApiKeyPairPermission... apiKeyPairPermissions) {
15161516
for (final APIChecker apiChecker : apiCheckers) {
1517-
apiChecker.checkAccess(caller, command);
1517+
apiChecker.checkAccess(caller, command, apiKeyPairPermissions);
15181518
}
15191519
}
15201520

15211521
@Override
1522-
public void checkApiAccess(Account caller, String command) {
1522+
public void checkApiAccess(Account caller, String command, String apiKey) {
15231523
List<APIChecker> apiCheckers = getEnabledApiCheckers();
1524-
checkApiAccess(apiCheckers, caller, command);
1524+
1525+
List<ApiKeyPairPermission> keyPairPermissions = new ArrayList<>();
1526+
if (apiKey != null) {
1527+
Ternary<User, Account, ApiKeyPair> keyPairTernary = findUserByApiKey(apiKey);
1528+
if (keyPairTernary != null) {
1529+
keyPairPermissions = keyPairManager.findAllPermissionsByKeyPairId(keyPairTernary.third().getId(), caller.getRoleId());
1530+
}
1531+
}
1532+
1533+
checkApiAccess(apiCheckers, caller, command, keyPairPermissions.toArray(new ApiKeyPairPermission[0]));
15251534
}
15261535

15271536
@NotNull

‎server/src/main/java/com/cloud/vm/UserVmManagerImpl.java‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
import javax.xml.parsers.DocumentBuilder;
6161
import javax.xml.parsers.ParserConfigurationException;
6262

63+
import com.cloud.serializer.GsonHelper;
6364
import com.cloud.storage.SnapshotPolicyVO;
6465
import com.cloud.storage.dao.SnapshotPolicyDao;
6566
import org.apache.cloudstack.acl.ControlledEntity;
@@ -132,6 +133,7 @@
132133
import org.apache.cloudstack.framework.config.ConfigKey;
133134
import org.apache.cloudstack.framework.config.Configurable;
134135
import org.apache.cloudstack.framework.config.dao.ConfigurationDao;
136+
import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO;
135137
import org.apache.cloudstack.framework.messagebus.MessageBus;
136138
import org.apache.cloudstack.framework.messagebus.PublishScope;
137139
import org.apache.cloudstack.managed.context.ManagedContextRunnable;
@@ -651,6 +653,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
651653

652654
protected static long ROOT_DEVICE_ID = 0;
653655

656+
private final Type jobParamsType = new TypeToken<HashMap<String, String>>() {}.getType();
657+
654658
public List<KubernetesServiceHelper> getKubernetesServiceHelpers() {
655659
return kubernetesServiceHelpers;
656660
}
@@ -3459,14 +3463,14 @@ protected boolean getConfigAllowUserExpungeRecoverVm(Long accountId) {
34593463
return AllowUserExpungeRecoverVm.valueIn(accountId);
34603464
}
34613465

3462-
protected void checkExpungeVmPermission (Account callingAccount) {
3466+
protected void checkExpungeVmPermission(Account callingAccount, String apiKey) {
34633467
logger.debug(String.format("Checking if [%s] has permission for expunging VMs.", callingAccount));
34643468
if (!_accountMgr.isAdmin(callingAccount.getId()) && !getConfigAllowUserExpungeRecoverVm(callingAccount.getId())) {
34653469
logger.error(String.format("Parameter [%s] can only be passed by Admin accounts or when the allow.user.expunge.recover.vm key is true.", ApiConstants.EXPUNGE));
34663470
throw new PermissionDeniedException("Account does not have permission for expunging.");
34673471
}
34683472
try {
3469-
_accountMgr.checkApiAccess(callingAccount, BaseCmd.getCommandNameByClass(ExpungeVMCmd.class));
3473+
_accountMgr.checkApiAccess(callingAccount, BaseCmd.getCommandNameByClass(ExpungeVMCmd.class), apiKey);
34703474
} catch (PermissionDeniedException ex) {
34713475
logger.error(String.format("Role [%s] of [%s] does not have permission for expunging VMs.", callingAccount.getRoleId(), callingAccount));
34723476
throw new PermissionDeniedException("Account does not have permission for expunging.");
@@ -3491,7 +3495,10 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C
34913495
boolean expunge = cmd.getExpunge();
34923496

34933497
if (expunge) {
3494-
checkExpungeVmPermission(ctx.getCallingAccount());
3498+
String jobParamsString = ((AsyncJobVO) cmd.getJob()).getCmdInfo();
3499+
HashMap<String,String> jobParams = GsonHelper.getGson().fromJson(jobParamsString, jobParamsType);
3500+
String apiKey = jobParams.get("apiKey");
3501+
checkExpungeVmPermission(ctx.getCallingAccount(), apiKey);
34953502
}
34963503

34973504
// check if VM exists

‎server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@
8989
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
9090
import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
9191
import org.apache.cloudstack.framework.config.ConfigKey;
92+
import org.apache.cloudstack.framework.jobs.impl.AsyncJobVO;
9293
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
9394
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
9495
import org.apache.cloudstack.storage.template.VnfTemplateManager;
@@ -1823,35 +1824,35 @@ public void checkExpungeVMPermissionTestAccountIsNotAdminConfigFalseThrowsPermis
18231824
Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong());
18241825
Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong());
18251826

1826-
Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock));
1827+
Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock, null));
18271828
}
18281829
@Test
18291830
public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueNoApiAccessThrowsPermissionDeniedException () {
18301831
Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong());
18311832
Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong());
1832-
doThrow(PermissionDeniedException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine");
1833+
doThrow(PermissionDeniedException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine", null);
18331834

1834-
Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock));
1835+
Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock, null));
18351836
}
18361837
@Test
18371838
public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueHasApiAccessReturnNothing () {
18381839
Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong());
18391840
Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong());
18401841

1841-
userVmManagerImpl.checkExpungeVmPermission(accountMock);
1842+
userVmManagerImpl.checkExpungeVmPermission(accountMock, null);
18421843
}
18431844
@Test
18441845
public void checkExpungeVmPermissionTestAccountIsAdminNoApiAccessThrowsPermissionDeniedException () {
18451846
Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong());
1846-
doThrow(PermissionDeniedException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine");
1847+
doThrow(PermissionDeniedException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine", null);
18471848

1848-
Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock));
1849+
Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock, null));
18491850
}
18501851
@Test
18511852
public void checkExpungeVmPermissionTestAccountIsAdminHasApiAccessReturnNothing () {
18521853
Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong());
18531854

1854-
userVmManagerImpl.checkExpungeVmPermission(accountMock);
1855+
userVmManagerImpl.checkExpungeVmPermission(accountMock, null);
18551856
}
18561857

18571858
@Test
@@ -3661,7 +3662,7 @@ public void testDestroyVm() throws ResourceUnavailableException {
36613662
when(callingAccount.getId()).thenReturn(accountId);
36623663
when(callContext.getCallingAccount()).thenReturn(callingAccount);
36633664
when(accountManager.isAdmin(callingAccount.getId())).thenReturn(true);
3664-
doNothing().when(accountManager).checkApiAccess(callingAccount, BaseCmd.getCommandNameByClass(ExpungeVMCmd.class));
3665+
doNothing().when(accountManager).checkApiAccess(callingAccount, BaseCmd.getCommandNameByClass(ExpungeVMCmd.class), null);
36653666
try (MockedStatic<CallContext> mockedCallContext = mockStatic(CallContext.class)) {
36663667
mockedCallContext.when(CallContext::current).thenReturn(callContext);
36673668
mockedCallContext.when(() -> CallContext.register(callContext, ApiCommandResourceType.Volume)).thenReturn(callContext);
@@ -3671,6 +3672,9 @@ public void testDestroyVm() throws ResourceUnavailableException {
36713672
when(cmd.getExpunge()).thenReturn(expunge);
36723673
List<Long> volumeIds = List.of(volumeId);
36733674
when(cmd.getVolumeIds()).thenReturn(volumeIds);
3675+
AsyncJobVO asyncJobMock = mock(AsyncJobVO.class);
3676+
when(cmd.getJob()).thenReturn(asyncJobMock);
3677+
when(asyncJobMock.getCmdInfo()).thenReturn("{}");
36743678

36753679
UserVmVO vm = mock(UserVmVO.class);
36763680
when(vm.getId()).thenReturn(vmId);

0 commit comments

Comments
 (0)