From 055a5ee7eb0784066aa6ea411179760a9ff4cd4b Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 13 Mar 2018 14:06:03 +0000 Subject: [PATCH 01/12] Add letter-scan bucket name to config --- app/config.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/config.py b/app/config.py index a69f61c68..981ed9ed2 100644 --- a/app/config.py +++ b/app/config.py @@ -328,6 +328,8 @@ class Development(Config): LETTERS_PDF_BUCKET_NAME = 'development-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'development-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notify.tools-ftp' + LETTERS_PDF_BUCKET_NAME = 'development-letters-pdf' + LETTERS_SCAN_BUCKET_NAME = 'development-letters-scan' ADMIN_CLIENT_SECRET = 'dev-notify-secret-key' SECRET_KEY = 'dev-notify-secret-key' @@ -365,6 +367,8 @@ class Test(Development): LETTERS_PDF_BUCKET_NAME = 'test-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'test-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'test.notify.com-ftp' + LETTERS_PDF_BUCKET_NAME = 'test-letters-pdf' + LETTERS_SCAN_BUCKET_NAME = 'test-letters-scan' # this is overriden in jenkins and on cloudfoundry SQLALCHEMY_DATABASE_URI = os.getenv('SQLALCHEMY_DATABASE_URI', 'postgresql://localhost/test_notification_api') @@ -393,6 +397,8 @@ class Preview(Config): LETTERS_PDF_BUCKET_NAME = 'preview-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'preview-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' + LETTERS_PDF_BUCKET_NAME = 'preview-letters-pdf' + LETTERS_SCAN_BUCKET_NAME = 'preview-letters-scan' FROM_NUMBER = 'preview' API_RATE_LIMIT_ENABLED = True CHECK_PROXY_HEADER = True @@ -405,6 +411,8 @@ class Staging(Config): LETTERS_PDF_BUCKET_NAME = 'staging-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'staging-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'staging-notify.works-ftp' + LETTERS_PDF_BUCKET_NAME = 'staging-letters-pdf' + LETTERS_SCAN_BUCKET_NAME = 'staging-letters-scan' STATSD_ENABLED = True FROM_NUMBER = 'stage' API_RATE_LIMIT_ENABLED = True @@ -419,6 +427,8 @@ class Live(Config): LETTERS_PDF_BUCKET_NAME = 'production-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'production-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notifications.service.gov.uk-ftp' + LETTERS_PDF_BUCKET_NAME = 'production-letters-pdf' + LETTERS_SCAN_BUCKET_NAME = 'production-letters-scan' STATSD_ENABLED = True FROM_NUMBER = 'GOVUK' FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = '6c1d81bb-dae2-4ee9-80b0-89a4aae9f649' @@ -440,6 +450,8 @@ class Sandbox(CloudFoundryConfig): LETTERS_PDF_BUCKET_NAME = 'cf-sandbox-letters-pdf' TEST_LETTERS_BUCKET_NAME = 'cf-sandbox-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' + LETTERS_PDF_BUCKET_NAME = 'cf-sandbox-letters-pdf' + LETTERS_SCAN_BUCKET_NAME = 'cf-sandbox-virus-check-pdf' FROM_NUMBER = 'sandbox' REDIS_ENABLED = False From 30e371fa4cfda1f2ce61daa2d2f8feaa1daca640 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 13 Mar 2018 14:08:01 +0000 Subject: [PATCH 02/12] Set precompiled letters to pending virus check initially --- app/v2/notifications/post_notifications.py | 14 +++++++++----- .../v2/notifications/test_post_notifications.py | 7 ++++++- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index b1fd958e6..42e7180c0 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -25,7 +25,8 @@ from app.models import ( KEY_TYPE_TEAM, NOTIFICATION_CREATED, NOTIFICATION_SENDING, - NOTIFICATION_DELIVERED + NOTIFICATION_DELIVERED, + NOTIFICATION_PENDING_VIRUS_CHECK, ) from app.celery.letters_pdf_tasks import create_letters_pdf from app.celery.research_mode_tasks import create_fake_letter_response_file @@ -220,8 +221,15 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text if not api_key.service.research_mode and api_key.service.restricted and api_key.key_type != KEY_TYPE_TEST: raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403) + should_send = not (api_key.service.research_mode or api_key.key_type == KEY_TYPE_TEST) + + # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up + status = NOTIFICATION_CREATED if should_send else NOTIFICATION_SENDING + if precompiled: try: + if should_send: + status = NOTIFICATION_PENDING_VIRUS_CHECK letter_content = base64.b64decode(letter_data['content']) pages = pdf_page_count(io.BytesIO(letter_content)) except ValueError: @@ -230,10 +238,6 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text current_app.logger.exception(msg='Invalid PDF received') raise BadRequestError(message='Letter content is not a valid PDF', status_code=400) - should_send = not (api_key.service.research_mode or api_key.key_type == KEY_TYPE_TEST) - - # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up - status = NOTIFICATION_CREATED if should_send else NOTIFICATION_SENDING notification = create_letter_notification(letter_data=letter_data, template=template, api_key=api_key, diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index e21b4ca68..eadb5dd88 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -7,8 +7,10 @@ from freezegun import freeze_time from app.dao.service_sms_sender_dao import dao_update_service_sms_sender from app.models import ( ScheduledNotification, - SCHEDULE_NOTIFICATIONS, EMAIL_TYPE, + NOTIFICATION_CREATED, + NOTIFICATION_PENDING_VIRUS_CHECK, + SCHEDULE_NOTIFICATIONS, SMS_TYPE ) from flask import json, current_app @@ -55,6 +57,7 @@ def test_post_sms_notification_returns_201(client, sample_template_with_placehol assert validate(resp_json, post_sms_response) == resp_json notifications = Notification.query.all() assert len(notifications) == 1 + assert notifications[0].status == NOTIFICATION_CREATED notification_id = notifications[0].id assert resp_json['id'] == str(notification_id) assert resp_json['reference'] == reference @@ -306,6 +309,7 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_ 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 resp_json['id'] == str(notification.id) assert resp_json['reference'] == reference assert notification.reference is None @@ -756,6 +760,7 @@ def test_post_precompiled_letter_notification_returns_201(client, notify_user, m notification = Notification.query.first() assert notification.billable_units == 3 + assert notification.status == NOTIFICATION_PENDING_VIRUS_CHECK resp_json = json.loads(response.get_data(as_text=True)) From 8733d84e752c415d24fff6858c9e1f767d0b5fa6 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 13 Mar 2018 14:08:34 +0000 Subject: [PATCH 03/12] Upload precompiled letter pdfs to letters-scan bucket --- app/config.py | 2 +- app/letters/utils.py | 5 ++++ tests/app/letters/test_letter_utils.py | 37 ++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 981ed9ed2..04ae40492 100644 --- a/app/config.py +++ b/app/config.py @@ -451,7 +451,7 @@ class Sandbox(CloudFoundryConfig): TEST_LETTERS_BUCKET_NAME = 'cf-sandbox-test-letters' DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp' LETTERS_PDF_BUCKET_NAME = 'cf-sandbox-letters-pdf' - LETTERS_SCAN_BUCKET_NAME = 'cf-sandbox-virus-check-pdf' + LETTERS_SCAN_BUCKET_NAME = 'cf-sandbox-letters-scan' FROM_NUMBER = 'sandbox' REDIS_ENABLED = False diff --git a/app/letters/utils.py b/app/letters/utils.py index 7b6205c68..34f270b9b 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -64,6 +64,11 @@ def upload_letter_pdf(notification, pdf_data, is_test_letter=False): else: bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + if notification.template.is_precompiled_letter: + bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + else: + bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + s3upload( filedata=pdf_data, region=current_app.config['AWS_REGION'], diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index 04cb8eea9..241e5a27d 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -1,3 +1,4 @@ +from flask import current_app import pytest from datetime import datetime @@ -21,6 +22,13 @@ def sample_precompiled_letter_notification_using_test_key(sample_letter_notifica sample_letter_notification.reference = 'foo' sample_letter_notification.created_at = datetime.utcnow() return sample_letter_notification +from app.letters.utils import ( + get_bucket_prefix_for_notification, + get_letter_pdf_filename, + upload_letter_pdf +) +from app.models import PRECOMPILED_TEMPLATE_NAME +from app.variables import Retention def test_get_bucket_prefix_for_notification_valid_notification(sample_notification): @@ -100,3 +108,32 @@ def test_get_letter_pdf_gets_pdf_from_correct_bucket( ret = get_letter_pdf(sample_precompiled_letter_notification_using_test_key) assert ret == b'pdf_content' + + +@pytest.mark.parametrize('is_precompiled_letter,bucket_config_name', [ + (False, 'LETTERS_PDF_BUCKET_NAME'), + (True, 'LETTERS_SCAN_BUCKET_NAME') +]) +def test_upload_letter_pdf_to_correct_bucket( + sample_letter_notification, mocker, is_precompiled_letter, bucket_config_name +): + if is_precompiled_letter: + sample_letter_notification.template.hidden = True + sample_letter_notification.template.name = PRECOMPILED_TEMPLATE_NAME + + mock_s3 = mocker.patch('app.letters.utils.s3upload') + + filename = get_letter_pdf_filename( + reference=sample_letter_notification.reference, + crown=sample_letter_notification.service.crown + ) + + upload_letter_pdf(sample_letter_notification, b'\x00\x01') + + mock_s3.assert_called_once_with( + bucket_name=current_app.config[bucket_config_name], + file_location=filename, + filedata=b'\x00\x01', + region=current_app.config['AWS_REGION'], + tags={Retention.KEY: Retention.ONE_WEEK} + ) From 8e78c5f286926923a72d919e2502b85d615aca8d Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 13 Mar 2018 14:28:38 +0000 Subject: [PATCH 04/12] Refactor letter test - move letter tests from test_post_notifications.py to test_post_letter_notifications.py --- .../test_post_letter_notifications.py | 123 ++++++++++++++++-- .../notifications/test_post_notifications.py | 2 - 2 files changed, 114 insertions(+), 11 deletions(-) diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index e145f5e88..efda61e7e 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -6,15 +6,20 @@ from flask import url_for import pytest from app.config import QueueNames -from app.models import EMAIL_TYPE -from app.models import Job -from app.models import KEY_TYPE_NORMAL -from app.models import KEY_TYPE_TEAM -from app.models import KEY_TYPE_TEST -from app.models import LETTER_TYPE -from app.models import Notification -from app.models import NOTIFICATION_SENDING, NOTIFICATION_DELIVERED -from app.models import SMS_TYPE +from app.models import ( + Job, + Notification, + EMAIL_TYPE, + KEY_TYPE_NORMAL, + KEY_TYPE_TEAM, + KEY_TYPE_TEST, + LETTER_TYPE, + NOTIFICATION_CREATED, + NOTIFICATION_SENDING, + NOTIFICATION_DELIVERED, + NOTIFICATION_PENDING_VIRUS_CHECK, + SMS_TYPE, +) from app.schema_validation import validate from app.v2.errors import RateLimitError from app.v2.notifications.notification_schemas import post_letter_response @@ -70,6 +75,7 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo assert validate(resp_json, post_letter_response) == resp_json assert Job.query.count() == 0 notification = Notification.query.one() + assert notification.status == NOTIFICATION_CREATED notification_id = notification.id assert resp_json['id'] == str(notification_id) assert resp_json['reference'] == reference @@ -429,3 +435,102 @@ def test_post_letter_notification_persists_notification_reply_to_text( notifications = Notification.query.all() assert len(notifications) == 1 assert notifications[0].reply_to_text == service_address + + +def test_post_precompiled_letter_requires_permission(client, sample_service, notify_user, mocker): + mocker.patch('app.v2.notifications.post_notifications.upload_letter_pdf') + data = { + "reference": "letter-reference", + "content": "bGV0dGVyLWNvbnRlbnQ=" + } + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.post( + path="v2/notifications/letter", + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400, response.get_data(as_text=True) + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['errors'][0]['message'] == 'Cannot send precompiled_letters' + + +def test_post_precompiled_letter_with_invalid_base64(client, notify_user, mocker): + sample_service = create_service(service_permissions=['letter', 'precompiled_letter']) + mocker.patch('app.v2.notifications.post_notifications.upload_letter_pdf') + + data = { + "reference": "letter-reference", + "content": "hi" + } + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.post( + path="v2/notifications/letter", + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400, response.get_data(as_text=True) + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['errors'][0]['message'] == 'Cannot decode letter content (invalid base64 encoding)' + + assert not Notification.query.first() + + +def test_post_precompiled_letter_notification_returns_201(client, notify_user, mocker): + sample_service = create_service(service_permissions=['letter', 'precompiled_letter']) + s3mock = mocker.patch('app.v2.notifications.post_notifications.upload_letter_pdf') + mocker.patch('app.v2.notifications.post_notifications.pdf_page_count', return_value=5) + data = { + "reference": "letter-reference", + "content": "bGV0dGVyLWNvbnRlbnQ=" + } + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.post( + path="v2/notifications/letter", + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201, response.get_data(as_text=True) + + s3mock.assert_called_once_with(ANY, b'letter-content') + + notification = Notification.query.first() + + assert notification.billable_units == 3 + assert notification.status == NOTIFICATION_PENDING_VIRUS_CHECK + + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json == { + 'content': {'body': None, 'subject': 'Pre-compiled PDF'}, + 'id': str(notification.id), + 'reference': 'letter-reference', + 'scheduled_for': None, + 'template': { + 'id': ANY, + 'uri': ANY, + 'version': 1 + }, + 'uri': ANY + } + + +def test_post_precompiled_letter_notification_returns_400_with_invalid_pdf(client, notify_user, mocker): + sample_service = create_service(service_permissions=['letter', 'precompiled_letter']) + s3mock = mocker.patch('app.v2.notifications.post_notifications.upload_letter_pdf') + data = { + "reference": "letter-reference", + "content": "bGV0dGVyLWNvbnRlbnQ=" + } + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.post( + path="v2/notifications/letter", + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + resp_json = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 400, response.get_data(as_text=True) + assert resp_json['errors'][0]['message'] == 'Letter content is not a valid PDF' + + assert s3mock.called is False + + assert Notification.query.count() == 0 diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index eadb5dd88..71c27c992 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -1,5 +1,4 @@ import uuid -from unittest.mock import ANY import pytest from freezegun import freeze_time @@ -9,7 +8,6 @@ from app.models import ( ScheduledNotification, EMAIL_TYPE, NOTIFICATION_CREATED, - NOTIFICATION_PENDING_VIRUS_CHECK, SCHEDULE_NOTIFICATIONS, SMS_TYPE ) From 4ace33cc0479e8090169bdaf08418bf68a92a3d1 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 19 Mar 2018 13:13:38 +0000 Subject: [PATCH 05/12] Add queue and task names to config --- app/config.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/config.py b/app/config.py index 04ae40492..d4ea01560 100644 --- a/app/config.py +++ b/app/config.py @@ -32,6 +32,7 @@ class QueueNames(object): CREATE_LETTERS_PDF = 'create-letters-pdf-tasks' CALLBACKS = 'service-callbacks' LETTERS = 'letter-tasks' + ANTIVIRUS = 'antivirus-tasks' @staticmethod def all_queues(): @@ -56,6 +57,7 @@ class TaskNames(object): DVLA_JOBS = 'send-jobs-to-dvla' PROCESS_INCOMPLETE_JOBS = 'process-incomplete-jobs' ZIP_AND_SEND_LETTER_PDFS = 'zip-and-send-letter-pdfs' + SCAN_FILE = 'scan-file' class Config(object): From 0c102f0727926f6fbf45b0172bcc75023bece5d9 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 19 Mar 2018 13:28:16 +0000 Subject: [PATCH 06/12] Update letter utils for methods to handle virus process - add function to get reference from filename - add function to move pdf from scan folder to process folder - add function to delete pdfs from scan bucket for failed virus scans --- app/letters/utils.py | 55 ++++++++++++++++++++------ requirements_for_test.txt | 2 +- tests/app/letters/test_letter_utils.py | 42 ++++++++++++++------ 3 files changed, 76 insertions(+), 23 deletions(-) diff --git a/app/letters/utils.py b/app/letters/utils.py index 34f270b9b..2a6f2bbd6 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -15,8 +15,8 @@ LETTERS_PDF_FILE_LOCATION_STRUCTURE = \ PRECOMPILED_BUCKET_PREFIX = '{folder}NOTIFY.{reference}' -def get_folder_name(_now, is_test_letter): - if is_test_letter: +def get_folder_name(_now, is_test_or_scan_letter=False): + if is_test_or_scan_letter: folder_name = '' else: print_datetime = _now @@ -26,11 +26,11 @@ def get_folder_name(_now, is_test_letter): return folder_name -def get_letter_pdf_filename(reference, crown, is_test_letter=False): +def get_letter_pdf_filename(reference, crown, is_test_or_scan_letter=False): now = datetime.utcnow() upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=get_folder_name(now, is_test_letter), + folder=get_folder_name(now, is_test_or_scan_letter), reference=reference, duplex="D", letter_class="2", @@ -52,22 +52,28 @@ def get_bucket_prefix_for_notification(notification, is_test_letter=False): return upload_file_name +def get_reference_from_filename(filename): + # filename looks like '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF' + filename_parts = filename.split('.') + return filename_parts[1] + + def upload_letter_pdf(notification, pdf_data, is_test_letter=False): current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format( notification.id, notification.reference, notification.created_at, len(pdf_data))) upload_file_name = get_letter_pdf_filename( - notification.reference, notification.service.crown, is_test_letter) + notification.reference, + notification.service.crown, + is_test_or_scan_letter=is_test_letter or notification.template.is_precompiled_letter) if is_test_letter: bucket_name = current_app.config['TEST_LETTERS_BUCKET_NAME'] else: - bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] - - if notification.template.is_precompiled_letter: - bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] - else: - bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + if notification.template.is_precompiled_letter: + bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + else: + bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] s3upload( filedata=pdf_data, @@ -79,6 +85,33 @@ def upload_letter_pdf(notification, pdf_data, is_test_letter=False): current_app.logger.info("Uploaded letters PDF {} to {} for notification id {}".format( upload_file_name, bucket_name, notification.id)) + return upload_file_name + + +def move_scanned_pdf_to_letters_pdf_bucket(filename): + source_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + target_bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + + s3 = boto3.resource('s3') + copy_source = {'Bucket': source_bucket_name, 'Key': filename} + target_filename = get_folder_name(datetime.utcnow()) + filename + target_bucket = s3.Bucket(target_bucket_name) + obj = target_bucket.Object(target_filename) + obj.copy(copy_source, ExtraArgs={'ServerSideEncryption': 'AES256'}) + + s3.Object(source_bucket_name, filename).delete() + + current_app.logger.info("Moved letter PDF: {}/{} to {}/{}".format( + source_bucket_name, filename, target_bucket_name, target_filename)) + + +def delete_pdf_from_letters_scan_bucket(filename): + bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + + s3 = boto3.resource('s3') + s3.Object(bucket_name, filename).delete() + + current_app.logger.info("Deleted letter PDF: {}/{}".format(bucket_name, filename)) def get_letter_pdf(notification): diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 3a1169708..c06268aec 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,6 +1,6 @@ -r requirements.txt flake8==3.5.0 -moto==1.1.25 +moto==1.2 pytest==3.4.2 pytest-env==0.6.2 pytest-mock==1.7.1 diff --git a/tests/app/letters/test_letter_utils.py b/tests/app/letters/test_letter_utils.py index 241e5a27d..981f97ddf 100644 --- a/tests/app/letters/test_letter_utils.py +++ b/tests/app/letters/test_letter_utils.py @@ -1,4 +1,3 @@ -from flask import current_app import pytest from datetime import datetime @@ -7,8 +6,15 @@ from flask import current_app from freezegun import freeze_time from moto import mock_s3 -from app.letters.utils import get_bucket_prefix_for_notification, get_letter_pdf_filename, get_letter_pdf +from app.letters.utils import ( + get_bucket_prefix_for_notification, + get_letter_pdf_filename, + get_letter_pdf, + upload_letter_pdf, + move_scanned_pdf_to_letters_pdf_bucket +) from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEST, PRECOMPILED_TEMPLATE_NAME +from app.variables import Retention FROZEN_DATE_TIME = "2018-03-14 17:00:00" @@ -22,13 +28,6 @@ def sample_precompiled_letter_notification_using_test_key(sample_letter_notifica sample_letter_notification.reference = 'foo' sample_letter_notification.created_at = datetime.utcnow() return sample_letter_notification -from app.letters.utils import ( - get_bucket_prefix_for_notification, - get_letter_pdf_filename, - upload_letter_pdf -) -from app.models import PRECOMPILED_TEMPLATE_NAME -from app.variables import Retention def test_get_bucket_prefix_for_notification_valid_notification(sample_notification): @@ -72,7 +71,7 @@ def test_get_letter_pdf_filename_returns_correct_filename( @freeze_time("2017-12-04 17:29:00") def test_get_letter_pdf_filename_returns_correct_filename_for_test_letters( notify_api, mocker): - filename = get_letter_pdf_filename(reference='foo', crown='C', is_test_letter=True) + filename = get_letter_pdf_filename(reference='foo', crown='C', is_test_or_scan_letter=True) assert filename == 'NOTIFY.FOO.D.2.C.C.20171204172900.PDF' @@ -125,7 +124,8 @@ def test_upload_letter_pdf_to_correct_bucket( filename = get_letter_pdf_filename( reference=sample_letter_notification.reference, - crown=sample_letter_notification.service.crown + crown=sample_letter_notification.service.crown, + is_test_or_scan_letter=is_precompiled_letter ) upload_letter_pdf(sample_letter_notification, b'\x00\x01') @@ -137,3 +137,23 @@ def test_upload_letter_pdf_to_correct_bucket( region=current_app.config['AWS_REGION'], tags={Retention.KEY: Retention.ONE_WEEK} ) + + +@mock_s3 +@freeze_time(FROZEN_DATE_TIME) +def test_move_scanned_letter_pdf_to_processing_bucket(notify_api): + filename = 'test.pdf' + source_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + target_bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] + + conn = boto3.resource('s3', region_name='eu-west-1') + source_bucket = conn.create_bucket(Bucket=source_bucket_name) + target_bucket = conn.create_bucket(Bucket=target_bucket_name) + + s3 = boto3.client('s3', region_name='eu-west-1') + s3.put_object(Bucket=source_bucket_name, Key=filename, Body=b'pdf_content') + + move_scanned_pdf_to_letters_pdf_bucket(filename) + + assert '2018-03-14/' + filename in [o.key for o in target_bucket.objects.all()] + assert filename not in [o.key for o in source_bucket.objects.all()] From b1ac580e0ab8d8e370cb6265362470b4fa3c2288 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 19 Mar 2018 13:52:01 +0000 Subject: [PATCH 07/12] Add celery tasks to handle virus scan passing and failing --- app/celery/letters_pdf_tasks.py | 57 ++++++++++++++++++++-- tests/app/celery/test_letters_pdf_tasks.py | 33 ++++++++++++- 2 files changed, 83 insertions(+), 7 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 4afd13065..b1c329f04 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -1,12 +1,14 @@ import math +from datetime import datetime +from botocore.exceptions import ClientError as BotoClientError from flask import current_app -from notifications_utils.statsd_decorators import statsd from requests import ( post as requests_post, RequestException ) -from botocore.exceptions import ClientError as BotoClientError + +from notifications_utils.statsd_decorators import statsd from app import notify_celery from app.aws import s3 @@ -16,9 +18,15 @@ from app.dao.notifications_dao import ( update_notification_status_by_id, dao_update_notification, dao_get_notifications_by_references, + dao_update_notifications_by_reference, ) -from app.letters.utils import upload_letter_pdf -from app.models import NOTIFICATION_CREATED +from app.letters.utils import ( + delete_pdf_from_letters_scan_bucket, + get_reference_from_filename, + move_scanned_pdf_to_letters_pdf_bucket, + upload_letter_pdf +) +from app.models import NOTIFICATION_CREATED, NOTIFICATION_PERMANENT_FAILURE @notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300) @@ -133,7 +141,7 @@ def group_letters(letter_pdfs): def letter_in_created_state(filename): # filename looks like '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF' subfolder = filename.split('/')[0] - ref = filename.split('.')[1] + ref = get_reference_from_filename(filename) notifications = dao_get_notifications_by_references([ref]) if notifications: if notifications[0].status == NOTIFICATION_CREATED: @@ -144,3 +152,42 @@ def letter_in_created_state(filename): notifications[0].status )) return False + + +@notify_celery.task(name='process-letter-scan-passed') +def process_letter_scan_passed(filename): + current_app.logger.info('Virus scan passed: {}'.format(filename)) + move_scanned_pdf_to_letters_pdf_bucket(filename) + reference = get_reference_from_filename(filename) + updated_count = update_letter_pdf_status(reference, NOTIFICATION_CREATED) + + if updated_count != 1: + raise Exception( + "There should only be one letter notification for each reference. Found {} notifications".format( + updated_count + ) + ) + + +@notify_celery.task(name='process-letter-scan-failed') +def process_letter_scan_failed(filename): + current_app.logger.info('Virus scan failed: {}'.format(filename)) + delete_pdf_from_letters_scan_bucket(filename) + reference = get_reference_from_filename(filename) + updated_count = update_letter_pdf_status(reference, NOTIFICATION_PERMANENT_FAILURE) + + if updated_count != 1: + raise Exception( + "There should only be one letter notification for each reference. Found {} notifications".format( + updated_count + ) + ) + + +def update_letter_pdf_status(reference, status): + return dao_update_notifications_by_reference( + references=[reference], + update_dict={ + 'status': status, + 'updated_at': datetime.utcnow() + }) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 757be3def..903d0ab73 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -16,10 +16,17 @@ from app.celery.letters_pdf_tasks import ( get_letters_pdf, collate_letter_pdfs_for_day, group_letters, - letter_in_created_state + letter_in_created_state, + process_letter_scan_passed, + process_letter_scan_failed, ) from app.letters.utils import get_letter_pdf_filename -from app.models import Notification, NOTIFICATION_SENDING +from app.models import ( + Notification, + NOTIFICATION_CREATED, + NOTIFICATION_PERMANENT_FAILURE, + NOTIFICATION_SENDING +) from tests.conftest import set_config_values @@ -308,3 +315,25 @@ def test_letter_in_created_state_fails_if_notification_doesnt_exist(sample_notif sample_notification.reference = 'QWERTY1234567890' filename = '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF' assert letter_in_created_state(filename) is False + + +def test_process_letter_task_check_virus_scan_passed(sample_letter_notification, mocker): + filename = 'NOTIFY.{}'.format(sample_letter_notification.reference) + sample_letter_notification.status = 'pending-virus-check' + mock_move_pdf = mocker.patch('app.celery.letters_pdf_tasks.move_scanned_pdf_to_letters_pdf_bucket') + + process_letter_scan_passed(filename) + + mock_move_pdf.assert_called_once_with(filename) + assert sample_letter_notification.status == NOTIFICATION_CREATED + + +def test_process_letter_task_check_virus_scan_failed(sample_letter_notification, mocker): + filename = 'NOTIFY.{}'.format(sample_letter_notification.reference) + sample_letter_notification.status = 'pending-virus-check' + mock_delete_pdf = mocker.patch('app.celery.letters_pdf_tasks.delete_pdf_from_letters_scan_bucket') + + process_letter_scan_failed(filename) + + mock_delete_pdf.assert_called_once_with(filename) + assert sample_letter_notification.status == NOTIFICATION_PERMANENT_FAILURE From 65733a30a1b20d2f164ae800faede05932a51ae9 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 19 Mar 2018 16:25:13 +0000 Subject: [PATCH 08/12] Add send_task to antivirus app for scanning precompiled letters --- app/v2/notifications/post_notifications.py | 18 ++++++++++++++---- .../test_post_letter_notifications.py | 1 + 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 42e7180c0..5a3bf6e41 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -4,12 +4,11 @@ import io import math 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 -from app.config import QueueNames +from app import api_user, authenticated_service, notify_celery +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 from app.dao.users_dao import get_user_by_id @@ -246,10 +245,21 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text if should_send: if precompiled: - upload_letter_pdf(notification, letter_content) + filename = upload_letter_pdf(notification, letter_content) pages_per_sheet = 2 notification.billable_units = math.ceil(pages / pages_per_sheet) dao_update_notification(notification) + + current_app.logger.info( + 'Calling task scan-file for {}'.format(filename) + ) + + # call task to add the filename to anti virus queue + notify_celery.send_task( + name=TaskNames.SCAN_FILE, + kwargs={'filename': filename}, + queue=QueueNames.ANTIVIRUS, + ) else: create_letters_pdf.apply_async( [str(notification.id)], diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index efda61e7e..00aef4770 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -479,6 +479,7 @@ def test_post_precompiled_letter_notification_returns_201(client, notify_user, m sample_service = create_service(service_permissions=['letter', 'precompiled_letter']) s3mock = mocker.patch('app.v2.notifications.post_notifications.upload_letter_pdf') mocker.patch('app.v2.notifications.post_notifications.pdf_page_count', return_value=5) + mocker.patch("app.letters.rest.notify_celery.send_task") data = { "reference": "letter-reference", "content": "bGV0dGVyLWNvbnRlbnQ=" From d9494dbc97e4392496f223497b776dabcdc14afb Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 20 Mar 2018 14:56:26 +0000 Subject: [PATCH 09/12] Refactor tests after rebase --- .../test_post_letter_notifications.py | 13 +-- .../notifications/test_post_notifications.py | 92 ------------------- 2 files changed, 1 insertion(+), 104 deletions(-) diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 00aef4770..c73094c68 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -500,18 +500,7 @@ def test_post_precompiled_letter_notification_returns_201(client, notify_user, m assert notification.status == NOTIFICATION_PENDING_VIRUS_CHECK resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json == { - 'content': {'body': None, 'subject': 'Pre-compiled PDF'}, - 'id': str(notification.id), - 'reference': 'letter-reference', - 'scheduled_for': None, - 'template': { - 'id': ANY, - 'uri': ANY, - 'version': 1 - }, - 'uri': ANY - } + assert resp_json == {'id': str(notification.id), 'reference': 'letter-reference'} def test_post_precompiled_letter_notification_returns_400_with_invalid_pdf(client, notify_user, mocker): diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 71c27c992..9c2927e76 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -697,95 +697,3 @@ 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_precompiled_letter_requires_permission(client, sample_service, notify_user, mocker): - mocker.patch('app.v2.notifications.post_notifications.upload_letter_pdf') - data = { - "reference": "letter-reference", - "content": "bGV0dGVyLWNvbnRlbnQ=" - } - auth_header = create_authorization_header(service_id=sample_service.id) - response = client.post( - path="v2/notifications/letter", - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - assert response.status_code == 400, response.get_data(as_text=True) - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['errors'][0]['message'] == 'Cannot send precompiled_letters' - - -def test_post_precompiled_letter_with_invalid_base64(client, notify_user, mocker): - sample_service = create_service(service_permissions=['letter', 'precompiled_letter']) - mocker.patch('app.v2.notifications.post_notifications.upload_letter_pdf') - - data = { - "reference": "letter-reference", - "content": "hi" - } - auth_header = create_authorization_header(service_id=sample_service.id) - response = client.post( - path="v2/notifications/letter", - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - assert response.status_code == 400, response.get_data(as_text=True) - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['errors'][0]['message'] == 'Cannot decode letter content (invalid base64 encoding)' - - assert not Notification.query.first() - - -def test_post_precompiled_letter_notification_returns_201(client, notify_user, mocker): - sample_service = create_service(service_permissions=['letter', 'precompiled_letter']) - s3mock = mocker.patch('app.v2.notifications.post_notifications.upload_letter_pdf') - mocker.patch('app.v2.notifications.post_notifications.pdf_page_count', return_value=5) - data = { - "reference": "letter-reference", - "content": "bGV0dGVyLWNvbnRlbnQ=" - } - auth_header = create_authorization_header(service_id=sample_service.id) - response = client.post( - path="v2/notifications/letter", - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - assert response.status_code == 201, response.get_data(as_text=True) - - s3mock.assert_called_once_with(ANY, b'letter-content') - - notification = Notification.query.first() - - assert notification.billable_units == 3 - assert notification.status == NOTIFICATION_PENDING_VIRUS_CHECK - - resp_json = json.loads(response.get_data(as_text=True)) - - assert resp_json == { - 'id': str(notification.id), - 'reference': 'letter-reference' - } - - -def test_post_precompiled_letter_notification_returns_400_with_invalid_pdf(client, notify_user, mocker): - sample_service = create_service(service_permissions=['letter', 'precompiled_letter']) - s3mock = mocker.patch('app.v2.notifications.post_notifications.upload_letter_pdf') - data = { - "reference": "letter-reference", - "content": "bGV0dGVyLWNvbnRlbnQ=" - } - auth_header = create_authorization_header(service_id=sample_service.id) - response = client.post( - path="v2/notifications/letter", - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - resp_json = json.loads(response.get_data(as_text=True)) - - assert response.status_code == 400, response.get_data(as_text=True) - assert resp_json['errors'][0]['message'] == 'Letter content is not a valid PDF' - - assert s3mock.called is False - - assert Notification.query.count() == 0 From 83913531131d05a174148a2f98c4ff8bc4a1a07f Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 20 Mar 2018 14:56:42 +0000 Subject: [PATCH 10/12] Update tasks in line with AV app --- app/celery/letters_pdf_tasks.py | 8 ++++---- tests/app/celery/test_letters_pdf_tasks.py | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index b1c329f04..7f2877284 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -154,8 +154,8 @@ def letter_in_created_state(filename): return False -@notify_celery.task(name='process-letter-scan-passed') -def process_letter_scan_passed(filename): +@notify_celery.task(name='process-virus-scan-passed') +def process_virus_scan_passed(filename): current_app.logger.info('Virus scan passed: {}'.format(filename)) move_scanned_pdf_to_letters_pdf_bucket(filename) reference = get_reference_from_filename(filename) @@ -169,8 +169,8 @@ def process_letter_scan_passed(filename): ) -@notify_celery.task(name='process-letter-scan-failed') -def process_letter_scan_failed(filename): +@notify_celery.task(name='process-virus-scan-failed') +def process_virus_scan_failed(filename): current_app.logger.info('Virus scan failed: {}'.format(filename)) delete_pdf_from_letters_scan_bucket(filename) reference = get_reference_from_filename(filename) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 903d0ab73..b13eeac8f 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -17,8 +17,8 @@ from app.celery.letters_pdf_tasks import ( collate_letter_pdfs_for_day, group_letters, letter_in_created_state, - process_letter_scan_passed, - process_letter_scan_failed, + process_virus_scan_passed, + process_virus_scan_failed, ) from app.letters.utils import get_letter_pdf_filename from app.models import ( @@ -322,7 +322,7 @@ def test_process_letter_task_check_virus_scan_passed(sample_letter_notification, sample_letter_notification.status = 'pending-virus-check' mock_move_pdf = mocker.patch('app.celery.letters_pdf_tasks.move_scanned_pdf_to_letters_pdf_bucket') - process_letter_scan_passed(filename) + process_virus_scan_passed(filename) mock_move_pdf.assert_called_once_with(filename) assert sample_letter_notification.status == NOTIFICATION_CREATED @@ -333,7 +333,7 @@ def test_process_letter_task_check_virus_scan_failed(sample_letter_notification, sample_letter_notification.status = 'pending-virus-check' mock_delete_pdf = mocker.patch('app.celery.letters_pdf_tasks.delete_pdf_from_letters_scan_bucket') - process_letter_scan_failed(filename) + process_virus_scan_failed(filename) mock_delete_pdf.assert_called_once_with(filename) assert sample_letter_notification.status == NOTIFICATION_PERMANENT_FAILURE From 5481d743ac0a3704c6a9a7ff06d945bdd1933fba Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 21 Mar 2018 15:30:58 +0000 Subject: [PATCH 11/12] change failed virus scan log to error --- app/celery/letters_pdf_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 7f2877284..2d13019de 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -171,7 +171,7 @@ def process_virus_scan_passed(filename): @notify_celery.task(name='process-virus-scan-failed') def process_virus_scan_failed(filename): - current_app.logger.info('Virus scan failed: {}'.format(filename)) + current_app.logger.error('Virus scan failed: {}'.format(filename)) delete_pdf_from_letters_scan_bucket(filename) reference = get_reference_from_filename(filename) updated_count = update_letter_pdf_status(reference, NOTIFICATION_PERMANENT_FAILURE) From d6f6669610628e5ad22302e63d76a3d797275934 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 21 Mar 2018 16:29:48 +0000 Subject: [PATCH 12/12] Add comments for copying s3 objects --- app/letters/utils.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/letters/utils.py b/app/letters/utils.py index 2a6f2bbd6..83af29b86 100644 --- a/app/letters/utils.py +++ b/app/letters/utils.py @@ -97,6 +97,10 @@ def move_scanned_pdf_to_letters_pdf_bucket(filename): target_filename = get_folder_name(datetime.utcnow()) + filename target_bucket = s3.Bucket(target_bucket_name) obj = target_bucket.Object(target_filename) + + # Tags are copied across but the expiration time is reset in the destination bucket + # e.g. if a file has 5 days left to expire on a ONE_WEEK retention in the source bucket, + # in the destination bucket the expiration time will be reset to 7 days left to expire obj.copy(copy_source, ExtraArgs={'ServerSideEncryption': 'AES256'}) s3.Object(source_bucket_name, filename).delete()