From 99bd193688a1015b4a037f65b2e2509db244ab40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Sun, 3 Sep 2017 03:11:48 +0200 Subject: [PATCH] Rename 'dispvm_allowed' to 'template_for_dispvms' 'dispvm_allowed' name was confusing, because it suggested being able to spawn new DispVMs, not being a template for DispVM. Fixes QubesOS/qubes-issues#3047 --- qubes/api/internal.py | 3 ++- qubes/tests/api_admin.py | 4 ++-- qubes/tests/vm/appvm.py | 25 +++++++++++++++++++++++++ qubes/tests/vm/dispvm.py | 8 ++++---- qubes/vm/appvm.py | 11 ++++++++++- qubes/vm/dispvm.py | 12 ++++++------ qubespolicy/__init__.py | 8 ++++---- qubespolicy/cli.py | 3 ++- qubespolicy/graph.py | 2 +- qubespolicy/tests/__init__.py | 22 +++++++++++----------- qubespolicy/tests/cli.py | 8 ++++---- 11 files changed, 71 insertions(+), 35 deletions(-) diff --git a/qubes/api/internal.py b/qubes/api/internal.py index a458309c..3ae5423b 100644 --- a/qubes/api/internal.py +++ b/qubes/api/internal.py @@ -46,7 +46,8 @@ class QubesInternalAPI(qubes.api.AbstractQubesAPI): domain.name: { 'tags': list(domain.tags), 'type': domain.__class__.__name__, - 'dispvm_allowed': getattr(domain, 'dispvm_allowed', False), + 'template_for_dispvms': + getattr(domain, 'template_for_dispvms', False), 'default_dispvm': (str(domain.default_dispvm) if getattr(domain, 'default_dispvm', None) else None), 'icon': str(domain.label.icon), diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py index 7ce27f4a..c5933be1 100644 --- a/qubes/tests/api_admin.py +++ b/qubes/tests/api_admin.py @@ -2102,7 +2102,7 @@ class TC_00_VMs(AdminAPITestCase): @unittest.mock.patch('qubes.storage.Storage.create') def test_640_vm_create_disposable(self, mock_storage): mock_storage.side_effect = self.dummy_coro - self.vm.dispvm_allowed = True + self.vm.template_for_dispvms = True retval = self.call_mgmt_func(b'admin.vm.CreateDisposable', b'test-vm1') self.assertTrue(retval.startswith('disp')) @@ -2115,7 +2115,7 @@ class TC_00_VMs(AdminAPITestCase): @unittest.mock.patch('qubes.storage.Storage.create') def test_641_vm_create_disposable_default(self, mock_storage): mock_storage.side_effect = self.dummy_coro - self.vm.dispvm_allowed = True + self.vm.template_for_dispvms = True self.app.default_dispvm = self.vm retval = self.call_mgmt_func(b'admin.vm.CreateDisposable', b'dom0') diff --git a/qubes/tests/vm/appvm.py b/qubes/tests/vm/appvm.py index ecfb9283..4aeb1487 100644 --- a/qubes/tests/vm/appvm.py +++ b/qubes/tests/vm/appvm.py @@ -20,6 +20,8 @@ from unittest import mock +import lxml.etree + import qubes.storage import qubes.tests import qubes.tests.vm.qubesvm @@ -118,3 +120,26 @@ class TC_90_AppVM(qubes.tests.vm.qubesvm.QubesVMTestsMixin, mock_power.return_value = 'Running' with self.assertRaises(qubes.exc.QubesVMNotHaltedError): vm.template = self.template + + def test_500_property_migrate_template_for_dispvms(self): + xml_template = ''' + + + 1 + testvm + + {value} + + + ''' + xml = lxml.etree.XML(xml_template.format(value='True')) + vm = qubes.vm.appvm.AppVM(self.app, xml) + self.assertEqual(vm.template_for_dispvms, True) + with self.assertRaises(AttributeError): + vm.dispvm_allowed + + xml = lxml.etree.XML(xml_template.format(value='False')) + vm = qubes.vm.appvm.AppVM(self.app, xml) + self.assertEqual(vm.template_for_dispvms, False) + with self.assertRaises(AttributeError): + vm.dispvm_allowed diff --git a/qubes/tests/vm/dispvm.py b/qubes/tests/vm/dispvm.py index c568dfcb..69c9d096 100644 --- a/qubes/tests/vm/dispvm.py +++ b/qubes/tests/vm/dispvm.py @@ -67,7 +67,7 @@ class TC_00_DispVM(qubes.tests.QubesTestCase): @mock.patch('qubes.storage.Storage') def test_000_from_appvm(self, mock_storage, mock_makedirs, mock_symlink): mock_storage.return_value.create.side_effect = self.mock_coro - self.appvm.dispvm_allowed = True + self.appvm.template_for_dispvms = True orig_getitem = self.app.domains.__getitem__ with mock.patch.object(self.app, 'domains', wraps=self.app.domains) \ as mock_domains: @@ -78,7 +78,7 @@ class TC_00_DispVM(qubes.tests.QubesTestCase): dispvm = self.loop.run_until_complete( qubes.vm.dispvm.DispVM.from_appvm(self.appvm)) mock_domains.get_new_unused_dispid.assert_called_once_with() - self.assertTrue(dispvm.name.startswith('disp')) + self.assertEqual(dispvm.name, 'disp42') self.assertEqual(dispvm.template, self.appvm) self.assertEqual(dispvm.label, self.appvm.label) self.assertEqual(dispvm.label, self.appvm.label) @@ -95,7 +95,7 @@ class TC_00_DispVM(qubes.tests.QubesTestCase): qubes.vm.dispvm.DispVM.from_appvm(self.appvm)) def test_010_create_direct(self): - self.appvm.dispvm_allowed = True + self.appvm.template_for_dispvms = True orig_getitem = self.app.domains.__getitem__ with mock.patch.object(self.app, 'domains', wraps=self.app.domains) \ as mock_domains: @@ -113,7 +113,7 @@ class TC_00_DispVM(qubes.tests.QubesTestCase): self.assertEqual(dispvm.auto_cleanup, False) def test_011_create_direct_generate_name(self): - self.appvm.dispvm_allowed = True + self.appvm.template_for_dispvms = True orig_getitem = self.app.domains.__getitem__ with mock.patch.object(self.app, 'domains', wraps=self.app.domains) \ as mock_domains: diff --git a/qubes/vm/appvm.py b/qubes/vm/appvm.py index efaaa23b..b853265b 100644 --- a/qubes/vm/appvm.py +++ b/qubes/vm/appvm.py @@ -36,7 +36,7 @@ class AppVM(qubes.vm.qubesvm.QubesVM): vmclass=qubes.vm.templatevm.TemplateVM, doc='Template, on which this AppVM is based.') - dispvm_allowed = qubes.property('dispvm_allowed', + template_for_dispvms = qubes.property('template_for_dispvms', type=bool, default=False, doc='Should this VM be allowed to start as Disposable VM') @@ -72,6 +72,15 @@ class AppVM(qubes.vm.qubesvm.QubesVM): } def __init__(self, app, xml, **kwargs): + # migrate renamed properties + if xml is not None: + node_dispvm_allowed = xml.find( + './properties/property[@name=\'dispvm_allowed\']') + if node_dispvm_allowed is not None: + kwargs['template_for_dispvms'] = \ + qubes.property.bool(None, None, node_dispvm_allowed.text) + node_dispvm_allowed.getparent().remove(node_dispvm_allowed) + self.volume_config = copy.deepcopy(self.default_volume_config) template = kwargs.get('template', None) diff --git a/qubes/vm/dispvm.py b/qubes/vm/dispvm.py index 553d7b9b..c70f4449 100644 --- a/qubes/vm/dispvm.py +++ b/qubes/vm/dispvm.py @@ -79,10 +79,10 @@ class DispVM(qubes.vm.qubesvm.QubesVM): if xml is None: assert template is not None - if not template.dispvm_allowed: + if not template.template_for_dispvms: raise qubes.exc.QubesValueError( 'template for DispVM ({}) needs to have ' - 'dispvm_allowed=True'.format(template.name)) + 'template_for_dispvms=True'.format(template.name)) if 'dispid' not in kwargs: kwargs['dispid'] = app.domains.get_new_unused_dispid() @@ -161,10 +161,10 @@ class DispVM(qubes.vm.qubesvm.QubesVM): This method modifies :file:`qubes.xml` file. The qube returned is not started. ''' - if not appvm.dispvm_allowed: + if not appvm.template_for_dispvms: raise qubes.exc.QubesException( 'Refusing to create DispVM out of this AppVM, because ' - 'dispvm_allowed=False') + 'template_for_appvms=False') app = appvm.app dispvm = app.add_new_vm( cls, @@ -198,9 +198,9 @@ class DispVM(qubes.vm.qubesvm.QubesVM): # pylint: disable=arguments-differ # sanity check, if template_for_dispvm got changed in the meantime - if not self.template.dispvm_allowed: + if not self.template.template_for_dispvms: raise qubes.exc.QubesException( 'template for DispVM ({}) needs to have ' - 'dispvm_allowed=True'.format(self.template.name)) + 'template_for_dispvms=True'.format(self.template.name)) yield from super(DispVM, self).start(**kwargs) diff --git a/qubespolicy/__init__.py b/qubespolicy/__init__.py index c3b5da10..1f48dd3f 100755 --- a/qubespolicy/__init__.py +++ b/qubespolicy/__init__.py @@ -80,7 +80,7 @@ def verify_target_value(system_info, value): if dispvm_base not in system_info['domains']: return False dispvm_base_info = system_info['domains'][dispvm_base] - return bool(dispvm_base_info['dispvm_allowed']) + return bool(dispvm_base_info['template_for_dispvms']) else: return value in system_info['domains'] @@ -307,13 +307,13 @@ class PolicyRule(object): for name, domain in system_info['domains'].items(): if name != 'dom0': yield name - if domain['dispvm_allowed']: + if domain['template_for_dispvms']: yield '$dispvm:' + name yield '$dispvm' elif self.target.startswith('$dispvm:'): dispvm_base = self.target.split(':', 1)[1] try: - if system_info['domains'][dispvm_base]['dispvm_allowed']: + if system_info['domains'][dispvm_base]['template_for_dispvms']: yield self.target except KeyError: # TODO log a warning? @@ -685,7 +685,7 @@ def get_system_info(): - ``: - tags: list of tags - type: domain type - - dispvm_allowed: should DispVM based on this VM be allowed + - template_for_dispvms: should DispVM based on this VM be allowed - default_dispvm: name of default AppVM for DispVMs started from here ''' diff --git a/qubespolicy/cli.py b/qubespolicy/cli.py index 14359e43..25c6fa63 100644 --- a/qubespolicy/cli.py +++ b/qubespolicy/cli.py @@ -115,7 +115,8 @@ def main(args=None): icons = {name: system_info['domains'][name]['icon'] for name in system_info['domains'].keys()} for dispvm_base in system_info['domains']: - if not system_info['domains'][dispvm_base]['dispvm_allowed']: + if not (system_info['domains'][dispvm_base] + ['template_for_dispvms']): continue dispvm_api_name = '$dispvm:' + dispvm_base icons[dispvm_api_name] = \ diff --git a/qubespolicy/graph.py b/qubespolicy/graph.py index 159de772..46308350 100644 --- a/qubespolicy/graph.py +++ b/qubespolicy/graph.py @@ -87,7 +87,7 @@ def main(args=None): targets = list(system_info['domains'].keys()) targets.append('$dispvm') targets.extend('$dispvm:' + dom for dom in system_info['domains'] - if system_info['domains'][dom]['dispvm_allowed']) + if system_info['domains'][dom]['template_for_dispvms']) connections = set() diff --git a/qubespolicy/tests/__init__.py b/qubespolicy/tests/__init__.py index f1e4ac7c..86adb460 100644 --- a/qubespolicy/tests/__init__.py +++ b/qubespolicy/tests/__init__.py @@ -34,55 +34,55 @@ system_info = { 'tags': ['dom0-tag'], 'type': 'AdminVM', 'default_dispvm': 'default-dvm', - 'dispvm_allowed': False, + 'template_for_dispvms': False, }, 'test-vm1': { 'tags': ['tag1', 'tag2'], 'type': 'AppVM', 'default_dispvm': 'default-dvm', - 'dispvm_allowed': False, + 'template_for_dispvms': False, }, 'test-vm2': { 'tags': ['tag2'], 'type': 'AppVM', 'default_dispvm': 'default-dvm', - 'dispvm_allowed': False, + 'template_for_dispvms': False, }, 'test-vm3': { 'tags': [], 'type': 'AppVM', 'default_dispvm': 'default-dvm', - 'dispvm_allowed': True, + 'template_for_dispvms': True, }, 'default-dvm': { 'tags': [], 'type': 'AppVM', 'default_dispvm': 'default-dvm', - 'dispvm_allowed': True, + 'template_for_dispvms': True, }, 'test-invalid-dvm': { 'tags': ['tag1', 'tag2'], 'type': 'AppVM', 'default_dispvm': 'test-vm1', - 'dispvm_allowed': False, + 'template_for_dispvms': False, }, 'test-no-dvm': { 'tags': ['tag1', 'tag2'], 'type': 'AppVM', 'default_dispvm': None, - 'dispvm_allowed': False, + 'template_for_dispvms': False, }, 'test-template': { 'tags': ['tag1', 'tag2'], 'type': 'TemplateVM', 'default_dispvm': 'default-dvm', - 'dispvm_allowed': False, + 'template_for_dispvms': False, }, 'test-standalone': { 'tags': ['tag1', 'tag2'], 'type': 'StandaloneVM', 'default_dispvm': 'default-dvm', - 'dispvm_allowed': False, + 'template_for_dispvms': False, }, } } @@ -268,10 +268,10 @@ class TC_00_PolicyRule(qubes.tests.QubesTestCase): self.assertFalse(is_match_single(system_info, '$default', 'test-vm1')) self.assertFalse(is_match_single(system_info, '$tag:tag1', 'test-vm3')) self.assertFalse(is_match_single(system_info, '$anyvm', 'no-such-vm')) - # test-vm1.dispvm_allowed=False + # test-vm1.template_for_dispvms=False self.assertFalse(is_match_single(system_info, '$anyvm', '$dispvm:test-vm1')) - # test-vm1.dispvm_allowed=False + # test-vm1.template_for_dispvms=False self.assertFalse(is_match_single(system_info, '$dispvm:test-vm1', '$dispvm:test-vm1')) self.assertFalse(is_match_single(system_info, '$anyvm', 'dom0')) diff --git a/qubespolicy/tests/cli.py b/qubespolicy/tests/cli.py index a7387939..0668972d 100644 --- a/qubespolicy/tests/cli.py +++ b/qubespolicy/tests/cli.py @@ -41,10 +41,10 @@ class TC_00_qrexec_policy(qubes.tests.QubesTestCase): self.system_info_mock = self.system_info_patch.start() self.system_info = { - 'domains': {'dom0': {'icon': 'black', 'dispvm_allowed': False}, - 'test-vm1': {'icon': 'red', 'dispvm_allowed': False}, - 'test-vm2': {'icon': 'red', 'dispvm_allowed': False}, - 'test-vm3': {'icon': 'green', 'dispvm_allowed': True}, }} + 'domains': {'dom0': {'icon': 'black', 'template_for_dispvms': False}, + 'test-vm1': {'icon': 'red', 'template_for_dispvms': False}, + 'test-vm2': {'icon': 'red', 'template_for_dispvms': False}, + 'test-vm3': {'icon': 'green', 'template_for_dispvms': True}, }} self.system_info_mock.return_value = self.system_info self.dbus_patch = unittest.mock.patch('pydbus.SystemBus')