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

@@ -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)

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

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)

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)