From 204aaf172d87755ec6c48ed3b4163cb9b98326d3 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 4 Apr 2018 17:31:02 +0100 Subject: [PATCH 1/5] Add a document download client Allows uploading documents to the Document Download API. The client is configured with an API host and auth token. There's no need for a flag to disable the client in the test environments at the moment since the upload is only triggered by a specific payload which would only be sent with an explicit goal of using document download. --- app/__init__.py | 3 ++ app/clients/document_download.py | 27 +++++++++++++++ app/config.py | 3 ++ tests/app/clients/test_document_download.py | 37 +++++++++++++++++++++ 4 files changed, 70 insertions(+) create mode 100644 app/clients/document_download.py create mode 100644 tests/app/clients/test_document_download.py diff --git a/app/__init__.py b/app/__init__.py index 81eeec6e0..df76d9b81 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -16,6 +16,7 @@ from werkzeug.local import LocalProxy from app.celery.celery import NotifyCelery from app.clients import Clients +from app.clients.document_download import DocumentDownloadClient from app.clients.email.aws_ses import AwsSesClient from app.clients.sms.firetext import FiretextClient from app.clients.sms.loadtesting import LoadtestingClient @@ -39,6 +40,7 @@ deskpro_client = DeskproClient() statsd_client = StatsdClient() redis_store = RedisClient() performance_platform_client = PerformancePlatformClient() +document_download_client = DocumentDownloadClient() clients = Clients() @@ -71,6 +73,7 @@ def create_app(application): encryption.init_app(application) redis_store.init_app(application) performance_platform_client.init_app(application) + document_download_client.init_app(application) clients.init_app(sms_clients=[firetext_client, mmg_client, loadtest_client], email_clients=[aws_ses_client]) register_blueprint(application) diff --git a/app/clients/document_download.py b/app/clients/document_download.py new file mode 100644 index 000000000..cb99ac88e --- /dev/null +++ b/app/clients/document_download.py @@ -0,0 +1,27 @@ +import base64 + +import requests + + +class DocumentDownloadClient: + + def init_app(self, app): + self.api_host = app.config['DOCUMENT_DOWNLOAD_API_HOST'] + self.auth_token = app.config['DOCUMENT_DOWNLOAD_API_KEY'] + + def get_upload_url(self, service_id): + return "{}/services/{}/documents".format(self.api_host, service_id) + + def upload_document(self, service_id, file_contents): + response = requests.post( + self.get_upload_url(service_id), + headers={ + 'Authorization': "Bearer {}".format(self.auth_token), + }, + files={ + 'document': base64.b64decode(file_contents) + } + ) + response.raise_for_status() + + return response.json()['document']['url'] diff --git a/app/config.py b/app/config.py index b9c6b9d9b..035416c70 100644 --- a/app/config.py +++ b/app/config.py @@ -318,6 +318,9 @@ class Config(object): TEMPLATE_PREVIEW_API_HOST = os.environ.get('TEMPLATE_PREVIEW_API_HOST', 'http://localhost:6013') TEMPLATE_PREVIEW_API_KEY = os.environ.get('TEMPLATE_PREVIEW_API_KEY', 'my-secret-key') + DOCUMENT_DOWNLOAD_API_HOST = os.environ.get('DOCUMENT_DOWNLOAD_API_HOST', 'http://localhost:7000') + DOCUMENT_DOWNLOAD_API_KEY = os.environ.get('DOCUMENT_DOWNLOAD_API_KEY', 'auth-token') + LETTER_PROCESSING_DEADLINE = time(17, 30) MMG_URL = "https://api.mmg.co.uk/json/api.php" diff --git a/tests/app/clients/test_document_download.py b/tests/app/clients/test_document_download.py new file mode 100644 index 000000000..e01094192 --- /dev/null +++ b/tests/app/clients/test_document_download.py @@ -0,0 +1,37 @@ +import requests +import requests_mock +import pytest + +from app.clients.document_download import DocumentDownloadClient + + +@pytest.fixture(scope='function') +def document_download(client, mocker): + client = DocumentDownloadClient() + current_app = mocker.Mock(config={ + 'DOCUMENT_DOWNLOAD_API_HOST': 'https://document-download', + 'DOCUMENT_DOWNLOAD_API_KEY': 'test-key' + }) + client.init_app(current_app) + return client + + +def test_get_upload_url(document_download): + assert document_download.get_upload_url('service-id') == 'https://document-download/services/service-id/documents' + + +def test_upload_document(document_download): + with requests_mock.Mocker() as request_mock: + request_mock.post('https://document-download/services/service-id/documents', json={ + 'document': {'url': 'https://document-download/services/service-id/documents/uploaded-url'} + }, status_code=201) + + resp = document_download.upload_document('service-id', 'abababab') + + assert resp == 'https://document-download/services/service-id/documents/uploaded-url' + + +def test_should_raise_for_status(document_download): + with pytest.raises(requests.HTTPError), requests_mock.Mocker() as request_mock: + request_mock.post('https://document-download/services/service-id/documents', json={}, status_code=403) + document_download.upload_document('service-id', 'abababab') From f2e163dc4324fda8fe5c8d1b1b83b8fefab0fbab Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 4 Apr 2018 17:34:14 +0100 Subject: [PATCH 2/5] Upload files from personalisation data to document download Adds support for a new personalisation value type: file upload. File uploads are represented as a dictionary with a "file" key and a base64-encoded file data as the key's value: ``` personalisation={ 'field1': {'file': ''} } ``` Post notification endpoint checks the request personalisation data looking for the file uploads in personalisation data. If any are found and the service has permissions to upload documents the files are sent to document download API and personalisation values are replaced with the URLs returned in the document download response. A fake document URL is returned for simulated notifications, no documents are stored in Document Download. Multiple files can be uploaded for one notification by providing a file upload in more than one personalisation field. --- app/v2/notifications/post_notifications.py | 33 ++++++- .../notifications/test_post_notifications.py | 96 ++++++++++++++++++- 2 files changed, 124 insertions(+), 5 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index ef1941d01..998636859 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -7,7 +7,7 @@ from flask import request, jsonify, current_app, abort from notifications_utils.pdf import pdf_page_count, PdfReadError from notifications_utils.recipients import try_validate_and_format_phone_number -from app import api_user, authenticated_service, notify_celery +from app import api_user, authenticated_service, notify_celery, document_download_client from app.config import QueueNames, TaskNames from app.dao.notifications_dao import dao_update_notification, update_notification_status_by_reference from app.dao.templates_dao import dao_create_template @@ -19,6 +19,7 @@ from app.models import ( EMAIL_TYPE, LETTER_TYPE, PRECOMPILED_LETTER, + UPLOAD_DOCUMENT, PRIORITY, KEY_TYPE_TEST, KEY_TYPE_TEAM, @@ -136,7 +137,7 @@ def post_notification(notification_type): reply_to_text=reply_to ) else: - notification = process_sms_or_email_notification( + notification, personalisation = process_sms_or_email_notification( form=form, notification_type=notification_type, api_key=api_user, @@ -145,6 +146,8 @@ def post_notification(notification_type): reply_to_text=reply_to ) + template_with_content.values = personalisation + if notification_type == SMS_TYPE: create_resp_partial = functools.partial( create_post_sms_response_from_notification, @@ -182,12 +185,14 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ # Do not persist or send notification to the queue if it is a simulated recipient simulated = simulated_recipient(send_to, notification_type) + personalisation = process_document_uploads(form.get('personalisation'), service, simulated=simulated) + notification = persist_notification( template_id=template.id, template_version=template.version, recipient=form_send_to, service=service, - personalisation=form.get('personalisation', None), + personalisation=personalisation, notification_type=notification_type, api_key_id=api_key.id, key_type=api_key.key_type, @@ -210,7 +215,27 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ else: current_app.logger.debug("POST simulated notification for id: {}".format(notification.id)) - return notification + return notification, personalisation + + +def process_document_uploads(personalisation_data, service, simulated=False): + file_keys = [k for k, v in (personalisation_data or {}).items() if isinstance(v, dict) and 'file' in v] + if not file_keys: + return personalisation_data + + personalisation_data = personalisation_data.copy() + + check_service_has_permission(UPLOAD_DOCUMENT, authenticated_service.permissions) + + for key in file_keys: + if simulated: + personalisation_data[key] = document_download_client.get_upload_url(service.id) + '/test-document' + else: + personalisation_data[key] = document_download_client.upload_document( + service.id, personalisation_data[key]['file'] + ) + + return personalisation_data def process_letter_notification(*, letter_data, api_key, template, reply_to_text, precompiled=False): diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 9c2927e76..2bd3c1b96 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -9,7 +9,8 @@ from app.models import ( EMAIL_TYPE, NOTIFICATION_CREATED, SCHEDULE_NOTIFICATIONS, - SMS_TYPE + SMS_TYPE, + UPLOAD_DOCUMENT ) from flask import json, current_app @@ -697,3 +698,96 @@ def test_post_email_notification_with_invalid_reply_to_id_returns_400(client, sa assert 'email_reply_to_id {} does not exist in database for service id {}'. \ format(fake_uuid, sample_email_template.service_id) in resp_json['errors'][0]['message'] assert 'BadRequestError' in resp_json['errors'][0]['error'] + + +def test_post_notification_with_document_upload(client, notify_db, notify_db_session, mocker): + service = sample_service(notify_db, notify_db_session, permissions=[EMAIL_TYPE, UPLOAD_DOCUMENT]) + template = create_sample_template( + notify_db, notify_db_session, service=service, + template_type='email', + content="Document: ((document))" + ) + + mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + document_download_mock = mocker.patch('app.v2.notifications.post_notifications.document_download_client') + document_download_mock.upload_document.return_value = 'https://document-url/' + + data = { + "email_address": service.users[0].email_address, + "template_id": template.id, + "personalisation": {"document": {"file": "abababab"}} + } + + auth_header = create_authorization_header(service_id=service.id) + response = client.post( + path="v2/notifications/email", + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201, response.get_data(as_text=True) + resp_json = json.loads(response.get_data(as_text=True)) + assert validate(resp_json, post_email_response) == resp_json + + notification = Notification.query.one() + assert notification.status == NOTIFICATION_CREATED + assert notification.personalisation == {'document': 'https://document-url/'} + + assert resp_json['content']['body'] == 'Document: https://document-url/' + + +def test_post_notification_with_document_upload_simulated(client, notify_db, notify_db_session, mocker): + service = sample_service(notify_db, notify_db_session, permissions=[EMAIL_TYPE, UPLOAD_DOCUMENT]) + template = create_sample_template( + notify_db, notify_db_session, service=service, + template_type='email', + content="Document: ((document))" + ) + + mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + document_download_mock = mocker.patch('app.v2.notifications.post_notifications.document_download_client') + document_download_mock.get_upload_url.return_value = 'https://document-url' + + data = { + "email_address": 'simulate-delivered@notifications.service.gov.uk', + "template_id": template.id, + "personalisation": {"document": {"file": "abababab"}} + } + + auth_header = create_authorization_header(service_id=service.id) + response = client.post( + path="v2/notifications/email", + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201, response.get_data(as_text=True) + resp_json = json.loads(response.get_data(as_text=True)) + assert validate(resp_json, post_email_response) == resp_json + + assert resp_json['content']['body'] == 'Document: https://document-url/test-document' + + +def test_post_notification_without_document_upload_permission(client, notify_db, notify_db_session, mocker): + service = sample_service(notify_db, notify_db_session, permissions=[EMAIL_TYPE]) + template = create_sample_template( + notify_db, notify_db_session, service=service, + template_type='email', + content="Document: ((document))" + ) + + mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + document_download_mock = mocker.patch('app.v2.notifications.post_notifications.document_download_client') + document_download_mock.upload_document.return_value = 'https://document-url/' + + data = { + "email_address": service.users[0].email_address, + "template_id": template.id, + "personalisation": {"document": {"file": "abababab"}} + } + + auth_header = create_authorization_header(service_id=service.id) + response = client.post( + path="v2/notifications/email", + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400, response.get_data(as_text=True) From b097a16a86eaa7edc1f6ba2875c3a3a623942c2b Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 9 Apr 2018 16:09:54 +0100 Subject: [PATCH 3/5] Handle and log document-download-api request errors Catches the requests exception for document-download-api calls, logs a warning and returns a matching response code and message. Connection errors to document download result in 503 response to the user. --- app/clients/document_download.py | 48 ++++++++++++++++----- app/v2/notifications/post_notifications.py | 10 +++-- tests/app/clients/test_document_download.py | 25 +++++++++-- 3 files changed, 67 insertions(+), 16 deletions(-) diff --git a/app/clients/document_download.py b/app/clients/document_download.py index cb99ac88e..b40184452 100644 --- a/app/clients/document_download.py +++ b/app/clients/document_download.py @@ -2,6 +2,25 @@ import base64 import requests +from flask import current_app + + +class DocumentDownloadError(Exception): + def __init__(self, message, status_code): + self.message = message + self.status_code = status_code + + @classmethod + def from_exception(cls, e): + try: + message = e.response.json()['error'] + status_code = e.response.status_code + except (TypeError, ValueError, AttributeError, KeyError): + message = 'connection error' + status_code = 503 + + return cls(message, status_code) + class DocumentDownloadClient: @@ -13,15 +32,24 @@ class DocumentDownloadClient: return "{}/services/{}/documents".format(self.api_host, service_id) def upload_document(self, service_id, file_contents): - response = requests.post( - self.get_upload_url(service_id), - headers={ - 'Authorization': "Bearer {}".format(self.auth_token), - }, - files={ - 'document': base64.b64decode(file_contents) - } - ) - response.raise_for_status() + try: + response = requests.post( + self.get_upload_url(service_id), + headers={ + 'Authorization': "Bearer {}".format(self.auth_token), + }, + files={ + 'document': base64.b64decode(file_contents) + } + ) + + response.raise_for_status() + except requests.RequestException as e: + error = DocumentDownloadError.from_exception(e) + current_app.logger.warning( + 'Document download request failed with error: {}'.format(error.message) + ) + + raise error return response.json()['document']['url'] diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 998636859..388a78168 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -8,6 +8,7 @@ from notifications_utils.pdf import pdf_page_count, PdfReadError from notifications_utils.recipients import try_validate_and_format_phone_number from app import api_user, authenticated_service, notify_celery, document_download_client +from app.clients.document_download import DocumentDownloadError from app.config import QueueNames, TaskNames from app.dao.notifications_dao import dao_update_notification, update_notification_status_by_reference from app.dao.templates_dao import dao_create_template @@ -231,9 +232,12 @@ def process_document_uploads(personalisation_data, service, simulated=False): if simulated: personalisation_data[key] = document_download_client.get_upload_url(service.id) + '/test-document' else: - personalisation_data[key] = document_download_client.upload_document( - service.id, personalisation_data[key]['file'] - ) + try: + personalisation_data[key] = document_download_client.upload_document( + service.id, personalisation_data[key]['file'] + ) + except DocumentDownloadError as e: + raise BadRequestError(message=e.message, status_code=e.status_code) return personalisation_data diff --git a/tests/app/clients/test_document_download.py b/tests/app/clients/test_document_download.py index e01094192..0d6cc6473 100644 --- a/tests/app/clients/test_document_download.py +++ b/tests/app/clients/test_document_download.py @@ -2,7 +2,7 @@ import requests import requests_mock import pytest -from app.clients.document_download import DocumentDownloadClient +from app.clients.document_download import DocumentDownloadClient, DocumentDownloadError @pytest.fixture(scope='function') @@ -32,6 +32,25 @@ def test_upload_document(document_download): def test_should_raise_for_status(document_download): - with pytest.raises(requests.HTTPError), requests_mock.Mocker() as request_mock: - request_mock.post('https://document-download/services/service-id/documents', json={}, status_code=403) + with pytest.raises(DocumentDownloadError) as excinfo, requests_mock.Mocker() as request_mock: + request_mock.post('https://document-download/services/service-id/documents', json={ + 'error': 'Invalid encoding' + }, status_code=403) + document_download.upload_document('service-id', 'abababab') + + assert excinfo.value.message == 'Invalid encoding' + assert excinfo.value.status_code == 403 + + +def test_should_raise_for_connection_errors(document_download): + with pytest.raises(DocumentDownloadError) as excinfo, requests_mock.Mocker() as request_mock: + request_mock.post( + 'https://document-download/services/service-id/documents', + exc=requests.exceptions.ConnectTimeout + ) + + document_download.upload_document('service-id', 'abababab') + + assert excinfo.value.message == 'connection error' + assert excinfo.value.status_code == 503 From c189f09687b3fbd016896b8abd67365d2ce1feaa Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 9 Apr 2018 16:27:56 +0100 Subject: [PATCH 4/5] Update template content values from notification personalisation We need to set template_with_content placeholder values with an updated personalisation dictionary after processing the document uploads in order to avoid returning base64 file data instead of the document URLs in the response contents. --- app/v2/notifications/post_notifications.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 388a78168..178893218 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -138,7 +138,7 @@ def post_notification(notification_type): reply_to_text=reply_to ) else: - notification, personalisation = process_sms_or_email_notification( + notification = process_sms_or_email_notification( form=form, notification_type=notification_type, api_key=api_user, @@ -147,7 +147,7 @@ def post_notification(notification_type): reply_to_text=reply_to ) - template_with_content.values = personalisation + template_with_content.values = notification.personalisation if notification_type == SMS_TYPE: create_resp_partial = functools.partial( @@ -216,7 +216,7 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ else: current_app.logger.debug("POST simulated notification for id: {}".format(notification.id)) - return notification, personalisation + return notification def process_document_uploads(personalisation_data, service, simulated=False): From 89f2f8b9a7b6992ecd2f1da01678cf10b2fc55d7 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 9 Apr 2018 16:29:53 +0100 Subject: [PATCH 5/5] Test that document download requests set the auth header --- tests/app/clients/test_document_download.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/app/clients/test_document_download.py b/tests/app/clients/test_document_download.py index 0d6cc6473..8a15ba1c1 100644 --- a/tests/app/clients/test_document_download.py +++ b/tests/app/clients/test_document_download.py @@ -24,6 +24,8 @@ def test_upload_document(document_download): with requests_mock.Mocker() as request_mock: request_mock.post('https://document-download/services/service-id/documents', json={ 'document': {'url': 'https://document-download/services/service-id/documents/uploaded-url'} + }, request_headers={ + 'Authorization': 'Bearer test-key', }, status_code=201) resp = document_download.upload_document('service-id', 'abababab')