diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 71fa734951..3c168a6d10 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -185,10 +185,10 @@ class BootDeviceController(rest.RestController): 'supported': ['GET'], } - def _get_boot_device(self, node_ident, supported=False): + def _get_boot_device(self, rpc_node, supported=False): """Get the current boot device or a list of supported devices. - :param node_ident: the UUID or logical name of a node. + :param rpc_node: RPC Node object. :param supported: Boolean value. If true return a list of supported boot devices, if false return the current boot device. Default: False. @@ -196,7 +196,6 @@ class BootDeviceController(rest.RestController): boot devices. """ - rpc_node = api_utils.get_rpc_node(node_ident) topic = api.request.rpcapi.get_topic_for(rpc_node) if supported: return api.request.rpcapi.get_supported_boot_devices( @@ -221,10 +220,9 @@ class BootDeviceController(rest.RestController): Default: False. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:set_boot_device', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:set_boot_device', node_ident) - rpc_node = api_utils.get_rpc_node(node_ident) topic = api.request.rpcapi.get_topic_for(rpc_node) api.request.rpcapi.set_boot_device(api.request.context, rpc_node.uuid, @@ -246,10 +244,10 @@ class BootDeviceController(rest.RestController): future boots or not, None if it is unknown. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:get_boot_device', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:get_boot_device', node_ident) - return self._get_boot_device(node_ident) + return self._get_boot_device(rpc_node) @METRICS.timer('BootDeviceController.supported') @expose.expose(wtypes.text, types.uuid_or_name) @@ -261,10 +259,10 @@ class BootDeviceController(rest.RestController): devices. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:get_boot_device', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:get_boot_device', node_ident) - boot_devices = self._get_boot_device(node_ident, supported=True) + boot_devices = self._get_boot_device(rpc_node, supported=True) return {'supported_boot_devices': boot_devices} @@ -293,10 +291,9 @@ class InjectNmiController(rest.RestController): if not api_utils.allow_inject_nmi(): raise exception.NotFound() - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:inject_nmi', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:inject_nmi', node_ident) - rpc_node = api_utils.get_rpc_node(node_ident) topic = api.request.rpcapi.get_topic_for(rpc_node) api.request.rpcapi.inject_nmi(api.request.context, rpc_node.uuid, @@ -337,10 +334,9 @@ class NodeConsoleController(rest.RestController): :param node_ident: UUID or logical name of a node. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:get_console', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:get_console', node_ident) - rpc_node = api_utils.get_rpc_node(node_ident) topic = api.request.rpcapi.get_topic_for(rpc_node) try: console = api.request.rpcapi.get_console_information( @@ -362,10 +358,9 @@ class NodeConsoleController(rest.RestController): :param enabled: Boolean value; whether to enable or disable the console. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:set_console_state', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:set_console_state', node_ident) - rpc_node = api_utils.get_rpc_node(node_ident) topic = api.request.rpcapi.get_topic_for(rpc_node) api.request.rpcapi.set_console_mode(api.request.context, rpc_node.uuid, enabled, topic) @@ -453,13 +448,12 @@ class NodeStatesController(rest.RestController): :param node_ident: the UUID or logical_name of a node. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:get_states', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:get_states', node_ident) # NOTE(lucasagomes): All these state values come from the # DB. Ironic counts with a periodic task that verify the current # power states of the nodes and update the DB accordingly. - rpc_node = api_utils.get_rpc_node(node_ident) return NodeStates.convert(rpc_node) @METRICS.timer('NodeStatesController.raid') @@ -477,12 +471,11 @@ class NodeStatesController(rest.RestController): :raises: NotAcceptable, if requested version of the API is less than 1.12. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:set_raid_state', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:set_raid_state', node_ident) if not api_utils.allow_raid_config(): raise exception.NotAcceptable() - rpc_node = api_utils.get_rpc_node(node_ident) topic = api.request.rpcapi.get_topic_for(rpc_node) try: api.request.rpcapi.set_target_raid_config( @@ -514,12 +507,11 @@ class NodeStatesController(rest.RestController): :raises: Invalid (HTTP 400) if timeout value is less than 1. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:set_power_state', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:set_power_state', node_ident) # TODO(lucasagomes): Test if it's able to transition to the # target state from the current one - rpc_node = api_utils.get_rpc_node(node_ident) topic = api.request.rpcapi.get_topic_for(rpc_node) if ((target in [ir_states.SOFT_REBOOT, ir_states.SOFT_POWER_OFF] @@ -653,11 +645,10 @@ class NodeStatesController(rest.RestController): :raises: NotAcceptable (HTTP 406) if the API version specified does not allow the requested state transition. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:set_provision_state', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:set_provision_state', node_ident) api_utils.check_allow_management_verbs(target) - rpc_node = api_utils.get_rpc_node(node_ident) if (target in (ir_states.ACTIVE, ir_states.REBUILD) and rpc_node.maintenance): @@ -777,9 +768,8 @@ class NodeTraitsController(rest.RestController): @expose.expose(Traits) def get_all(self): """List node traits.""" - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:traits:list', cdict, cdict) - node = api_utils.get_rpc_node(self.node_ident) + node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:traits:list', self.node_ident) traits = objects.TraitList.get_by_node_id(api.request.context, node.id) return Traits(traits=traits.get_trait_names()) @@ -797,9 +787,8 @@ class NodeTraitsController(rest.RestController): traits with this list. """ context = api.request.context - cdict = context.to_policy_values() - policy.authorize('baremetal:node:traits:set', cdict, cdict) - node = api_utils.get_rpc_node(self.node_ident) + node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:traits:set', self.node_ident) if (trait and traits is not None) or not (trait or traits is not None): msg = _("A single node trait may be added via PUT " @@ -854,9 +843,8 @@ class NodeTraitsController(rest.RestController): None, all traits are removed. """ context = api.request.context - cdict = context.to_policy_values() - policy.authorize('baremetal:node:traits:delete', cdict, cdict) - node = api_utils.get_rpc_node(self.node_ident) + node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:traits:delete', self.node_ident) if trait: traits = [trait] @@ -1385,12 +1373,10 @@ class NodeVendorPassthruController(rest.RestController): entries. :raises: NodeNotFound if the node is not found. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:vendor_passthru', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:vendor_passthru', node_ident) # Raise an exception if node is not found - rpc_node = api_utils.get_rpc_node(node_ident) - if rpc_node.driver not in _VENDOR_METHODS: topic = api.request.rpcapi.get_topic_for(rpc_node) ret = api.request.rpcapi.get_node_vendor_passthru_methods( @@ -1409,11 +1395,10 @@ class NodeVendorPassthruController(rest.RestController): :param method: name of the method in vendor driver. :param data: body of data to supply to the specified method. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:vendor_passthru', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:vendor_passthru', node_ident) # Raise an exception if node is not found - rpc_node = api_utils.get_rpc_node(node_ident) topic = api.request.rpcapi.get_topic_for(rpc_node) return api_utils.vendor_passthru(rpc_node.uuid, method, topic, data=data) @@ -1421,9 +1406,8 @@ class NodeVendorPassthruController(rest.RestController): class NodeMaintenanceController(rest.RestController): - def _set_maintenance(self, node_ident, maintenance_mode, reason=None): + def _set_maintenance(self, rpc_node, maintenance_mode, reason=None): context = api.request.context - rpc_node = api_utils.get_rpc_node(node_ident) rpc_node.maintenance = maintenance_mode rpc_node.maintenance_reason = reason notify.emit_start_notification(context, rpc_node, 'maintenance_set') @@ -1449,10 +1433,10 @@ class NodeMaintenanceController(rest.RestController): :param reason: Optional, the reason why it's in maintenance. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:set_maintenance', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:set_maintenance', node_ident) - self._set_maintenance(node_ident, True, reason=reason) + self._set_maintenance(rpc_node, True, reason=reason) @METRICS.timer('NodeMaintenanceController.delete') @expose.expose(None, types.uuid_or_name, status_code=http_client.ACCEPTED) @@ -1462,10 +1446,10 @@ class NodeMaintenanceController(rest.RestController): :param node_ident: the UUID or logical name of a node. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:clear_maintenance', cdict, cdict) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:clear_maintenance', node_ident) - self._set_maintenance(node_ident, False) + self._set_maintenance(rpc_node, False) # NOTE(vsaienko) We don't support pagination with VIFs, so we don't use @@ -1488,8 +1472,9 @@ class NodeVIFController(rest.RestController): def __init__(self, node_ident): self.node_ident = node_ident - def _get_node_and_topic(self): - rpc_node = api_utils.get_rpc_node(self.node_ident) + def _get_node_and_topic(self, policy_name): + rpc_node = api_utils.check_node_policy_and_retrieve( + policy_name, self.node_ident) try: return rpc_node, api.request.rpcapi.get_topic_for(rpc_node) except exception.NoValidHost as e: @@ -1500,9 +1485,7 @@ class NodeVIFController(rest.RestController): @expose.expose(VifCollection) def get_all(self): """Get a list of attached VIFs""" - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:vif:list', cdict, cdict) - rpc_node, topic = self._get_node_and_topic() + rpc_node, topic = self._get_node_and_topic('baremetal:node:vif:list') vifs = api.request.rpcapi.vif_list(api.request.context, rpc_node.uuid, topic=topic) return VifCollection.collection_from_list(vifs) @@ -1517,9 +1500,7 @@ class NodeVIFController(rest.RestController): It must have an 'id' key, whose value is a unique identifier for that VIF. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:vif:attach', cdict, cdict) - rpc_node, topic = self._get_node_and_topic() + rpc_node, topic = self._get_node_and_topic('baremetal:node:vif:attach') api.request.rpcapi.vif_attach(api.request.context, rpc_node.uuid, vif_info=vif, topic=topic) @@ -1531,9 +1512,7 @@ class NodeVIFController(rest.RestController): :param vif_id: The ID of a VIF to detach """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:vif:detach', cdict, cdict) - rpc_node, topic = self._get_node_and_topic() + rpc_node, topic = self._get_node_and_topic('baremetal:node:vif:detach') api.request.rpcapi.vif_detach(api.request.context, rpc_node.uuid, vif_id=vif_id, topic=topic) @@ -1842,8 +1821,7 @@ class NodesController(rest.RestController): with description field contains matching value. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:get', cdict, cdict) + owner = api_utils.check_node_list_policy(owner) api_utils.check_allow_specify_fields(fields) api_utils.check_allowed_fields(fields) @@ -1917,8 +1895,7 @@ class NodesController(rest.RestController): with description field contains matching value. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:get', cdict, cdict) + owner = api_utils.check_node_list_policy(owner) api_utils.check_for_invalid_state_and_allow_filter(provision_state) api_utils.check_allow_specify_driver(driver) @@ -1960,9 +1937,6 @@ class NodesController(rest.RestController): :param node: UUID or name of a node. :param node_uuid: UUID of a node. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:validate', cdict, cdict) - if node is not None: # We're invoking this interface using positional notation, or # explicitly using 'node'. Try and determine which one. @@ -1970,7 +1944,8 @@ class NodesController(rest.RestController): and not uuidutils.is_uuid_like(node)): raise exception.NotAcceptable() - rpc_node = api_utils.get_rpc_node(node_uuid or node) + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:validate', node_uuid or node) topic = api.request.rpcapi.get_topic_for(rpc_node) return api.request.rpcapi.validate_driver_interfaces( @@ -1985,16 +1960,15 @@ class NodesController(rest.RestController): :param fields: Optional, a list with a specified set of fields of the resource to be returned. """ - cdict = api.request.context.to_policy_values() - policy.authorize('baremetal:node:get', cdict, cdict) - if self.from_chassis: raise exception.OperationNotPermitted() + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:get', node_ident, with_suffix=True) + api_utils.check_allow_specify_fields(fields) api_utils.check_allowed_fields(fields) - rpc_node = api_utils.get_rpc_node_with_suffix(node_ident) return Node.convert_with_links(rpc_node, fields=fields) @METRICS.timer('NodesController.post') @@ -2004,13 +1978,13 @@ class NodesController(rest.RestController): :param node: a node within the request body. """ + if self.from_chassis: + raise exception.OperationNotPermitted() + context = api.request.context cdict = context.to_policy_values() policy.authorize('baremetal:node:create', cdict, cdict) - if self.from_chassis: - raise exception.OperationNotPermitted() - if node.conductor is not wtypes.Unset: msg = _("Cannot specify conductor on node creation.") raise exception.Invalid(msg) @@ -2112,17 +2086,15 @@ class NodesController(rest.RestController): defaults. Only valid when updating the driver field. :param patch: a json PATCH document to apply to this node. """ - context = api.request.context - cdict = context.to_policy_values() - policy.authorize('baremetal:node:update', cdict, cdict) - if (reset_interfaces is not None and not api_utils.allow_reset_interfaces()): raise exception.NotAcceptable() self._validate_patch(patch, reset_interfaces) - rpc_node = api_utils.get_rpc_node_with_suffix(node_ident) + context = api.request.context + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:update', node_ident, with_suffix=True) remove_inst_uuid_patch = [{'op': 'remove', 'path': '/instance_uuid'}] if rpc_node.maintenance and patch == remove_inst_uuid_patch: @@ -2196,14 +2168,13 @@ class NodesController(rest.RestController): :param node_ident: UUID or logical name of a node. """ - context = api.request.context - cdict = context.to_policy_values() - policy.authorize('baremetal:node:delete', cdict, cdict) - if self.from_chassis: raise exception.OperationNotPermitted() - rpc_node = api_utils.get_rpc_node_with_suffix(node_ident) + context = api.request.context + rpc_node = api_utils.check_node_policy_and_retrieve( + 'baremetal:node:delete', node_ident, with_suffix=True) + chassis_uuid = _get_chassis_uuid(rpc_node) notify.emit_start_notification(context, rpc_node, 'delete', chassis_uuid=chassis_uuid) diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 98a6a1462f..e7bb207e66 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -1165,6 +1165,58 @@ def check_policy(policy_name): policy.authorize(policy_name, cdict, cdict) +def check_node_policy_and_retrieve(policy_name, node_ident, with_suffix=False): + """Check if the specified policy authorizes this request on a node. + + :param: policy_name: Name of the policy to check. + :param: node_ident: the UUID or logical name of a node. + :param: with_suffix: whether the RPC node should include the suffix + + :raises: HTTPForbidden if the policy forbids access. + :raises: NodeNotFound if the node is not found. + :return: RPC node identified by node_ident + """ + cdict = api.request.context.to_policy_values() + + try: + if with_suffix: + rpc_node = get_rpc_node_with_suffix(node_ident) + else: + rpc_node = get_rpc_node(node_ident) + except exception.NodeNotFound: + # don't expose non-existence of node unless requester + # has generic access to policy + policy.authorize(policy_name, cdict, cdict) + raise + + target_dict = dict(cdict) + target_dict['node.owner'] = rpc_node['owner'] + policy.authorize(policy_name, target_dict, cdict) + + return rpc_node + + +def check_node_list_policy(owner=None): + """Check if the specified policy authorizes this request on a node. + + :param: owner: owner filter for list query, if any + + :raises: HTTPForbidden if the policy forbids access. + :raises: NodeNotFound if the node is not found. + :return: owner that should be used for list query, if needed + """ + cdict = api.request.context.to_policy_values() + try: + policy.authorize('baremetal:node:list_all', cdict, cdict) + except exception.HTTPForbidden: + project_owner = cdict.get('project_id') + if (not project_owner or (owner and owner != project_owner)): + raise + policy.authorize('baremetal:node:list', cdict, cdict) + return project_owner + 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 d7bc4dae8e..da02187fa4 100644 --- a/ironic/common/policy.py +++ b/ironic/common/policy.py @@ -63,6 +63,9 @@ default_policies = [ policy.RuleDefault('is_admin', 'rule:admin_api or (rule:is_member and role:baremetal_admin)', # noqa description='Full read/write API access'), + policy.RuleDefault('is_node_owner', + 'project_id:%(node.owner)s', + description='Owner of node'), ] # NOTE(deva): to follow policy-in-code spec, we define defaults for @@ -79,10 +82,20 @@ node_policies = [ policy.DocumentedRuleDefault( 'baremetal:node:get', 'rule:is_admin or rule:is_observer', - 'Retrieve Node records', + 'Retrieve a single Node record', + [{'path': '/nodes/{node_ident}', 'method': 'GET'}]), + policy.DocumentedRuleDefault( + 'baremetal:node:list', + 'rule:baremetal:node:get', + 'Retrieve multiple Node records, filtered by owner', [{'path': '/nodes', 'method': 'GET'}, - {'path': '/nodes/detail', 'method': 'GET'}, - {'path': '/nodes/{node_ident}', 'method': 'GET'}]), + {'path': '/nodes/detail', 'method': 'GET'}]), + policy.DocumentedRuleDefault( + 'baremetal:node:list_all', + 'rule:baremetal:node:get', + 'Retrieve multiple Node records', + [{'path': '/nodes', 'method': 'GET'}, + {'path': '/nodes/detail', 'method': 'GET'}]), policy.DocumentedRuleDefault( 'baremetal:node:update', 'rule:is_admin', diff --git a/ironic/tests/unit/api/controllers/v1/test_expose.py b/ironic/tests/unit/api/controllers/v1/test_expose.py index 8951769967..183a5c8937 100644 --- a/ironic/tests/unit/api/controllers/v1/test_expose.py +++ b/ironic/tests/unit/api/controllers/v1/test_expose.py @@ -61,12 +61,14 @@ class TestExposedAPIMethodsCheckPolicy(test_base.TestCase): for func in self.exposed_methods: src = inspect.getsource(func) - self.assertIn('policy.authorize', src, - 'policy.authorize call not found in exposed ' - 'method %s' % func) - self.assertIn('context.to_policy_values', src, - 'context.to_policy_values call not found in ' - 'exposed method %s' % func) + self.assertTrue( + ('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 + ('policy.authorize' in src and + 'context.to_policy_values' in src), + 'no policy check found in in exposed ' + 'method %s' % func) def test_chassis_api_policy(self): self._test('ironic.api.controllers.v1.chassis') diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index acd7c0df89..4415ca5088 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -36,6 +36,7 @@ from ironic.api.controllers.v1 import versions from ironic.common import boot_devices from ironic.common import driver_factory from ironic.common import exception +from ironic.common import policy from ironic.common import states from ironic.conductor import rpcapi from ironic import objects @@ -684,6 +685,75 @@ class TestListNodes(test_api_base.BaseApiTest): expect_errors=True) self.assertEqual(http_client.NOT_FOUND, response.status_int) + @mock.patch.object(policy, 'authorize', spec=True) + def test_detail_forbidden(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + raise exception.HTTPForbidden(resource='fake') + mock_authorize.side_effect = mock_authorize_function + + response = self.get_json('/nodes/detail', expect_errors=True, + headers={ + api_base.Version.string: '1.50', + 'X-Project-Id': '12345' + }) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_detail_list_all_forbidden_no_project(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + response = self.get_json('/nodes/detail', expect_errors=True, + headers={ + api_base.Version.string: '1.49', + }) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_detail_list_all_forbid_owner_proj_mismatch(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + response = self.get_json('/nodes/detail?owner=54321', + expect_errors=True, + headers={ + api_base.Version.string: '1.50', + 'X-Project-Id': '12345' + }) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_detail_list_all_forbidden(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + nodes = [] + for id in range(5): + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid(), + owner='12345') + nodes.append(node.uuid) + for id in range(2): + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid()) + + data = self.get_json('/nodes/detail', headers={ + api_base.Version.string: '1.50', + 'X-Project-Id': '12345'}) + self.assertEqual(len(nodes), len(data['nodes'])) + + uuids = [n['uuid'] for n in data['nodes']] + self.assertEqual(sorted(nodes), sorted(uuids)) + def test_mask_available_state(self): node = obj_utils.create_test_node(self.context, provision_state=states.AVAILABLE) @@ -856,6 +926,75 @@ class TestListNodes(test_api_base.BaseApiTest): self.assertEqual(len(nodes), len(data['nodes'])) self.assertEqual(sorted(node_names), sorted(names)) + @mock.patch.object(policy, 'authorize', spec=True) + def test_many_forbidden(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + raise exception.HTTPForbidden(resource='fake') + mock_authorize.side_effect = mock_authorize_function + + response = self.get_json('/nodes', expect_errors=True, + headers={ + api_base.Version.string: '1.50', + 'X-Project-Id': '12345' + }) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_many_list_all_forbidden_no_project(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + response = self.get_json('/nodes', expect_errors=True, + headers={ + api_base.Version.string: '1.49', + }) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_many_list_all_forbid_owner_proj_mismatch(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + response = self.get_json('/nodes?owner=54321', + expect_errors=True, + headers={ + api_base.Version.string: '1.50', + 'X-Project-Id': '12345' + }) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + + @mock.patch.object(policy, 'authorize', spec=True) + def test_many_list_all_forbidden(self, mock_authorize): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node:list_all': + raise exception.HTTPForbidden(resource='fake') + return True + mock_authorize.side_effect = mock_authorize_function + + nodes = [] + for id in range(5): + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid(), + owner='12345') + nodes.append(node.uuid) + for id in range(2): + node = obj_utils.create_test_node(self.context, + uuid=uuidutils.generate_uuid()) + + data = self.get_json('/nodes', headers={ + api_base.Version.string: '1.50', + 'X-Project-Id': '12345'}) + self.assertEqual(len(nodes), len(data['nodes'])) + + uuids = [n['uuid'] for n in data['nodes']] + self.assertEqual(sorted(nodes), sorted(uuids)) + def _test_links(self, public_url=None): cfg.CONF.set_override('public_endpoint', public_url, 'api') uuid = uuidutils.generate_uuid() diff --git a/ironic/tests/unit/api/controllers/v1/test_utils.py b/ironic/tests/unit/api/controllers/v1/test_utils.py index c1d06d1d78..dd45bba7ef 100644 --- a/ironic/tests/unit/api/controllers/v1/test_utils.py +++ b/ironic/tests/unit/api/controllers/v1/test_utils.py @@ -787,3 +787,207 @@ class TestPortgroupIdent(base.TestCase): self.assertRaises(exception.InvalidUuidOrName, utils.get_rpc_portgroup, self.invalid_name) + + +class TestCheckNodePolicyAndRetrieve(base.TestCase): + def setUp(self): + super(TestCheckNodePolicyAndRetrieve, self).setUp() + self.valid_node_uuid = uuidutils.generate_uuid() + self.node = test_api_utils.post_get_test_node() + self.node['owner'] = '12345' + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + @mock.patch.object(utils, 'get_rpc_node') + @mock.patch.object(utils, 'get_rpc_node_with_suffix') + def test_check_node_policy_and_retrieve( + self, mock_grnws, mock_grn, mock_authorize, mock_pr + ): + mock_pr.version.minor = 50 + mock_pr.context.to_policy_values.return_value = {} + mock_grn.return_value = self.node + + rpc_node = utils.check_node_policy_and_retrieve( + 'fake_policy', self.valid_node_uuid + ) + mock_grn.assert_called_once_with(self.valid_node_uuid) + mock_grnws.assert_not_called() + mock_authorize.assert_called_once_with( + 'fake_policy', {'node.owner': '12345'}, {}) + self.assertEqual(self.node, rpc_node) + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + @mock.patch.object(utils, 'get_rpc_node') + @mock.patch.object(utils, 'get_rpc_node_with_suffix') + def test_check_node_policy_and_retrieve_with_suffix( + self, mock_grnws, mock_grn, mock_authorize, mock_pr + ): + mock_pr.version.minor = 50 + mock_pr.context.to_policy_values.return_value = {} + mock_grnws.return_value = self.node + + rpc_node = utils.check_node_policy_and_retrieve( + 'fake_policy', self.valid_node_uuid, True + ) + mock_grn.assert_not_called() + mock_grnws.assert_called_once_with(self.valid_node_uuid) + mock_authorize.assert_called_once_with( + 'fake_policy', {'node.owner': '12345'}, {}) + self.assertEqual(self.node, rpc_node) + + @mock.patch.object(api, 'request', spec_set=["context"]) + @mock.patch.object(policy, 'authorize', spec=True) + @mock.patch.object(utils, 'get_rpc_node') + def test_check_node_policy_and_retrieve_no_node_policy_forbidden( + self, mock_grn, mock_authorize, mock_pr + ): + mock_pr.context.to_policy_values.return_value = {} + mock_authorize.side_effect = exception.HTTPForbidden(resource='fake') + mock_grn.side_effect = exception.NodeNotFound( + node=self.valid_node_uuid) + + self.assertRaises( + exception.HTTPForbidden, + utils.check_node_policy_and_retrieve, + 'fake-policy', + self.valid_node_uuid + ) + + @mock.patch.object(api, 'request', spec_set=["context"]) + @mock.patch.object(policy, 'authorize', spec=True) + @mock.patch.object(utils, 'get_rpc_node') + def test_check_node_policy_and_retrieve_no_node( + self, mock_grn, mock_authorize, mock_pr + ): + mock_pr.context.to_policy_values.return_value = {} + mock_grn.side_effect = exception.NodeNotFound( + node=self.valid_node_uuid) + + self.assertRaises( + exception.NodeNotFound, + utils.check_node_policy_and_retrieve, + 'fake-policy', + self.valid_node_uuid + ) + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + @mock.patch.object(utils, 'get_rpc_node') + def test_check_node_policy_and_retrieve_policy_forbidden( + self, mock_grn, 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_grn.return_value = self.node + + self.assertRaises( + exception.HTTPForbidden, + utils.check_node_policy_and_retrieve, + 'fake-policy', + self.valid_node_uuid + ) + + +class TestCheckNodeListPolicy(base.TestCase): + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + def test_check_node_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_node_list_policy() + self.assertIsNone(owner) + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + def test_check_node_list_policy_with_owner( + self, mock_authorize, mock_pr + ): + mock_pr.context.to_policy_values.return_value = { + 'project_id': '12345' + } + mock_pr.version.minor = 50 + + owner = utils.check_node_list_policy('12345') + self.assertEqual(owner, '12345') + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + def test_check_node_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_node_list_policy, + ) + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + def test_check_node_list_policy_forbidden_no_project( + self, mock_authorize, mock_pr + ): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node: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_node_list_policy, + ) + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + def test_check_node_list_policy_non_admin( + self, mock_authorize, mock_pr + ): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node: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_node_list_policy() + self.assertEqual(owner, '12345') + + @mock.patch.object(api, 'request', spec_set=["context", "version"]) + @mock.patch.object(policy, 'authorize', spec=True) + def test_check_node_list_policy_non_admin_owner_proj_mismatch( + self, mock_authorize, mock_pr + ): + def mock_authorize_function(rule, target, creds): + if rule == 'baremetal:node: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 + + self.assertRaises( + exception.HTTPForbidden, + utils.check_node_list_policy, + '54321' + ) diff --git a/ironic/tests/unit/common/test_policy.py b/ironic/tests/unit/common/test_policy.py index 2ab5f71d52..448d629fe0 100644 --- a/ironic/tests/unit/common/test_policy.py +++ b/ironic/tests/unit/common/test_policy.py @@ -56,6 +56,19 @@ class PolicyInCodeTestCase(base.TestCase): c = {'project_name': 'demo1', 'project_domain_id': 'default2'} self.assertFalse(policy.check('is_member', c, c)) + def test_is_node_owner(self): + c1 = {'project_id': '1234', + 'project_name': 'demo', + 'project_domain_id': 'default'} + c2 = {'project_id': '5678', + 'project_name': 'demo', + 'project_domain_id': 'default'} + target = dict.copy(c1) + target['node.owner'] = '1234' + + self.assertTrue(policy.check('is_node_owner', target, c1)) + self.assertFalse(policy.check('is_node_owner', target, c2)) + def test_node_get(self): creds = {'roles': ['baremetal_observer'], 'project_name': 'demo', 'project_domain_id': 'default'} diff --git a/releasenotes/notes/node-owner-policy-d7168976bba70566.yaml b/releasenotes/notes/node-owner-policy-d7168976bba70566.yaml new file mode 100644 index 0000000000..0958eb047c --- /dev/null +++ b/releasenotes/notes/node-owner-policy-d7168976bba70566.yaml @@ -0,0 +1,6 @@ +--- +features: + - Adds a ``is_node_owner`` policy rule. This rule can be used with node + policy rules in order to expose specific node APIs to a project ID + specified by a node's ``owner`` field. Default rules are unaffected, + so default behavior is unchanged.