From 6303741be2394de9301f03f28f7ad20216aad7f6 Mon Sep 17 00:00:00 2001 From: Goutham Pacha Ravi Date: Wed, 14 Sep 2022 22:13:11 -0700 Subject: [PATCH] [RBAC] Return 404 if share is inaccessible When a user is prevented from listing a non-public share, the API service would return a 403 Forbidden. This isn't consistent with the API SIG's guidance on resources restricted by virtue of RBAC policy since users with malicious intent may use the signal to mean that the resource exists. Depends-On: I27fdd7dfffeb15965b66dbb3f6b1568c11ff9ad4 Change-Id: I7e05dcb343c932cc7fec8d395919053d0a1801ce Closes-Bug: #1901210 Signed-off-by: Goutham Pacha Ravi --- manila/share/api.py | 5 ++++- manila/tests/api/v2/test_share_accesses.py | 2 +- manila/tests/api/v2/test_share_instances.py | 2 +- manila/tests/share/test_api.py | 21 ++++++++++++++++++- ...are-access-forbidden-02ca9a9552ad3e15.yaml | 6 ++++++ 5 files changed, 32 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/bug-1901210-return-404-if-share-access-forbidden-02ca9a9552ad3e15.yaml diff --git a/manila/share/api.py b/manila/share/api.py index 883a3a0d34..280c5068b2 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -1952,7 +1952,10 @@ class API(base.Base): def get(self, context, share_id): rv = self.db.share_get(context, share_id) if not rv['is_public']: - policy.check_policy(context, 'share', 'get', rv) + authorized = policy.check_policy( + context, 'share', 'get', rv, do_raise=False) + if not authorized: + raise exception.NotFound() return rv def get_all(self, context, search_opts=None, sort_key='created_at', diff --git a/manila/tests/api/v2/test_share_accesses.py b/manila/tests/api/v2/test_share_accesses.py index 2f3aa33baf..ab0586a9ee 100644 --- a/manila/tests/api/v2/test_share_accesses.py +++ b/manila/tests/api/v2/test_share_accesses.py @@ -137,7 +137,7 @@ class ShareAccessesAPITest(test.TestCase): mock.call(req.environ['manila.context'], 'share', 'access_get'), mock.call(req.environ['manila.context'], - 'share', 'get', mock.ANY)]) + 'share', 'get', mock.ANY, do_raise=False)]) 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', diff --git a/manila/tests/api/v2/test_share_instances.py b/manila/tests/api/v2/test_share_instances.py index 5017ce77ec..1a0ab4501f 100644 --- a/manila/tests/api/v2/test_share_instances.py +++ b/manila/tests/api/v2/test_share_instances.py @@ -201,7 +201,7 @@ class ShareInstancesAPITest(test.TestCase): req = self._get_request('fake', version=version) req_context = req.environ['manila.context'] share_policy_check_call = mock.call( - req_context, 'share', 'get', mock.ANY) + req_context, 'share', 'get', mock.ANY, do_raise=False) get_instances_policy_check_call = mock.call( req_context, 'share_instance', 'index') diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index e1a7ed2d9a..44b9c53d19 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -2640,10 +2640,29 @@ class ShareAPITestCase(test.TestCase): result = self.api.get(self.context, 'fakeid') self.assertEqual(share, result) share_api.policy.check_policy.assert_called_once_with( - self.context, 'share', 'get', share) + self.context, 'share', 'get', share, do_raise=False) db_api.share_get.assert_called_once_with( self.context, 'fakeid') + def test_get_not_authorized(self): + share = db_utils.create_share( + is_public=False, + project_id='5db325fc4de14fe1a860ff69f190c78c') + share_api.policy.check_policy.return_value = False + ctx = context.RequestContext('df6d65cc1f8946ba86be06b8140ec4b3', + 'e8133457b853436591a7e4610e7ce679', + is_admin=False) + with mock.patch.object(db_api, 'share_get', + mock.Mock(return_value=share)): + + self.assertRaises(exception.NotFound, + self.api.get, + ctx, + share['id']) + share_api.policy.check_policy.assert_called_once_with( + ctx, 'share', 'get', share, do_raise=False) + db_api.share_get.assert_called_once_with(ctx, share['id']) + @mock.patch.object(db_api, 'share_snapshot_get_all_by_project', mock.Mock()) def test_get_all_snapshots_admin_not_all_tenants(self): diff --git a/releasenotes/notes/bug-1901210-return-404-if-share-access-forbidden-02ca9a9552ad3e15.yaml b/releasenotes/notes/bug-1901210-return-404-if-share-access-forbidden-02ca9a9552ad3e15.yaml new file mode 100644 index 0000000000..80f72c7916 --- /dev/null +++ b/releasenotes/notes/bug-1901210-return-404-if-share-access-forbidden-02ca9a9552ad3e15.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + The GET /shares/{share_id} API now responds with HTTP 404 (Not Found) + for inaccessible resources. See `bug 1901210 + `_ for further information.