From 6f16a2268ce9c7e17c09a3ebd5f48c544e93c808 Mon Sep 17 00:00:00 2001 From: Tzu-Mainn Chen Date: Tue, 19 Nov 2019 00:08:54 +0000 Subject: [PATCH] Allow node owners to administer associated ports Expose a port's node's owner to policy, giving Ironic admins the option of modifying the policy file to allow users specified by a node's owner field to perform API actions on that node's associated ports. Change-Id: I47ecbad9729f18dacc703e2a625ef3e92bfb00ed Story: #2006506 Task: #37539 --- ironic/api/controllers/v1/port.py | 62 +++--- ironic/api/controllers/v1/utils.py | 46 ++++ ironic/common/policy.py | 16 +- ironic/db/sqlalchemy/api.py | 25 ++- ironic/objects/port.py | 24 ++- .../unit/api/controllers/v1/test_expose.py | 2 + .../unit/api/controllers/v1/test_port.py | 203 +++++++++++++++++- .../unit/api/controllers/v1/test_utils.py | 156 ++++++++++++++ ironic/tests/unit/db/test_ports.py | 49 ++++- ironic/tests/unit/objects/test_port.py | 2 +- ...e-owner-policy-ports-1d3193fd897feaa6.yaml | 8 + 11 files changed, 542 insertions(+), 51 deletions(-) create mode 100644 releasenotes/notes/node-owner-policy-ports-1d3193fd897feaa6.yaml diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py index 982ac6572e..26c0b14097 100644 --- a/ironic/api/controllers/v1/port.py +++ b/ironic/api/controllers/v1/port.py @@ -339,7 +339,8 @@ class PortsController(rest.RestController): def _get_ports_collection(self, node_ident, address, portgroup_ident, marker, limit, sort_key, sort_dir, - resource_url=None, fields=None, detail=None): + resource_url=None, fields=None, detail=None, + owner=None): limit = api_utils.validate_limit(limit) sort_dir = api_utils.validate_sort_dir(sort_dir) @@ -370,7 +371,8 @@ class PortsController(rest.RestController): portgroup.id, limit, marker_obj, sort_key=sort_key, - sort_dir=sort_dir) + sort_dir=sort_dir, + owner=owner) elif node_ident: # FIXME(comstud): Since all we need is the node ID, we can # make this more efficient by only querying @@ -380,13 +382,14 @@ class PortsController(rest.RestController): ports = objects.Port.list_by_node_id(api.request.context, node.id, limit, marker_obj, sort_key=sort_key, - sort_dir=sort_dir) + sort_dir=sort_dir, + owner=owner) elif address: - ports = self._get_ports_by_address(address) + ports = self._get_ports_by_address(address, owner=owner) else: ports = objects.Port.list(api.request.context, limit, marker_obj, sort_key=sort_key, - sort_dir=sort_dir) + sort_dir=sort_dir, owner=owner) parameters = {} if detail is not None: @@ -399,7 +402,7 @@ class PortsController(rest.RestController): sort_dir=sort_dir, **parameters) - def _get_ports_by_address(self, address): + def _get_ports_by_address(self, address, owner=None): """Retrieve a port by its address. :param address: MAC address of a port, to get the port which has @@ -408,7 +411,8 @@ class PortsController(rest.RestController): """ try: - port = objects.Port.get_by_address(api.request.context, address) + port = objects.Port.get_by_address(api.request.context, address, + owner=owner) return [port] except exception.PortNotFound: return [] @@ -469,8 +473,7 @@ class PortsController(rest.RestController): for that portgroup. :raises: NotAcceptable, HTTPNotFound """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:port:get', cdict, cdict) + owner = api_utils.check_port_list_policy() api_utils.check_allow_specify_fields(fields) self._check_allowed_port_fields(fields) @@ -493,7 +496,7 @@ class PortsController(rest.RestController): return self._get_ports_collection(node_uuid or node, address, portgroup, marker, limit, sort_key, sort_dir, fields=fields, - detail=detail) + detail=detail, owner=owner) @METRICS.timer('PortsController.detail') @expose.expose(PortCollection, types.uuid_or_name, types.uuid, @@ -523,8 +526,7 @@ class PortsController(rest.RestController): :param sort_dir: direction to sort. "asc" or "desc". Default: asc. :raises: NotAcceptable, HTTPNotFound """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:port:get', cdict, cdict) + owner = api_utils.check_port_list_policy() self._check_allowed_port_fields([sort_key]) if portgroup and not api_utils.allow_portgroups_subcontrollers(): @@ -546,7 +548,7 @@ class PortsController(rest.RestController): resource_url = '/'.join(['ports', 'detail']) return self._get_ports_collection(node_uuid or node, address, portgroup, marker, limit, sort_key, - sort_dir, resource_url) + sort_dir, resource_url, owner=owner) @METRICS.timer('PortsController.get_one') @expose.expose(Port, types.uuid, types.listtype) @@ -558,16 +560,15 @@ class PortsController(rest.RestController): of the resource to be returned. :raises: NotAcceptable, HTTPNotFound """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:port:get', cdict, cdict) - if self.parent_node_ident or self.parent_portgroup_ident: raise exception.OperationNotPermitted() + rpc_port, rpc_node = api_utils.check_port_policy_and_retrieve( + 'baremetal:port:get', port_uuid) + api_utils.check_allow_specify_fields(fields) self._check_allowed_port_fields(fields) - rpc_port = objects.Port.get_by_uuid(api.request.context, port_uuid) return Port.convert_with_links(rpc_port, fields=fields) @METRICS.timer('PortsController.post') @@ -578,13 +579,13 @@ class PortsController(rest.RestController): :param port: a port within the request body. :raises: NotAcceptable, HTTPNotFound, Conflict """ + if self.parent_node_ident or self.parent_portgroup_ident: + raise exception.OperationNotPermitted() + context = api.request.context cdict = context.to_policy_values() policy.authorize('baremetal:port:create', cdict, cdict) - if self.parent_node_ident or self.parent_portgroup_ident: - raise exception.OperationNotPermitted() - pdict = port.as_dict() self._check_allowed_port_fields(pdict) @@ -660,13 +661,14 @@ class PortsController(rest.RestController): :param patch: a json PATCH document to apply to this port. :raises: NotAcceptable, HTTPNotFound """ - context = api.request.context - cdict = context.to_policy_values() - policy.authorize('baremetal:port:update', cdict, cdict) - if self.parent_node_ident or self.parent_portgroup_ident: raise exception.OperationNotPermitted() + rpc_port, rpc_node = api_utils.check_port_policy_and_retrieve( + 'baremetal:port:update', port_uuid) + + context = api.request.context + fields_to_check = set() for field in (self.advanced_net_fields + ['portgroup_uuid', 'physical_network', @@ -677,7 +679,6 @@ class PortsController(rest.RestController): fields_to_check.add(field) self._check_allowed_port_fields(fields_to_check) - rpc_port = objects.Port.get_by_uuid(context, port_uuid) port_dict = rpc_port.as_dict() # NOTE(lucasagomes): # 1) Remove node_id because it's an internal value and @@ -708,7 +709,6 @@ class PortsController(rest.RestController): if rpc_port[field] != patch_val: rpc_port[field] = patch_val - rpc_node = objects.Node.get_by_id(context, rpc_port.node_id) if (rpc_node.provision_state == ir_states.INSPECTING and api_utils.allow_inspect_wait_state()): msg = _('Cannot update port "%(port)s" on "%(node)s" while it is ' @@ -742,15 +742,13 @@ class PortsController(rest.RestController): :param port_uuid: UUID of a port. :raises: OperationNotPermitted, HTTPNotFound """ - context = api.request.context - cdict = context.to_policy_values() - policy.authorize('baremetal:port:delete', cdict, cdict) - if self.parent_node_ident or self.parent_portgroup_ident: raise exception.OperationNotPermitted() - rpc_port = objects.Port.get_by_uuid(context, port_uuid) - rpc_node = objects.Node.get_by_id(context, rpc_port.node_id) + rpc_port, rpc_node = api_utils.check_port_policy_and_retrieve( + 'baremetal:port:delete', port_uuid) + + context = api.request.context portgroup_uuid = None if rpc_port.portgroup_id: diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index fc6d00a9d2..7989c8440d 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -1228,6 +1228,52 @@ def check_node_list_policy(owner=None): return owner +def check_port_policy_and_retrieve(policy_name, port_uuid): + """Check if the specified policy authorizes this request on a port. + + :param: policy_name: Name of the policy to check. + :param: port_uuid: the UUID of a port. + + :raises: HTTPForbidden if the policy forbids access. + :raises: NodeNotFound if the node is not found. + :return: RPC port identified by port_uuid and associated node + """ + context = api.request.context + cdict = context.to_policy_values() + + try: + rpc_port = objects.Port.get_by_uuid(context, port_uuid) + except exception.PortNotFound: + # don't expose non-existence of port unless requester + # has generic access to policy + policy.authorize(policy_name, cdict, cdict) + raise + + rpc_node = objects.Node.get_by_id(context, rpc_port.node_id) + target_dict = dict(cdict) + target_dict['node.owner'] = rpc_node['owner'] + policy.authorize(policy_name, target_dict, cdict) + + return rpc_port, rpc_node + + +def check_port_list_policy(): + """Check if the specified policy authorizes this request on a port. + + :raises: HTTPForbidden if the policy forbids access. + :return: owner that should be used for list query, if needed + """ + cdict = api.request.context.to_policy_values() + try: + policy.authorize('baremetal:port:list_all', cdict, cdict) + except exception.HTTPForbidden: + owner = cdict.get('project_id') + if not owner: + raise + policy.authorize('baremetal:port:list', cdict, cdict) + return owner + + def allow_build_configdrive(): """Check if building configdrive is allowed. diff --git a/ironic/common/policy.py b/ironic/common/policy.py index 9e019ccc02..51bd3b3dcb 100644 --- a/ironic/common/policy.py +++ b/ironic/common/policy.py @@ -231,14 +231,24 @@ port_policies = [ 'baremetal:port:get', 'rule:is_admin or rule:is_observer', 'Retrieve Port records', - [{'path': '/ports', 'method': 'GET'}, - {'path': '/ports/detail', 'method': 'GET'}, - {'path': '/ports/{port_id}', 'method': 'GET'}, + [{'path': '/ports/{port_id}', 'method': 'GET'}, {'path': '/nodes/{node_ident}/ports', 'method': 'GET'}, {'path': '/nodes/{node_ident}/ports/detail', 'method': 'GET'}, {'path': '/portgroups/{portgroup_ident}/ports', 'method': 'GET'}, {'path': '/portgroups/{portgroup_ident}/ports/detail', 'method': 'GET'}]), + policy.DocumentedRuleDefault( + 'baremetal:port:list', + 'rule:baremetal:port:get', + 'Retrieve multiple Port records, filtered by owner', + [{'path': '/ports', 'method': 'GET'}, + {'path': '/ports/detail', 'method': 'GET'}]), + policy.DocumentedRuleDefault( + 'baremetal:port:list_all', + 'rule:baremetal:port:get', + 'Retrieve multiple Port records', + [{'path': '/ports', 'method': 'GET'}, + {'path': '/ports/detail', 'method': 'GET'}]), policy.DocumentedRuleDefault( 'baremetal:port:create', 'rule:is_admin', diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 8951eff3bb..7993e39fb8 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -149,6 +149,12 @@ def add_port_filter_by_node(query, value): return query.filter(models.Node.uuid == value) +def add_port_filter_by_node_owner(query, value): + query = query.join(models.Node, + models.Port.node_id == models.Node.id) + return query.filter(models.Node.owner == value) + + def add_portgroup_filter(query, value): """Adds a portgroup-specific filter to a query. @@ -671,29 +677,38 @@ class Connection(api.Connection): except NoResultFound: raise exception.PortNotFound(port=port_uuid) - def get_port_by_address(self, address): + def get_port_by_address(self, address, owner=None): query = model_query(models.Port).filter_by(address=address) + if owner: + query = add_port_filter_by_node_owner(query, owner) try: return query.one() except NoResultFound: raise exception.PortNotFound(port=address) def get_port_list(self, limit=None, marker=None, - sort_key=None, sort_dir=None): + sort_key=None, sort_dir=None, owner=None): + query = model_query(models.Port) + if owner: + query = add_port_filter_by_node_owner(query, owner) return _paginate_query(models.Port, limit, marker, - sort_key, sort_dir) + sort_key, sort_dir, query) def get_ports_by_node_id(self, node_id, limit=None, marker=None, - sort_key=None, sort_dir=None): + sort_key=None, sort_dir=None, owner=None): query = model_query(models.Port) query = query.filter_by(node_id=node_id) + if owner: + query = add_port_filter_by_node_owner(query, owner) return _paginate_query(models.Port, limit, marker, sort_key, sort_dir, query) def get_ports_by_portgroup_id(self, portgroup_id, limit=None, marker=None, - sort_key=None, sort_dir=None): + sort_key=None, sort_dir=None, owner=None): query = model_query(models.Port) query = query.filter_by(portgroup_id=portgroup_id) + if owner: + query = add_port_filter_by_node_owner(query, owner) return _paginate_query(models.Port, limit, marker, sort_key, sort_dir, query) diff --git a/ironic/objects/port.py b/ironic/objects/port.py index 7bc829dfa2..6c75c8c21e 100644 --- a/ironic/objects/port.py +++ b/ironic/objects/port.py @@ -203,17 +203,18 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): # Implications of calling new remote procedures should be thought through. # @object_base.remotable_classmethod @classmethod - def get_by_address(cls, context, address): + def get_by_address(cls, context, address, owner=None): """Find a port based on address and return a :class:`Port` object. :param cls: the :class:`Port` :param context: Security context :param address: the address of a port. + :param owner: a node owner to match against :returns: a :class:`Port` object. :raises: PortNotFound """ - db_port = cls.dbapi.get_port_by_address(address) + db_port = cls.dbapi.get_port_by_address(address, owner=owner) port = cls._from_db_object(context, cls(), db_port) return port @@ -223,7 +224,7 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): # @object_base.remotable_classmethod @classmethod def list(cls, context, limit=None, marker=None, - sort_key=None, sort_dir=None): + sort_key=None, sort_dir=None, owner=None): """Return a list of Port objects. :param context: Security context. @@ -231,6 +232,7 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): :param marker: pagination marker for large data sets. :param sort_key: column to sort results by. :param sort_dir: direction to sort. "asc" or "desc". + :param owner: a node owner to match against :returns: a list of :class:`Port` object. :raises: InvalidParameterValue @@ -238,7 +240,8 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): db_ports = cls.dbapi.get_port_list(limit=limit, marker=marker, sort_key=sort_key, - sort_dir=sort_dir) + sort_dir=sort_dir, + owner=owner) return cls._from_db_object_list(context, db_ports) # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable @@ -247,7 +250,7 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): # @object_base.remotable_classmethod @classmethod def list_by_node_id(cls, context, node_id, limit=None, marker=None, - sort_key=None, sort_dir=None): + sort_key=None, sort_dir=None, owner=None): """Return a list of Port objects associated with a given node ID. :param context: Security context. @@ -256,13 +259,15 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): :param marker: pagination marker for large data sets. :param sort_key: column to sort results by. :param sort_dir: direction to sort. "asc" or "desc". + :param owner: a node owner to match against :returns: a list of :class:`Port` object. """ db_ports = cls.dbapi.get_ports_by_node_id(node_id, limit=limit, marker=marker, sort_key=sort_key, - sort_dir=sort_dir) + sort_dir=sort_dir, + owner=owner) return cls._from_db_object_list(context, db_ports) # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable @@ -271,7 +276,8 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): # @object_base.remotable_classmethod @classmethod def list_by_portgroup_id(cls, context, portgroup_id, limit=None, - marker=None, sort_key=None, sort_dir=None): + marker=None, sort_key=None, sort_dir=None, + owner=None): """Return a list of Port objects associated with a given portgroup ID. :param context: Security context. @@ -280,6 +286,7 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): :param marker: pagination marker for large data sets. :param sort_key: column to sort results by. :param sort_dir: direction to sort. "asc" or "desc". + :param owner: a node owner to match against :returns: a list of :class:`Port` object. """ @@ -287,7 +294,8 @@ class Port(base.IronicObject, object_base.VersionedObjectDictCompat): limit=limit, marker=marker, sort_key=sort_key, - sort_dir=sort_dir) + sort_dir=sort_dir, + owner=owner) return cls._from_db_object_list(context, db_ports) # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable diff --git a/ironic/tests/unit/api/controllers/v1/test_expose.py b/ironic/tests/unit/api/controllers/v1/test_expose.py index eee02036d7..b68a2b2bfe 100644 --- a/ironic/tests/unit/api/controllers/v1/test_expose.py +++ b/ironic/tests/unit/api/controllers/v1/test_expose.py @@ -54,6 +54,8 @@ class TestExposedAPIMethodsCheckPolicy(test_base.TestCase): ('api_utils.check_node_policy_and_retrieve' in src) or ('api_utils.check_node_list_policy' in src) or ('self._get_node_and_topic' in src) or + ('api_utils.check_port_policy_and_retrieve' in src) or + ('api_utils.check_port_list_policy' in src) or ('policy.authorize' in src and 'context.to_policy_values' in src), 'no policy check found in in exposed ' diff --git a/ironic/tests/unit/api/controllers/v1/test_port.py b/ironic/tests/unit/api/controllers/v1/test_port.py index 51a84209bc..9870c2e04b 100644 --- a/ironic/tests/unit/api/controllers/v1/test_port.py +++ b/ironic/tests/unit/api/controllers/v1/test_port.py @@ -32,6 +32,7 @@ from ironic.api.controllers.v1 import port as api_port from ironic.api.controllers.v1 import utils as api_utils from ironic.api.controllers.v1 import versions from ironic.common import exception +from ironic.common import policy from ironic.common import states from ironic.common import utils as common_utils from ironic.conductor import rpcapi @@ -189,7 +190,7 @@ class TestListPorts(test_api_base.BaseApiTest): def setUp(self): super(TestListPorts, self).setUp() - self.node = obj_utils.create_test_node(self.context) + self.node = obj_utils.create_test_node(self.context, owner='12345') def test_empty(self): data = self.get_json('/ports') @@ -250,6 +251,42 @@ class TestListPorts(test_api_base.BaseApiTest): self.assertEqual(port.uuid, data['ports'][0]["uuid"]) self.assertIsNone(data['ports'][0]["portgroup_uuid"]) + @mock.patch.object(policy, 'authorize', spec=True) + def test_list_non_admin_forbidden(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + raise exception.HTTPForbidden(resource='fake') + mock_authorize.side_effect = mock_authorize_function + + address_template = "aa:bb:cc:dd:ee:f%d" + for id_ in range(3): + obj_utils.create_test_port(self.context, + node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address=address_template % id_) + + response = self.get_json('/ports', + headers={'X-Project-Id': '12345'}, + expect_errors=True) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_list_non_admin_forbidden_no_project(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:port:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + address_template = "aa:bb:cc:dd:ee:f%d" + for id_ in range(3): + obj_utils.create_test_port(self.context, + node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address=address_template % id_) + + response = self.get_json('/ports', expect_errors=True) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + def test_get_one(self): port = obj_utils.create_test_port(self.context, node_id=self.node.id) data = self.get_json('/ports/%s' % port.uuid) @@ -581,6 +618,33 @@ class TestListPorts(test_api_base.BaseApiTest): uuids = [n['uuid'] for n in data['ports']] self.assertCountEqual(ports, uuids) + @mock.patch.object(policy, 'authorize', spec=True) + def test_many_non_admin(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:port:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + ports = [] + # these ports should be retrieved by the API call + for id_ in range(0, 2): + port = obj_utils.create_test_port( + self.context, node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address='52:54:00:cf:2d:3%s' % id_) + ports.append(port.uuid) + # these ports should NOT be retrieved by the API call + for id_ in range(3, 5): + port = obj_utils.create_test_port( + self.context, uuid=uuidutils.generate_uuid(), + address='52:54:00:cf:2d:3%s' % id_) + data = self.get_json('/ports', headers={'X-Project-Id': '12345'}) + self.assertEqual(len(ports), len(data['ports'])) + + uuids = [n['uuid'] for n in data['ports']] + self.assertCountEqual(ports, uuids) + def _test_links(self, public_url=None): cfg.CONF.set_override('public_endpoint', public_url, 'api') uuid = uuidutils.generate_uuid() @@ -686,6 +750,47 @@ class TestListPorts(test_api_base.BaseApiTest): self.assertEqual('application/json', response.content_type) self.assertIn(invalid_address, response.json['error_message']) + @mock.patch.object(policy, 'authorize', spec=True) + def test_port_by_address_non_admin(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:port:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + address_template = "aa:bb:cc:dd:ee:f%d" + for id_ in range(3): + obj_utils.create_test_port(self.context, + node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address=address_template % id_) + + target_address = address_template % 1 + data = self.get_json('/ports?address=%s' % target_address, + headers={'X-Project-Id': '12345'}) + self.assertThat(data['ports'], matchers.HasLength(1)) + self.assertEqual(target_address, data['ports'][0]['address']) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_port_by_address_non_admin_no_match(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:port:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + address_template = "aa:bb:cc:dd:ee:f%d" + for id_ in range(3): + obj_utils.create_test_port(self.context, + node_id=self.node.id, + uuid=uuidutils.generate_uuid(), + address=address_template % id_) + + target_address = address_template % 1 + data = self.get_json('/ports?address=%s' % target_address, + headers={'X-Project-Id': '54321'}) + self.assertThat(data['ports'], matchers.HasLength(0)) + def test_sort_key(self): ports = [] for id_ in range(3): @@ -765,6 +870,60 @@ class TestListPorts(test_api_base.BaseApiTest): headers={api_base.Version.string: '1.5'}) self.assertEqual(3, len(data['ports'])) + @mock.patch.object(policy, 'authorize', spec=True) + @mock.patch.object(api_utils, 'get_rpc_node') + def test_get_all_by_node_name_non_admin( + self, mock_get_rpc_node, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:port:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + mock_get_rpc_node.return_value = self.node + + for i in range(5): + if i < 3: + node_id = self.node.id + else: + node_id = 100000 + i + obj_utils.create_test_port(self.context, + node_id=node_id, + uuid=uuidutils.generate_uuid(), + address='52:54:00:cf:2d:3%s' % i) + data = self.get_json("/ports?node=%s" % 'test-node', + headers={ + api_base.Version.string: '1.5', + 'X-Project-Id': '12345' + }) + self.assertEqual(3, len(data['ports'])) + + @mock.patch.object(policy, 'authorize', spec=True) + @mock.patch.object(api_utils, 'get_rpc_node') + def test_get_all_by_node_name_non_admin_no_match( + self, mock_get_rpc_node, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:port:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + mock_get_rpc_node.return_value = self.node + + for i in range(5): + if i < 3: + node_id = self.node.id + else: + node_id = 100000 + i + obj_utils.create_test_port(self.context, + node_id=node_id, + uuid=uuidutils.generate_uuid(), + address='52:54:00:cf:2d:3%s' % i) + data = self.get_json("/ports?node=%s" % 'test-node', + headers={ + api_base.Version.string: '1.5', + 'X-Project-Id': '54321' + }) + self.assertEqual(0, len(data['ports'])) + @mock.patch.object(api_utils, 'get_rpc_node') def test_get_all_by_node_uuid_and_name(self, mock_get_rpc_node): # GET /v1/ports specifying node and uuid - should only use node_uuid @@ -832,6 +991,48 @@ class TestListPorts(test_api_base.BaseApiTest): self.assertEqual('application/json', response.content_type) self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_int) + @mock.patch.object(policy, 'authorize', spec=True) + def test_get_all_by_portgroup_uuid_non_admin(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:port:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + pg = obj_utils.create_test_portgroup(self.context, + node_id=self.node.id) + port = obj_utils.create_test_port(self.context, node_id=self.node.id, + portgroup_id=pg.id) + data = self.get_json('/ports/detail?portgroup=%s' % pg.uuid, + headers={ + api_base.Version.string: '1.24', + 'X-Project-Id': '12345' + }) + + self.assertEqual(port.uuid, data['ports'][0]['uuid']) + self.assertEqual(pg.uuid, + data['ports'][0]['portgroup_uuid']) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_get_all_by_portgroup_uuid_non_admin_no_match( + self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:port:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + pg = obj_utils.create_test_portgroup(self.context) + obj_utils.create_test_port(self.context, node_id=self.node.id, + portgroup_id=pg.id) + data = self.get_json('/ports/detail?portgroup=%s' % pg.uuid, + headers={ + api_base.Version.string: '1.24', + 'X-Project-Id': '54321' + }) + + self.assertThat(data['ports'], matchers.HasLength(0)) + def test_get_all_by_portgroup_name(self): pg = obj_utils.create_test_portgroup(self.context, node_id=self.node.id) diff --git a/ironic/tests/unit/api/controllers/v1/test_utils.py b/ironic/tests/unit/api/controllers/v1/test_utils.py index 3f25469d18..69547b6fbe 100644 --- a/ironic/tests/unit/api/controllers/v1/test_utils.py +++ b/ironic/tests/unit/api/controllers/v1/test_utils.py @@ -1030,3 +1030,159 @@ class TestCheckNodeListPolicy(base.TestCase): utils.check_node_list_policy, '54321' ) + + +class TestCheckPortPolicyAndRetrieve(base.TestCase): + def setUp(self): + super(TestCheckPortPolicyAndRetrieve, self).setUp() + self.valid_port_uuid = uuidutils.generate_uuid() + self.node = test_api_utils.post_get_test_node() + self.node['owner'] = '12345' + self.port = objects.Port(self.context, node_id=42) + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + @mock.patch.object(objects.Port, 'get_by_uuid') + @mock.patch.object(objects.Node, 'get_by_id') + def test_check_port_policy_and_retrieve( + self, mock_ngbi, mock_pgbu, mock_authorize, mock_pr + ): + mock_pr.version.minor = 50 + mock_pr.context.to_policy_values.return_value = {} + mock_pgbu.return_value = self.port + mock_ngbi.return_value = self.node + + rpc_port, rpc_node = utils.check_port_policy_and_retrieve( + 'fake_policy', self.valid_port_uuid + ) + mock_pgbu.assert_called_once_with(mock_pr.context, + self.valid_port_uuid) + mock_ngbi.assert_called_once_with(mock_pr.context, 42) + mock_authorize.assert_called_once_with( + 'fake_policy', {'node.owner': '12345'}, {}) + self.assertEqual(self.port, rpc_port) + self.assertEqual(self.node, rpc_node) + + @mock.patch.object(api, 'request', spec_set=["context"]) + @mock.patch.object(policy, 'authorize', spec=True) + @mock.patch.object(objects.Port, 'get_by_uuid') + def test_check_port_policy_and_retrieve_no_port_policy_forbidden( + self, mock_pgbu, mock_authorize, mock_pr + ): + mock_pr.context.to_policy_values.return_value = {} + mock_authorize.side_effect = exception.HTTPForbidden(resource='fake') + mock_pgbu.side_effect = exception.PortNotFound( + port=self.valid_port_uuid) + + self.assertRaises( + exception.HTTPForbidden, + utils.check_port_policy_and_retrieve, + 'fake-policy', + self.valid_port_uuid + ) + + @mock.patch.object(api, 'request', spec_set=["context"]) + @mock.patch.object(policy, 'authorize', spec=True) + @mock.patch.object(objects.Port, 'get_by_uuid') + def test_check_port_policy_and_retrieve_no_port( + self, mock_pgbu, mock_authorize, mock_pr + ): + mock_pr.context.to_policy_values.return_value = {} + mock_pgbu.side_effect = exception.PortNotFound( + port=self.valid_port_uuid) + + self.assertRaises( + exception.PortNotFound, + utils.check_port_policy_and_retrieve, + 'fake-policy', + self.valid_port_uuid + ) + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + @mock.patch.object(objects.Port, 'get_by_uuid') + @mock.patch.object(objects.Node, 'get_by_id') + def test_check_port_policy_and_retrieve_policy_forbidden( + self, mock_ngbi, mock_pgbu, mock_authorize, mock_pr + ): + mock_pr.version.minor = 50 + mock_pr.context.to_policy_values.return_value = {} + mock_authorize.side_effect = exception.HTTPForbidden(resource='fake') + mock_pgbu.return_value = self.port + mock_ngbi.return_value = self.node + + self.assertRaises( + exception.HTTPForbidden, + utils.check_port_policy_and_retrieve, + 'fake-policy', + self.valid_port_uuid + ) + + +class TestCheckPortListPolicy(base.TestCase): + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + def test_check_port_list_policy( + self, mock_authorize, mock_pr + ): + mock_pr.context.to_policy_values.return_value = { + 'project_id': '12345' + } + mock_pr.version.minor = 50 + + owner = utils.check_port_list_policy() + self.assertIsNone(owner) + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + def test_check_port_list_policy_forbidden( + self, mock_authorize, mock_pr + ): + def mock_authorize_function(rule, target, creds): + raise exception.HTTPForbidden(resource='fake') + mock_authorize.side_effect = mock_authorize_function + mock_pr.context.to_policy_values.return_value = { + 'project_id': '12345' + } + mock_pr.version.minor = 50 + + self.assertRaises( + exception.HTTPForbidden, + utils.check_port_list_policy, + ) + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + def test_check_port_list_policy_forbidden_no_project( + self, mock_authorize, mock_pr + ): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:port:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + mock_pr.context.to_policy_values.return_value = {} + mock_pr.version.minor = 50 + + self.assertRaises( + exception.HTTPForbidden, + utils.check_port_list_policy, + ) + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + def test_check_port_list_policy_non_admin( + self, mock_authorize, mock_pr + ): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:port:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + mock_pr.context.to_policy_values.return_value = { + 'project_id': '12345' + } + mock_pr.version.minor = 50 + + owner = utils.check_port_list_policy() + self.assertEqual(owner, '12345') diff --git a/ironic/tests/unit/db/test_ports.py b/ironic/tests/unit/db/test_ports.py index fa12c4c257..e0d2e1d665 100644 --- a/ironic/tests/unit/db/test_ports.py +++ b/ironic/tests/unit/db/test_ports.py @@ -28,7 +28,7 @@ class DbPortTestCase(base.DbTestCase): # This method creates a port for every test and # replaces a test for creating a port. super(DbPortTestCase, self).setUp() - self.node = db_utils.create_test_node() + self.node = db_utils.create_test_node(owner='12345') self.portgroup = db_utils.create_test_portgroup(node_id=self.node.id) self.port = db_utils.create_test_port(node_id=self.node.id, portgroup_id=self.portgroup.id) @@ -45,6 +45,17 @@ class DbPortTestCase(base.DbTestCase): res = self.dbapi.get_port_by_address(self.port.address) self.assertEqual(self.port.id, res.id) + def test_get_port_by_address_filter_by_owner(self): + res = self.dbapi.get_port_by_address(self.port.address, + owner=self.node.owner) + self.assertEqual(self.port.id, res.id) + + def test_get_port_by_address_filter_by_owner_no_match(self): + self.assertRaises(exception.PortNotFound, + self.dbapi.get_port_by_address, + self.port.address, + owner='54321') + def test_get_port_list(self): uuids = [] for i in range(1, 6): @@ -72,10 +83,36 @@ class DbPortTestCase(base.DbTestCase): self.assertRaises(exception.InvalidParameterValue, self.dbapi.get_port_list, sort_key='foo') + def test_get_port_list_filter_by_node_owner(self): + uuids = [] + for i in range(1, 3): + port = db_utils.create_test_port(uuid=uuidutils.generate_uuid(), + address='52:54:00:cf:2d:4%s' % i) + for i in range(4, 6): + port = db_utils.create_test_port(uuid=uuidutils.generate_uuid(), + node_id=self.node.id, + address='52:54:00:cf:2d:4%s' % i) + uuids.append(str(port.uuid)) + # Also add the uuid for the port created in setUp() + uuids.append(str(self.port.uuid)) + res = self.dbapi.get_port_list(owner=self.node.owner) + res_uuids = [r.uuid for r in res] + self.assertCountEqual(uuids, res_uuids) + def test_get_ports_by_node_id(self): res = self.dbapi.get_ports_by_node_id(self.node.id) self.assertEqual(self.port.address, res[0].address) + def test_get_ports_by_node_id_filter_by_node_owner(self): + res = self.dbapi.get_ports_by_node_id(self.node.id, + owner=self.node.owner) + self.assertEqual(self.port.address, res[0].address) + + def test_get_ports_by_node_id_filter_by_node_owner_no_match(self): + res = self.dbapi.get_ports_by_node_id(self.node.id, + owner='54321') + self.assertEqual([], res) + def test_get_ports_by_node_id_that_does_not_exist(self): self.assertEqual([], self.dbapi.get_ports_by_node_id(99)) @@ -83,6 +120,16 @@ class DbPortTestCase(base.DbTestCase): res = self.dbapi.get_ports_by_portgroup_id(self.portgroup.id) self.assertEqual(self.port.address, res[0].address) + def test_get_ports_by_portgroup_id_filter_by_node_owner(self): + res = self.dbapi.get_ports_by_portgroup_id(self.portgroup.id, + owner=self.node.owner) + self.assertEqual(self.port.address, res[0].address) + + def test_get_ports_by_portgroup_id_filter_by_node_owner_no_match(self): + res = self.dbapi.get_ports_by_portgroup_id(self.portgroup.id, + owner='54321') + self.assertEqual([], res) + def test_get_ports_by_portgroup_id_that_does_not_exist(self): self.assertEqual([], self.dbapi.get_ports_by_portgroup_id(99)) diff --git a/ironic/tests/unit/objects/test_port.py b/ironic/tests/unit/objects/test_port.py index 19b15faf38..32df1e52a1 100644 --- a/ironic/tests/unit/objects/test_port.py +++ b/ironic/tests/unit/objects/test_port.py @@ -66,7 +66,7 @@ class TestPortObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): port = objects.Port.get(self.context, address) - mock_get_port.assert_called_once_with(address) + mock_get_port.assert_called_once_with(address, owner=None) self.assertEqual(self.context, port._context) def test_get_bad_id_and_uuid_and_address(self): diff --git a/releasenotes/notes/node-owner-policy-ports-1d3193fd897feaa6.yaml b/releasenotes/notes/node-owner-policy-ports-1d3193fd897feaa6.yaml new file mode 100644 index 0000000000..49a984dd0d --- /dev/null +++ b/releasenotes/notes/node-owner-policy-ports-1d3193fd897feaa6.yaml @@ -0,0 +1,8 @@ +--- +features: + - | + A port is owned by its associated node's owner. This owner is now exposed + to policy checks, giving Ironic admins the option of modifying the policy + file to allow users specified by a node's owner field to perform API + actions on that node's associated ports through the ``is_node_owner`` + rule.