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 commit9f69258cab
) (cherry picked from commit7a6955dd7f
)
This commit is contained in:
parent
a0d23e55ab
commit
95aea67f91
@ -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)
|
||||
|
@ -111,20 +111,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(
|
||||
@ -159,6 +152,7 @@ class ShareNetworkController(wsgi.Controller):
|
||||
'limit',
|
||||
'offset',
|
||||
'security_service_id',
|
||||
'project_id'
|
||||
]
|
||||
for opt in opts_to_remove:
|
||||
search_opts.pop(opt, None)
|
||||
|
@ -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):
|
||||
|
@ -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',
|
||||
|
@ -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))
|
||||
|
@ -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})
|
||||
|
@ -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()
|
||||
|
@ -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(
|
||||
|
@ -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.
|
Loading…
Reference in New Issue
Block a user