Fix access rule visibility locks

Access rule visibility and deletion locks were not being properly
retrieved when listing, showing and deleting access rules, leading
to an unexpected behavior.

Fixes that issue by elevating the context and making sure that
we are looking for all of the locks placed against the access
rule.

Closes-Bug: #2089061
Change-Id: Ib6667df25c8935826e673f180848887fe4fff8d6
Signed-off-by: Carlos Eduardo <ces.eduardo98@gmail.com>

manila/api/v1/shares.py

@@ -615,12 +615,14 @@ class ShareMixin(object):
         unrestrict = access_data.get('unrestrict', False)
         search_opts = {
             'resource_id': access_id,
-            'resource_action': constants.RESOURCE_ACTION_DELETE
+            'resource_action': constants.RESOURCE_ACTION_DELETE,
+            'all_projects': True,
         }
 
         locks, locks_count = (
             self.resource_locks_api.get_all(
-                context, search_opts=search_opts, show_count=True) or []
+                context.elevated(), search_opts=search_opts,
+                show_count=True) or []
         )
 
         # no locks placed, nothing to do
@@ -646,7 +648,9 @@ class ShareMixin(object):
             try:
                 self.resource_locks_api.ensure_context_can_delete_lock(
                     context, lock['id'])
-            except exception.NotAuthorized:
+            except (exception.NotAuthorized, exception.ResourceLockNotFound):
+                # If it is not found, then it means that the context doesn't
+                # have access to this resource and should be denied.
                 non_deletable_locks.append(lock)
 
         if non_deletable_locks:

manila/api/v2/share_accesses.py

