From 3d80e8668bb3ce96d9d0c099964bb5ca0a6df60b Mon Sep 17 00:00:00 2001 From: Carlos Eduardo Date: Tue, 19 Nov 2024 19:53:11 -0300 Subject: [PATCH] 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 --- manila/api/v1/shares.py | 10 +++-- manila/api/v2/share_accesses.py | 5 ++- manila/tests/api/v1/test_shares.py | 34 +++++++++++---- manila/tests/lock/test_api.py | 42 +++++++++++++++++++ ...s-rules-locks-lookup-b5efbd41397acba3.yaml | 8 ++++ 5 files changed, 87 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/bug-2089061-fix-access-rules-locks-lookup-b5efbd41397acba3.yaml diff --git a/manila/api/v1/shares.py b/manila/api/v1/shares.py index 7e5bb7e9a0..eb49d88bc4 100644 --- a/manila/api/v1/shares.py +++ b/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: diff --git a/manila/api/v2/share_accesses.py b/manila/api/v2/share_accesses.py index e139cfac33..191f07127e 100644 --- a/manila/api/v2/share_accesses.py +++ b/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, diff --git a/manila/tests/api/v1/test_shares.py b/manila/tests/api/v1/test_shares.py index 796eb3f156..290e946e2f 100644 --- a/manila/tests/api/v1/test_shares.py +++ b/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'])) diff --git a/manila/tests/lock/test_api.py b/manila/tests/lock/test_api.py index e3a6c09131..18d6e6b1b3 100644 --- a/manila/tests/lock/test_api.py +++ b/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) diff --git a/releasenotes/notes/bug-2089061-fix-access-rules-locks-lookup-b5efbd41397acba3.yaml b/releasenotes/notes/bug-2089061-fix-access-rules-locks-lookup-b5efbd41397acba3.yaml new file mode 100644 index 0000000000..8cacc2fab2 --- /dev/null +++ b/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 `_.