From 9f69258cab9baf77384282b67feb390d0487d3b9 Mon Sep 17 00:00:00 2001 From: Jiao Pengju Date: Thu, 23 Nov 2017 23:15:36 +0800 Subject: [PATCH] Fix getting share networks and security services error It will fail when non-admin tenants try to get share networks and security services with option '{all_tenants: 1}'. The reason is that the policy of 'get_all_share_networks' and 'get_all_security_services' are admin api, they do not allow the non-admin tenants list the share networks and security services with all_tenants=1. This patch removes the policy check of non-admin tenants and allows non-admin tenants to request to list with 'all_tenants=1', however 'all_tenants' in the request is just ignored. Change-Id: Ied021b66333f1254cd232bbc38562a4a9b762ad2 Co-Authored-By: Goutham Pacha Ravi Related-Bug: #1721787 --- manila/api/v1/security_service.py | 2 +- manila/api/v2/share_networks.py | 12 +++------- manila/tests/api/v1/test_security_service.py | 16 ++++++------- manila/tests/api/v2/test_share_networks.py | 24 ++++++++++--------- .../tests/api/test_security_services.py | 8 +++++++ .../api/test_security_services_negative.py | 6 ----- .../tests/api/test_share_networks.py | 20 ++++++++++++++++ .../tests/api/test_share_networks_negative.py | 12 ---------- ...urity-services-error-7e5e7981fcbf2b53.yaml | 6 +++++ 9 files changed, 58 insertions(+), 48 deletions(-) create mode 100755 releasenotes/notes/bug-1721787-fix-getting-share-networks-and-security-services-error-7e5e7981fcbf2b53.yaml diff --git a/manila/api/v1/security_service.py b/manila/api/v1/security_service.py index 3e41788fb9..03b4d94e28 100644 --- a/manila/api/v1/security_service.py +++ b/manila/api/v1/security_service.py @@ -105,7 +105,7 @@ class SecurityServiceController(wsgi.Controller): security_services = share_nw['security_services'] del search_opts['share_network_id'] else: - if 'all_tenants' in search_opts: + if 'all_tenants' in search_opts and context.is_admin: policy.check_policy(context, RESOURCE_NAME, 'get_all_security_services') security_services = db.security_service_get_all(context) diff --git a/manila/api/v2/share_networks.py b/manila/api/v2/share_networks.py index 5089db59cf..57b30b76e7 100644 --- a/manila/api/v2/share_networks.py +++ b/manila/api/v2/share_networks.py @@ -112,20 +112,13 @@ class ShareNetworkController(wsgi.Controller): search_opts = {} search_opts.update(req.GET) - if ('all_tenants' in search_opts or - ('project_id' in search_opts and - search_opts['project_id'] != context.project_id)): - policy.check_policy(context, RESOURCE_NAME, - 'get_all_share_networks') - if 'security_service_id' in search_opts: networks = db_api.share_network_get_all_by_security_service( context, search_opts['security_service_id']) - elif ('project_id' in search_opts and - search_opts['project_id'] != context.project_id): + elif context.is_admin and 'project_id' in search_opts: networks = db_api.share_network_get_all_by_project( context, search_opts['project_id']) - elif 'all_tenants' in search_opts: + elif context.is_admin and 'all_tenants' in search_opts: networks = db_api.share_network_get_all(context) else: networks = db_api.share_network_get_all_by_project( @@ -160,6 +153,7 @@ class ShareNetworkController(wsgi.Controller): 'limit', 'offset', 'security_service_id', + 'project_id' ] for opt in opts_to_remove: search_opts.pop(opt, None) diff --git a/manila/tests/api/v1/test_security_service.py b/manila/tests/api/v1/test_security_service.py index 7cd4ddcb65..56125b45ef 100644 --- a/manila/tests/api/v1/test_security_service.py +++ b/manila/tests/api/v1/test_security_service.py @@ -302,18 +302,16 @@ class ShareApiTest(test.TestCase): db.security_service_get_all.assert_called_once_with( req.environ['manila.context']) - @mock.patch.object(db, 'security_service_get_all', mock.Mock()) + @mock.patch.object(db, 'security_service_get_all_by_project', mock.Mock()) def test_security_services_list_all_tenants_non_admin_context(self): - self.check_policy_patcher.stop() - db.security_service_get_all.return_value = [ - self.ss_active_directory, - self.ss_ldap, - ] + db.security_service_get_all_by_project.return_value = [] req = fakes.HTTPRequest.blank( '/security-services?all_tenants=1') - self.assertRaises(exception.PolicyNotAuthorized, self.controller.index, - req) - self.assertFalse(db.security_service_get_all.called) + fake_context = req.environ['manila.context'] + self.controller.index(req) + db.security_service_get_all_by_project.assert_called_once_with( + fake_context, fake_context.project_id + ) @mock.patch.object(db, 'security_service_get_all_by_project', mock.Mock()) def test_security_services_list_admin_context_invalid_opts(self): diff --git a/manila/tests/api/v2/test_share_networks.py b/manila/tests/api/v2/test_share_networks.py index 3f1659fb50..a08546a408 100644 --- a/manila/tests/api/v2/test_share_networks.py +++ b/manila/tests/api/v2/test_share_networks.py @@ -384,13 +384,15 @@ class ShareNetworkAPITest(test.TestCase): result[share_networks.RESOURCES_NAME][0], fake_sn_with_ss_shortened) - @mock.patch.object(db_api, 'share_network_get_all', mock.Mock()) + @mock.patch.object(db_api, 'share_network_get_all_by_project', mock.Mock()) def test_index_all_tenants_non_admin_context(self): req = fakes.HTTPRequest.blank( '/share_networks?all_tenants=1') - self.assertRaises(exception.PolicyNotAuthorized, self.controller.index, - req) - self.assertFalse(db_api.share_network_get_all.called) + fake_context = req.environ['manila.context'] + db_api.share_network_get_all_by_project.return_value = [] + self.controller.index(req) + db_api.share_network_get_all_by_project.assert_called_with( + fake_context, fake_context.project_id) @mock.patch.object(db_api, 'share_network_get_all', mock.Mock()) def test_index_all_tenants_admin_context(self): @@ -410,15 +412,16 @@ class ShareNetworkAPITest(test.TestCase): def test_index_filter_by_project_id_non_admin_context(self): req = fakes.HTTPRequest.blank( '/share_networks?project_id=fake project') - self.assertRaises(exception.PolicyNotAuthorized, self.controller.index, - req) - self.assertFalse(db_api.share_network_get_all_by_project.called) + fake_context = req.environ['manila.context'] + db_api.share_network_get_all_by_project.return_value = [] + self.controller.index(req) + db_api.share_network_get_all_by_project.assert_called_with( + fake_context, fake_context.project_id) @mock.patch.object(db_api, 'share_network_get_all_by_project', mock.Mock()) def test_index_filter_by_project_id_admin_context(self): db_api.share_network_get_all_by_project.return_value = [ - fake_share_network, - fake_share_network_with_ss, + fake_share_network_with_ss ] req = fakes.HTTPRequest.blank( '/share_networks?project_id=fake', @@ -435,8 +438,7 @@ class ShareNetworkAPITest(test.TestCase): mock.Mock()) def test_index_filter_by_ss_and_project_id_admin_context(self): db_api.share_network_get_all_by_security_service.return_value = [ - fake_share_network, - fake_share_network_with_ss, + fake_share_network_with_ss ] req = fakes.HTTPRequest.blank( '/share_networks?security_service_id=fake-ss-id&project_id=fake', diff --git a/manila_tempest_tests/tests/api/test_security_services.py b/manila_tempest_tests/tests/api/test_security_services.py index fd884f2b48..30cf6a9293 100644 --- a/manila_tempest_tests/tests/api/test_security_services.py +++ b/manila_tempest_tests/tests/api/test_security_services.py @@ -200,3 +200,11 @@ class SecurityServicesTest(base.BaseSharesTest, self.assertTrue(any(self.ss_ldap['id'] == ss['id'] for ss in listed)) self.assertTrue(any(self.ss_kerberos['id'] == ss['id'] for ss in listed)) + + @tc.attr(base.TAG_POSITIVE, base.TAG_API) + def test_try_list_security_services_all_tenants(self): + listed = self.shares_client.list_security_services( + params={'all_tenants': 1}) + self.assertTrue(any(self.ss_ldap['id'] == ss['id'] for ss in listed)) + self.assertTrue(any(self.ss_kerberos['id'] == ss['id'] + for ss in listed)) diff --git a/manila_tempest_tests/tests/api/test_security_services_negative.py b/manila_tempest_tests/tests/api/test_security_services_negative.py index 1f8e18851d..0814b6d9f5 100644 --- a/manila_tempest_tests/tests/api/test_security_services_negative.py +++ b/manila_tempest_tests/tests/api/test_security_services_negative.py @@ -120,9 +120,3 @@ class SecurityServicesNegativeTest(base.BaseSharesTest): self.assertRaises(lib_exc.NotFound, self.shares_client.get_security_service, ss["id"]) - - @tc.attr(base.TAG_NEGATIVE, base.TAG_API) - def test_try_list_security_services_all_tenants(self): - self.assertRaises(lib_exc.Forbidden, - self.shares_client.list_security_services, - params={'all_tenants': 1}) diff --git a/manila_tempest_tests/tests/api/test_share_networks.py b/manila_tempest_tests/tests/api/test_share_networks.py index 3484264d5b..4a44c12240 100644 --- a/manila_tempest_tests/tests/api/test_share_networks.py +++ b/manila_tempest_tests/tests/api/test_share_networks.py @@ -35,6 +35,26 @@ class ShareNetworkListMixin(object): keys = ["name", "id"] [self.assertIn(key, sn.keys()) for sn in listed for key in keys] + @tc.attr(base.TAG_POSITIVE, base.TAG_API) + def test_try_list_share_networks_all_tenants(self): + listed = self.shares_client.list_share_networks_with_detail( + params={'all_tenants': 1}) + any(self.sn_with_ldap_ss["id"] in sn["id"] for sn in listed) + + # verify keys + keys = ["name", "id"] + [self.assertIn(key, sn.keys()) for sn in listed for key in keys] + + @tc.attr(base.TAG_POSITIVE, base.TAG_API) + def test_try_list_share_networks_project_id(self): + listed = self.shares_client.list_share_networks_with_detail( + params={'project_id': 'some_project'}) + any(self.sn_with_ldap_ss["id"] in sn["id"] for sn in listed) + + # verify keys + keys = ["name", "id"] + [self.assertIn(key, sn.keys()) for sn in listed for key in keys] + @tc.attr(base.TAG_POSITIVE, base.TAG_API) def test_list_share_networks_with_detail(self): listed = self.shares_v2_client.list_share_networks_with_detail() diff --git a/manila_tempest_tests/tests/api/test_share_networks_negative.py b/manila_tempest_tests/tests/api/test_share_networks_negative.py index 5fdc684954..02bbdb9a45 100644 --- a/manila_tempest_tests/tests/api/test_share_networks_negative.py +++ b/manila_tempest_tests/tests/api/test_share_networks_negative.py @@ -81,18 +81,6 @@ class ShareNetworksNegativeTest(base.BaseSharesTest): self.shares_client.get_security_service, sn["id"]) - @tc.attr(base.TAG_NEGATIVE, base.TAG_API) - def test_try_list_share_networks_all_tenants(self): - self.assertRaises(lib_exc.Forbidden, - self.shares_client.list_share_networks_with_detail, - params={'all_tenants': 1}) - - @tc.attr(base.TAG_NEGATIVE, base.TAG_API) - def test_try_list_share_networks_project_id(self): - self.assertRaises(lib_exc.Forbidden, - self.shares_client.list_share_networks_with_detail, - params={'project_id': 'some_project'}) - @tc.attr(base.TAG_NEGATIVE, base.TAG_API) def test_try_list_share_networks_wrong_created_since_value(self): self.assertRaises( diff --git a/releasenotes/notes/bug-1721787-fix-getting-share-networks-and-security-services-error-7e5e7981fcbf2b53.yaml b/releasenotes/notes/bug-1721787-fix-getting-share-networks-and-security-services-error-7e5e7981fcbf2b53.yaml new file mode 100755 index 0000000000..351345ea0f --- /dev/null +++ b/releasenotes/notes/bug-1721787-fix-getting-share-networks-and-security-services-error-7e5e7981fcbf2b53.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - Non admin users may invoke GET /share-networks and GET /security-services + APIs with the 'all-tenants' flag in the query, however, the flag is + ignored, and only resources belonging to the project will be served. This + API change was made to fix bug 1721787 in the manila client project.