From 39e58151afc205fcf10f23256db123bc05007b8b Mon Sep 17 00:00:00 2001 From: Sam Betts Date: Tue, 10 May 2016 12:29:12 +0100 Subject: [PATCH] Prevent URL collisions with sub-controllers: nodes/ports This patch removes the route v1/nodes/ports by removing the ports controllers from being a class attribute of the nodes controller, and starts using the pecan _lookup function to process sub-resources. Co-Authored-By: Vladyslav Drok Partial-Bug: #1580997 Change-Id: I1f76a5f2ea45629a000e61a215b2b32fdd52fb37 --- ironic/api/controllers/v1/node.py | 18 ++++++---- ironic/api/controllers/v1/port.py | 20 +++++------ ironic/tests/unit/api/v1/test_nodes.py | 33 ++++++++++++++++--- ironic/tests/unit/api/v1/test_utils.py | 2 +- .../fix-url-collisions-43abfc8364ca34e7.yaml | 4 +++ 5 files changed, 54 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/fix-url-collisions-43abfc8364ca34e7.yaml diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 4a7644cb39..f7f7fd35ea 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1023,19 +1023,12 @@ class NodesController(rest.RestController): """A resource used for vendors to expose a custom functionality in the API""" - ports = port.PortsController() - """Expose ports as a sub-element of nodes""" - management = NodeManagementController() """Expose management as a sub-element of nodes""" maintenance = NodeMaintenanceController() """Expose maintenance as a sub-element of nodes""" - # Set the flag to indicate that the requests to this resource are - # coming from a top-level resource - ports.from_nodes = True - from_chassis = False """A flag to indicate if the requests to this controller are coming from the top-level resource Chassis""" @@ -1049,6 +1042,17 @@ class NodesController(rest.RestController): 'instance_info', 'driver_internal_info', 'clean_step', 'raid_config', 'target_raid_config'] + _subcontroller_map = { + 'ports': port.PortsController + } + + @pecan.expose() + def _lookup(self, ident, subres, *remainder): + ident = types.uuid_or_name.validate(ident) + subcontroller = self._subcontroller_map.get(subres) + if subcontroller: + return subcontroller(node_ident=ident), remainder + def _get_nodes_collection(self, chassis_uuid, instance_uuid, associated, maintenance, provision_state, marker, limit, sort_key, sort_dir, driver=None, diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py index c3e553c431..9ab9194f78 100644 --- a/ironic/api/controllers/v1/port.py +++ b/ironic/api/controllers/v1/port.py @@ -215,10 +215,6 @@ class PortCollection(collection.Collection): class PortsController(rest.RestController): """REST controller for Ports.""" - from_nodes = False - """A flag to indicate if the requests to this controller are coming - from the top-level resource Nodes.""" - _custom_actions = { 'detail': ['GET'], } @@ -227,12 +223,13 @@ class PortsController(rest.RestController): advanced_net_fields = ['pxe_enabled', 'local_link_connection'] + def __init__(self, node_ident=None): + super(PortsController, self).__init__() + self.parent_node_ident = node_ident + def _get_ports_collection(self, node_ident, address, marker, limit, sort_key, sort_dir, resource_url=None, fields=None): - if self.from_nodes and not node_ident: - raise exception.MissingParameterValue( - _("Node identifier not specified.")) limit = api_utils.validate_limit(limit) sort_dir = api_utils.validate_sort_dir(sort_dir) @@ -247,6 +244,7 @@ class PortsController(rest.RestController): _("The sort_key value %(key)s is an invalid field for " "sorting") % {'key': sort_key}) + node_ident = self.parent_node_ident or node_ident if node_ident: # FIXME(comstud): Since all we need is the node ID, we can # make this more efficient by only querying @@ -395,7 +393,7 @@ class PortsController(rest.RestController): cdict = pecan.request.context.to_dict() policy.authorize('baremetal:port:get', cdict, cdict) - if self.from_nodes: + if self.parent_node_ident: raise exception.OperationNotPermitted() api_utils.check_allow_specify_fields(fields) @@ -414,7 +412,7 @@ class PortsController(rest.RestController): cdict = pecan.request.context.to_dict() policy.authorize('baremetal:port:create', cdict, cdict) - if self.from_nodes: + if self.parent_node_ident: raise exception.OperationNotPermitted() pdict = port.as_dict() @@ -443,7 +441,7 @@ class PortsController(rest.RestController): cdict = pecan.request.context.to_dict() policy.authorize('baremetal:port:update', cdict, cdict) - if self.from_nodes: + if self.parent_node_ident: raise exception.OperationNotPermitted() if not api_utils.allow_port_advanced_net_fields(): for field in self.advanced_net_fields: @@ -495,7 +493,7 @@ class PortsController(rest.RestController): cdict = pecan.request.context.to_dict() policy.authorize('baremetal:port:delete', cdict, cdict) - if self.from_nodes: + if self.parent_node_ident: raise exception.OperationNotPermitted() rpc_port = objects.Port.get_by_uuid(pecan.request.context, port_uuid) diff --git a/ironic/tests/unit/api/v1/test_nodes.py b/ironic/tests/unit/api/v1/test_nodes.py index 5991e3a2da..ac88537121 100644 --- a/ironic/tests/unit/api/v1/test_nodes.py +++ b/ironic/tests/unit/api/v1/test_nodes.py @@ -477,7 +477,7 @@ class TestListNodes(test_api_base.BaseApiTest): obj_utils.create_test_port(self.context, node_id=node.id) # No node id specified response = self.get_json('/nodes/ports', expect_errors=True) - self.assertEqual(http_client.BAD_REQUEST, response.status_int) + self.assertEqual(http_client.NOT_FOUND, response.status_int) def test_ports_subresource_node_not_found(self): non_existent_uuid = 'eeeeeeee-cccc-aaaa-bbbb-cccccccccccc' @@ -1184,10 +1184,18 @@ class TestPatch(test_api_base.BaseApiTest): self.mock_update_node.assert_called_once_with( mock.ANY, mock.ANY, 'test-topic') - def test_patch_ports_subresource(self): + def test_patch_ports_subresource_no_port_id(self): response = self.patch_json('/nodes/%s/ports' % self.node.uuid, [{'path': '/extra/foo', 'value': 'bar', 'op': 'add'}], expect_errors=True) + self.assertEqual(http_client.BAD_REQUEST, response.status_int) + + def test_patch_ports_subresource(self): + response = self.patch_json( + '/nodes/%s/ports/9bb50f13-0b8d-4ade-ad2d-d91fefdef9cc' % + self.node.uuid, + [{'path': '/extra/foo', 'value': 'bar', + 'op': 'add'}], expect_errors=True) self.assertEqual(http_client.FORBIDDEN, response.status_int) def test_remove_uuid(self): @@ -1876,12 +1884,20 @@ class TestPost(test_api_base.BaseApiTest): self.assertEqual(http_client.BAD_REQUEST, response.status_code) self.assertTrue(response.json['error_message']) - def test_post_ports_subresource(self): + def test_post_ports_subresource_no_node_id(self): node = obj_utils.create_test_node(self.context) pdict = test_api_utils.port_post_data(node_id=None) pdict['node_uuid'] = node.uuid response = self.post_json('/nodes/ports', pdict, expect_errors=True) + self.assertEqual(http_client.BAD_REQUEST, response.status_int) + + def test_post_ports_subresource(self): + node = obj_utils.create_test_node(self.context) + pdict = test_api_utils.port_post_data(node_id=None) + pdict['node_uuid'] = node.uuid + response = self.post_json('/nodes/%s/ports' % node.uuid, pdict, + expect_errors=True) self.assertEqual(http_client.FORBIDDEN, response.status_int) def test_create_node_no_mandatory_field_driver(self): @@ -2074,10 +2090,19 @@ class TestDelete(test_api_base.BaseApiTest): self.assertTrue(response.json['error_message']) mock_gbn.assert_called_once_with(mock.ANY, node.name) - def test_delete_ports_subresource(self): + def test_delete_ports_subresource_no_port_id(self): node = obj_utils.create_test_node(self.context) response = self.delete('/nodes/%s/ports' % node.uuid, expect_errors=True) + self.assertEqual(http_client.BAD_REQUEST, response.status_int) + + def test_delete_ports_subresource(self): + node = obj_utils.create_test_node(self.context) + port = obj_utils.create_test_port(self.context, node_id=node.id) + response = self.delete( + '/nodes/%(node_uuid)s/ports/%(port_uuid)s' % + {'node_uuid': node.uuid, 'port_uuid': port.uuid}, + expect_errors=True) self.assertEqual(http_client.FORBIDDEN, response.status_int) @mock.patch.object(rpcapi.ConductorAPI, 'destroy_node') diff --git a/ironic/tests/unit/api/v1/test_utils.py b/ironic/tests/unit/api/v1/test_utils.py index 7c9c8c0a08..b6bed34fe2 100644 --- a/ironic/tests/unit/api/v1/test_utils.py +++ b/ironic/tests/unit/api/v1/test_utils.py @@ -462,7 +462,7 @@ class TestVendorPassthru(base.TestCase): self._test_vendor_passthru_attach(b'\x00\x01', b'\x00\x01') def test_get_controller_reserved_names(self): - expected = ['maintenance', 'management', 'ports', 'states', + expected = ['maintenance', 'management', 'states', 'vendor_passthru', 'validate', 'detail'] self.assertEqual(sorted(expected), sorted(utils.get_controller_reserved_names( diff --git a/releasenotes/notes/fix-url-collisions-43abfc8364ca34e7.yaml b/releasenotes/notes/fix-url-collisions-43abfc8364ca34e7.yaml new file mode 100644 index 0000000000..8a4332cfb4 --- /dev/null +++ b/releasenotes/notes/fix-url-collisions-43abfc8364ca34e7.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - Removed invalid accidental API URL v1/nodes/ports + See https://bugs.launchpad.net/ironic/+bug/1580997