From 65d15e604033089250b75d1f7db9cebd277bfa86 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Mon, 26 Jun 2017 13:06:22 +0200
Subject: [PATCH 1/7] api/admin: skip firewall in vm.Clone

This operation is going to be removed, so apply a quick fix for tests.

QubesOS/qubes-issues#2622
---
 qubes/api/admin.py       | 2 +-
 qubes/tests/api_admin.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/qubes/api/admin.py b/qubes/api/admin.py
index 108e4555..ce198f19 100644
--- a/qubes/api/admin.py
+++ b/qubes/api/admin.py
@@ -853,7 +853,7 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI):
             dst_vm.clone_properties(src_vm)
             dst_vm.tags.update(src_vm.tags)
             dst_vm.features.update(src_vm.features)
-            dst_vm.firewall.clone(src_vm.firewall)
+            #dst_vm.firewall.clone(src_vm.firewall)
             for devclass in src_vm.devices:
                 for device_assignment in src_vm.devices[devclass].assignments():
                     dst_vm.devices[devclass].attach(device_assignment.clone())
diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py
index 7eec7619..fde83162 100644
--- a/qubes/tests/api_admin.py
+++ b/qubes/tests/api_admin.py
@@ -1250,7 +1250,7 @@ class TC_00_VMs(AdminAPITestCase):
         self.assertEqual(vm.template, self.app.domains['test-template'])
         self.assertEqual(vm.tags, self.vm.tags)
         self.assertEqual(vm.features, self.vm.features)
-        self.assertEqual(vm.firewall, self.vm.firewall)
+        #self.assertEqual(vm.firewall, self.vm.firewall)
         self.assertEqual(mock_clone.mock_calls,
             [unittest.mock.call(self.app.domains['test-vm2']).clone(
                 self.app.domains['test-vm1'])])

From ae600e24bf7f4ce5138cfbb8dc58a64e6cf31cc1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Mon, 26 Jun 2017 02:46:49 +0200
Subject: [PATCH 2/7] storage: simplify pool.volumes usage

Add convenient collection wrapper for easier getting selected volume.
Storage pool implementation may still provide only volume listing
function (pool.list_volumes), or, additionally, optimized
pool.get_volume.

This means it is both possible to iterate over volumes:
```python
for volume in pool.volumes:
    ...

```

And get a single volume:
```python
volume = pool.volumes[vid]
```

QubesOS/qubes-issues#2256
---
 qubes/storage/__init__.py | 66 ++++++++++++++++++++++++++++++++++++++-
 qubes/storage/file.py     |  3 +-
 qubes/storage/lvm.py      |  3 +-
 3 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py
index 1f80c2bb..b8d4f7be 100644
--- a/qubes/storage/__init__.py
+++ b/qubes/storage/__init__.py
@@ -614,6 +614,58 @@ class Storage(object):
         return self.vm.volumes[volume].import_data_end(success=success)
 
 
