Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow (#8689)

Author: gpordeus

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

@@ -514,4 +514,9 @@ public String getConfigComponentName() {
     public ConfigKey<?>[] getConfigKeys() {
         return null;
     }
+
+    @Override
+    public void checkApiAccess(Account account, String command) throws PermissionDeniedException {
+
+    }
 }

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

@@ -199,4 +199,6 @@ void buildACLViewSearchCriteria(SearchCriteria<? extends ControlledViewEntity> s
     UserTwoFactorAuthenticationSetupResponse setupUserTwoFactorAuthentication(SetupUserTwoFactorAuthenticationCmd cmd);
 
     List<String> getApiNameList();
+
+    void checkApiAccess(Account caller, String command);
 }

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

@@ -1369,6 +1369,12 @@ private void checkApiAccess(List<APIChecker> apiCheckers, Account caller, String
         }
     }
 
+    @Override
+    public void checkApiAccess(Account caller, String command) {
+        List<APIChecker> apiCheckers = getEnabledApiCheckers();
+        checkApiAccess(apiCheckers, caller, command);
+    }
+
     @NotNull
     private List<APIChecker> getEnabledApiCheckers() {
         // we are really only interested in the dynamic access checker

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

@@ -65,9 +65,11 @@
 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 UserVm rebootVirtualMachine(RebootVMCmd cmd) throws InsufficientCapacityE
         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 UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C
         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);
 

server/src/test/java/com/cloud/user/MockAccountManagerImpl.java

@@ -464,6 +464,10 @@ public UserTwoFactorAuthenticator getUserTwoFactorAuthenticationProvider(Long do
         return null;
     }
 
+    @Override
+    public void checkApiAccess(Account account, String command) throws PermissionDeniedException {
+
+    }
     @Override
     public void checkAccess(User user, ControlledEntity entity)
         throws PermissionDeniedException {

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

@@ -1595,4 +1595,40 @@ public void testGetRootVolumeSizeForVmRestoreNullDiskOfferingAndEmptyDetails() {
         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);
+    }
 }

test/integration/smoke/test_vm_life_cycle.py

@@ -31,6 +31,7 @@
 from marvin.lib.utils import *
 
 from marvin.lib.base import (Account,
+                             Role,
                              ServiceOffering,
                              VirtualMachine,
                              Host,
@@ -94,17 +95,21 @@ def setUpClass(cls):
 
         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 @@ def setUpClass(cls):
             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 @@ def test_deploy_vm_multiple(self):
         )
 
     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 @@ def setUpClass(cls):
         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 @@ def setUpClass(cls):
         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 @@ def setUp(self):
         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 @@ def test_12_start_vm_multiple_volumes_allocated(self):
                 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 @@ def test_12_start_vm_multiple_volumes_allocated(self):
             "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):
 

ui/src/views/compute/DestroyVM.vue

@@ -34,7 +34,7 @@
           <a-form-item
             name="expunge"
             ref="expunge"
-            v-if="$store.getters.userInfo.roletype === 'Admin' || $store.getters.features.allowuserexpungerecovervm">
+            v-if="'expungeVirtualMachine' in $store.getters.apis && ($store.getters.userInfo.roletype === 'Admin' || $store.getters.features.allowuserexpungerecovervm)">
             <template #label>
               <tooltip-label :title="$t('label.expunge')" :tooltip="apiParams.expunge.description"/>
             </template>