From 5d6886242b54970c66f4603f257151ff65b095cd Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 21 Nov 2019 15:21:18 +0000 Subject: [PATCH] Check that the request payload data is valid json. By adding `force=True` to request.get_json() the mime type is ignore. If the data is not valid json the method will return a `BadRequestError` we catch that and throw our own error with a clear error message "Invalid JSON supplied in POST data". If the json is valid return the json data or an empty dict if None is passed in. This PR improves the error messages if the json is invalid, previously, the error message was "None object type" message which is not very helpful. --- app/schema_validation/__init__.py | 5 --- app/v2/notifications/post_notifications.py | 11 +----- app/v2/template/post_template.py | 16 +++++--- app/v2/utils.py | 13 +++++++ .../notifications/test_post_notifications.py | 5 ++- tests/app/v2/template/test_post_template.py | 37 ++++++++++++++++--- 6 files changed, 60 insertions(+), 27 deletions(-) create mode 100644 app/v2/utils.py diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 089ad9509..e1995f528 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -7,8 +7,6 @@ from jsonschema import (Draft7Validator, ValidationError, FormatChecker) from notifications_utils.recipients import (validate_phone_number, validate_email_address, InvalidPhoneError, InvalidEmailError) -from app.v2.errors import BadRequestError - format_checker = FormatChecker() @@ -57,9 +55,6 @@ def validate_schema_date_with_hour(instance): def validate(json_to_validate, schema): - if json_to_validate is None: - raise BadRequestError(message="Request body is empty.", - status_code=400) validator = Draft7Validator(schema, format_checker=format_checker) errors = list(validator.iter_errors(json_to_validate)) if errors.__len__() > 0: diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index bd6c7d293..1f8b80ae7 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -3,7 +3,6 @@ import functools from flask import request, jsonify, current_app, abort from notifications_utils.recipients import try_validate_and_format_phone_number -from werkzeug.exceptions import BadRequest from app import api_user, authenticated_service, notify_celery, document_download_client from app.celery.letters_pdf_tasks import create_letters_pdf, process_virus_scan_passed @@ -58,6 +57,7 @@ from app.v2.notifications.notification_schemas import ( post_letter_request, post_precompiled_letter_request ) +from app.v2.utils import get_valid_json @v2_notification_blueprint.route('/{}'.format(LETTER_TYPE), methods=['POST']) @@ -173,15 +173,6 @@ def post_notification(notification_type): return jsonify(resp), 201 -def get_valid_json(): - try: - request_json = request.get_json(force=True) - except BadRequest: - raise BadRequestError(message="Invalid JSON supplied in POST data", - status_code=400) - return request_json - - def process_sms_or_email_notification(*, form, notification_type, api_key, template, service, reply_to_text=None): form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number'] diff --git a/app/v2/template/post_template.py b/app/v2/template/post_template.py index 3fae8e2f7..d738c651f 100644 --- a/app/v2/template/post_template.py +++ b/app/v2/template/post_template.py @@ -6,17 +6,21 @@ from app.schema_validation import validate from app.utils import get_template_instance from app.v2.errors import BadRequestError from app.v2.template import v2_template_blueprint -from app.v2.template.template_schemas import post_template_preview_request, create_post_template_preview_response +from app.v2.template.template_schemas import ( + post_template_preview_request, + create_post_template_preview_response +) +from app.v2.utils import get_valid_json @v2_template_blueprint.route("//preview", methods=['POST']) def post_template_preview(template_id): - if not request.content_type or request.content_type != 'application/json': - raise BadRequestError(message="Content-Type header is not set to application/json.", - status_code=400) - _data = request.get_json() - if _data is None: + # If the payload is is empty if there is no personalisation in the template. + _data = request.get_data(as_text=True) + if not _data: _data = {} + else: + _data = get_valid_json() _data['id'] = template_id diff --git a/app/v2/utils.py b/app/v2/utils.py new file mode 100644 index 000000000..699bf18ed --- /dev/null +++ b/app/v2/utils.py @@ -0,0 +1,13 @@ +from flask import request + +from app.v2.errors import BadRequestError +from werkzeug.exceptions import BadRequest + + +def get_valid_json(): + try: + request_json = request.get_json(force=True) + except BadRequest: + raise BadRequestError(message="Invalid JSON supplied in POST data", + status_code=400) + return request_json or {} diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index dee1cd4a0..3d337dfac 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -931,4 +931,7 @@ def test_post_email_notification_when_data_is_empty_returns_400( ) error_msg = json.loads(response.get_data(as_text=True))["errors"][0]["message"] assert response.status_code == 400 - assert error_msg == 'Request body is empty.' + if notification_type == 'sms': + assert error_msg == 'phone_number is a required property' + else: + assert error_msg == 'email_address is a required property' diff --git a/tests/app/v2/template/test_post_template.py b/tests/app/v2/template/test_post_template.py index 2951f7734..4e392c8c6 100644 --- a/tests/app/v2/template/test_post_template.py +++ b/tests/app/v2/template/test_post_template.py @@ -154,13 +154,40 @@ def test_post_template_with_non_existent_template_id_returns_404(client, fake_uu } -def test_post_template_without_content_header(client, sample_template): +def test_post_template_returns_200_without_personalisation(client, sample_template): + response = client.post( + path='/v2/template/{}/preview'.format(sample_template.id), + data=None, + headers=[('Content-Type', 'application/json'), + create_authorization_header(service_id=sample_template.service_id)] + ) + assert response.status_code == 200 + + +def test_post_template_returns_200_without_personalisation_and_missing_content_header(client, sample_template): + response = client.post( + path='/v2/template/{}/preview'.format(sample_template.id), + data=None, + headers=[create_authorization_header(service_id=sample_template.service_id)] + ) + assert response.status_code == 200 + + +def test_post_template_returns_200_without_personalisation_as_valid_json_and_missing_content_header( + client, sample_template +): + response = client.post( + path='/v2/template/{}/preview'.format(sample_template.id), + data=json.dumps(None), + headers=[create_authorization_header(service_id=sample_template.service_id)] + ) + assert response.status_code == 200 + + +def test_post_template_returns_200_with_valid_json_and_missing_content_header(client, sample_template): response = client.post( path='/v2/template/{}/preview'.format(sample_template.id), data=json.dumps(valid_personalisation), headers=[create_authorization_header(service_id=sample_template.service_id)] ) - - assert response.status_code == 400 - json_response = json.loads(response.get_data(as_text=True)) - assert json_response['errors'][0]['message'] == 'Content-Type header is not set to application/json.' + assert response.status_code == 200