From dfad178a9e334776e4ba6c056523b5bd627ccace Mon Sep 17 00:00:00 2001 From: Likitha Shetty Date: Mon, 6 May 2013 09:58:18 +0530 Subject: [PATCH] Bug CLOUDSTACK-1390: Allow Root/Domain admin to move a User VM to another user under a different domain Add unit tests --- client/tomcatconf/commands.properties.in | 2 +- .../src/com/cloud/vm/UserVmManagerImpl.java | 14 ++-- .../test/com/cloud/vm/UserVmManagerTest.java | 83 ++++++++++++++++++- 3 files changed, 88 insertions(+), 11 deletions(-) diff --git a/client/tomcatconf/commands.properties.in b/client/tomcatconf/commands.properties.in index cdc19929c19..0a6ec708166 100644 --- a/client/tomcatconf/commands.properties.in +++ b/client/tomcatconf/commands.properties.in @@ -67,7 +67,7 @@ getVMPassword=15 restoreVirtualMachine=15 changeServiceForVirtualMachine=15 scaleVirtualMachine=15 -assignVirtualMachine=1 +assignVirtualMachine=7 migrateVirtualMachine=1 migrateVirtualMachineWithVolume=1 recoverVirtualMachine=7 diff --git a/server/src/com/cloud/vm/UserVmManagerImpl.java b/server/src/com/cloud/vm/UserVmManagerImpl.java index 9fbc5099374..f7f5fc7ad74 100755 --- a/server/src/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/com/cloud/vm/UserVmManagerImpl.java @@ -3725,19 +3725,14 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Use + cmd.getAccountName() + " is disabled."); } - // make sure the accounts are under same domain - if (oldAccount.getDomainId() != newAccount.getDomainId()) { - throw new InvalidParameterValueException( - "The account should be under same domain for moving VM between two accounts. Old owner domain =" - + oldAccount.getDomainId() - + " New owner domain=" - + newAccount.getDomainId()); - } + //check caller has access to both the old and new account + _accountMgr.checkAccess(caller, null, true, oldAccount); + _accountMgr.checkAccess(caller, null, true, newAccount); // make sure the accounts are not same if (oldAccount.getAccountId() == newAccount.getAccountId()) { throw new InvalidParameterValueException( - "The account should be same domain for moving VM between two accounts. Account id =" + "The new account is the same as the old account. Account id =" + oldAccount.getAccountId()); } @@ -3829,6 +3824,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Use _resourceLimitMgr.decrementResourceCount(oldAccount.getAccountId(), ResourceType.primary_storage, new Long(volume.getSize())); volume.setAccountId(newAccount.getAccountId()); + volume.setDomainId(newAccount.getDomainId()); _volsDao.persist(volume); _resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), ResourceType.volume); _resourceLimitMgr.incrementResourceCount(newAccount.getAccountId(), ResourceType.primary_storage, diff --git a/server/test/com/cloud/vm/UserVmManagerTest.java b/server/test/com/cloud/vm/UserVmManagerTest.java index 08f2a9c2abc..dfd7465aba0 100755 --- a/server/test/com/cloud/vm/UserVmManagerTest.java +++ b/server/test/com/cloud/vm/UserVmManagerTest.java @@ -17,19 +17,26 @@ package com.cloud.vm; +import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyBoolean; import static org.mockito.Matchers.anyFloat; import static org.mockito.Matchers.anyInt; import static org.mockito.Matchers.anyLong; +import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.when; import java.lang.reflect.Field; import java.util.List; +import java.util.UUID; +import org.apache.cloudstack.acl.ControlledEntity; +import org.apache.cloudstack.acl.SecurityChecker.AccessType; import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.command.admin.vm.AssignVMCmd; import org.apache.cloudstack.api.command.user.vm.RestoreVMCmd; import org.apache.cloudstack.api.command.user.vm.ScaleVMCmd; import org.junit.Before; @@ -44,9 +51,11 @@ import com.cloud.configuration.dao.ConfigurationDao; import com.cloud.exception.ConcurrentOperationException; import com.cloud.exception.InsufficientCapacityException; import com.cloud.exception.InvalidParameterValueException; +import com.cloud.exception.PermissionDeniedException; import com.cloud.exception.ResourceAllocationException; import com.cloud.exception.ResourceUnavailableException; import com.cloud.hypervisor.Hypervisor; +import com.cloud.hypervisor.Hypervisor.HypervisorType; import com.cloud.offering.ServiceOffering; import com.cloud.service.ServiceOfferingVO; import com.cloud.storage.VMTemplateVO; @@ -57,6 +66,7 @@ import com.cloud.storage.dao.VMTemplateDao; import com.cloud.storage.dao.VolumeDao; import com.cloud.user.Account; import com.cloud.user.AccountManager; +import com.cloud.user.AccountService; import com.cloud.user.AccountVO; import com.cloud.user.UserContext; import com.cloud.user.UserVO; @@ -73,6 +83,7 @@ public class UserVmManagerTest { @Mock VolumeManager _storageMgr; @Mock Account _account; @Mock AccountManager _accountMgr; + @Mock AccountService _accountService; @Mock ConfigurationManager _configMgr; @Mock CapacityManager _capacityMgr; @Mock AccountDao _accountDao; @@ -91,6 +102,7 @@ public class UserVmManagerTest { @Mock VMTemplateVO _templateMock; @Mock VolumeVO _volumeMock; @Mock List _rootVols; + @Mock Account _accountMock2; @Before public void setup(){ MockitoAnnotations.initMocks(this); @@ -102,6 +114,7 @@ public class UserVmManagerTest { _userVmMgr._itMgr = _itMgr; _userVmMgr.volumeMgr = _storageMgr; _userVmMgr._accountDao = _accountDao; + _userVmMgr._accountService = _accountService; _userVmMgr._userDao = _userDao; _userVmMgr._accountMgr = _accountMgr; _userVmMgr._configMgr = _configMgr; @@ -370,6 +383,74 @@ public class UserVmManagerTest { return serviceOffering; } - + // Test Move VM b/w accounts where caller is not ROOT/Domain admin + @Test(expected=InvalidParameterValueException.class) + public void testMoveVmToUser1() throws Exception { + AssignVMCmd cmd = new AssignVMCmd(); + Class _class = cmd.getClass(); + + Field virtualmachineIdField = _class.getDeclaredField("virtualMachineId"); + virtualmachineIdField.setAccessible(true); + virtualmachineIdField.set(cmd, 1L); + + Field accountNameField = _class.getDeclaredField("accountName"); + accountNameField.setAccessible(true); + accountNameField.set(cmd, "account"); + + Field domainIdField = _class.getDeclaredField("domainId"); + domainIdField.setAccessible(true); + domainIdField.set(cmd, 1L); + + // caller is of type 0 + Account caller = (Account) new AccountVO("testaccount", 1, "networkdomain", (short) 0, + UUID.randomUUID().toString()); + UserContext.registerContext(1, caller, null, true); + + _userVmMgr.moveVMToUser(cmd); + } + + + // Test Move VM b/w accounts where caller doesn't have access to the old or new account + @Test(expected=PermissionDeniedException.class) + public void testMoveVmToUser2() throws Exception { + AssignVMCmd cmd = new AssignVMCmd(); + Class _class = cmd.getClass(); + + Field virtualmachineIdField = _class.getDeclaredField("virtualMachineId"); + virtualmachineIdField.setAccessible(true); + virtualmachineIdField.set(cmd, 1L); + + Field accountNameField = _class.getDeclaredField("accountName"); + accountNameField.setAccessible(true); + accountNameField.set(cmd, "account"); + + Field domainIdField = _class.getDeclaredField("domainId"); + domainIdField.setAccessible(true); + domainIdField.set(cmd, 1L); + + // caller is of type 0 + Account caller = (Account) new AccountVO("testaccount", 1, "networkdomain", (short) 1, + UUID.randomUUID().toString()); + UserContext.registerContext(1, caller, null, true); + + Account oldAccount = (Account) new AccountVO("testaccount", 1, "networkdomain", (short) 0, + UUID.randomUUID().toString()); + Account newAccount = (Account) new AccountVO("testaccount", 1, "networkdomain", (short) 1, + UUID.randomUUID().toString()); + + UserVmVO vm = new UserVmVO(10L, "test", "test", 1L, HypervisorType.Any, 1L, false, false, 1L, 1L, + 5L, "test", "test", 1L); + vm.setState(VirtualMachine.State.Stopped); + when(_vmDao.findById(anyLong())).thenReturn(vm); + + when(_accountService.getActiveAccountById(anyLong())).thenReturn(oldAccount); + + when(_accountService.getActiveAccountByName(anyString(), anyLong())).thenReturn(newAccount); + + doThrow(new PermissionDeniedException("Access check failed")).when(_accountMgr).checkAccess(any(Account.class), any(AccessType.class), + any(Boolean.class), any(ControlledEntity.class)); + + _userVmMgr.moveVMToUser(cmd); + } } \ No newline at end of file