From 54433cb8c096298972f661b029091f611d73870e Mon Sep 17 00:00:00 2001
From: Gorka Eguileor <geguileo@redhat.com>
Date: Fri, 31 Jul 2020 12:20:03 +0200
Subject: [PATCH] Fix revert to snapshot for non admins

When a non user calls revert to snapshot using the snapshot name the
client returns a failure status:

  $ cinder --os-volume-api-version 3.59 revert-to-snapshot snap1
    ERROR: No snapshot with a name or ID of 'snap1' exists.

The revert to snapshot API requires an UUID, so the cinderclient lists
the snapshots passing "all_tenants=1" and the name of the snapshot to
find its first when it's not a UUID.

The problem that we were only removing the "all_tenants" filter for
admins, leaving it for normal users and passing it to the DB which
wouldn't understand that filter and return None.

This patch ensure we always remove the "all_tenants" filter (as we do in
other places) before calling the OVO method to list snapshots.

Change-Id: I5fd3d7840b36f6805143cd1b837258232e7bc58a
Fixes-Bug: #1889758
(cherry picked from commit 71a080b22688c2fb15545736e807d233932e22e8)
(cherry picked from commit d51dc099a3c7bc1008dd3e158664a81de54b2cdd)
---
 cinder/tests/unit/volume/test_snapshot.py     | 35 +++++++++++++++++++
 cinder/volume/api.py                          |  7 ++--
 ...t-snapshot-non-admin-8485be55060eab0d.yaml |  5 +++
 3 files changed, 44 insertions(+), 3 deletions(-)
 create mode 100644 releasenotes/notes/revert-snapshot-non-admin-8485be55060eab0d.yaml

diff --git a/cinder/tests/unit/volume/test_snapshot.py b/cinder/tests/unit/volume/test_snapshot.py
index 74dcf3b16e9..8b57cba43ea 100644
--- a/cinder/tests/unit/volume/test_snapshot.py
+++ b/cinder/tests/unit/volume/test_snapshot.py
@@ -630,3 +630,38 @@ class SnapshotTestCase(base.BaseVolumeTestCase):
         snapshot.refresh()
         self.assertEqual(fields.SnapshotStatus.ERROR_DELETING,
                          snapshot.status)
+
+    @ddt.data({'all_tenants': '1', 'name': 'snap1'},
+              {'all_tenants': 'true', 'name': 'snap1'},
+              {'all_tenants': 'false', 'name': 'snap1'},
+              {'all_tenants': '0', 'name': 'snap1'},
+              {'name': 'snap1'})
+    @mock.patch.object(objects, 'SnapshotList')
+    @mock.patch.object(context.RequestContext, 'authorize')
+    def test_get_all_snapshots_non_admin(self, search_opts, auth_mock,
+                                         snaplist_mock):
+        ctxt = context.RequestContext(user_id=None, is_admin=False,
+                                      project_id=mock.sentinel.project_id,
+                                      read_deleted='no', overwrite=False)
+        volume_api = cinder.volume.api.API()
+        res = volume_api.get_all_snapshots(ctxt,
+                                           search_opts,
+                                           mock.sentinel.marker,
+                                           mock.sentinel.limit,
+                                           mock.sentinel.sort_keys,
+                                           mock.sentinel.sort_dirs,
+                                           mock.sentinel.offset)
+
+        auth_mock.assert_called_once_with(
+            cinder.volume.api.snapshot_policy.GET_ALL_POLICY)
+        snaplist_mock.get_all.assert_not_called()
+        snaplist_mock.get_all_by_project.assert_called_once_with(
+            ctxt,
+            mock.sentinel.project_id,
+            {'name': 'snap1'},
+            mock.sentinel.marker,
+            mock.sentinel.limit,
+            mock.sentinel.sort_keys,
+            mock.sentinel.sort_dirs,
+            mock.sentinel.offset)
+        self.assertEqual(snaplist_mock.get_all_by_project.return_value, res)
diff --git a/cinder/volume/api.py b/cinder/volume/api.py
index dbe7599cbc2..d2aa98be760 100644
--- a/cinder/volume/api.py
+++ b/cinder/volume/api.py
@@ -675,9 +675,10 @@ class API(base.Base):
 
         search_opts = search_opts or {}
 
-        if context.is_admin and 'all_tenants' in search_opts:
-            # Need to remove all_tenants to pass the filtering below.
-            del search_opts['all_tenants']
+        # Need to remove all_tenants to pass the filtering below.
+        all_tenants = strutils.bool_from_string(search_opts.pop('all_tenants',
+                                                                'false'))
+        if context.is_admin and all_tenants:
             snapshots = objects.SnapshotList.get_all(
                 context, search_opts, marker, limit, sort_keys, sort_dirs,
                 offset)
diff --git a/releasenotes/notes/revert-snapshot-non-admin-8485be55060eab0d.yaml b/releasenotes/notes/revert-snapshot-non-admin-8485be55060eab0d.yaml
new file mode 100644
index 00000000000..0ce87b44f48
--- /dev/null
+++ b/releasenotes/notes/revert-snapshot-non-admin-8485be55060eab0d.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fix revert to snapshot not working for non admin users when using the
+    snapshot's name (bug #1889758).