From f64f5725d16cea8ca3546941e5a36ee2b2709ceb Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 20 Feb 2020 17:48:48 +0000 Subject: [PATCH 1/2] Treat 400s from the api as internal server errors if we expect a 400 (for example, the api returns 400 if the service name is already in use) then we should handle that from the view function so we can correctly display a relevation error message to the user. But if it returns a 400 that we didn't expect (for example, because we sent variables of the wrong type through to an endpoint with a schema), then that is a bug that we should fix as any other raised exception. By treating it as a 500 we encourage users to report it to us, and also will get an alert email --- app/__init__.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index c2921bafc..10c82a8a1 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -614,15 +614,10 @@ def register_errorhandlers(application): # noqa (C901 too complex) error.message )) error_code = error.status_code - if error_code == 400: - if isinstance(error.message, str): - msg = [error.message] - else: - msg = list(itertools.chain(*[error.message[x] for x in error.message.keys()])) - resp = make_response(render_template("error/400.html", message=msg)) - return useful_headers_after_request(resp) - elif error_code not in [401, 404, 403, 410]: - # probably a 500 or 503 + if error_code not in [401, 404, 403, 410]: + # probably a 500 or 503. + # it might be a 400, which we should handle as if it's an internal server error. If the API might + # legitimately return a 400, we should handle that within the view or the client that calls it. application.logger.exception("API {} failed with status {} message {}".format( error.response.url if error.response else 'unknown', error.status_code, From d83250c7c2608d2a6f1d4e0dd5d983d46ac430e3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 20 Feb 2020 17:46:48 +0000 Subject: [PATCH 2/2] remove the generic 400 error page handler it just shows a h1, so isn't helpful for people. We can re-use the 500 error page, which includes instructings "Try again later" and instructions on what to do next (check the status page, email notify support). this required refactoring to ensure we can show the 500 error page while still returning the required status code --- app/__init__.py | 27 +++++++++++---------------- app/main/views/uploads.py | 1 - app/templates/error/400.html | 11 ----------- tests/app/main/test_errorhandlers.py | 8 ++++---- tests/app/main/views/test_uploads.py | 1 + 5 files changed, 16 insertions(+), 32 deletions(-) delete mode 100644 app/templates/error/400.html diff --git a/app/__init__.py b/app/__init__.py index 10c82a8a1..11e7f72c0 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -1,4 +1,3 @@ -import itertools import os import urllib from datetime import datetime, timedelta, timezone @@ -602,8 +601,10 @@ def useful_headers_after_request(response): def register_errorhandlers(application): # noqa (C901 too complex) - def _error_response(error_code): - resp = make_response(render_template("error/{0}.html".format(error_code)), error_code) + def _error_response(error_code, error_page_template=None): + if error_page_template is None: + error_page_template = error_code + resp = make_response(render_template("error/{0}.html".format(error_page_template)), error_code) return useful_headers_after_request(resp) @application.errorhandler(HTTPError) @@ -627,8 +628,10 @@ def register_errorhandlers(application): # noqa (C901 too complex) return _error_response(error_code) @application.errorhandler(400) - def handle_400(error): - return _error_response(400) + def handle_client_error(error): + # This is tripped if we call `abort(400)`. + application.logger.exception('Unhandled 400 client error') + return _error_response(400, error_page_template=500) @application.errorhandler(410) def handle_gone(error): @@ -671,19 +674,11 @@ def register_errorhandlers(application): # noqa (C901 too complex) u'csrf.invalid_token: Aborting request, user_id: {user_id}', extra={'user_id': session['user_id']}) - resp = make_response(render_template( - "error/400.html", - message=['Something went wrong, please go back and try again.'] - ), 400) - return useful_headers_after_request(resp) + return _error_response(400, error_page_template=500) @application.errorhandler(405) - def handle_405(error): - resp = make_response(render_template( - "error/400.html", - message=['Something went wrong, please go back and try again.'] - ), 405) - return useful_headers_after_request(resp) + def handle_method_not_allowed(error): + return _error_response(405, error_page_template=500) @application.errorhandler(WerkzeugHTTPException) def handle_http_error(error): diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 60c28c894..332b6f9aa 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -234,7 +234,6 @@ def uploaded_letter_preview(service_id, file_id): @main.route("/services//preview-letter-image/") @user_has_permissions('send_messages') def view_letter_upload_as_preview(service_id, file_id): - try: page = int(request.args.get('page')) except ValueError: diff --git a/app/templates/error/400.html b/app/templates/error/400.html deleted file mode 100644 index f874031c1..000000000 --- a/app/templates/error/400.html +++ /dev/null @@ -1,11 +0,0 @@ -{% extends "withoutnav_template.html" %} -{% block per_page_title %}Bad request{% endblock %} -{% block maincolumn_content %} -
-
-

- {{ message|join(",")}} -

-
-
-{% endblock %} diff --git a/tests/app/main/test_errorhandlers.py b/tests/app/main/test_errorhandlers.py index 6c52612bb..eeb6bac00 100644 --- a/tests/app/main/test_errorhandlers.py +++ b/tests/app/main/test_errorhandlers.py @@ -51,8 +51,8 @@ def test_csrf_returns_400(logged_in_client, mocker): assert response.status_code == 400 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Something went wrong, please go back and try again.' - assert page.title.string.strip() == 'Bad request – GOV.UK Notify' + assert page.h1.string.strip() == 'Sorry, there’s a problem with GOV.UK Notify' + assert page.title.string.strip() == 'Sorry, there’s a problem with the service – GOV.UK Notify' def test_csrf_redirects_to_sign_in_page_if_not_signed_in(client, mocker): @@ -70,5 +70,5 @@ def test_405_returns_something_went_wrong_page(client, mocker): assert response.status_code == 405 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Something went wrong, please go back and try again.' - assert page.title.string.strip() == 'Bad request – GOV.UK Notify' + assert page.h1.string.strip() == 'Sorry, there’s a problem with GOV.UK Notify' + assert page.title.string.strip() == 'Sorry, there’s a problem with the service – GOV.UK Notify' diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index b4d1c9804..081e2fe35 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -504,6 +504,7 @@ def test_uploaded_letter_preview_image_400s_for_bad_page_type( file_id=fake_uuid, service_id=SERVICE_ONE_ID, page='foo', + _test_page_title=False, _expected_status=400, )