diff --git a/manila/share/api.py b/manila/share/api.py index d148129cd4..4a7d0492dd 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -1869,6 +1869,10 @@ class API(base.Base): """Returns access rule with the id.""" policy.check_policy(context, 'share', 'access_get') rule = self.db.share_access_get(context, access_id) + # NOTE(gouthamr): Check if the caller has access to the share that + # the rule belongs to: + self.get(context, rule['share_id']) + return rule @policy.wrap_check_policy('share') diff --git a/manila/tests/api/v2/test_share_accesses.py b/manila/tests/api/v2/test_share_accesses.py index 6d718633f3..1d405aba05 100644 --- a/manila/tests/api/v2/test_share_accesses.py +++ b/manila/tests/api/v2/test_share_accesses.py @@ -112,6 +112,37 @@ class ShareAccessesAPITest(test.TestCase): version="2.45") self.assertRaises(exc.HTTPBadRequest, self.controller.index, req) + def test_show_access_not_authorized(self): + share = db_utils.create_share( + project_id='c3c5ec1ccc4640d0af1914cbf11f05ad', + is_public=False) + access = db_utils.create_access( + id='76699c6b-f3da-47d7-b468-364f1347ba04', + share_id=share['id']) + req = fakes.HTTPRequest.blank( + '/v2/share-access-rules/%s' % access['id'], + version="2.45") + self.mock_object( + policy, 'check_policy', + mock.Mock(side_effect=[None, None, exception.NotAuthorized])) + + self.assertRaises(exception.NotAuthorized, + self.controller.show, + req, + access['id']) + policy.check_policy.assert_has_calls([ + mock.call(req.environ['manila.context'], + 'share_access_rule', 'get'), + mock.call(req.environ['manila.context'], + 'share', 'access_get'), + mock.call(req.environ['manila.context'], + 'share', 'get', mock.ANY)]) + policy_check_call_args_list = policy.check_policy.call_args_list[2][0] + share_being_checked = policy_check_call_args_list[3] + self.assertEqual('c3c5ec1ccc4640d0af1914cbf11f05ad', + share_being_checked['project_id']) + self.assertIs(False, share_being_checked['is_public']) + def test_show_access_not_found(self): self.assertRaises( exc.HTTPNotFound, diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index 469e55c2ee..2fcf87e4b2 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -2629,11 +2629,13 @@ class ShareAPITestCase(test.TestCase): def test_access_get(self): with mock.patch.object(db_api, 'share_access_get', - mock.Mock(return_value='fake')): + mock.Mock(return_value={'share_id': 'fake'})): + self.mock_object(self.api, 'get') rule = self.api.access_get(self.context, 'fakeid') - self.assertEqual('fake', rule) + self.assertEqual({'share_id': 'fake'}, rule) db_api.share_access_get.assert_called_once_with( self.context, 'fakeid') + self.api.get.assert_called_once_with(self.context, 'fake') def test_access_get_all(self): share = db_utils.create_share(id='fakeid') diff --git a/releasenotes/notes/bug-1917417-fix-rbac-check-on-share-access-rules-efdddaf9e6f68fdf.yaml b/releasenotes/notes/bug-1917417-fix-rbac-check-on-share-access-rules-efdddaf9e6f68fdf.yaml new file mode 100644 index 0000000000..371eb4cfef --- /dev/null +++ b/releasenotes/notes/bug-1917417-fix-rbac-check-on-share-access-rules-efdddaf9e6f68fdf.yaml @@ -0,0 +1,7 @@ +--- +security: + - | + An RBAC policy check has been enforced against the GET /share-access-rules + API to ensure that users are permitted to access the share that the + access rule belongs to. See `bug 1917417 + `_ for more details.