Don't allow inadvertent deletion of hidden details via API

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

@@ -2756,13 +2756,18 @@ public UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
         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 UserVm updateVirtualMachine(UpdateVMCmd cmd) throws ResourceUnavailableEx
                         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);

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

@@ -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 void updateVirtualMachineTestDisplayChanged() throws ResourceUnavailableE
         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 void updateVirtualMachineTestCleanUpTrue() throws ResourceUnavailableExce
         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 void updateVirtualMachineTestCleanUpFalseAndDetailsEmpty() throws Resourc
         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 @@ private void prepareAndExecuteMethodDealingWithDetails(boolean cleanUpDetails, b
         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 @@ private void prepareAndExecuteMethodDealingWithDetails(boolean cleanUpDetails, b
         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);
     }
 

test/integration/component/test_update_vm.py

@@ -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 @@ def test_update_vm_name(self):
             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 @@ def test_update_vm_name(self):
         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)