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

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

@@ -509,4 +509,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

@@ -197,4 +197,5 @@ void buildACLViewSearchCriteria(SearchCriteria<? extends ControlledViewEntity> s
     void verifyUsingTwoFactorAuthenticationCode(String code, Long domainId, Long userAccountId);
     UserTwoFactorAuthenticationSetupResponse setupUserTwoFactorAuthentication(SetupUserTwoFactorAuthenticationCmd cmd);
 
+    void checkApiAccess(Account caller, String command);
 }

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

@@ -1348,6 +1348,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

@@ -64,9 +64,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;
@@ -3303,17 +3305,38 @@ public UserVm rebootVirtualMachine(RebootVMCmd cmd) throws InsufficientCapacityE
         return  null;
     }
 
+    /**
+     *  Encapsulates AllowUserExpungeRecoverVm so we can unit test checkExpungeVmPermission.
+     */
+    protected boolean getConfigAllowUserExpungeRecoverVm(Long entityOwnerId) {
+        return AllowUserExpungeRecoverVm.valueIn(entityOwnerId);
+    }
+
+    protected void checkExpungeVmPermission (Account callingAccount, Long entityOwnerId) {
+        logger.debug(String.format("Checking if [%s] has permission for expunging VMs.", callingAccount));
+        if (!_accountMgr.isAdmin(callingAccount.getId()) && !getConfigAllowUserExpungeRecoverVm(entityOwnerId)) {
+            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.");
+        }
+    }
+
     @Override
     @ActionEvent(eventType = EventTypes.EVENT_VM_DESTROY, eventDescription = "destroying Vm", async = true)
     public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, ConcurrentOperationException {
         CallContext ctx = CallContext.current();
         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(), cmd.getEntityOwnerId());
         }
+
         // 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

@@ -1577,4 +1577,40 @@ public void testCheckVolumesLimits() {
             Assert.fail(e.getMessage());
         }
     }
+
+    @Test
+    public void checkExpungeVMPermissionTestAccountIsNotAdminConfigFalseThrowsPermissionDeniedException () {
+        Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong());
+        Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(1L);
+
+        Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L));
+    }
+    @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,1L));
+    }
+    @Test
+    public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueHasApiAccessReturnNothing () {
+        Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong());
+        Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong());
+
+        userVmManagerImpl.checkExpungeVmPermission(accountMock,1L);
+    }
+    @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,1L));
+    }
+    @Test
+    public void checkExpungeVmPermissionTestAccountIsAdminHasApiAccessReturnNothing () {
+        Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong());
+
+        userVmManagerImpl.checkExpungeVmPermission(accountMock,1L);
+    }
 }

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>