From f84e04372c98f82667214ab53f566c2e5f17af68 Mon Sep 17 00:00:00 2001 From: Gabriel Pordeus Santos Date: Tue, 20 Aug 2024 07:02:04 -0300 Subject: [PATCH] Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow (#8689) --- .../management/MockAccountManager.java | 5 + .../java/com/cloud/user/AccountManager.java | 2 + .../com/cloud/user/AccountManagerImpl.java | 6 + .../java/com/cloud/vm/UserVmManagerImpl.java | 29 ++++- .../cloud/user/MockAccountManagerImpl.java | 4 + .../com/cloud/vm/UserVmManagerImplTest.java | 36 ++++++ test/integration/smoke/test_vm_life_cycle.py | 114 +++++++++++++++--- ui/src/views/compute/DestroyVM.vue | 2 +- 8 files changed, 177 insertions(+), 21 deletions(-) diff --git a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java index 610f4aa82aa..5d2efa0dc9a 100644 --- a/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java +++ b/plugins/network-elements/juniper-contrail/src/test/java/org/apache/cloudstack/network/contrail/management/MockAccountManager.java @@ -514,4 +514,9 @@ public class MockAccountManager extends ManagerBase implements AccountManager { public ConfigKey[] getConfigKeys() { return null; } + + @Override + public void checkApiAccess(Account account, String command) throws PermissionDeniedException { + + } } diff --git a/server/src/main/java/com/cloud/user/AccountManager.java b/server/src/main/java/com/cloud/user/AccountManager.java index 6d2d1db5668..72235a808a4 100644 --- a/server/src/main/java/com/cloud/user/AccountManager.java +++ b/server/src/main/java/com/cloud/user/AccountManager.java @@ -199,4 +199,6 @@ public interface AccountManager extends AccountService, Configurable { UserTwoFactorAuthenticationSetupResponse setupUserTwoFactorAuthentication(SetupUserTwoFactorAuthenticationCmd cmd); List getApiNameList(); + + void checkApiAccess(Account caller, String command); } diff --git a/server/src/main/java/com/cloud/user/AccountManagerImpl.java b/server/src/main/java/com/cloud/user/AccountManagerImpl.java index dcb12cbdb74..0552739a05e 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1369,6 +1369,12 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M } } + @Override + public void checkApiAccess(Account caller, String command) { + List apiCheckers = getEnabledApiCheckers(); + checkApiAccess(apiCheckers, caller, command); + } + @NotNull private List getEnabledApiCheckers() { // we are really only interested in the dynamic access checker diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 958b81e8351..bf100c8c0d2 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -65,9 +65,11 @@ import org.apache.cloudstack.annotation.AnnotationService; import org.apache.cloudstack.annotation.dao.AnnotationDao; import org.apache.cloudstack.api.ApiCommandResourceType; import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.BaseCmd; import org.apache.cloudstack.api.BaseCmd.HTTPMethod; import org.apache.cloudstack.api.command.admin.vm.AssignVMCmd; import org.apache.cloudstack.api.command.admin.vm.DeployVMCmdByAdmin; +import org.apache.cloudstack.api.command.admin.vm.ExpungeVMCmd; import org.apache.cloudstack.api.command.admin.vm.RecoverVMCmd; import org.apache.cloudstack.api.command.user.vm.AddNicToVMCmd; import org.apache.cloudstack.api.command.user.vm.DeployVMCmd; @@ -3328,6 +3330,27 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir return null; } + /** + * Encapsulates AllowUserExpungeRecoverVm so we can unit test checkExpungeVmPermission. + */ + protected boolean getConfigAllowUserExpungeRecoverVm(Long accountId) { + return AllowUserExpungeRecoverVm.valueIn(accountId); + } + + protected void checkExpungeVmPermission (Account callingAccount) { + logger.debug(String.format("Checking if [%s] has permission for expunging VMs.", callingAccount)); + if (!_accountMgr.isAdmin(callingAccount.getId()) && !getConfigAllowUserExpungeRecoverVm(callingAccount.getId())) { + 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)); + throw new PermissionDeniedException("Account does not have permission for expunging."); + } + try { + _accountMgr.checkApiAccess(callingAccount, BaseCmd.getCommandNameByClass(ExpungeVMCmd.class)); + } catch (PermissionDeniedException ex) { + logger.error(String.format("Role [%s] of [%s] does not have permission for expunging VMs.", callingAccount.getRoleId(), callingAccount)); + throw new PermissionDeniedException("Account does not have permission for expunging."); + } + } + protected void checkPluginsIfVmCanBeDestroyed(UserVm vm) { try { KubernetesServiceHelper kubernetesServiceHelper = @@ -3345,10 +3368,10 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir long vmId = cmd.getId(); boolean expunge = cmd.getExpunge(); - // When trying to expunge, permission is denied when the caller is not an admin and the AllowUserExpungeRecoverVm is false for the caller. - if (expunge && !_accountMgr.isAdmin(ctx.getCallingAccount().getId()) && !AllowUserExpungeRecoverVm.valueIn(cmd.getEntityOwnerId())) { - throw new PermissionDeniedException("Parameter " + ApiConstants.EXPUNGE + " can be passed by Admin only. Or when the allow.user.expunge.recover.vm key is set."); + if (expunge) { + checkExpungeVmPermission(ctx.getCallingAccount()); } + // check if VM exists UserVmVO vm = _vmDao.findById(vmId); diff --git a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java index 334e1f33481..b4c2dafd664 100644 --- a/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java +++ b/server/src/test/java/com/cloud/user/MockAccountManagerImpl.java @@ -464,6 +464,10 @@ public class MockAccountManagerImpl extends ManagerBase implements Manager, Acco return null; } + @Override + public void checkApiAccess(Account account, String command) throws PermissionDeniedException { + + } @Override public void checkAccess(User user, ControlledEntity entity) throws PermissionDeniedException { diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index 39128f21c87..90d857b5816 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -1595,4 +1595,40 @@ public class UserVmManagerImplTest { Long actualSize = userVmManagerImpl.getRootVolumeSizeForVmRestore(null, template, userVm, diskOffering, details, false); Assert.assertEquals(20 * GiB_TO_BYTES, actualSize.longValue()); } + + @Test + public void checkExpungeVMPermissionTestAccountIsNotAdminConfigFalseThrowsPermissionDeniedException () { + Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong()); + Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); + + Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock)); + } + @Test + public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueNoApiAccessThrowsPermissionDeniedException () { + Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong()); + Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); + Mockito.doThrow(PermissionDeniedException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); + + Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock)); + } + @Test + public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueHasApiAccessReturnNothing () { + Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong()); + Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); + + userVmManagerImpl.checkExpungeVmPermission(accountMock); + } + @Test + public void checkExpungeVmPermissionTestAccountIsAdminNoApiAccessThrowsPermissionDeniedException () { + Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); + Mockito.doThrow(PermissionDeniedException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); + + Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock)); + } + @Test + public void checkExpungeVmPermissionTestAccountIsAdminHasApiAccessReturnNothing () { + Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); + + userVmManagerImpl.checkExpungeVmPermission(accountMock); + } } diff --git a/test/integration/smoke/test_vm_life_cycle.py b/test/integration/smoke/test_vm_life_cycle.py index aaffa63978a..c05ae2ad42e 100644 --- a/test/integration/smoke/test_vm_life_cycle.py +++ b/test/integration/smoke/test_vm_life_cycle.py @@ -31,6 +31,7 @@ from marvin.cloudstackAPI import (recoverVirtualMachine, from marvin.lib.utils import * from marvin.lib.base import (Account, + Role, ServiceOffering, VirtualMachine, Host, @@ -94,17 +95,21 @@ class TestDeployVM(cloudstackTestCase): cls.services["iso1"]["zoneid"] = cls.zone.id + cls._cleanup = [] + cls.account = Account.create( cls.apiclient, cls.services["account"], domainid=cls.domain.id ) + cls._cleanup.append(cls.account) cls.debug(cls.account.id) cls.service_offering = ServiceOffering.create( cls.apiclient, cls.services["service_offerings"]["tiny"] ) + cls._cleanup.append(cls.service_offering) cls.virtual_machine = VirtualMachine.create( cls.apiclient, @@ -115,17 +120,9 @@ class TestDeployVM(cloudstackTestCase): mode=cls.services['mode'] ) - cls.cleanup = [ - cls.service_offering, - cls.account - ] - @classmethod def tearDownClass(cls): - try: - cleanup_resources(cls.apiclient, cls.cleanup) - except Exception as e: - raise Exception("Warning: Exception during cleanup : %s" % e) + super(TestDeployVM, cls).tearDownClass() def setUp(self): self.apiclient = self.testClient.getApiClient() @@ -262,11 +259,7 @@ class TestDeployVM(cloudstackTestCase): ) def tearDown(self): - try: - # Clean up, terminate the created instance, volumes and snapshots - cleanup_resources(self.apiclient, self.cleanup) - except Exception as e: - raise Exception("Warning: Exception during cleanup : %s" % e) + super(TestDeployVM, self).tearDown() class TestVMLifeCycle(cloudstackTestCase): @@ -279,7 +272,7 @@ class TestVMLifeCycle(cloudstackTestCase): cls.hypervisor = testClient.getHypervisorInfo() # Get Zone, Domain and templates - domain = get_domain(cls.apiclient) + cls.domain = get_domain(cls.apiclient) cls.zone = get_zone(cls.apiclient, cls.testClient.getZoneForTests()) cls.services['mode'] = cls.zone.networktype @@ -309,7 +302,7 @@ class TestVMLifeCycle(cloudstackTestCase): cls.account = Account.create( cls.apiclient, cls.services["account"], - domainid=domain.id + domainid=cls.domain.id ) cls.small_offering = ServiceOffering.create( @@ -362,6 +355,7 @@ class TestVMLifeCycle(cloudstackTestCase): self.cleanup = [] def tearDown(self): + # This should be a super call instead (like tearDownClass), which reverses cleanup order. Kept for now since fixing requires adjusting test 12. try: # Clean up, terminate the created ISOs cleanup_resources(self.apiclient, self.cleanup) @@ -929,7 +923,7 @@ class TestVMLifeCycle(cloudstackTestCase): domainid=self.account.domainid, diskofferingid=custom_disk_offering.id ) - self.cleanup.append(volume) + self.cleanup.append(volume) # Needs adjusting when changing tearDown to a super call, since it will try to delete an attached volume. VirtualMachine.attach_volume(vm, self.apiclient, volume) # Start the VM @@ -955,6 +949,92 @@ class TestVMLifeCycle(cloudstackTestCase): "Check virtual machine is in running state" ) + @attr(tags=["devcloud", "advanced", "advancedns", "smoke", "basic", "sg"], required_hardware="false") + def test_13_destroy_and_expunge_vm(self): + """Test destroy virtual machine with expunge parameter depending on whether the caller's role has expunge permission. + """ + # Setup steps: + # 1. Create role with DENY expunge permission. + # 2. Create account with said role. + # 3. Create a VM of said account. + # 4. Create a VM of cls.account + # Validation steps: + # 1. Destroy the VM with the created account and verify it was not destroyed. + # 1. Destroy the other VM with cls.account and verify it was expunged. + + role = Role.importRole( + self.apiclient, + { + "name": "MarvinFake Import Role ", + "type": "DomainAdmin", + "description": "Fake Import Domain Admin Role created by Marvin test", + "rules" : [{"rule":"list*", "permission":"allow","description":"Listing apis"}, + {"rule":"get*", "permission":"allow","description":"Get apis"}, + {"rule":"update*", "permission":"allow","description":"Update apis"}, + {"rule":"queryAsyncJobResult", "permission":"allow","description":"Query async job result"}, + {"rule":"deployVirtualMachine", "permission":"allow","description":"Deploy virtual machine"}, + {"rule":"destroyVirtualMachine", "permission":"allow","description":"Destroy virtual machine"}, + {"rule":"expungeVirtualMachine", "permission":"deny","description":"Expunge virtual machine"}] + }, + ) + self.cleanup.append(role) + + domadm = Account.create( + self.apiclient, + self.services["account"], + admin=True, + roleid=role.id, + domainid=self.domain.id + ) + self.cleanup[-1]=domadm # Hacky way to reverse cleanup order to avoid deleting the role before account. Remove this line when tearDown is changed to call super(). + self.cleanup.append(role) # Should be self.cleanup.append(domadm) when tearDown is changed to call super(). + + domadm_apiclient = self.testClient.getUserApiClient(UserName=domadm.name, DomainName=self.domain.name, type=1) + + vm1 = VirtualMachine.create( + self.apiclient, + self.services["small"], + accountid=self.account.name, + domainid=self.account.domainid, + serviceofferingid=self.small_offering.id, + ) + + vm2 = VirtualMachine.create( + domadm_apiclient, + self.services["small"], + accountid=domadm.name, + domainid=domadm.domainid, + serviceofferingid=self.small_offering.id, + ) + + self.debug("Expunge VM-ID: %s" % vm1.id) + + cmd = destroyVirtualMachine.destroyVirtualMachineCmd() + cmd.id = vm1.id + cmd.expunge = True + response = self.apiclient.destroyVirtualMachine(cmd) + + self.debug("response: %s" % response) + self.debug("response: %s" % response.id) + self.assertEqual( + response.id, + None, + "Check if VM was expunged.", + ) + + self.debug("Expunge VM-ID: %s" % vm2.id) + + cmd = destroyVirtualMachine.destroyVirtualMachineCmd() + cmd.id = vm2.id + cmd.expunge = True + try: + domadm_apiclient.destroyVirtualMachine(cmd) + self.failed("Destroy VM with expunge should have raised an exception.") + except: + self.debug("Expected exception! Keep going.") + + return + class TestSecuredVmMigration(cloudstackTestCase): diff --git a/ui/src/views/compute/DestroyVM.vue b/ui/src/views/compute/DestroyVM.vue index c66c3bd5a90..13448252fb8 100644 --- a/ui/src/views/compute/DestroyVM.vue +++ b/ui/src/views/compute/DestroyVM.vue @@ -34,7 +34,7 @@ + v-if="'expungeVirtualMachine' in $store.getters.apis && ($store.getters.userInfo.roletype === 'Admin' || $store.getters.features.allowuserexpungerecovervm)">