+class VolumesCollection(object):
+    '''Convenient collection wrapper for pool.get_volume and
+    pool.list_volumes
+    '''
+    def __init__(self, pool):
+        self._pool = pool
+
+    def __getitem__(self, item):
+        ''' Get a single volume with given Volume ID.
+
+        You can also a Volume instance to get the same Volume or KeyError if
+        Volume no longer exists.
+
+        :param item: a Volume ID (str) or a Volume instance
+        '''
+        if isinstance(item, Volume):
+            if item.pool == self._pool:
+                return self[item.vid]
+            else:
+                raise KeyError(item)
+        try:
+            return self._pool.get_volume(item)
+        except NotImplementedError:
+            for vol in self:
+                if vol.vid == item:
+                    return vol
+            # if list_volumes is not implemented too, it will raise
+            # NotImplementedError again earlier
+            raise KeyError(item)
+
+    def __iter__(self):
+        ''' Get iterator over pool's volumes '''
+        return iter(self._pool.list_volumes())
+
+    def __contains__(self, item):
+        ''' Check if given volume (either Volume ID or Volume instance) is
+        present in the pool
+        '''
+        try:
+            return self[item] is not None
+        except KeyError:
+            return False
+
+    def keys(self):
+        ''' Return list of volume IDs '''
+        return [vol.vid for vol in self]
+
+    def values(self):
+        ''' Return list of Volumes'''
+        return [vol for vol in self]
+
+
 class Pool(object):
     ''' A Pool is used to manage different kind of volumes (File
         based/LVM/Btrfs/...).
@@ -626,6 +678,7 @@ class Pool(object):
 
     def __init__(self, name, revisions_to_keep=1, **kwargs):
         super(Pool, self).__init__(**kwargs)
+        self._volumes_collection = VolumesCollection(self)
         self.name = name
         self.revisions_to_keep = revisions_to_keep
         kwargs['name'] = self.name
@@ -684,8 +737,19 @@ class Pool(object):
 
     @property
     def volumes(self):
+        ''' Return a collection of volumes managed by this pool '''
+        return self._volumes_collection
+
+    def list_volumes(self):
         ''' Return a list of volumes managed by this pool '''
-        raise self._not_implemented("volumes")
+        raise self._not_implemented("list_volumes")
+
+    def get_volume(self, vid):
+        ''' Return a volume with *vid* from this pool
+
+        :raise KeyError: if no volume is found
+        '''
+        raise self._not_implemented("get_volume")
 
     def _not_implemented(self, method_name):
         ''' Helper for emitting helpful `NotImplementedError` exceptions '''
diff --git a/qubes/storage/file.py b/qubes/storage/file.py
index dd216508..0241e4e3 100644
--- a/qubes/storage/file.py
+++ b/qubes/storage/file.py
@@ -152,8 +152,7 @@ class FilePool(qubes.storage.Pool):
 
         return os.path.join(self.dir_path, self._vid_prefix(vm))
 
-    @property
-    def volumes(self):
+    def list_volumes(self):
         return self._volumes
 
 
diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py
index 37ae0837..e6784b03 100644
--- a/qubes/storage/lvm.py
+++ b/qubes/storage/lvm.py
@@ -120,8 +120,7 @@ class ThinPool(qubes.storage.Pool):
     def setup(self):
         pass  # TODO Should we create a non existing pool?
 
-    @property
-    def volumes(self):
+    def list_volumes(self):
         ''' Return a list of volumes managed by this pool '''
         volumes = []
         for vid, vol_info in size_cache.items():

From a748b393f4dcd071ea4d38b32503e9c6bbf8a640 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Mon, 26 Jun 2017 11:39:02 +0200
Subject: [PATCH 3/7] storage: move remove() to Volume

This is continuation of 0f12870 "storage: use direct object references,
not only identifiers".

QubesOS/qubes-issues#2256
---
 qubes/storage/__init__.py  | 17 ++++++++---------
 qubes/storage/file.py      | 20 ++++++++++----------
 qubes/storage/kernels.py   |  6 +++---
 qubes/storage/lvm.py       | 24 ++++++++++++------------
 qubes/tests/storage_lvm.py |  4 ++--
 5 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py
index b8d4f7be..47507969 100644
--- a/qubes/storage/__init__.py
+++ b/qubes/storage/__init__.py
@@ -146,6 +146,12 @@ class Volume(object):
         '''
         raise self._not_implemented("create")
 
+    def remove(self):
+        ''' Remove volume.
+
+        This can be implemented as a coroutine.'''
+        raise self._not_implemented("remove")
+
     def commit(self):
         ''' Write the snapshot to disk
 
@@ -520,7 +526,7 @@ class Storage(object):
         for name, volume in self.vm.volumes.items():
             self.log.info('Removing volume %s: %s' % (name, volume.vid))
             try:
-                ret = volume.pool.remove(volume)
+                ret = volume.remove()
                 if asyncio.iscoroutine(ret):
                     futures.append(ret)
             except (IOError, OSError) as e:
@@ -719,12 +725,6 @@ class Pool(object):
         '''
         raise self._not_implemented("init_volume")
 
-    def remove(self, volume):
-        ''' Remove volume.
-
-        This can be implemented as a coroutine.'''
-        raise self._not_implemented("remove")
-
     def rename(self, volume, old_name, new_name):
         ''' Called when the domain changes its name '''
         raise self._not_implemented("rename")
@@ -804,8 +804,7 @@ class VmCreationManager(object):
         if type is not None and value is not None and tb is not None:
             for volume in self.vm.volumes.values():
                 try:
-                    pool = volume.pool
-                    pool.remove(volume)
+                    volume.remove()
                 except Exception:  # pylint: disable=broad-except
                     pass
             os.rmdir(self.vm.dir_path)
diff --git a/qubes/storage/file.py b/qubes/storage/file.py
index 0241e4e3..eb999c09 100644
--- a/qubes/storage/file.py
+++ b/qubes/storage/file.py
@@ -79,16 +79,6 @@ class FilePool(qubes.storage.Pool):
         self._volumes += [volume]
         return volume
 
-    def remove(self, volume):
-        if not volume.internal:
-            return  # do not remove random attached file volumes
-        elif volume._is_snapshot:
-            return  # no need to remove, because it's just a snapshot
-        else:
-            _remove_if_exists(volume.path)
-            if volume._is_origin:
-                _remove_if_exists(volume.path_cow)
-
     def rename(self, volume, old_name, new_name):
         assert issubclass(volume.__class__, FileVolume)
         subdir, _, volume_path = volume.vid.split('/', 2)
