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 <gouthampravi@gmail.com>
Related-Bug: #1721787
(cherry picked from commit 9f69258cab)
This commit is contained in:
Jiao Pengju 2017-11-23 23:15:36 +08:00 committed by Tom Barron
parent ac4201d378
commit 7a6955dd7f
9 changed files with 58 additions and 48 deletions

View File

@ -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)

View File

@ -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)

View File

@ -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):

View File

@ -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',

View File

@ -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))

View File

@ -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})

View File

@ -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()

View File

@ -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(

View File

@ -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.