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 `_.