@@ -55,10 +55,11 @@ class ShareAccessesController(wsgi.Controller, wsgi.AdminActionsMixin):
         search_opts = {
             'resource_id': id,
             'resource_action': constants.RESOURCE_ACTION_SHOW,
-            'resource_type': 'access_rule'
+            'resource_type': 'access_rule',
+            'all_projects': True,
         }
         locks, count = self.resource_locks_api.get_all(
-            context, search_opts, show_count=True)
+            context.elevated(), search_opts, show_count=True)
 
         if count:
             return self.resource_locks_api.access_is_restricted(context,

manila/tests/api/v1/test_shares.py

@@ -1367,6 +1367,7 @@ class ShareActionsTest(test.TestCase):
 
         self.mock_object(share_api.API, "deny_access", _stub_deny_access)
         self.mock_object(share_api.API, "access_get", _fake_access_get)
+        self.mock_object(self.controller, '_check_for_access_rule_locks')
 
         id = 'fake_share_id'
         body = {"os-deny_access": {"access_id": 'fake_acces_id'}}
@@ -1384,14 +1385,20 @@ class ShareActionsTest(test.TestCase):
         share_network = db_utils.create_share_network()
         share = db_utils.create_share(share_network_id=share_network['id'])
         self.mock_object(share_api.API, 'get', mock.Mock(return_value=share))
+        self.mock_object(self.controller, '_check_for_access_rule_locks')
 
         id = 'fake_share_id'
-        body = {"os-deny_access": {"access_id": 'fake_acces_id'}}
+        access_data = {"access_id": 'fake_acces_id'}
+        body = {"os-deny_access": access_data}
         req = fakes.HTTPRequest.blank('/v1/tenant1/shares/%s/action' % id)
 
         res = self.controller._deny_access(req, id, body)
 
         self.assertEqual(202, res.status_int)
+        self.controller._check_for_access_rule_locks.assert_called_once_with(
+            req.environ['manila.context'], access_data,
+            access_data['access_id'], id
+        )
         self.mock_policy_check.assert_called_once_with(
             req.environ['manila.context'], 'share', 'deny_access')
 
@@ -1401,6 +1408,7 @@ class ShareActionsTest(test.TestCase):
 
         self.mock_object(share_api.API, "deny_access", _stub_deny_access)
         self.mock_object(share_api.API, "access_get", _fake_access_get)
+        self.mock_object(self.controller, '_check_for_access_rule_locks')
 
         id = 'super_fake_share_id'
         body = {"os-deny_access": {"access_id": 'fake_acces_id'}}
@@ -1448,12 +1456,14 @@ class ShareActionsTest(test.TestCase):
         access_id = 'fake_access_id'
         share_id = 'fake_share_id'
 
+        self.mock_object(context, 'elevated', mock.Mock(return_value=context))
         self.controller._check_for_access_rule_locks(
             context, {}, access_id, share_id)
 
         delete_search_opts = {
             'resource_id': access_id,
-            'resource_action': constants.RESOURCE_ACTION_DELETE
+            'resource_action': constants.RESOURCE_ACTION_DELETE,
+            'all_projects': True,
         }
 
         resource_locks.API.get_all.assert_called_once_with(
@@ -1472,6 +1482,7 @@ class ShareActionsTest(test.TestCase):
         access_id = 'fake_access_id'
         share_id = 'fake_share_id'
 
+        self.mock_object(context, 'elevated', mock.Mock(return_value=context))
         self.assertRaises(
             webob.exc.HTTPForbidden,
             self.controller._check_for_access_rule_locks,
@@ -1479,7 +1490,8 @@ class ShareActionsTest(test.TestCase):
 
         delete_search_opts = {
             'resource_id': access_id,
-            'resource_action': constants.RESOURCE_ACTION_DELETE
+            'resource_action': constants.RESOURCE_ACTION_DELETE,
+            'all_projects': True,
         }
 
         resource_locks.API.get_all.assert_called_once_with(
@@ -1504,6 +1516,7 @@ class ShareActionsTest(test.TestCase):
         access_id = 'fake_access_id'
         share_id = 'fake_share_id'
 
+        self.mock_object(context, 'elevated', mock.Mock(return_value=context))
         self.assertRaises(
             webob.exc.HTTPForbidden,
             self.controller._check_for_access_rule_locks,
@@ -1511,7 +1524,8 @@ class ShareActionsTest(test.TestCase):
 
         delete_search_opts = {
             'resource_id': access_id,
-            'resource_action': constants.RESOURCE_ACTION_DELETE
+            'resource_action': constants.RESOURCE_ACTION_DELETE,
+            'all_projects': True,
         }
 
         resource_locks.API.get_all.assert_called_once_with(
@@ -1542,6 +1556,7 @@ class ShareActionsTest(test.TestCase):
         access_id = 'fake_access_id'
         share_id = 'fake_share_id'
 
+        self.mock_object(context, 'elevated', mock.Mock(return_value=context))
         self.assertRaises(
             webob.exc.HTTPForbidden,
             self.controller._check_for_access_rule_locks,
@@ -1549,7 +1564,8 @@ class ShareActionsTest(test.TestCase):
         )
         delete_search_opts = {
             'resource_id': access_id,
-            'resource_action': constants.RESOURCE_ACTION_DELETE
+            'resource_action': constants.RESOURCE_ACTION_DELETE,
+            'all_projects': True,
         }
         resource_locks.API.get_all.assert_called_once_with(
             context, search_opts=delete_search_opts, show_count=True
@@ -1576,15 +1592,19 @@ class ShareActionsTest(test.TestCase):
         access_id = 'fake_access_id'
         share_id = 'fake_share_id'
 
+        self.mock_object(context, 'elevated', mock.Mock(return_value=context))
         self.controller._check_for_access_rule_locks(
             context, {'unrestrict': True}, access_id, share_id)
 
         delete_search_opts = {
             'resource_id': access_id,
-            'resource_action': constants.RESOURCE_ACTION_DELETE
+            'resource_action': constants.RESOURCE_ACTION_DELETE,
+            'all_projects': True,
         }
         resource_locks.API.get_all.assert_called_once_with(
-            context, search_opts=delete_search_opts, show_count=True)
+            context.elevated(), search_opts=delete_search_opts,
+            show_count=True
+        )
         (resource_locks.API.ensure_context_can_delete_lock
             .assert_called_once_with(
                 context, locks[0]['id']))

manila/tests/lock/test_api.py

@@ -517,3 +517,45 @@ class ResourceLockApiTest(test.TestCase):
             utils.IsAMatcher(context.RequestContext),
             'd767d3cd-1187-404a-a91f-8b172e0e768e'
         )
+
+    def test_ensure_context_can_delete_lock_policy_fails(self):
+        lock = {'id': 'd767d3cd-1187-404a-a91f-8b172e0e768e'}
+        self.mock_object(
+            self.lock_api.db, 'resource_lock_get', mock.Mock(return_value=lock)
+        )
+        self.mock_object(
+            policy,
+            'check_policy',
+            mock.Mock(side_effect=exception.PolicyNotAuthorized(
+                action="resource_lock:delete")),
+        )
+
+        self.assertRaises(
+            exception.NotAuthorized,
+            self.lock_api.ensure_context_can_delete_lock,
+            self.ctxt,
+            'd767d3cd-1187-404a-a91f-8b172e0e768e')
+
+        self.lock_api.db.resource_lock_get.assert_called_once_with(
+            self.ctxt, 'd767d3cd-1187-404a-a91f-8b172e0e768e'
+        )
+        policy.check_policy.assert_called_once_with(
+            self.ctxt, 'resource_lock', 'delete', lock)
+
+    def test_ensure_context_can_delete_lock(self):
+        lock = {'id': 'd767d3cd-1187-404a-a91f-8b172e0e768e'}
+        self.mock_object(
+            self.lock_api.db, 'resource_lock_get', mock.Mock(return_value=lock)
+        )
+        self.mock_object(policy, 'check_policy')
+        self.mock_object(self.lock_api, '_check_allow_lock_manipulation')
+
+        self.lock_api.ensure_context_can_delete_lock(
+            self.ctxt,
+            'd767d3cd-1187-404a-a91f-8b172e0e768e')
+
+        self.lock_api.db.resource_lock_get.assert_called_once_with(
+            self.ctxt, 'd767d3cd-1187-404a-a91f-8b172e0e768e'
+        )
+        policy.check_policy.assert_called_once_with(
+            self.ctxt, 'resource_lock', 'delete', lock)

releasenotes/notes/bug-2089061-fix-access-rules-locks-lookup-b5efbd41397acba3.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    While displaying and deleting access rules, manila was limiting the search
+    for locks to the context of the request. Now, manila will search within
+    all of the projects for locks and properly apply visibility and deletion
+    restrictions. For more details, please refer to
+    `launchpad bug #2089061 <https://bugs.launchpad.net/manila/+bug/2089061>`_.