apache/cloudstack
1
0
Fork 0
mirror of https://github.com/apache/cloudstack.git synced 2025-10-26 08:42:29 +01:00
Code Issues Packages Projects Releases Wiki Activity

server Don't allow inadvertent deletion of hidden details via API (#7880)

Browse Source 
* Don't allow inadvertent deletion of hidden details via API

* Update VM details unit test ensuring system/hidden details not removed

* Update test/integration/component/test_update_vm.py

---------

Co-authored-by: Marcus Sorensen <mls@apple.com>
Co-authored-by: dahn <daan.hoogland@gmail.com>
This commit is contained in:
Marcus Sorensen 2023-08-24 02:45:39 -06:00 committed by GitHub
parent 2e1c2821a8
commit a0702279aa
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 152 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
View File

@ -2756,13 +2756,18 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
final List<String> userReadOnlySettings = Stream.of(QueryService.UserVMReadOnlyDetails.value().split(","))
.map(item -> (item).trim())
.collect(Collectors.toList());
List<UserVmDetailVO> existingDetails = userVmDetailsDao.listDetails(id);
if (cleanupDetails){
if (caller != null && caller.getType() == Account.Type.ADMIN) {
userVmDetailsDao.removeDetails(id);
for (final UserVmDetailVO detail : existingDetails) {
if (detail != null && detail.isDisplay()) {
userVmDetailsDao.removeDetail(id, detail.getName());
}
}
} else {
for (final UserVmDetailVO detail : userVmDetailsDao.listDetails(id)) {
for (final UserVmDetailVO detail : existingDetails) {
if (detail != null && !userDenyListedSettings.contains(detail.getName())
&& !userReadOnlySettings.contains(detail.getName())) {
&& !userReadOnlySettings.contains(detail.getName()) && detail.isDisplay()) {
userVmDetailsDao.removeDetail(id, detail.getName());
}
}
@ -2782,15 +2787,25 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
if (userReadOnlySettings.contains(detailName)) {
throw new InvalidParameterValueException("You're not allowed to add or edit the read-only setting: " + detailName);
}
if (existingDetails.stream().anyMatch(d -> Objects.equals(d.getName(), detailName) && !d.isDisplay())){
throw new InvalidParameterValueException("You're not allowed to add or edit the non-displayable setting: " + detailName);
}
}
// Add any hidden/denied or read-only detail
for (final UserVmDetailVO detail : userVmDetailsDao.listDetails(id)) {
// Add any existing user denied or read-only details. We do it here because admins would already provide these (or can delete them).
for (final UserVmDetailVO detail : existingDetails) {
if (userDenyListedSettings.contains(detail.getName()) || userReadOnlySettings.contains(detail.getName())) {
details.put(detail.getName(), detail.getValue());
}
}
}
// ensure details marked as non-displayable are maintained, regardless of admin or not
for (final UserVmDetailVO existingDetail : existingDetails) {
if (!existingDetail.isDisplay()) {
details.put(existingDetail.getName(), existingDetail.getValue());
}
}
verifyVmLimits(vmInstance, details);
vmInstance.setDetails(details);
_vmDao.saveDetails(vmInstance);

29
server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java
View File

@ -149,7 +149,7 @@ public class UserVmManagerImplTest {
private EntityManager entityManager;
@Mock
private UserVmDetailsDao userVmDetailVO;
private UserVmDetailsDao userVmDetailsDao;
@Mock
private UserVmVO userVmVoMock;
@ -300,7 +300,7 @@ public class UserVmManagerImplTest {
verifyMethodsThatAreAlwaysExecuted();
Mockito.verify(userVmManagerImpl).updateDisplayVmFlag(false, vmId, userVmVoMock);
Mockito.verify(userVmDetailVO, Mockito.times(0)).removeDetails(vmId);
Mockito.verify(userVmDetailsDao, Mockito.times(0)).removeDetail(anyLong(), anyString());
}
@Test
@ -310,12 +310,15 @@ public class UserVmManagerImplTest {
Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering);
Mockito.when(updateVmCommand.isCleanupDetails()).thenReturn(true);
Mockito.lenient().doNothing().when(userVmManagerImpl).updateDisplayVmFlag(false, vmId, userVmVoMock);
Mockito.doNothing().when(userVmDetailVO).removeDetails(vmId);
Mockito.when(updateVmCommand.getUserdataId()).thenReturn(null);
prepareExistingDetails(vmId, "userdetail");
userVmManagerImpl.updateVirtualMachine(updateVmCommand);
verifyMethodsThatAreAlwaysExecuted();
Mockito.verify(userVmDetailVO).removeDetails(vmId);
Mockito.verify(userVmDetailsDao).removeDetail(vmId, "userdetail");
Mockito.verify(userVmDetailsDao, Mockito.times(0)).removeDetail(vmId, "systemdetail");
Mockito.verify(userVmManagerImpl, Mockito.times(0)).updateDisplayVmFlag(false, vmId, userVmVoMock);
}
@ -340,6 +343,16 @@ public class UserVmManagerImplTest {
prepareAndExecuteMethodDealingWithDetails(false, false);
}
private List<UserVmDetailVO> prepareExistingDetails(Long vmId, String... existingDetailKeys) {
List<UserVmDetailVO> existingDetails = new ArrayList<>();
for (String detail : existingDetailKeys) {
existingDetails.add(new UserVmDetailVO(vmId, detail, "foo", true));
}
existingDetails.add(new UserVmDetailVO(vmId, "systemdetail", "bar", false));
Mockito.when(userVmDetailsDao.listDetails(vmId)).thenReturn(existingDetails);
return existingDetails;
}
private void prepareAndExecuteMethodDealingWithDetails(boolean cleanUpDetails, boolean isDetailsEmpty) throws ResourceUnavailableException, InsufficientCapacityException {
configureDoNothingForMethodsThatWeDoNotWantToTest();
@ -360,8 +373,9 @@ public class UserVmManagerImplTest {
lenient().doNothing().when(_networkMgr).saveExtraDhcpOptions(anyString(), anyLong(), anyMap());
HashMap<String, String> details = new HashMap<>();
if(!isDetailsEmpty) {
details.put("", "");
details.put("newdetail", "foo");
}
prepareExistingDetails(vmId, "existingdetail");
Mockito.when(updateVmCommand.getUserdataId()).thenReturn(null);
Mockito.when(updateVmCommand.getDetails()).thenReturn(details);
Mockito.when(updateVmCommand.isCleanupDetails()).thenReturn(cleanUpDetails);
@ -371,14 +385,15 @@ public class UserVmManagerImplTest {
verifyMethodsThatAreAlwaysExecuted();
Mockito.verify(userVmVoMock, Mockito.times(cleanUpDetails || isDetailsEmpty ? 0 : 1)).setDetails(details);
Mockito.verify(userVmDetailVO, Mockito.times(cleanUpDetails ? 1: 0)).removeDetails(vmId);
Mockito.verify(userVmDetailsDao, Mockito.times(cleanUpDetails ? 1 : 0)).removeDetail(vmId, "existingdetail");
Mockito.verify(userVmDetailsDao, Mockito.times(0)).removeDetail(vmId, "systemdetail");
Mockito.verify(userVmDao, Mockito.times(cleanUpDetails || isDetailsEmpty ? 0 : 1)).saveDetails(userVmVoMock);
Mockito.verify(userVmManagerImpl, Mockito.times(0)).updateDisplayVmFlag(false, vmId, userVmVoMock);
}
private void configureDoNothingForDetailsMethod() {
Mockito.lenient().doNothing().when(userVmManagerImpl).updateDisplayVmFlag(false, vmId, userVmVoMock);
Mockito.doNothing().when(userVmDetailVO).removeDetails(vmId);
Mockito.doNothing().when(userVmDetailsDao).removeDetail(anyLong(), anyString());
Mockito.doNothing().when(userVmDao).saveDetails(userVmVoMock);
}

124
test/integration/component/test_update_vm.py
View File

@ -15,11 +15,11 @@
# specific language governing permissions and limitations
# under the License.
from marvin.cloudstackTestCase import cloudstackTestCase
from marvin.lib.base import Account, VirtualMachine, ServiceOffering
from marvin.lib.utils import cleanup_resources
from marvin.lib.utils import (validateList, cleanup_resources)
from marvin.lib.common import get_zone, get_domain, get_template
from marvin.codes import PASS
from nose.plugins.attrib import attr
class TestData(object):
@ -59,6 +59,7 @@ class TestUpdateVirtualMachine(cloudstackTestCase):
def setUp(self):
self.testdata = TestData().testdata
self.apiclient = self.testClient.getApiClient()
self.dbclient = self.testClient.getDbConnection()
# Get Zone, Domain and Default Built-in template
self.domain = get_domain(self.apiclient)
@ -106,18 +107,7 @@ class TestUpdateVirtualMachine(cloudstackTestCase):
templateid=self.template.id
)
list_vms = VirtualMachine.list(self.apiclient, id=self.virtual_machine.id)
self.assertEqual(
isinstance(list_vms, list),
True,
"List VM response was not a valid list"
)
self.assertNotEqual(
len(list_vms),
0,
"List VM response was empty"
)
vm = list_vms[0]
vm = self.listVmById(self.virtual_machine.id)
self.debug(
"VirtualMachine launched with id, name, displayname: %s %s %s"\
@ -152,6 +142,112 @@ class TestUpdateVirtualMachine(cloudstackTestCase):
self.assertEqual(vmnew.displayname, vmnewstarted.displayname,
msg="display name changed on start, displayname is %s" % vmnewstarted.displayname)
@attr(tags=['advanced', 'simulator', 'basic', 'sg', 'details'], required_hardware="false")
def test_update_vm_details_admin(self):
"""Test Update VirtualMachine Details
# Set up a VM
# Set up hidden detail in DB for VM
# Validate the following:
# 1. Can add two details (detail1, detail2)
# 2. Can fetch new details on VM
# 3. Can delete detail1
# 4. Hidden detail not removed
# 6. The detail2 remains
# 7. Ensure cleanup parameter doesn't remove hidden details
"""
hidden_detail_name = "configDriveLocation"
detail1 = "detail1"
detail2 = "detail2"
# set up a VM
self.virtual_machine = VirtualMachine.create(
self.apiclient,
self.testdata["virtual_machine"],
accountid=self.account.name,
zoneid=self.zone.id,
domainid=self.account.domainid,
serviceofferingid=self.service_offering.id,
templateid=self.template.id
)
self.cleanup.append(self.virtual_machine)
vm = self.listVmById(self.virtual_machine.id)
self.debug(
"VirtualMachine launched with id, name, displayname: %s %s %s" \
% (self.virtual_machine.id, vm.name, vm.displayname)
)
# set up a hidden detail
dbresult = self.dbclient.execute("select id from vm_instance where uuid='%s'" % vm.id)
self.assertEqual(validateList(dbresult)[0], PASS, "sql query returned invalid response")
vm_db_id = dbresult[0][0]
self.debug("VM has database id %d" % vm_db_id)
self.dbclient.execute("insert into user_vm_details (vm_id, name, value, display) values (%d,'%s','HOST', 0)" % (vm_db_id, hidden_detail_name))
vm = self.listVmById(self.virtual_machine.id)
self.debug("VirtualMachine fetched with details: %s of type %s" % (vm.details, type(vm.details)))
self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden")
# add two details by appending to what was returned via API
updating_vm_details = vm.details.__dict__
updating_vm_details[detail1] = "foo"
updating_vm_details[detail2] = "bar"
self.debug("Updating VM to new details: %s" % updating_vm_details)
vm = self.virtual_machine.update(self.apiclient, details=[updating_vm_details])
self.assertIsNotNone(vm.details[detail1], "Expect " + detail1)
self.assertIsNotNone(vm.details[detail2], "Expect " + detail2)
self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden")
self.assertTrue(self.detailInDatabase(vm_db_id, hidden_detail_name), "hidden detail should still exist in db")
# delete one detail
updating_vm_details = vm.details.__dict__
del updating_vm_details["detail1"]
self.debug("Deleting one detail by updating details: %s" % updating_vm_details)
vm = self.virtual_machine.update(self.apiclient, details=[updating_vm_details])
self.assertIsNone(vm.details[detail1], "Do not expect " + detail1)
self.assertIsNotNone(vm.details[detail2], "Expect " + detail2)
self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden")
self.assertTrue(self.detailInDatabase(vm_db_id, hidden_detail_name), "hidden detail should still exist in db")
# cleanup, ensure hidden detail is not deleted
vm = self.virtual_machine.update(self.apiclient, cleanupdetails="true")
self.assertIsNone(vm.details[detail1], "Do not expect " + detail1)
self.assertIsNone(vm.details[detail2], "Do not expect " + detail2)
self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden")
self.assertTrue(self.detailInDatabase(vm_db_id, hidden_detail_name), "hidden detail should still exist in db")
def detailInDatabase(self, vm_id, detail_name):
dbresult = self.dbclient.execute("select id from user_vm_details where vm_id=%s and name='%s'" % (vm_id, detail_name))
self.debug("Detail %s for VM %s: %s" % (detail_name, vm_id, dbresult))
if validateList(dbresult)[0] == PASS:
return True
return False
def listVmById(self, id):
list_vms = VirtualMachine.list(self.apiclient, id=id)
self.assertEqual(
isinstance(list_vms, list),
True,
"List VM response was not a valid list"
)
self.assertNotEqual(
len(list_vms),
0,
"List VM response was empty"
)
return list_vms[0]
def tearDown(self):
try:
cleanup_resources(self.apiclient, self.cleanup)