@@ -201,6 +191,16 @@ class FileVolume(qubes.storage.Volume):
             else:
                 create_sparse_file(self.path, self.size)
 
+    def remove(self):
+        if not self.internal:
+            return  # do not remove random attached file volumes
+        elif self._is_snapshot:
+            return  # no need to remove, because it's just a snapshot
+        else:
+            _remove_if_exists(self.path)
+            if self._is_origin:
+                _remove_if_exists(self.path_cow)
+
     def is_dirty(self):
         return False  # TODO: How to implement this?
 
diff --git a/qubes/storage/kernels.py b/qubes/storage/kernels.py
index 3637dea5..1f965e92 100644
--- a/qubes/storage/kernels.py
+++ b/qubes/storage/kernels.py
@@ -93,6 +93,9 @@ class LinuxModules(Volume):
     def create(self):
         return self
 
+    def remove(self):
+        pass
+
     def commit(self):
         return self
 
@@ -154,9 +157,6 @@ class LinuxKernel(Pool):
     def import_volume(self, dst_pool, dst_volume, src_pool, src_volume):
         pass
 
-    def remove(self, volume):
-        pass
-
     def rename(self, volume, old_name, new_name):
         return volume
 
diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py
index e6784b03..1c5e66c2 100644
--- a/qubes/storage/lvm.py
+++ b/qubes/storage/lvm.py
@@ -88,18 +88,6 @@ class ThinPool(qubes.storage.Pool):
         volume_config['pool'] = self
         return ThinVolume(**volume_config)
 
