From 183c718f46f783f10ede27e85d26cbe5b4d0ec64 Mon Sep 17 00:00:00 2001 From: Felipe Monteiro Date: Sat, 21 Oct 2017 22:56:55 +0100 Subject: [PATCH] Make middleware enforce and validate content-type This PS manually removes logic for adding application/x-yaml content type inside controllers and moves it into a better layer of abstraction: middleware. This PS also fixes a bug with using recently removed function to_yaml_body from the VersionsResource and adds unit tests for testing YAMLTranslator middleware. Change-Id: I4388eb212efee7b300f242eebbc20e22b766fd7d --- deckhand/control/buckets.py | 1 - deckhand/control/middleware.py | 20 +++++ deckhand/control/revision_diffing.py | 1 - deckhand/control/revision_documents.py | 2 - deckhand/control/revision_tags.py | 5 -- deckhand/control/revisions.py | 3 - deckhand/control/rollback.py | 1 - deckhand/control/versions.py | 5 +- .../unit/control/test_buckets_controller.py | 36 ++++++--- .../tests/unit/control/test_middleware.py | 73 +++++++++++++++++++ .../control/test_validations_controller.py | 53 +++++++++----- 11 files changed, 154 insertions(+), 46 deletions(-) create mode 100644 deckhand/tests/unit/control/test_middleware.py diff --git a/deckhand/control/buckets.py b/deckhand/control/buckets.py index 46916f44..8653b79d 100644 --- a/deckhand/control/buckets.py +++ b/deckhand/control/buckets.py @@ -73,7 +73,6 @@ class BucketsResource(api_base.BaseResource): if created_documents: resp.body = self.view_builder.list(created_documents) resp.status = falcon.HTTP_200 - resp.append_header('Content-Type', 'application/x-yaml') def _prepare_secret_documents(self, secret_documents): # Encrypt data for secret documents, if any. diff --git a/deckhand/control/middleware.py b/deckhand/control/middleware.py index f8f76e00..965b466e 100644 --- a/deckhand/control/middleware.py +++ b/deckhand/control/middleware.py @@ -18,6 +18,7 @@ import falcon from oslo_config import cfg from oslo_log import log as logging from oslo_serialization import jsonutils as json +import six import deckhand.context from deckhand import errors @@ -114,7 +115,26 @@ class YAMLTranslator(HookableMiddlewareMixin, object): ``falcon`` middleware. """ + def process_request(self, req, resp): + """Performs content type enforcement on behalf of REST verbs.""" + valid_content_types = ['application/x-yaml'] + content_type = (req.content_type.split(';', 1)[0].strip() + if req.content_type else '') + + if not content_type: + raise falcon.HTTPMissingHeader('Content-Type') + elif content_type not in valid_content_types: + message = ( + "Unexpected content type: {type}. Expected content types " + "are: {expected}." + ).format( + type=six.b(req.content_type).decode('utf-8'), + expected=valid_content_types + ) + raise falcon.HTTPUnsupportedMediaType(description=message) + def process_response(self, req, resp, resource): + """Converts responses to ``application/x-yaml`` content type.""" resp.set_header('Content-Type', 'application/x-yaml') for attr in ('body', 'data'): diff --git a/deckhand/control/revision_diffing.py b/deckhand/control/revision_diffing.py index cd6f728c..85f48bcd 100644 --- a/deckhand/control/revision_diffing.py +++ b/deckhand/control/revision_diffing.py @@ -37,5 +37,4 @@ class RevisionDiffingResource(api_base.BaseResource): raise falcon.HTTPNotFound(description=e.format_message()) resp.status = falcon.HTTP_200 - resp.append_header('Content-Type', 'application/x-yaml') resp.body = resp_body diff --git a/deckhand/control/revision_documents.py b/deckhand/control/revision_documents.py index 85a6762f..639ab3f1 100644 --- a/deckhand/control/revision_documents.py +++ b/deckhand/control/revision_documents.py @@ -61,7 +61,6 @@ class RevisionDocumentsResource(api_base.BaseResource): raise falcon.HTTPNotFound(description=e.format_message()) resp.status = falcon.HTTP_200 - resp.append_header('Content-Type', 'application/x-yaml') resp.body = self.view_builder.list(documents) @@ -108,5 +107,4 @@ class RenderedDocumentsResource(api_base.BaseResource): rendered_documents = secrets_substitution.substitute_all() resp.status = falcon.HTTP_200 - resp.append_header('Content-Type', 'application/x-yaml') resp.body = self.view_builder.list(rendered_documents) diff --git a/deckhand/control/revision_tags.py b/deckhand/control/revision_tags.py index 4eac65b3..39925323 100644 --- a/deckhand/control/revision_tags.py +++ b/deckhand/control/revision_tags.py @@ -51,7 +51,6 @@ class RevisionTagsResource(api_base.BaseResource): resp_body = revision_tag_view.ViewBuilder().show(resp_tag) resp.status = falcon.HTTP_201 - resp.append_header('Content-Type', 'application/x-yaml') resp.body = resp_body def on_get(self, req, resp, revision_id, tag=None): @@ -72,7 +71,6 @@ class RevisionTagsResource(api_base.BaseResource): resp_body = revision_tag_view.ViewBuilder().show(resp_tag) resp.status = falcon.HTTP_200 - resp.append_header('Content-Type', 'application/x-yaml') resp.body = resp_body @policy.authorize('deckhand:list_tags') @@ -85,7 +83,6 @@ class RevisionTagsResource(api_base.BaseResource): resp_body = revision_tag_view.ViewBuilder().list(resp_tags) resp.status = falcon.HTTP_200 - resp.append_header('Content-Type', 'application/x-yaml') resp.body = resp_body def on_delete(self, req, resp, revision_id, tag=None): @@ -104,7 +101,6 @@ class RevisionTagsResource(api_base.BaseResource): errors.RevisionTagNotFound) as e: raise falcon.HTTPNotFound(description=e.format_message()) - resp.append_header('Content-Type', 'application/x-yaml') resp.status = falcon.HTTP_204 @policy.authorize('deckhand:delete_tags') @@ -115,5 +111,4 @@ class RevisionTagsResource(api_base.BaseResource): except errors.RevisionNotFound as e: raise falcon.HTTPNotFound(description=e.format_message()) - resp.append_header('Content-Type', 'application/x-yaml') resp.status = falcon.HTTP_204 diff --git a/deckhand/control/revisions.py b/deckhand/control/revisions.py index ac7084e2..04db035a 100644 --- a/deckhand/control/revisions.py +++ b/deckhand/control/revisions.py @@ -53,7 +53,6 @@ class RevisionsResource(api_base.BaseResource): revision_resp = self.view_builder.show(revision) resp.status = falcon.HTTP_200 - resp.append_header('Content-Type', 'application/x-yaml') resp.body = revision_resp @policy.authorize('deckhand:list_revisions') @@ -63,11 +62,9 @@ class RevisionsResource(api_base.BaseResource): revisions_resp = self.view_builder.list(revisions) resp.status = falcon.HTTP_200 - resp.append_header('Content-Type', 'application/x-yaml') resp.body = revisions_resp @policy.authorize('deckhand:delete_revisions') def on_delete(self, req, resp): db_api.revision_delete_all() - resp.append_header('Content-Type', 'application/x-yaml') resp.status = falcon.HTTP_204 diff --git a/deckhand/control/rollback.py b/deckhand/control/rollback.py index 42099765..d03e9385 100644 --- a/deckhand/control/rollback.py +++ b/deckhand/control/rollback.py @@ -47,5 +47,4 @@ class RollbackResource(api_base.BaseResource): revision_resp = self.view_builder.show(rollback_revision) resp.status = falcon.HTTP_201 - resp.append_header('Content-Type', 'application/x-yaml') resp.body = revision_resp diff --git a/deckhand/control/versions.py b/deckhand/control/versions.py index 906d0b84..51c986d3 100644 --- a/deckhand/control/versions.py +++ b/deckhand/control/versions.py @@ -20,11 +20,10 @@ from deckhand.control import base as api_base class VersionsResource(api_base.BaseResource): def on_get(self, req, resp): - resp.body = self.to_yaml_body({ + resp.body = { 'v1.0': { 'path': '/api/v1.0', 'status': 'stable' } - }) - resp.append_header('Content-Type', 'application/x-yaml') + } resp.status = falcon.HTTP_200 diff --git a/deckhand/tests/unit/control/test_buckets_controller.py b/deckhand/tests/unit/control/test_buckets_controller.py index 0bf21eec..dd6f297d 100644 --- a/deckhand/tests/unit/control/test_buckets_controller.py +++ b/deckhand/tests/unit/control/test_buckets_controller.py @@ -40,8 +40,10 @@ class TestBucketsController(test_base.BaseControllerTest): } payload = documents_factory.gen_test(document_mapping) - resp = self.app.simulate_put('/api/v1.0/bucket/mop/documents', - body=yaml.safe_dump_all(payload)) + resp = self.app.simulate_put( + '/api/v1.0/bucket/mop/documents', + headers={'Content-Type': 'application/x-yaml'}, + body=yaml.safe_dump_all(payload)) self.assertEqual(200, resp.status_code) created_documents = list(yaml.safe_load_all(resp.text)) self.assertEqual(3, len(created_documents)) @@ -53,8 +55,10 @@ class TestBucketsController(test_base.BaseControllerTest): def test_put_bucket_with_secret(self): def _do_test(payload): - resp = self.app.simulate_put('/api/v1.0/bucket/mop/documents', - body=yaml.safe_dump_all(payload)) + resp = self.app.simulate_put( + '/api/v1.0/bucket/mop/documents', + headers={'Content-Type': 'application/x-yaml'}, + body=yaml.safe_dump_all(payload)) self.assertEqual(200, resp.status_code) created_documents = list(yaml.safe_load_all(resp.text)) self.assertEqual(1, len(created_documents)) @@ -123,8 +127,10 @@ schema: '.*mapping values are not allowed here.*'] for idx, payload in enumerate(invalid_payloads): - resp = self.app.simulate_put('/api/v1.0/bucket/mop/documents', - body=payload) + resp = self.app.simulate_put( + '/api/v1.0/bucket/mop/documents', + headers={'Content-Type': 'application/x-yaml'}, + body=payload) self.assertEqual(400, resp.status_code) self.assertRegexpMatches(resp.text, error_re[idx]) @@ -141,8 +147,10 @@ class TestBucketsControllerNegativeRBAC(test_base.BaseControllerTest): documents_factory = factories.DocumentFactory(2, [1, 1]) payload = documents_factory.gen_test({}) - resp = self.app.simulate_put('/api/v1.0/bucket/mop/documents', - body=yaml.safe_dump_all(payload)) + resp = self.app.simulate_put( + '/api/v1.0/bucket/mop/documents', + headers={'Content-Type': 'application/x-yaml'}, + body=yaml.safe_dump_all(payload)) self.assertEqual(403, resp.status_code) def test_put_bucket_cleartext_secret_except_forbidden(self): @@ -152,8 +160,10 @@ class TestBucketsControllerNegativeRBAC(test_base.BaseControllerTest): secrets_factory = factories.DocumentSecretFactory() payload = [secrets_factory.gen_test('Certificate', 'cleartext')] - resp = self.app.simulate_put('/api/v1.0/bucket/mop/documents', - body=yaml.safe_dump_all(payload)) + resp = self.app.simulate_put( + '/api/v1.0/bucket/mop/documents', + headers={'Content-Type': 'application/x-yaml'}, + body=yaml.safe_dump_all(payload)) self.assertEqual(403, resp.status_code) def test_put_bucket_encrypted_secret_except_forbidden(self): @@ -163,6 +173,8 @@ class TestBucketsControllerNegativeRBAC(test_base.BaseControllerTest): secrets_factory = factories.DocumentSecretFactory() payload = [secrets_factory.gen_test('Certificate', 'encrypted')] - resp = self.app.simulate_put('/api/v1.0/bucket/mop/documents', - body=yaml.safe_dump_all(payload)) + resp = self.app.simulate_put( + '/api/v1.0/bucket/mop/documents', + headers={'Content-Type': 'application/x-yaml'}, + body=yaml.safe_dump_all(payload)) self.assertEqual(403, resp.status_code) diff --git a/deckhand/tests/unit/control/test_middleware.py b/deckhand/tests/unit/control/test_middleware.py new file mode 100644 index 00000000..b60e7e47 --- /dev/null +++ b/deckhand/tests/unit/control/test_middleware.py @@ -0,0 +1,73 @@ +# Copyright 2017 AT&T Intellectual Property. All other rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import yaml + +from deckhand.tests.unit.control import base as test_base + + +class TestYAMLTranslator(test_base.BaseControllerTest): + + def test_request_with_correct_content_type(self): + resp = self.app.simulate_get( + '/versions', headers={'Content-Type': 'application/x-yaml'}) + self.assertEqual(200, resp.status_code) + + def test_request_with_correct_content_type_plus_encoding(self): + resp = self.app.simulate_get( + '/versions', + headers={'Content-Type': 'application/x-yaml;encoding=utf-8'}) + self.assertEqual(200, resp.status_code) + + +class TestYAMLTranslatorNegative(test_base.BaseControllerTest): + + def test_request_without_content_type_raises_exception(self): + resp = self.app.simulate_get('/versions') + self.assertEqual(400, resp.status_code) + + expected = { + 'description': 'The Content-Type header is required.', + 'title': 'Missing header value' + } + self.assertEqual(expected, yaml.safe_load(resp.content)) + + def test_request_with_invalid_content_type_raises_exception(self): + resp = self.app.simulate_get( + '/versions', headers={'Content-Type': 'application/json'}) + self.assertEqual(415, resp.status_code) + + expected = { + 'description': "Unexpected content type: application/json. " + "Expected content types are: " + "['application/x-yaml'].", + 'title': 'Unsupported media type' + } + self.assertEqual(expected, yaml.safe_load(resp.content)) + + def test_request_with_invalid_yaml_content_type_raises_exception(self): + """Only application/x-yaml should be supported, not application/yaml, + because it hasn't been registered as an official MIME type yet. + """ + resp = self.app.simulate_get( + '/versions', headers={'Content-Type': 'application/yaml'}) + self.assertEqual(415, resp.status_code) + + expected = { + 'description': "Unexpected content type: application/yaml. " + "Expected content types are: " + "['application/x-yaml'].", + 'title': 'Unsupported media type' + } + self.assertEqual(expected, yaml.safe_load(resp.content)) diff --git a/deckhand/tests/unit/control/test_validations_controller.py b/deckhand/tests/unit/control/test_validations_controller.py index 52269104..ee16c576 100644 --- a/deckhand/tests/unit/control/test_validations_controller.py +++ b/deckhand/tests/unit/control/test_validations_controller.py @@ -61,8 +61,10 @@ class TestValidationsController(test_base.BaseControllerTest): if not payload: documents_factory = factories.DocumentFactory(2, [1, 1]) payload = documents_factory.gen_test({}) - resp = self.app.simulate_put('/api/v1.0/bucket/mop/documents', - body=yaml.safe_dump_all(payload)) + resp = self.app.simulate_put( + '/api/v1.0/bucket/mop/documents', + headers={'Content-Type': 'application/x-yaml'}, + body=yaml.safe_dump_all(payload)) self.assertEqual(200, resp.status_code) revision_id = list(yaml.safe_load_all(resp.text))[0]['status'][ 'revision'] @@ -72,7 +74,7 @@ class TestValidationsController(test_base.BaseControllerTest): resp = self.app.simulate_post( '/api/v1.0/revisions/%s/validations/%s' % (revision_id, validation_name), - body=policy) + headers={'Content-Type': 'application/x-yaml'}, body=policy) return resp def test_create_validation(self): @@ -103,7 +105,8 @@ class TestValidationsController(test_base.BaseControllerTest): revision_id = self._create_revision() resp = self.app.simulate_get( - '/api/v1.0/revisions/%s/validations' % revision_id) + '/api/v1.0/revisions/%s/validations' % revision_id, + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) # Validate that the internal deckhand validation was created already. @@ -132,7 +135,8 @@ class TestValidationsController(test_base.BaseControllerTest): VALIDATION_RESULT) resp = self.app.simulate_get( - '/api/v1.0/revisions/%s/validations' % revision_id) + '/api/v1.0/revisions/%s/validations' % revision_id, + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) @@ -164,7 +168,8 @@ class TestValidationsController(test_base.BaseControllerTest): # /api/v1.0/revisions/1/validations/deckhand-schema-validation resp = self.app.simulate_get( '/api/v1.0/revisions/%s/validations/%s' % ( - revision_id, types.DECKHAND_SCHEMA_VALIDATION)) + revision_id, types.DECKHAND_SCHEMA_VALIDATION), + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) expected_body = { @@ -181,7 +186,8 @@ class TestValidationsController(test_base.BaseControllerTest): # Validate that the entry is present. resp = self.app.simulate_get( '/api/v1.0/revisions/%s/validations/%s' % (revision_id, - validation_name)) + validation_name), + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) @@ -207,7 +213,8 @@ class TestValidationsController(test_base.BaseControllerTest): # Validate that the entry is present. resp = self.app.simulate_get( '/api/v1.0/revisions/%s/validations/%s' % (revision_id, - validation_name)) + validation_name), + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) @@ -224,7 +231,8 @@ class TestValidationsController(test_base.BaseControllerTest): # Validate that 2 entries now exist. resp = self.app.simulate_get( '/api/v1.0/revisions/%s/validations/%s' % (revision_id, - validation_name)) + validation_name), + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) expected_body = { @@ -250,7 +258,8 @@ class TestValidationsController(test_base.BaseControllerTest): resp = self.app.simulate_get( '/api/v1.0/revisions/%s/validations/%s' % (revision_id, - validation_name)) + validation_name), + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) @@ -275,7 +284,8 @@ class TestValidationsController(test_base.BaseControllerTest): resp = self.app.simulate_get( '/api/v1.0/revisions/%s/validations/%s/0' % (revision_id, - validation_name)) + validation_name), + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) @@ -329,7 +339,8 @@ class TestValidationsController(test_base.BaseControllerTest): # Validate that the internal deckhand validation was created. resp = self.app.simulate_get( - '/api/v1.0/revisions/%s/validations' % revision_id) + '/api/v1.0/revisions/%s/validations' % revision_id, + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) expected_body = { @@ -353,7 +364,8 @@ class TestValidationsController(test_base.BaseControllerTest): # Validate that the validation was created and passed. resp = self.app.simulate_get( - '/api/v1.0/revisions/%s/validations' % revision_id) + '/api/v1.0/revisions/%s/validations' % revision_id, + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) expected_body = { @@ -390,7 +402,8 @@ class TestValidationsController(test_base.BaseControllerTest): # Validate that the internal deckhand validation was created. resp = self.app.simulate_get( - '/api/v1.0/revisions/%s/validations' % revision_id) + '/api/v1.0/revisions/%s/validations' % revision_id, + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) expected_body = { @@ -414,7 +427,8 @@ class TestValidationsController(test_base.BaseControllerTest): # Validate that the validation was created and reports failure. resp = self.app.simulate_get( - '/api/v1.0/revisions/%s/validations' % revision_id) + '/api/v1.0/revisions/%s/validations' % revision_id, + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) expected_body = { @@ -451,7 +465,8 @@ class TestValidationsController(test_base.BaseControllerTest): # Validate that the internal deckhand validation was created. resp = self.app.simulate_get( - '/api/v1.0/revisions/%s/validations' % revision_id) + '/api/v1.0/revisions/%s/validations' % revision_id, + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) expected_body = { @@ -478,7 +493,8 @@ class TestValidationsController(test_base.BaseControllerTest): # Validate that the validation reports failure since `fail_doc` # should've failed validation. resp = self.app.simulate_get( - '/api/v1.0/revisions/%s/validations' % revision_id) + '/api/v1.0/revisions/%s/validations' % revision_id, + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text) expected_body = { @@ -509,7 +525,8 @@ class TestValidationsController(test_base.BaseControllerTest): # Validate that the entry is present. resp = self.app.simulate_get( '/api/v1.0/revisions/%s/validations/%s' % ( - revision_id, types.DECKHAND_SCHEMA_VALIDATION)) + revision_id, types.DECKHAND_SCHEMA_VALIDATION), + headers={'Content-Type': 'application/x-yaml'}) self.assertEqual(200, resp.status_code) body = yaml.safe_load(resp.text)