From fefb3201eeb3289709a83ddebb1fd51d6c709209 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 11 Mar 2019 13:27:14 +0100 Subject: [PATCH] Check microversions before validations for allocations and deploy templates Currently we check them after the input is checked, which allows leaking the API to older versions. This patch fixes it by moving the checks to the _route call. Additional unit tests are added for this case. Change-Id: Iccd14ed1c4c9f0c03cb1156f184519ad358905dc --- ironic/api/controllers/v1/allocation.py | 33 +++++++++---------- ironic/api/controllers/v1/deploy_template.py | 18 +++++----- .../api/controllers/v1/test_allocation.py | 33 +++++++++++++++++++ .../controllers/v1/test_deploy_template.py | 17 ++++++++++ 4 files changed, 73 insertions(+), 28 deletions(-) diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py index ab157d698c..8dcbf56cfc 100644 --- a/ironic/api/controllers/v1/allocation.py +++ b/ironic/api/controllers/v1/allocation.py @@ -16,6 +16,7 @@ from ironic_lib import metrics_utils from oslo_utils import uuidutils import pecan from six.moves import http_client +from webob import exc as webob_exc import wsme from wsme import types as wtypes @@ -200,6 +201,13 @@ class AllocationsController(pecan.rest.RestController): invalid_sort_key_list = ['extra', 'candidate_nodes', 'traits'] + @pecan.expose() + def _route(self, args, request=None): + if not api_utils.allow_allocations(): + raise webob_exc.HTTPNotFound(_( + "The API version does not allow allocations")) + return super(AllocationsController, self)._route(args, request) + def _get_allocations_collection(self, node_ident=None, resource_class=None, state=None, marker=None, limit=None, sort_key='id', sort_dir='asc', @@ -282,9 +290,6 @@ class AllocationsController(pecan.rest.RestController): :param fields: Optional, a list with a specified set of fields of the resource to be returned. """ - if not api_utils.allow_allocations(): - raise exception.NotFound() - cdict = pecan.request.context.to_policy_values() policy.authorize('baremetal:allocation:get', cdict, cdict) @@ -302,9 +307,6 @@ class AllocationsController(pecan.rest.RestController): :param fields: Optional, a list with a specified set of fields of the resource to be returned. """ - if not api_utils.allow_allocations(): - raise exception.NotFound() - cdict = pecan.request.context.to_policy_values() policy.authorize('baremetal:allocation:get', cdict, cdict) @@ -320,9 +322,6 @@ class AllocationsController(pecan.rest.RestController): :param allocation: an allocation within the request body. """ - if not api_utils.allow_allocations(): - raise exception.NotFound() - context = pecan.request.context cdict = context.to_policy_values() policy.authorize('baremetal:allocation:create', cdict, cdict) @@ -383,9 +382,6 @@ class AllocationsController(pecan.rest.RestController): :param allocation_ident: UUID or logical name of an allocation. """ - if not api_utils.allow_allocations(): - raise exception.NotFound() - context = pecan.request.context cdict = context.to_policy_values() policy.authorize('baremetal:allocation:delete', cdict, cdict) @@ -414,6 +410,13 @@ class NodeAllocationController(pecan.rest.RestController): invalid_sort_key_list = ['extra', 'candidate_nodes', 'traits'] + @pecan.expose() + def _route(self, args, request=None): + if not api_utils.allow_allocations(): + raise webob_exc.HTTPNotFound(_( + "The API version does not allow allocations")) + return super(NodeAllocationController, self)._route(args, request) + def __init__(self, node_ident): super(NodeAllocationController, self).__init__() self.parent_node_ident = node_ident @@ -422,9 +425,6 @@ class NodeAllocationController(pecan.rest.RestController): @METRICS.timer('NodeAllocationController.get_all') @expose.expose(Allocation, types.listtype) def get_all(self, fields=None): - if not api_utils.allow_allocations(): - raise exception.NotFound() - cdict = pecan.request.context.to_policy_values() policy.authorize('baremetal:allocation:get', cdict, cdict) @@ -440,9 +440,6 @@ class NodeAllocationController(pecan.rest.RestController): @METRICS.timer('NodeAllocationController.delete') @expose.expose(None, status_code=http_client.NO_CONTENT) def delete(self): - if not api_utils.allow_allocations(): - raise exception.NotFound() - context = pecan.request.context cdict = context.to_policy_values() policy.authorize('baremetal:allocation:delete', cdict, cdict) diff --git a/ironic/api/controllers/v1/deploy_template.py b/ironic/api/controllers/v1/deploy_template.py index 2f1f67a75c..5b5c8a046e 100644 --- a/ironic/api/controllers/v1/deploy_template.py +++ b/ironic/api/controllers/v1/deploy_template.py @@ -20,6 +20,7 @@ from oslo_utils import uuidutils import pecan from pecan import rest from six.moves import http_client +from webob import exc as webob_exc import wsme from wsme import types as wtypes @@ -46,11 +47,6 @@ _DEPLOY_INTERFACE_TYPE = wtypes.Enum( wtypes.text, *conductor_utils.DEPLOYING_INTERFACE_PRIORITY) -def _check_api_version(): - if not api_utils.allow_deploy_templates(): - raise exception.NotFound() - - class DeployStepType(wtypes.Base, base.AsDictMixin): """A type describing a deployment step.""" @@ -260,6 +256,13 @@ class DeployTemplatesController(rest.RestController): invalid_sort_key_list = ['extra', 'steps'] + @pecan.expose() + def _route(self, args, request=None): + if not api_utils.allow_deploy_templates(): + raise webob_exc.HTTPNotFound(_( + "The API version does not allow deploy templates")) + return super(DeployTemplatesController, self)._route(args, request) + def _update_changed_fields(self, template, rpc_template): """Update rpc_template based on changed fields in a template.""" for field in objects.DeployTemplate.fields: @@ -295,7 +298,6 @@ class DeployTemplatesController(rest.RestController): :param detail: Optional, boolean to indicate whether retrieve a list of deploy templates with detail. """ - _check_api_version() api_utils.check_policy('baremetal:deploy_template:get') api_utils.check_allowed_fields(fields) @@ -338,7 +340,6 @@ class DeployTemplatesController(rest.RestController): :param fields: Optional, a list with a specified set of fields of the resource to be returned. """ - _check_api_version() api_utils.check_policy('baremetal:deploy_template:get') api_utils.check_allowed_fields(fields) @@ -356,7 +357,6 @@ class DeployTemplatesController(rest.RestController): :param template: a deploy template within the request body. """ - _check_api_version() api_utils.check_policy('baremetal:deploy_template:create') context = pecan.request.context @@ -387,7 +387,6 @@ class DeployTemplatesController(rest.RestController): :param template_ident: UUID or logical name of a deploy template. :param patch: a json PATCH document to apply to this deploy template. """ - _check_api_version() api_utils.check_policy('baremetal:deploy_template:update') context = pecan.request.context @@ -434,7 +433,6 @@ class DeployTemplatesController(rest.RestController): :param template_ident: UUID or logical name of a deploy template. """ - _check_api_version() api_utils.check_policy('baremetal:deploy_template:delete') context = pecan.request.context diff --git a/ironic/tests/unit/api/controllers/v1/test_allocation.py b/ironic/tests/unit/api/controllers/v1/test_allocation.py index 0637903aef..45e157f777 100644 --- a/ironic/tests/unit/api/controllers/v1/test_allocation.py +++ b/ironic/tests/unit/api/controllers/v1/test_allocation.py @@ -152,6 +152,14 @@ class TestListAllocations(test_api_base.BaseApiTest): expect_errors=True) self.assertEqual(http_client.NOT_FOUND, response.status_int) + def test_get_one_invalid_api_version_without_check(self): + # Invalid name, but the check happens after the microversion check. + response = self.get_json( + '/allocations/ba!na!na!', + headers={api_base.Version.string: str(api_v1.min_version())}, + expect_errors=True) + self.assertEqual(http_client.NOT_FOUND, response.status_int) + def test_many(self): allocations = [] for id_ in range(5): @@ -326,6 +334,14 @@ class TestListAllocations(test_api_base.BaseApiTest): self.assertEqual({}, data["extra"]) self.assertEqual(self.node.uuid, data["node_uuid"]) + def test_get_by_node_resource_invalid_api_version(self): + obj_utils.create_test_allocation(self.context, node_id=self.node.id) + response = self.get_json( + '/nodes/%s/allocation' % self.node.uuid, + headers={api_base.Version.string: str(api_v1.min_version())}, + expect_errors=True) + self.assertEqual(http_client.NOT_FOUND, response.status_int) + def test_get_by_node_resource_with_fields(self): obj_utils.create_test_allocation(self.context, node_id=self.node.id) data = self.get_json('/nodes/%s/allocation?fields=name,extra' % @@ -657,6 +673,14 @@ class TestDelete(test_api_base.BaseApiTest): headers={api_base.Version.string: '1.14'}) self.assertEqual(http_client.NOT_FOUND, response.status_int) + def test_delete_allocation_invalid_api_version_without_check(self, + mock_destroy): + # Invalid name, but the check happens after the microversion check. + response = self.delete('/allocations/ba!na!na1', + expect_errors=True, + headers={api_base.Version.string: '1.14'}) + self.assertEqual(http_client.NOT_FOUND, response.status_int) + def test_delete_allocation_by_name(self, mock_destroy): self.delete('/allocations/%s' % self.allocation.name, headers=self.headers) @@ -699,3 +723,12 @@ class TestDelete(test_api_base.BaseApiTest): res = self.delete('/nodes/%s/allocation' % uuidutils.generate_uuid(), expect_errors=True, headers=self.headers) self.assertEqual(http_client.NOT_FOUND, res.status_code) + + def test_delete_allocation_by_node_invalid_api_version(self, mock_destroy): + obj_utils.create_test_allocation(self.context, node_id=self.node.id) + response = self.delete( + '/nodes/%s/allocation' % self.node.uuid, + headers={api_base.Version.string: str(api_v1.min_version())}, + expect_errors=True) + self.assertEqual(http_client.NOT_FOUND, response.status_int) + self.assertFalse(mock_destroy.called) diff --git a/ironic/tests/unit/api/controllers/v1/test_deploy_template.py b/ironic/tests/unit/api/controllers/v1/test_deploy_template.py index 2c2bbebb11..6bc77f1916 100644 --- a/ironic/tests/unit/api/controllers/v1/test_deploy_template.py +++ b/ironic/tests/unit/api/controllers/v1/test_deploy_template.py @@ -417,6 +417,16 @@ class TestPatch(BaseDeployTemplatesAPITest): self.assertEqual(http_client.NOT_FOUND, response.status_int) self.assertFalse(mock_save.called) + def test_update_by_name_old_api_version(self, mock_save): + name = 'CUSTOM_DT2' + response = self.patch_json('/deploy_templates/%s' % self.template.name, + [{'path': '/name', + 'value': name, + 'op': 'add'}], + expect_errors=True) + self.assertEqual(http_client.NOT_FOUND, response.status_int) + self.assertFalse(mock_save.called) + def test_update_not_found(self, mock_save): name = 'CUSTOM_DT2' uuid = uuidutils.generate_uuid() @@ -936,6 +946,13 @@ class TestDelete(BaseDeployTemplatesAPITest): headers=self.invalid_version_headers) self.assertEqual(http_client.NOT_FOUND, response.status_int) + def test_delete_old_api_version(self, mock_dpt): + # Names like CUSTOM_1 were not valid in API 1.1, but the check should + # go after the microversion check. + response = self.delete('/deploy_templates/%s' % self.template.name, + expect_errors=True) + self.assertEqual(http_client.NOT_FOUND, response.status_int) + def test_delete_by_name_non_existent(self, mock_dpt): res = self.delete('/deploy_templates/%s' % 'blah', expect_errors=True, headers=self.headers)