-    def remove(self, volume):
-        assert volume.vid
-        if volume.is_dirty():
-            cmd = ['remove', volume._vid_snap]
-            qubes_lvm(cmd, self.log)
-
-        if not os.path.exists(volume.path):
-            return
-        cmd = ['remove', volume.vid]
-        qubes_lvm(cmd, self.log)
-        reset_cache()
-
     def rename(self, volume, old_name, new_name):
         ''' Called when the domain changes its name '''
         new_vid = "{!s}/vm-{!s}-{!s}".format(self.volume_group, new_name,
@@ -296,6 +284,18 @@ class ThinVolume(qubes.storage.Volume):
             reset_cache()
         return self
 
+    def remove(self):
+        assert self.vid
+        if self.is_dirty():
+            cmd = ['remove', self._vid_snap]
+            qubes_lvm(cmd, self.log)
+
+        if not os.path.exists(self.path):
+            return
+        cmd = ['remove', self.vid]
+        qubes_lvm(cmd, self.log)
+        reset_cache()
+
     def export(self):
         ''' Returns an object that can be `open()`. '''
         devpath = '/dev/' + self.vid
diff --git a/qubes/tests/storage_lvm.py b/qubes/tests/storage_lvm.py
index 4d1d90ce..1426e932 100644
--- a/qubes/tests/storage_lvm.py
+++ b/qubes/tests/storage_lvm.py
@@ -137,7 +137,7 @@ class TC_00_ThinPool(ThinPoolBase):
         volume.create()
         path = "/dev/%s" % volume.vid
         self.assertTrue(os.path.exists(path))
-        self.pool.remove(volume)
+        volume.remove()
 
     def test_003_read_write_volume(self):
         ''' Test read-write volume creation '''
@@ -157,7 +157,7 @@ class TC_00_ThinPool(ThinPoolBase):
         volume.create()
         path = "/dev/%s" % volume.vid
         self.assertTrue(os.path.exists(path))
-        self.pool.remove(volume)
+        volume.remove()
 
 @skipUnlessLvmPoolExists
 class TC_01_ThinPool(qubes.tests.SystemTestsMixin, ThinPoolBase):

From 28f78ed3b851fad56c010af5556e05887f380774 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Mon, 26 Jun 2017 11:42:24 +0200
Subject: [PATCH 4/7] storage/lvm: minor fixes

QubesOS/qubes-issues#2256
---
 qubes/storage/lvm.py | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py
index 1c5e66c2..8c1ca86b 100644
--- a/qubes/storage/lvm.py
+++ b/qubes/storage/lvm.py
@@ -120,7 +120,7 @@ class ThinPool(qubes.storage.Pool):
                 # implementation detail volume
                 continue
             config = {
-                'pool': self.name,
+                'pool': self,
                 'vid': vid,
                 'name': vid,
                 'volume_group': self.volume_group,
@@ -305,8 +305,6 @@ class ThinVolume(qubes.storage.Volume):
         if not src_volume.save_on_stop:
             return self
 
-        src_path = src_volume.export()
-
         # HACK: neat trick to speed up testing if you have same physical thin
         # pool assigned to two qubes-pools i.e: qubes_dom0 and test-lvm
         # pylint: disable=line-too-long
@@ -316,7 +314,7 @@ class ThinVolume(qubes.storage.Volume):
             qubes_lvm(cmd, self.log)
         else:
             self.create()
-
+            src_path = src_volume.export()
             cmd = ['sudo', 'dd', 'if=' + src_path, 'of=/dev/' + self.vid,
                 'conv=sparse']
             subprocess.check_call(cmd)

From fabd8119b49520652de469d71c944adc8008b24b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Mon, 26 Jun 2017 11:59:53 +0200
Subject: [PATCH 5/7] storage: volume.import_volume now expect create()d volume

This is much more logical for *import*_volume function.

QubesOS/qubes-issues#2256
---
 qubes/storage/__init__.py | 19 ++++++++-----------
 qubes/storage/file.py     |  1 +
 qubes/storage/lvm.py      |  3 ++-
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/qubes/storage/__init__.py b/qubes/storage/__init__.py
index 47507969..22123b81 100644
--- a/qubes/storage/__init__.py
+++ b/qubes/storage/__init__.py
@@ -177,7 +177,11 @@ class Volume(object):
 
     def import_volume(self, src_volume):
         ''' Imports data from a different volume (possibly in a different
-        pool '''
+        pool.
+
+        The needs to be create()d first.
+
+        This can be implemented as a coroutine. '''
         # pylint: disable=unused-argument
         raise self._not_implemented("import_volume")
 
@@ -448,16 +452,9 @@ class Storage(object):
         dst_pool = self.vm.app.get_pool(config['pool'])
         dst = dst_pool.init_volume(self.vm, config)
         src_volume = src_vm.volumes[name]
-        src_pool = src_volume.pool
-        if dst_pool == src_pool:
-            msg = "Cloning volume {!s} from vm {!s}"
-            self.vm.log.info(msg.format(src_volume.name, src_vm.name))
-            clone_op_ret = dst_pool.clone(src_volume, dst)
-        else:
-            msg = "Importing volume {!s} from vm {!s}"
-            self.vm.log.info(msg.format(src_volume.name, src_vm.name))
-            clone_op_ret = dst_pool.import_volume(
-                dst_pool, dst, src_pool, src_volume)
+        msg = "Importing volume {!s} from vm {!s}"
+        self.vm.log.info(msg.format(src_volume.name, src_vm.name))
+        clone_op_ret = dst.import_volume(src_volume)
 
         # clone/import functions may be either synchronous or asynchronous
         # in the later case, we need to wait for them to finish
diff --git a/qubes/storage/file.py b/qubes/storage/file.py
index eb999c09..5b352254 100644
--- a/qubes/storage/file.py
+++ b/qubes/storage/file.py
@@ -261,6 +261,7 @@ class FileVolume(qubes.storage.Volume):
         msg = msg.format(src_volume, self)
         assert not src_volume.snap_on_start, msg
         if self.save_on_stop:
+            _remove_if_exists(self.path)
             copy_file(src_volume.export(), self.path)
         return self
 
diff --git a/qubes/storage/lvm.py b/qubes/storage/lvm.py
index 8c1ca86b..77f94d0c 100644
--- a/qubes/storage/lvm.py
+++ b/qubes/storage/lvm.py
@@ -310,10 +310,11 @@ class ThinVolume(qubes.storage.Volume):
         # pylint: disable=line-too-long
         if isinstance(src_volume.pool, ThinPool) and \
                 src_volume.pool.thin_pool == self.pool.thin_pool:  # NOQA
+            cmd = ['remove', self.vid]
+            qubes_lvm(cmd, self.log)
             cmd = ['clone', str(src_volume), str(self)]
             qubes_lvm(cmd, self.log)
         else:
-            self.create()
             src_path = src_volume.export()
             cmd = ['sudo', 'dd', 'if=' + src_path, 'of=/dev/' + self.vid,
                 'conv=sparse']

From 3dcd29afeab97a5d3d5111fad71b907bd3dd6306 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Mon, 26 Jun 2017 12:48:34 +0200
Subject: [PATCH 6/7] api/admin: remove admin.vm.Clone operation

The same can be achieved with Create+volume.Clone

QubesOS/qubes-issues#2622
---
 Makefile                 |  1 -
 qubes/api/admin.py       | 34 ---------------------
 qubes/tests/api_admin.py | 66 ----------------------------------------
 3 files changed, 101 deletions(-)

diff --git a/Makefile b/Makefile
index 64492ec4..eaf706b2 100644
--- a/Makefile
+++ b/Makefile
@@ -35,7 +35,6 @@ ADMIN_API_METHODS_SIMPLE = \
 	admin.property.List \
 	admin.property.Reset \
 	admin.property.Set \
-	admin.vm.Clone \
 	admin.vm.Create.AppVM \
 	admin.vm.Create.DispVM \
 	admin.vm.Create.StandaloneVM \
diff --git a/qubes/api/admin.py b/qubes/api/admin.py
index ce198f19..cc875743 100644
--- a/qubes/api/admin.py
+++ b/qubes/api/admin.py
@@ -829,40 +829,6 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI):
 
         self.app.save()
 
-    @qubes.api.method('admin.vm.Clone')
-    @asyncio.coroutine
-    def vm_clone(self, untrusted_payload):
-        assert not self.arg
-
-        assert untrusted_payload.startswith(b'name=')
-        untrusted_name = untrusted_payload[5:].decode('ascii')
-        qubes.vm.validate_name(None, None, untrusted_name)
-        new_name = untrusted_name
-
-        del untrusted_payload
-
-        if new_name in self.app.domains:
-            raise qubes.exc.QubesValueError('Already exists')
-
-        self.fire_event_for_permission(new_name=new_name)
-
-        src_vm = self.dest
-
-        dst_vm = self.app.add_new_vm(src_vm.__class__, name=new_name)
-        try:
-            dst_vm.clone_properties(src_vm)
-            dst_vm.tags.update(src_vm.tags)
-            dst_vm.features.update(src_vm.features)
-            #dst_vm.firewall.clone(src_vm.firewall)
-            for devclass in src_vm.devices:
-                for device_assignment in src_vm.devices[devclass].assignments():
-                    dst_vm.devices[devclass].attach(device_assignment.clone())
-            yield from dst_vm.clone_disk_files(src_vm)
-        except:
-            del self.app.domains[dst_vm]
-            raise
-        self.app.save()
-
     @qubes.api.method('admin.vm.device.{endpoint}.Available', endpoints=(ep.name
             for ep in pkg_resources.iter_entry_points('qubes.devices')),
             no_payload=True)
diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py
index fde83162..4da03a97 100644
--- a/qubes/tests/api_admin.py
+++ b/qubes/tests/api_admin.py
@@ -1236,72 +1236,6 @@ class TC_00_VMs(AdminAPITestCase):
         self.assertNotIn('test-vm2', self.app.domains)
         self.assertFalse(self.app.save.called)
 
-    @unittest.mock.patch('qubes.storage.Storage.clone')
-    @unittest.mock.patch('qubes.storage.Storage.verify')
-    def test_350_vm_clone(self, mock_verify, mock_clone):
-        mock_clone.side_effect = self.dummy_coro
-        mock_verify.side_effect = self.dummy_coro
-        self.call_mgmt_func(b'admin.vm.Clone',
-            b'test-vm1', b'', b'name=test-vm2')
-
-        self.assertIn('test-vm2', self.app.domains)
-        vm = self.app.domains['test-vm2']
-        self.assertEqual(vm.label, self.app.get_label('red'))
-        self.assertEqual(vm.template, self.app.domains['test-template'])
-        self.assertEqual(vm.tags, self.vm.tags)
-        self.assertEqual(vm.features, self.vm.features)
-        #self.assertEqual(vm.firewall, self.vm.firewall)
-        self.assertEqual(mock_clone.mock_calls,
-            [unittest.mock.call(self.app.domains['test-vm2']).clone(
-                self.app.domains['test-vm1'])])
-        self.assertTrue(os.path.exists(os.path.join(
-            self.test_base_dir, 'appvms', 'test-vm2')))
-
-        self.assertTrue(self.app.save.called)
-
-    @unittest.mock.patch('qubes.storage.Storage.clone')
-    @unittest.mock.patch('qubes.storage.Storage.verify')
-    def test_351_vm_clone_extra_params(self, mock_verify, mock_clone):
-        mock_clone.side_effect = self.dummy_coro
-        mock_verify.side_effect = self.dummy_coro
-        with self.assertRaises(qubes.exc.QubesException):
-            self.call_mgmt_func(b'admin.vm.Clone',
-                b'test-vm1', b'', b'name=test-vm2 label=red')
-
-        self.assertNotIn('test-vm2', self.app.domains)
-        self.assertEqual(mock_clone.mock_calls, [])
-        self.assertFalse(os.path.exists(os.path.join(
-            self.test_base_dir, 'appvms', 'test-vm2')))
-
-        self.assertFalse(self.app.save.called)
-
-    @unittest.mock.patch('qubes.storage.Storage.clone')
-    @unittest.mock.patch('qubes.storage.Storage.verify')
-    def test_352_vm_clone_duplicate_name(self, mock_verify, mock_clone):
-        mock_clone.side_effect = self.dummy_coro
-        mock_verify.side_effect = self.dummy_coro
-        with self.assertRaises(qubes.exc.QubesException):
-            self.call_mgmt_func(b'admin.vm.Clone',
-                b'test-vm1', b'', b'name=test-vm1')
-
-        self.assertFalse(self.app.save.called)
-
-    @unittest.mock.patch('qubes.storage.Storage.clone')
-    @unittest.mock.patch('qubes.storage.Storage.verify')
-    def test_353_vm_clone_invalid_name(self, mock_verify, mock_clone):
-        mock_clone.side_effect = self.dummy_coro
-        mock_verify.side_effect = self.dummy_coro
-        with self.assertRaises(qubes.exc.QubesException):
-            self.call_mgmt_func(b'admin.vm.Clone',
-                b'test-vm1', b'', b'name=test-vm2/..')
-
-        self.assertNotIn('test-vm2/..', self.app.domains)
-        self.assertEqual(mock_clone.mock_calls, [])
-        self.assertFalse(os.path.exists(os.path.join(
-            self.test_base_dir, 'appvms', 'test-vm2/..')))
-
-        self.assertFalse(self.app.save.called)
-
 
     def test_400_property_list(self):
         # actual function tested for admin.vm.property.* already

From 26a997443233cd3b2255c46a7878cdf78a08c5e7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@invisiblethingslab.com>
Date: Mon, 26 Jun 2017 12:50:34 +0200
Subject: [PATCH 7/7] api/admin: split vm.volume.Clone to CloneFrom and CloneTo

The first operation returns a token, which can be passed to the second
one to actually perform clone operation. This way the caller needs have
power over both source and destination VMs (or at least appropriate
volumes), so it's easier to enforce appropriate qrexec policy.

The pending tokens are stored on Qubes() instance (as QubesAdminAPI is
not persistent). It is design choice to keep them in RAM only - those
are one time use and this way restarting qubesd is a simple way to
invalidate all of them. Otherwise we'd need some additional calls like
CloneCancel or such.

QubesOS/qubes-issues#2622
---
 qubes/api/admin.py       |  55 ++++++++++---
 qubes/tests/api_admin.py | 161 +++++++++++++++++++++------------------
 2 files changed, 130 insertions(+), 86 deletions(-)

diff --git a/qubes/api/admin.py b/qubes/api/admin.py
index cc875743..6fab0166 100644
--- a/qubes/api/admin.py
+++ b/qubes/api/admin.py
@@ -293,22 +293,57 @@ class QubesAdminAPI(qubes.api.AbstractQubesAPI):
         self.dest.storage.get_pool(volume).revert(revision)
         self.app.save()
 
-    @qubes.api.method('admin.vm.volume.Clone')
+    @qubes.api.method('admin.vm.volume.CloneFrom', no_payload=True)
     @asyncio.coroutine
-    def vm_volume_clone(self, untrusted_payload):
+    def vm_volume_clone_from(self):
         assert self.arg in self.dest.volumes.keys()
-        untrusted_target = untrusted_payload.decode('ascii').strip()
-        del untrusted_payload
-        qubes.vm.validate_name(None, None, untrusted_target)
-        target_vm = self.app.domains[untrusted_target]
-        del untrusted_target
-        assert self.arg in target_vm.volumes.keys()
 
         volume = self.dest.volumes[self.arg]
 
-        self.fire_event_for_permission(target_vm=target_vm, volume=volume)
+        self.fire_event_for_permission(volume=volume)
 
-        yield from target_vm.storage.clone_volume(self.dest, self.arg)
+        token = qubes.utils.random_string(32)
+        # save token on self.app, as self is not persistent
+        if not hasattr(self.app, 'api_admin_pending_clone'):
+            self.app.api_admin_pending_clone = {}
+        # don't handle collisions any better - if someone is so much out of
+        # luck, can try again anyway
+        assert token not in self.app.api_admin_pending_clone
+
+        self.app.api_admin_pending_clone[token] = volume
+        return token
+
+    @qubes.api.method('admin.vm.volume.CloneTo')
+    @asyncio.coroutine
+    def vm_volume_clone_to(self, untrusted_payload):
+        assert self.arg in self.dest.volumes.keys()
+        untrusted_token = untrusted_payload.decode('ascii').strip()
+        del untrusted_payload
+        assert untrusted_token in getattr(self.app,
+            'api_admin_pending_clone', {})
+        token = untrusted_token
+        del untrusted_token
+
+        src_volume = self.app.api_admin_pending_clone[token]
+        del self.app.api_admin_pending_clone[token]
+
+        # make sure the volume still exists, but invalidate token anyway
+        assert str(src_volume.pool) in self.app.pools
+        assert src_volume in self.app.pools[str(src_volume.pool)].volumes
+
+        dst_volume = self.dest.volumes[self.arg]
+
+        self.fire_event_for_permission(src_volume=src_volume,
+            dst_volume=dst_volume)
+
+        op_retval = dst_volume.import_volume(src_volume)
+
+        # clone/import functions may be either synchronous or asynchronous
+        # in the later case, we need to wait for them to finish
+        if asyncio.iscoroutine(op_retval):
+            op_retval = yield from op_retval
+
+        self.dest.volumes[self.arg] = op_retval
         self.app.save()
 
     @qubes.api.method('admin.vm.volume.Resize')
diff --git a/qubes/tests/api_admin.py b/qubes/tests/api_admin.py
index 4da03a97..8a33eae9 100644
--- a/qubes/tests/api_admin.py
+++ b/qubes/tests/api_admin.py
@@ -21,6 +21,7 @@
 ''' Tests for management calls endpoints '''
 
 import asyncio
+import operator
 import os
 import shutil
 import unittest.mock
@@ -31,6 +32,7 @@ import qubes
 import qubes.devices
 import qubes.api.admin
 import qubes.tests
+import qubes.storage
 
 # properties defined in API
 volume_properties = [
@@ -1532,93 +1534,100 @@ class TC_00_VMs(AdminAPITestCase):
                 self.call_mgmt_func(b'admin.vm.volume.Import', b'test-vm1',
                     b'private')
 
+    def setup_for_clone(self):
+        self.pool = unittest.mock.MagicMock()
+        self.app.pools['test'] = self.pool
+        self.vm2 = self.app.add_new_vm('AppVM', label='red',
+            name='test-vm2',
+            template='test-template', kernel='')
+        self.pool.configure_mock(**{
+            'volumes': qubes.storage.VolumesCollection(self.pool),
+            'init_volume.return_value.pool': self.pool,
+            '__str__.return_value': 'test',
+            'get_volume.side_effect': (lambda vid:
+                self.vm.volumes['private']
+                    if vid is self.vm.volumes['private'].vid
+                    else self.vm2.volumes['private']
+            ),
+        })
+        self.loop.run_until_complete(
+            self.vm.create_on_disk(pool='test'))
+        self.loop.run_until_complete(
+            self.vm2.create_on_disk(pool='test'))
+
+        # the call replaces self.vm.volumes[...] with result of import
+        # operation - make sure it stays as the same object
+        self.vm.volumes['private'].import_volume.return_value = \
+            self.vm.volumes['private']
+        self.vm2.volumes['private'].import_volume.return_value = \
+            self.vm2.volumes['private']
+
     def test_520_vm_volume_clone(self):
-        self.vm2 = self.app.add_new_vm('AppVM', label='red', name='test-vm2',
-            template='test-template')
-        self.vm.volumes = unittest.mock.MagicMock()
-        self.vm2.volumes = unittest.mock.MagicMock()
-        volumes_conf = {
-            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
-        }
-        self.vm.volumes.configure_mock(**volumes_conf)
-        self.vm2.volumes.configure_mock(**volumes_conf)
-        self.vm2.storage = unittest.mock.Mock()
-        func_mock = unittest.mock.Mock()
-
-        @asyncio.coroutine
-        def coroutine_mock(*args, **kwargs):
-            return func_mock(*args, **kwargs)
-        self.vm2.storage.clone_volume = coroutine_mock
-
-        self.call_mgmt_func(b'admin.vm.volume.Clone',
-                b'test-vm1', b'private', b'test-vm2')
-        self.assertEqual(self.vm.volumes.mock_calls,
-            [('keys', (), {}),
-             ('__getitem__', ('private', ), {}),
-             ('__getitem__().__hash__', (), {}),
-            ])
-        self.assertEqual(self.vm2.volumes.mock_calls,
-            [unittest.mock.call.keys()])
-        self.assertEqual(self.vm2.storage.mock_calls, [])
-        self.assertEqual(func_mock.mock_calls, [
-            unittest.mock.call(self.vm, 'private')
-        ])
+        self.setup_for_clone()
+        token = self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
+                b'test-vm1', b'private', b'')
+        # token
+        self.assertEqual(len(token), 32)
+        self.assertFalse(self.app.save.called)
+        value = self.call_mgmt_func(b'admin.vm.volume.CloneTo',
+                b'test-vm2', b'private', token.encode())
+        self.assertIsNone(value)
+        self.vm2.volumes['private'].import_volume.assert_called_once_with(
+            self.vm.volumes['private']
+        )
+        self.vm2.volumes['private'].import_volume.assert_called_once_with(
+            self.vm2.volumes['private']
+        )
         self.app.save.assert_called_once_with()
 
     def test_521_vm_volume_clone_invalid_volume(self):
-        self.vm2 = self.app.add_new_vm('AppVM', label='red', name='test-vm2',
-            template='test-template')
-        self.vm.volumes = unittest.mock.MagicMock()
-        self.vm2.volumes = unittest.mock.MagicMock()
-        volumes_conf = {
-            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
-        }
-        self.vm.volumes.configure_mock(**volumes_conf)
-        self.vm2.volumes.configure_mock(**volumes_conf)
-        self.vm2.storage = unittest.mock.Mock()
-        func_mock = unittest.mock.Mock()
-
-        @asyncio.coroutine
-        def coroutine_mock(*args, **kwargs):
-            return func_mock(*args, **kwargs)
-        self.vm2.storage.clone_volume = coroutine_mock
+        self.setup_for_clone()
 
         with self.assertRaises(AssertionError):
-            self.call_mgmt_func(b'admin.vm.volume.Clone',
-                    b'test-vm1', b'private123', b'test-vm2')
-        self.assertEqual(self.vm.volumes.mock_calls,
-            [('keys', (), {})])
-        self.assertEqual(self.vm2.volumes.mock_calls, [])
-        self.assertEqual(self.vm2.storage.mock_calls, [])
-        self.assertEqual(func_mock.mock_calls, [])
+            self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
+                    b'test-vm1', b'private123', None)
+        self.assertNotIn('init_volume().import_volume',
+            map(operator.itemgetter(0), self.pool.mock_calls))
         self.assertFalse(self.app.save.called)
 
-    def test_522_vm_volume_clone_invalid_vm(self):
-        self.vm2 = self.app.add_new_vm('AppVM', label='red', name='test-vm2',
-            template='test-template')
-        self.vm.volumes = unittest.mock.MagicMock()
-        self.vm2.volumes = unittest.mock.MagicMock()
-        volumes_conf = {
-            'keys.return_value': ['root', 'private', 'volatile', 'kernel'],
-        }
-        self.vm.volumes.configure_mock(**volumes_conf)
-        self.vm2.volumes.configure_mock(**volumes_conf)
-        self.vm2.storage = unittest.mock.Mock()
-        func_mock = unittest.mock.Mock()
+    def test_522_vm_volume_clone_invalid_volume2(self):
+        self.setup_for_clone()
 
-        @asyncio.coroutine
-        def coroutine_mock(*args, **kwargs):
-            return func_mock(*args, **kwargs)
-        self.vm2.storage.clone_volume = coroutine_mock
+        token = self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
+                b'test-vm1', b'private', b'')
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'admin.vm.volume.CloneTo',
+                    b'test-vm1', b'private123', token.encode())
+        self.assertNotIn('init_volume().import_volume',
+            map(operator.itemgetter(0), self.pool.mock_calls))
+        self.assertFalse(self.app.save.called)
+
+    def test_523_vm_volume_clone_removed_volume(self):
+        self.setup_for_clone()
+
+        token = self.call_mgmt_func(b'admin.vm.volume.CloneFrom',
+                b'test-vm1', b'private', b'')
+        def get_volume(vid):
+            if vid == self.vm.volumes['private']:
+                raise KeyError(vid)
+            else:
+                return unittest.mock.DEFAULT
+        self.pool.get_volume.side_effect = get_volume
+        with self.assertRaises(AssertionError):
+            self.call_mgmt_func(b'admin.vm.volume.CloneTo',
+                    b'test-vm1', b'private', token.encode())
+        self.assertNotIn('init_volume().import_volume',
+            map(operator.itemgetter(0), self.pool.mock_calls))
+        self.assertFalse(self.app.save.called)
+
+    def test_524_vm_volume_clone_invlid_token(self):
+        self.setup_for_clone()
 
         with self.assertRaises(AssertionError):
-            self.call_mgmt_func(b'admin.vm.volume.Clone',
-                    b'test-vm1', b'private123', b'no-such-vm')
-        self.assertEqual(self.vm.volumes.mock_calls,
-            [('keys', (), {})])
-        self.assertEqual(self.vm2.volumes.mock_calls, [])
-        self.assertEqual(self.vm2.storage.mock_calls, [])
-        self.assertEqual(func_mock.mock_calls, [])
+            self.call_mgmt_func(b'admin.vm.volume.CloneTo',
+                    b'test-vm1', b'private', b'no-such-token')
+        self.assertNotIn('init_volume().import_volume',
+            map(operator.itemgetter(0), self.pool.mock_calls))
         self.assertFalse(self.app.save.called)
 
     def test_530_tag_list(self):