From f2f8ac4617d3892cb2474b6a27dbf5d3fd5ae61d Mon Sep 17 00:00:00 2001 From: Gabriel P Santos Date: Tue, 20 Feb 2024 09:55:42 -0300 Subject: [PATCH 01/10] Add expungeVirtualMachine restriction to expunge parameter on destroyVirtualMachine. --- .../management/MockAccountManager.java | 5 ++ .../java/com/cloud/user/AccountManager.java | 1 + .../com/cloud/user/AccountManagerImpl.java | 6 ++ .../java/com/cloud/vm/UserVmManagerImpl.java | 31 +++++++++- .../cloud/user/MockAccountManagerImpl.java | 4 ++ .../com/cloud/vm/UserVmManagerImplTest.java | 62 +++++++++++++++++++ ui/src/views/compute/DestroyVM.vue | 2 +- 7 files changed, 107 insertions(+), 4 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 836bb7213296..95d25bd01816 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 @@ -509,4 +509,9 @@ public String getConfigComponentName() { 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 c95047a6c42b..f3ed36a3273c 100644 --- a/server/src/main/java/com/cloud/user/AccountManager.java +++ b/server/src/main/java/com/cloud/user/AccountManager.java @@ -197,4 +197,5 @@ void buildACLViewSearchCriteria(SearchCriteria s void verifyUsingTwoFactorAuthenticationCode(String code, Long domainId, Long userAccountId); UserTwoFactorAuthenticationSetupResponse setupUserTwoFactorAuthentication(SetupUserTwoFactorAuthenticationCmd cmd); + 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 1a30f192173a..e0714667cf7e 100644 --- a/server/src/main/java/com/cloud/user/AccountManagerImpl.java +++ b/server/src/main/java/com/cloud/user/AccountManagerImpl.java @@ -1348,6 +1348,12 @@ private void checkApiAccess(List apiCheckers, Account caller, String } } + @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 770826ecfa54..419fc4b3c933 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -52,6 +52,8 @@ import javax.xml.parsers.ParserConfigurationException; import com.cloud.utils.exception.ExceptionProxyObject; +import com.cloud.exception.UnavailableCommandException; +import com.cloud.resourcelimit.CheckedReservation; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.ControlledEntity.ACLType; import org.apache.cloudstack.acl.SecurityChecker.AccessType; @@ -64,9 +66,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,6 +3307,27 @@ 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)) { + throw new PermissionDeniedException(String.format("Parameter [%s] can only be passed by Admin accounts or when the allow.user.expunge.recover.vm key is set true.", + ApiConstants.EXPUNGE)); + } + try { + _accountMgr.checkApiAccess(callingAccount, BaseCmd.getCommandNameByClass(ExpungeVMCmd.class)); + } catch (UnavailableCommandException ex) { + logger.warn(String.format("Role [%s] of [%s] does not have permission for expunging VMs.", callingAccount.getRoleId(), callingAccount)); + throw new PermissionDeniedException("Account role 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 { @@ -3310,10 +3335,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(), cmd.getEntityOwnerId()); } + // 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 fe7748b85815..a3396b77f015 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 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 { diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index 4ed0e5b6250a..a96d78f86b5e 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -40,6 +40,7 @@ import java.util.List; import java.util.Map; +import com.cloud.exception.UnavailableCommandException; import org.apache.cloudstack.api.BaseCmd.HTTPMethod; import org.apache.cloudstack.api.command.user.vm.DeployVMCmd; import org.apache.cloudstack.api.command.user.vm.DeployVnfApplianceCmd; @@ -1577,4 +1578,65 @@ 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); + Mockito.doThrow(UnavailableCommandException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); + + Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); + } + @Test + public void checkExpungeVmPermissionTestAccountIsAdminConfigFalseThrowsPermissionDeniedException () { + Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); + Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); + Mockito.doThrow(UnavailableCommandException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); + + Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); + } + @Test + public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueThrowsPermissionDeniedException () { + Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong()); + Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); + Mockito.doThrow(UnavailableCommandException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); + + Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); + } + @Test + public void checkExpungeVmPermissionTestAccountIsAdminConfigTrueThrowsPermissionDeniedException () { + Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); + Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); + Mockito.doThrow(UnavailableCommandException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); + + Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); + } + @Test + public void checkExpungeVmPermissionTestAccountIsAdminConfigFalseNoApiAccessThrowsPermissionDeniedException () { + Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong()); + Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); + + 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 checkExpungeVmPermissionTestAccountIsAdminConfigFalseHasApiAccessReturnNothing () { + Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); + Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); + + userVmManagerImpl.checkExpungeVmPermission(accountMock,1L); + } + @Test + public void checkExpungeVmPermissionTestAccountIsAdminConfigTrueHasApiAccessReturnNothing () { + Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); + Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); + + userVmManagerImpl.checkExpungeVmPermission(accountMock,1L); + } } diff --git a/ui/src/views/compute/DestroyVM.vue b/ui/src/views/compute/DestroyVM.vue index c66c3bd5a902..13448252fb85 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)"> From 1b9d35ddfbe1eae89d1ddf3282a0aa4f6d9e930f Mon Sep 17 00:00:00 2001 From: Gabriel P Santos Date: Tue, 20 Feb 2024 11:38:47 -0300 Subject: [PATCH 02/10] fix tests --- .../com/cloud/vm/UserVmManagerImplTest.java | 35 +++---------------- 1 file changed, 5 insertions(+), 30 deletions(-) diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index a96d78f86b5e..fc58c164b8ae 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -1583,20 +1583,11 @@ public void testCheckVolumesLimits() { public void checkExpungeVMPermissionTestAccountIsNotAdminConfigFalseThrowsPermissionDeniedException () { Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong()); Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(1L); - Mockito.doThrow(UnavailableCommandException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); - - Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); - } - @Test - public void checkExpungeVmPermissionTestAccountIsAdminConfigFalseThrowsPermissionDeniedException () { - Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); - Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); - Mockito.doThrow(UnavailableCommandException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); } @Test - public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueThrowsPermissionDeniedException () { + public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueNoApiAccessThrowsPermissionDeniedException () { Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong()); Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); Mockito.doThrow(UnavailableCommandException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); @@ -1604,21 +1595,6 @@ public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueThrowsPermiss Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); } @Test - public void checkExpungeVmPermissionTestAccountIsAdminConfigTrueThrowsPermissionDeniedException () { - Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); - Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); - Mockito.doThrow(UnavailableCommandException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); - - Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); - } - @Test - public void checkExpungeVmPermissionTestAccountIsAdminConfigFalseNoApiAccessThrowsPermissionDeniedException () { - Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong()); - Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); - - 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()); @@ -1626,16 +1602,15 @@ public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueHasApiAccessR userVmManagerImpl.checkExpungeVmPermission(accountMock,1L); } @Test - public void checkExpungeVmPermissionTestAccountIsAdminConfigFalseHasApiAccessReturnNothing () { + public void checkExpungeVmPermissionTestAccountIsAdminNoApiAccessThrowsPermissionDeniedException () { Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); - Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); + Mockito.doThrow(UnavailableCommandException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); - userVmManagerImpl.checkExpungeVmPermission(accountMock,1L); + Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); } @Test - public void checkExpungeVmPermissionTestAccountIsAdminConfigTrueHasApiAccessReturnNothing () { + public void checkExpungeVmPermissionTestAccountIsAdminHasApiAccessReturnNothing () { Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); - Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); userVmManagerImpl.checkExpungeVmPermission(accountMock,1L); } From 6ae0d7cb5f64c34c3d2fdf568dd6129f0c8abf00 Mon Sep 17 00:00:00 2001 From: Gabriel P Santos Date: Tue, 20 Feb 2024 11:39:20 -0300 Subject: [PATCH 03/10] fix checkstyle and logs --- server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 419fc4b3c933..d529372861bd 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -288,7 +288,6 @@ import com.cloud.org.Grouping; import com.cloud.resource.ResourceManager; import com.cloud.resource.ResourceState; -import com.cloud.resourcelimit.CheckedReservation; import com.cloud.server.ManagementService; import com.cloud.server.ResourceTag; import com.cloud.server.StatsCollector; @@ -3317,14 +3316,14 @@ protected boolean getConfigAllowUserExpungeRecoverVm(Long 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)) { - throw new PermissionDeniedException(String.format("Parameter [%s] can only be passed by Admin accounts or when the allow.user.expunge.recover.vm key is set true.", - ApiConstants.EXPUNGE)); + 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 (UnavailableCommandException ex) { - logger.warn(String.format("Role [%s] of [%s] does not have permission for expunging VMs.", callingAccount.getRoleId(), callingAccount)); - throw new PermissionDeniedException("Account role does not have permission for expunging."); + 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."); } } From 6eadb19c4798524582ffa8434e6744599a9bf8c2 Mon Sep 17 00:00:00 2001 From: Gabriel P Santos Date: Tue, 20 Feb 2024 13:13:10 -0300 Subject: [PATCH 04/10] unneeded exception --- server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | 3 +-- server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index d529372861bd..34e8db24dfb2 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -52,7 +52,6 @@ import javax.xml.parsers.ParserConfigurationException; import com.cloud.utils.exception.ExceptionProxyObject; -import com.cloud.exception.UnavailableCommandException; import com.cloud.resourcelimit.CheckedReservation; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.ControlledEntity.ACLType; @@ -3321,7 +3320,7 @@ protected void checkExpungeVmPermission (Account callingAccount, Long entityOwne } try { _accountMgr.checkApiAccess(callingAccount, BaseCmd.getCommandNameByClass(ExpungeVMCmd.class)); - } catch (UnavailableCommandException ex) { + } 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."); } diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index fc58c164b8ae..d23ca198096e 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -40,7 +40,6 @@ import java.util.List; import java.util.Map; -import com.cloud.exception.UnavailableCommandException; import org.apache.cloudstack.api.BaseCmd.HTTPMethod; import org.apache.cloudstack.api.command.user.vm.DeployVMCmd; import org.apache.cloudstack.api.command.user.vm.DeployVnfApplianceCmd; @@ -1590,7 +1589,7 @@ public void checkExpungeVMPermissionTestAccountIsNotAdminConfigFalseThrowsPermis public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueNoApiAccessThrowsPermissionDeniedException () { Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong()); Mockito.doReturn(true).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); - Mockito.doThrow(UnavailableCommandException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); + Mockito.doThrow(PermissionDeniedException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); } @@ -1604,7 +1603,7 @@ public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueHasApiAccessR @Test public void checkExpungeVmPermissionTestAccountIsAdminNoApiAccessThrowsPermissionDeniedException () { Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); - Mockito.doThrow(UnavailableCommandException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); + Mockito.doThrow(PermissionDeniedException.class).when(accountManager).checkApiAccess(accountMock, "expungeVirtualMachine"); Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); } From c53258df449e9fe2e93cdbca30e9688042eef43c Mon Sep 17 00:00:00 2001 From: Gabriel P Santos Date: Tue, 20 Feb 2024 13:36:52 -0300 Subject: [PATCH 05/10] import adjust --- server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 34e8db24dfb2..b950466e6640 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -52,7 +52,6 @@ import javax.xml.parsers.ParserConfigurationException; import com.cloud.utils.exception.ExceptionProxyObject; -import com.cloud.resourcelimit.CheckedReservation; import org.apache.cloudstack.acl.ControlledEntity; import org.apache.cloudstack.acl.ControlledEntity.ACLType; import org.apache.cloudstack.acl.SecurityChecker.AccessType; @@ -287,6 +286,7 @@ import com.cloud.org.Grouping; import com.cloud.resource.ResourceManager; import com.cloud.resource.ResourceState; +import com.cloud.resourcelimit.CheckedReservation; import com.cloud.server.ManagementService; import com.cloud.server.ResourceTag; import com.cloud.server.StatsCollector; From 716c9f9cfdede340888bf73c956dc1ff81426fee Mon Sep 17 00:00:00 2001 From: Gabriel P Santos Date: Wed, 21 Feb 2024 12:31:28 -0300 Subject: [PATCH 06/10] swap entityownerid for callingaccountid --- .../main/java/com/cloud/vm/UserVmManagerImpl.java | 10 +++++----- .../java/com/cloud/vm/UserVmManagerImplTest.java | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index b950466e6640..6e296adc3a60 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -3308,13 +3308,13 @@ public UserVm rebootVirtualMachine(RebootVMCmd cmd) throws InsufficientCapacityE /** * Encapsulates AllowUserExpungeRecoverVm so we can unit test checkExpungeVmPermission. */ - protected boolean getConfigAllowUserExpungeRecoverVm(Long entityOwnerId) { - return AllowUserExpungeRecoverVm.valueIn(entityOwnerId); + protected boolean getConfigAllowUserExpungeRecoverVm(Long accountId) { + return AllowUserExpungeRecoverVm.valueIn(accountId); } - protected void checkExpungeVmPermission (Account callingAccount, Long entityOwnerId) { + protected void checkExpungeVmPermission (Account callingAccount) { logger.debug(String.format("Checking if [%s] has permission for expunging VMs.", callingAccount)); - if (!_accountMgr.isAdmin(callingAccount.getId()) && !getConfigAllowUserExpungeRecoverVm(entityOwnerId)) { + 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."); } @@ -3334,7 +3334,7 @@ public UserVm destroyVm(DestroyVMCmd cmd) throws ResourceUnavailableException, C boolean expunge = cmd.getExpunge(); if (expunge) { - checkExpungeVmPermission(ctx.getCallingAccount(), cmd.getEntityOwnerId()); + checkExpungeVmPermission(ctx.getCallingAccount()); } // check if VM exists diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index d23ca198096e..73b74e8f0034 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -1581,9 +1581,9 @@ public void testCheckVolumesLimits() { @Test public void checkExpungeVMPermissionTestAccountIsNotAdminConfigFalseThrowsPermissionDeniedException () { Mockito.doReturn(false).when(accountManager).isAdmin(Mockito.anyLong()); - Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(1L); + Mockito.doReturn(false).when(userVmManagerImpl).getConfigAllowUserExpungeRecoverVm(Mockito.anyLong()); - Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock,1L)); + Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock)); } @Test public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueNoApiAccessThrowsPermissionDeniedException () { @@ -1591,26 +1591,26 @@ public void checkExpungeVmPermissionTestAccountIsNotAdminConfigTrueNoApiAccessTh 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)); + 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,1L); + 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,1L)); + Assert.assertThrows(PermissionDeniedException.class, () -> userVmManagerImpl.checkExpungeVmPermission(accountMock)); } @Test public void checkExpungeVmPermissionTestAccountIsAdminHasApiAccessReturnNothing () { Mockito.doReturn(true).when(accountManager).isAdmin(Mockito.anyLong()); - userVmManagerImpl.checkExpungeVmPermission(accountMock,1L); + userVmManagerImpl.checkExpungeVmPermission(accountMock); } } From a17136511fa3a47b19ce1d9e1e02b0d9aa71a043 Mon Sep 17 00:00:00 2001 From: Gabriel P Santos Date: Tue, 12 Mar 2024 12:02:51 -0300 Subject: [PATCH 07/10] integration test --- test/integration/smoke/test_vm_life_cycle.py | 91 +++++++++++++++++++- 1 file changed, 89 insertions(+), 2 deletions(-) diff --git a/test/integration/smoke/test_vm_life_cycle.py b/test/integration/smoke/test_vm_life_cycle.py index 6c824c1fe7d3..d7967c5b7606 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.lib.utils import * from marvin.lib.base import (Account, + Role, ServiceOffering, VirtualMachine, Host, @@ -279,7 +280,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 +310,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( @@ -955,6 +956,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"}] + }, + ) + + domadm = Account.create( + self.apiclient, + self.services["account"], + admin=True, + roleid=role.id, + domainid=self.domain.id + ) + + self.cleanup.append(domadm) + self.cleanup.append(role) + + 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): From 7695a88f2cf4721ea8b826bdfba90a7fd27d7b21 Mon Sep 17 00:00:00 2001 From: Gabriel P Santos Date: Mon, 18 Mar 2024 16:59:42 -0300 Subject: [PATCH 08/10] apply suggestion --- test/integration/smoke/test_vm_life_cycle.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/smoke/test_vm_life_cycle.py b/test/integration/smoke/test_vm_life_cycle.py index d7967c5b7606..0dc4c6d05441 100644 --- a/test/integration/smoke/test_vm_life_cycle.py +++ b/test/integration/smoke/test_vm_life_cycle.py @@ -984,6 +984,7 @@ def test_13_destroy_and_expunge_vm(self): {"rule":"expungeVirtualMachine", "permission":"deny","description":"Expunge virtual machine"}] }, ) + self.cleanup.append(role) domadm = Account.create( self.apiclient, @@ -992,8 +993,7 @@ def test_13_destroy_and_expunge_vm(self): roleid=role.id, domainid=self.domain.id ) - - self.cleanup.append(domadm) + self.cleanup[-1]=domadm self.cleanup.append(role) domadm_apiclient = self.testClient.getUserApiClient(UserName=domadm.name, DomainName=self.domain.name, type=1) From 007f1e10a60c51acf341db5f7ed4b7369e120669 Mon Sep 17 00:00:00 2001 From: Gabriel P Santos Date: Mon, 25 Mar 2024 14:30:57 -0300 Subject: [PATCH 09/10] comment on cleanup position swapping and change testDeployVM cleanup --- test/integration/smoke/test_vm_life_cycle.py | 27 ++++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/test/integration/smoke/test_vm_life_cycle.py b/test/integration/smoke/test_vm_life_cycle.py index 0dc4c6d05441..e47cdc2164ab 100644 --- a/test/integration/smoke/test_vm_life_cycle.py +++ b/test/integration/smoke/test_vm_life_cycle.py @@ -95,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, @@ -116,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() @@ -263,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): @@ -363,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) @@ -930,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 @@ -993,8 +986,8 @@ def test_13_destroy_and_expunge_vm(self): roleid=role.id, domainid=self.domain.id ) - self.cleanup[-1]=domadm - self.cleanup.append(role) + 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) From bfbb0a560e62b23e5f942dc2b639b4417785ae16 Mon Sep 17 00:00:00 2001 From: Gabriel P Santos Date: Mon, 10 Jun 2024 15:30:57 -0300 Subject: [PATCH 10/10] oops, fix missing closing } --- server/src/main/java/com/cloud/vm/UserVmManagerImpl.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 22f5153dd6c9..31d4b04f51ec 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -3336,6 +3336,7 @@ protected void checkExpungeVmPermission (Account callingAccount) { 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 {