From ee9b035cf17f0070dfb392505e44ea2961e2cf4b Mon Sep 17 00:00:00 2001 From: Morgan Fainberg Date: Wed, 10 Oct 2018 13:12:32 -0700 Subject: [PATCH] Replace JSON Body middleware with flask-native func Replace the JSON Body middleware with flask-native before-request function. The body filtering and storing data in request.environ['openstack.params'] was not used in the code base and has been dropped. Test Changes: * JSON Body middleware has been removed, no testing of the removed code * JSON Body Before Request Method has been implemented and associated testing (mirroring the JSON Body middleware code). * Test entry points no longer looks for JSON Body middleware. Change-Id: I84491865870b6bf2b8f094b524ee8b77510f0054 Partial-Bug: #1776504 --- keystone/middleware/core.py | 53 ------------------ keystone/server/flask/application.py | 6 +++ keystone/server/flask/core.py | 3 -- .../flask/request_processing/__init__.py | 0 .../flask/request_processing/json_body.py | 54 +++++++++++++++++++ .../tests/unit/server/test_keystone_flask.py | 42 +++++++++++++++ keystone/tests/unit/test_entry_points.py | 1 - keystone/tests/unit/test_middleware.py | 51 ------------------ setup.cfg | 1 - 9 files changed, 102 insertions(+), 109 deletions(-) create mode 100644 keystone/server/flask/request_processing/__init__.py create mode 100644 keystone/server/flask/request_processing/json_body.py diff --git a/keystone/middleware/core.py b/keystone/middleware/core.py index 8133f06565..1a627a9baa 100644 --- a/keystone/middleware/core.py +++ b/keystone/middleware/core.py @@ -13,66 +13,13 @@ # under the License. from oslo_log import log -from oslo_serialization import jsonutils from keystone.common import wsgi -from keystone import exception LOG = log.getLogger(__name__) -class JsonBodyMiddleware(wsgi.Middleware): - """Middleware to allow method arguments to be passed as serialized JSON. - - Accepting arguments as JSON is useful for accepting data that may be more - complex than simple primitives. - - Filters out the parameters `self`, `context` and anything beginning with - an underscore. - - """ - - def process_request(self, request): - # Abort early if we don't have any work to do - params_json = request.body - if not params_json: - return - - # Reject unrecognized content types. Empty string indicates - # the client did not explicitly set the header - if request.content_type not in ('application/json', ''): - e = exception.ValidationError(attribute='application/json', - target='Content-Type header') - return wsgi.render_exception(e, request=request) - - params_parsed = {} - try: - params_parsed = jsonutils.loads(params_json) - except ValueError: - e = exception.ValidationError(attribute='valid JSON', - target='request body') - return wsgi.render_exception(e, request=request) - finally: - if not params_parsed: - params_parsed = {} - - if not isinstance(params_parsed, dict): - e = exception.ValidationError(attribute='valid JSON object', - target='request body') - return wsgi.render_exception(e, request=request) - - params = {} - for k, v in params_parsed.items(): - if k in ('self', 'context'): - continue - if k.startswith('_'): - continue - params[k] = v - - request.environ[wsgi.PARAMS_ENV] = params - - class NormalizingFilter(wsgi.Middleware): """Middleware filter to handle URL normalization.""" diff --git a/keystone/server/flask/application.py b/keystone/server/flask/application.py index 217567fc30..9e4258d406 100644 --- a/keystone/server/flask/application.py +++ b/keystone/server/flask/application.py @@ -25,6 +25,7 @@ import werkzeug.wsgi import keystone.api from keystone.common import wsgi as keystone_wsgi +from keystone.server.flask.request_processing import json_body # TODO(morgan): _MOVED_API_PREFIXES to be removed when the legacy dispatch # support is removed. @@ -179,6 +180,11 @@ def application_factory(name='public'): # NOTE(morgan): The Flask App actually dispatches nothing until we migrate # some routers to Flask-Blueprints, it is simply a placeholder. app = flask.Flask(name) + + # Add core before request functions + app.before_request(json_body.json_body_before_request) + + # Add core after request functions app.after_request(_add_vary_x_auth_token_header) # NOTE(morgan): Configure the Flask Environment for our needs. diff --git a/keystone/server/flask/core.py b/keystone/server/flask/core.py index ddcbe4295a..36a494a9b4 100644 --- a/keystone/server/flask/core.py +++ b/keystone/server/flask/core.py @@ -54,9 +54,6 @@ _APP_MIDDLEWARE = ( _Middleware(namespace='keystone.server_middleware', ep='url_normalize', conf={}), - _Middleware(namespace='keystone.server_middleware', - ep='json_body', - conf={}), _Middleware(namespace='keystone.server_middleware', ep='osprofiler', conf={}), diff --git a/keystone/server/flask/request_processing/__init__.py b/keystone/server/flask/request_processing/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/keystone/server/flask/request_processing/json_body.py b/keystone/server/flask/request_processing/json_body.py new file mode 100644 index 0000000000..1203401581 --- /dev/null +++ b/keystone/server/flask/request_processing/json_body.py @@ -0,0 +1,54 @@ +# 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. + +# Before request processing for JSON Body enforcement + +import flask +from werkzeug import exceptions as werkzeug_exceptions + +from keystone import exception +from keystone.i18n import _ + + +def json_body_before_request(): + """Enforce JSON Request Body.""" + # TODO(morgan): Allow other content-types when OpenAPI Doc or improved + # federation is implemented for known/valid paths. This function should + # be removed long term. + + # exit if there is nothing to be done, (no body) + if not flask.request.get_data(): + return None + + try: + # flask does loading for us for json, use the flask default loader + # in the case that the data is *not* json or a dict, we should see a + # raise of werkzeug.exceptions.BadRequest, re-spin this to the keystone + # ValidationError message (as expected by our contract) + + # Explicitly check if the content is supposed to be json. + if (flask.request.is_json + or flask.request.headers['Content-Type'] == ''): + json_decoded = flask.request.get_json(force=True) + if not isinstance(json_decoded, dict): + # In the case that the returned value was not a dict, force + # a raise that will be caught the same way that a Decode error + # would be handled. + raise werkzeug_exceptions.BadRequest( + _('resulting JSON load was not a dict')) + else: + raise exception.ValidationError(attribute='application/json', + target='Content-Type header') + + except werkzeug_exceptions.BadRequest: + raise exception.ValidationError(attribute='valid JSON', + target='request body') diff --git a/keystone/tests/unit/server/test_keystone_flask.py b/keystone/tests/unit/server/test_keystone_flask.py index 69ea3343d1..94a7a00d8b 100644 --- a/keystone/tests/unit/server/test_keystone_flask.py +++ b/keystone/tests/unit/server/test_keystone_flask.py @@ -26,6 +26,7 @@ from keystone.common import rbac_enforcer import keystone.conf from keystone import exception from keystone.server.flask import common as flask_common +from keystone.server.flask.request_processing import json_body from keystone.tests.unit import rest @@ -654,3 +655,44 @@ class TestKeystoneFlaskCommon(rest.RestfulTestCase): self.assertTrue(ref['links']['self'].startswith( 'https://localhost/v3/%s' % view_arg) ) + + def test_json_body_before_req_func_valid_json(self): + with self.test_request_context( + headers={'Content-Type': 'application/json'}, + data='{"key": "value"}'): + # No exception should be raised, everything is happy. + json_body.json_body_before_request() + + def test_json_body_before_req_func_invalid_json(self): + with self.test_request_context( + headers={'Content-Type': 'application/json'}, + data='invalid JSON'): + # keystone.exception.ValidationError should be raised + self.assertRaises(exception.ValidationError, + json_body.json_body_before_request) + + def test_json_body_before_req_func_no_content_type(self): + # Unset + with self.test_request_context(data='{"key": "value"}'): + # No exception should be raised, everything is happy. + json_body.json_body_before_request() + + # Explicitly set to '' + with self.test_request_context( + headers={'Content-Type': ''}, data='{"key": "value"}'): + # No exception should be raised, everything is happy. + json_body.json_body_before_request() + + def test_json_body_before_req_func_unrecognized_content_type(self): + with self.test_request_context( + headers={'Content-Type': 'unrecognized/content-type'}, + data='{"key": "value"'): + # keystone.exception.ValidationError should be raised + self.assertRaises(exception.ValidationError, + json_body.json_body_before_request) + + def test_json_body_before_req_func_unrecognized_conten_type_no_body(self): + with self.test_request_context( + headers={'Content-Type': 'unrecognized/content-type'}): + # No exception should be raised, everything is happy. + json_body.json_body_before_request() diff --git a/keystone/tests/unit/test_entry_points.py b/keystone/tests/unit/test_entry_points.py index ba7f06cfcb..340be0b920 100644 --- a/keystone/tests/unit/test_entry_points.py +++ b/keystone/tests/unit/test_entry_points.py @@ -23,7 +23,6 @@ class TestEntryPoints(test.TestCase): 'build_auth_context', 'cors', 'debug', - 'json_body', 'request_id', 'sizelimit', 'url_normalize', diff --git a/keystone/tests/unit/test_middleware.py b/keystone/tests/unit/test_middleware.py index 5bf1e0c2c3..d1a21a4fd0 100644 --- a/keystone/tests/unit/test_middleware.py +++ b/keystone/tests/unit/test_middleware.py @@ -103,57 +103,6 @@ class MiddlewareRequestTestBase(unit.TestCase): return self._do_middleware_response(*args, **kwargs).request -class JsonBodyMiddlewareTest(MiddlewareRequestTestBase): - - MIDDLEWARE_CLASS = middleware.JsonBodyMiddleware - - def test_request_with_params(self): - headers = {'Content-Type': 'application/json'} - params = '{"arg1": "one", "arg2": ["a"]}' - req = self._do_middleware_request(params=params, - headers=headers, - method='post') - self.assertEqual({"arg1": "one", "arg2": ["a"]}, - req.environ[wsgi.PARAMS_ENV]) - - def test_malformed_json(self): - headers = {'Content-Type': 'application/json'} - self._do_middleware_response(params='{"arg1": "on', - headers=headers, - method='post', - status=http_client.BAD_REQUEST) - - def test_not_dict_body(self): - headers = {'Content-Type': 'application/json'} - resp = self._do_middleware_response(params='42', - headers=headers, - method='post', - status=http_client.BAD_REQUEST) - - self.assertIn('valid JSON object', resp.json['error']['message']) - - def test_no_content_type(self): - headers = {'Content-Type': ''} - params = '{"arg1": "one", "arg2": ["a"]}' - req = self._do_middleware_request(params=params, - headers=headers, - method='post') - self.assertEqual({"arg1": "one", "arg2": ["a"]}, - req.environ[wsgi.PARAMS_ENV]) - - def test_unrecognized_content_type(self): - headers = {'Content-Type': 'text/plain'} - self._do_middleware_response(params='{"arg1": "one", "arg2": ["a"]}', - headers=headers, - method='post', - status=http_client.BAD_REQUEST) - - def test_unrecognized_content_type_without_body(self): - headers = {'Content-Type': 'text/plain'} - req = self._do_middleware_request(headers=headers) - self.assertEqual({}, req.environ.get(wsgi.PARAMS_ENV, {})) - - class AuthContextMiddlewareTest(test_backend_sql.SqlTests, MiddlewareRequestTestBase): diff --git a/setup.cfg b/setup.cfg index a2471fb0be..c0b30b8d43 100644 --- a/setup.cfg +++ b/setup.cfg @@ -196,5 +196,4 @@ keystone.server_middleware = request_id = oslo_middleware:RequestId build_auth_context = keystone.middleware:AuthContextMiddleware token_auth = keystone.middleware:TokenAuthMiddleware - json_body = keystone.middleware:JsonBodyMiddleware debug = oslo_middleware:Debug