From 29707cf3474c5893564ffc1cf9e56ff364c856a9 Mon Sep 17 00:00:00 2001 From: Salvatore Orlando Date: Tue, 17 Jul 2012 09:32:16 -0700 Subject: [PATCH] Avoid removal of attributes used by policy engine Fixes bug1025150 Ensures that each attribute which is used by the policy engine is not removed by the plugin when the 'fields' query parameter is specified. This could be better achieved by not having the plugin perform this selection, but as it is part of the plugin interface, it's probably better not to introduce changes which alter it. Change-Id: I68ab0998b7113f06da5df74ccd44e390640de475 --- quantum/api/v2/base.py | 32 +++++++++--- quantum/api/v2/router.py | 13 +++-- quantum/api/v2/views.py | 20 +++++--- quantum/policy.py | 3 -- quantum/tests/unit/test_api_v2.py | 15 ++++-- quantum/tests/unit/test_db_plugin.py | 73 ++++++++++++++++++++++++++-- 6 files changed, 126 insertions(+), 30 deletions(-) diff --git a/quantum/api/v2/base.py b/quantum/api/v2/base.py index 3e45c506e7..8ae3d65e61 100644 --- a/quantum/api/v2/base.py +++ b/quantum/api/v2/base.py @@ -106,17 +106,31 @@ class Controller(object): self._collection = collection self._resource = resource self._attr_info = attr_info + self._policy_attrs = [name for (name, info) in self._attr_info.items() + if 'required_by_policy' in info + and info['required_by_policy']] self._view = getattr(views, self._resource) + def _do_field_list(self, original_fields): + fields_to_add = None + # don't do anything if fields were not specified in the request + if original_fields: + fields_to_add = [attr for attr in self._policy_attrs + if attr not in original_fields] + original_fields.extend(self._policy_attrs) + return original_fields, fields_to_add + def _items(self, request, do_authz=False): """Retrieves and formats a list of elements of the requested entity""" + # NOTE(salvatore-orlando): The following ensures that fields which + # are needed for authZ policy validation are not stripped away by the + # plugin before returning. + original_fields, fields_to_add = self._do_field_list(fields(request)) kwargs = {'filters': filters(request), 'verbose': verbose(request), - 'fields': fields(request)} - + 'fields': original_fields} obj_getter = getattr(self._plugin, "get_%s" % self._collection) obj_list = obj_getter(request.context, **kwargs) - # Check authz if do_authz: # Omit items from list that should not be visible @@ -125,12 +139,18 @@ class Controller(object): "get_%s" % self._resource, obj)] - return {self._collection: [self._view(obj) for obj in obj_list]} + return {self._collection: [self._view(obj, + fields_to_strip=fields_to_add) + for obj in obj_list]} def _item(self, request, id, do_authz=False): """Retrieves and formats a single element of the requested entity""" + # NOTE(salvatore-orlando): The following ensures that fields which + # are needed for authZ policy validation are not stripped away by the + # plugin before returning. + field_list, added_fields = self._do_field_list(fields(request)) kwargs = {'verbose': verbose(request), - 'fields': fields(request)} + 'fields': field_list} action = "get_%s" % self._resource obj_getter = getattr(self._plugin, action) obj = obj_getter(request.context, id, **kwargs) @@ -139,7 +159,7 @@ class Controller(object): if do_authz: policy.enforce(request.context, action, obj) - return {self._resource: self._view(obj)} + return {self._resource: self._view(obj, fields_to_strip=added_fields)} def index(self, request): """Returns a list of the requested entity""" diff --git a/quantum/api/v2/router.py b/quantum/api/v2/router.py index 9e14b887cb..faf6bd7855 100644 --- a/quantum/api/v2/router.py +++ b/quantum/api/v2/router.py @@ -48,6 +48,10 @@ ATTR_NOT_SPECIFIED = object() # and the default gateway_ip will be generated. # However, if gateway_ip is specified as None, this means that # the subnet does not have a gateway IP. +# Some of the following attributes are used by the policy engine. +# They are explicitly marked with the required_by_policy flag to ensure +# they are always returned by a plugin for policy processing, even if +# they are not specified in the 'fields' query param RESOURCE_ATTRIBUTE_MAP = { 'networks': { @@ -57,7 +61,8 @@ RESOURCE_ATTRIBUTE_MAP = { 'admin_state_up': {'allow_post': True, 'allow_put': True, 'default': True}, 'status': {'allow_post': False, 'allow_put': False}, - 'tenant_id': {'allow_post': True, 'allow_put': False}, + 'tenant_id': {'allow_post': True, 'allow_put': False, + 'required_by_policy': True}, }, 'ports': { 'id': {'allow_post': False, 'allow_put': False}, @@ -71,7 +76,8 @@ RESOURCE_ATTRIBUTE_MAP = { 'host_routes': {'allow_post': True, 'allow_put': True, 'default': ATTR_NOT_SPECIFIED}, 'device_id': {'allow_post': True, 'allow_put': True, 'default': ''}, - 'tenant_id': {'allow_post': True, 'allow_put': False}, + 'tenant_id': {'allow_post': True, 'allow_put': False, + 'required_by_policy': True}, }, 'subnets': { 'id': {'allow_post': False, 'allow_put': False}, @@ -87,7 +93,8 @@ RESOURCE_ATTRIBUTE_MAP = { 'default': ATTR_NOT_SPECIFIED}, 'additional_host_routes': {'allow_post': True, 'allow_put': True, 'default': ATTR_NOT_SPECIFIED}, - 'tenant_id': {'allow_post': True, 'allow_put': False}, + 'tenant_id': {'allow_post': True, 'allow_put': False, + 'required_by_policy': True}, } } diff --git a/quantum/api/v2/views.py b/quantum/api/v2/views.py index 8f51d18866..29e5ae600d 100644 --- a/quantum/api/v2/views.py +++ b/quantum/api/v2/views.py @@ -14,27 +14,31 @@ # limitations under the License. -def resource(data, keys): +def resource(data, keys, fields_to_strip=None): """Formats the specified entity""" - return dict(item for item in data.iteritems() if item[0] in keys) + # make sure fields_to_strip is iterable + if not fields_to_strip: + fields_to_strip = [] + return dict(item for item in data.iteritems() + if item[0] in keys and not item[0] in fields_to_strip) -def port(port_data): +def port(port_data, fields_to_strip=None): """Represents a view for a port object""" keys = ('id', 'network_id', 'mac_address', 'fixed_ips', 'device_id', 'admin_state_up', 'tenant_id', 'status') - return resource(port_data, keys) + return resource(port_data, keys, fields_to_strip) -def network(network_data): +def network(network_data, fields_to_strip=None): """Represents a view for a network object""" keys = ('id', 'name', 'subnets', 'admin_state_up', 'status', 'tenant_id', 'mac_ranges') - return resource(network_data, keys) + return resource(network_data, keys, fields_to_strip) -def subnet(subnet_data): +def subnet(subnet_data, fields_to_strip=None): """Represents a view for a subnet object""" keys = ('id', 'network_id', 'tenant_id', 'gateway_ip', 'ip_version', 'cidr', 'allocation_pools') - return resource(subnet_data, keys) + return resource(subnet_data, keys, fields_to_strip) diff --git a/quantum/policy.py b/quantum/policy.py index a91c28b9c4..16c09853f7 100644 --- a/quantum/policy.py +++ b/quantum/policy.py @@ -63,12 +63,9 @@ def check(context, action, target): :return: Returns True if access is permitted else False. """ - init() - match_list = ('rule:%s' % action,) credentials = context.to_dict() - return policy.enforce(match_list, target, credentials) diff --git a/quantum/tests/unit/test_api_v2.py b/quantum/tests/unit/test_api_v2.py index 7695ee4820..57d9118fa6 100644 --- a/quantum/tests/unit/test_api_v2.py +++ b/quantum/tests/unit/test_api_v2.py @@ -232,7 +232,8 @@ class APIv2TestCase(unittest.TestCase): self.api.get(_get_path('networks'), {'fields': 'foo'}) instance.get_networks.assert_called_once_with(mock.ANY, filters=mock.ANY, - fields=['foo'], + fields=['foo', + 'tenant_id'], verbose=mock.ANY) def test_fields_multiple(self): @@ -242,7 +243,8 @@ class APIv2TestCase(unittest.TestCase): self.api.get(_get_path('networks'), {'fields': ['foo', 'bar']}) instance.get_networks.assert_called_once_with(mock.ANY, filters=mock.ANY, - fields=['foo', 'bar'], + fields=['foo', 'bar', + 'tenant_id'], verbose=mock.ANY) def test_fields_multiple_with_empty(self): @@ -252,7 +254,8 @@ class APIv2TestCase(unittest.TestCase): self.api.get(_get_path('networks'), {'fields': ['foo', '']}) instance.get_networks.assert_called_once_with(mock.ANY, filters=mock.ANY, - fields=['foo'], + fields=['foo', + 'tenant_id'], verbose=mock.ANY) def test_fields_empty(self): @@ -350,7 +353,8 @@ class APIv2TestCase(unittest.TestCase): filters = {'foo': ['bar']} instance.get_networks.assert_called_once_with(mock.ANY, filters=filters, - fields=['foo'], + fields=['foo', + 'tenant_id'], verbose=mock.ANY) def test_filters_with_verbose(self): @@ -375,7 +379,8 @@ class APIv2TestCase(unittest.TestCase): filters = {'foo': ['bar']} instance.get_networks.assert_called_once_with(mock.ANY, filters=filters, - fields=['foo'], + fields=['foo', + 'tenant_id'], verbose=True) diff --git a/quantum/tests/unit/test_db_plugin.py b/quantum/tests/unit/test_db_plugin.py index 280ee2bb0c..454b08d600 100644 --- a/quantum/tests/unit/test_db_plugin.py +++ b/quantum/tests/unit/test_db_plugin.py @@ -78,7 +78,8 @@ class QuantumDbPluginV2TestCase(unittest2.TestCase): db._MAKER = None cfg.CONF.reset() - def _req(self, method, resource, data=None, fmt='json', id=None): + def _req(self, method, resource, data=None, fmt='json', + id=None, params=None): if id: path = '/%(resource)s/%(id)s.%(fmt)s' % locals() else: @@ -87,13 +88,17 @@ class QuantumDbPluginV2TestCase(unittest2.TestCase): body = None if data: body = Serializer().serialize(data, content_type) - return create_request(path, body, content_type, method) + return create_request(path, + body, + content_type, + method, + query_string=params) def new_create_request(self, resource, data, fmt='json'): return self._req('POST', resource, data, fmt) - def new_list_request(self, resource, fmt='json'): - return self._req('GET', resource, None, fmt) + def new_list_request(self, resource, fmt='json', params=None): + return self._req('GET', resource, None, fmt, params=params) def new_show_request(self, resource, id, fmt='json'): return self._req('GET', resource, None, fmt, id=id) @@ -109,11 +114,22 @@ class QuantumDbPluginV2TestCase(unittest2.TestCase): data = self._deserializers[ctype].deserialize(response.body)['body'] return data - def _create_network(self, fmt, name, admin_status_up): + def _create_network(self, fmt, name, admin_status_up, **kwargs): data = {'network': {'name': name, 'admin_state_up': admin_status_up, 'tenant_id': self._tenant_id}} + for arg in ('admin_state_up', 'tenant_id', 'public'): + # Arg must be present and not empty + if arg in kwargs and kwargs[arg]: + data['network'][arg] = kwargs[arg] network_req = self.new_create_request('networks', data, fmt) + if ('set_context' in kwargs and + kwargs['set_context'] is True and + 'tenant_id' in kwargs): + # create a specific auth context for this request + network_req.environ['quantum.context'] = context.Context( + '', kwargs['tenant_id']) + return network_req.get_response(self.api) def _create_subnet(self, fmt, tenant_id, net_id, gateway_ip, cidr, @@ -247,6 +263,53 @@ class TestV2HTTPResponse(QuantumDbPluginV2TestCase): res = req.get_response(self.api) self.assertEquals(res.status_int, 200) + def _check_list_with_fields(self, res, field_name): + self.assertEquals(res.status_int, 200) + body = self.deserialize('json', res) + # further checks: 1 networks + self.assertEquals(len(body['networks']), 1) + # 1 field in the network record + self.assertEquals(len(body['networks'][0]), 1) + # field is 'name' + self.assertIn(field_name, body['networks'][0]) + + def test_list_with_fields(self): + self._create_network('json', 'some_net', True) + req = self.new_list_request('networks', params="fields=name") + res = req.get_response(self.api) + self._check_list_with_fields(res, 'name') + + def test_list_with_fields_noadmin(self): + tenant_id = 'some_tenant' + self._create_network('json', + 'some_net', + True, + tenant_id=tenant_id, + set_context=True) + req = self.new_list_request('networks', params="fields=name") + req.environ['quantum.context'] = context.Context('', tenant_id) + res = req.get_response(self.api) + self._check_list_with_fields(res, 'name') + + def test_list_with_fields_noadmin_and_policy_field(self): + """ If a field used by policy is selected, do not duplicate it. + + Verifies that if the field parameter explicitly specifies a field + which is used by the policy engine, then it is not duplicated + in the response. + + """ + tenant_id = 'some_tenant' + self._create_network('json', + 'some_net', + True, + tenant_id=tenant_id, + set_context=True) + req = self.new_list_request('networks', params="fields=tenant_id") + req.environ['quantum.context'] = context.Context('', tenant_id) + res = req.get_response(self.api) + self._check_list_with_fields(res, 'tenant_id') + def test_show_returns_200(self): with self.network() as net: req = self.new_show_request('networks', net['network']['id'])