From 3f9240a8f5a33d781d59809c275c000e96fc130a 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 (cherry picked from commit 3d80e8668bb3ce96d9d0c099964bb5ca0a6df60b) (cherry picked from commit 112791a8ea2bca2160fb5f6189d7797193db0a2b) (cherry picked from commit 57408d5abf703fff79307a75bb203350aa9a2756) --- 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 441849b01e..8a791cee86 100644 --- a/manila/api/v1/shares.py +++ b/manila/api/v1/shares.py @@ -585,12 +585,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 @@ -616,7 +618,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 339fd3401d..f22a0494b9 100644 --- a/manila/tests/api/v1/test_shares.py +++ b/manila/tests/api/v1/test_shares.py @@ -1282,6 +1282,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'}} @@ -1299,14 +1300,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') @@ -1316,6 +1323,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'}} @@ -1363,12 +1371,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( @@ -1387,6 +1397,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, @@ -1394,7 +1405,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( @@ -1419,6 +1431,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, @@ -1426,7 +1439,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( @@ -1457,6 +1471,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, @@ -1464,7 +1479,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 @@ -1491,15 +1507,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 `_.