From 1703ae6031481e17988aef102f08d72d935a54ee Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 23 Jan 2020 17:32:59 +0000 Subject: [PATCH 1/4] Bump utils to version 36.5.0 This version of utils has the Encryption module included in it. --- requirements-app.txt | 2 +- requirements.txt | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/requirements-app.txt b/requirements-app.txt index d1368dce5..5bc6ec5e0 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -29,6 +29,6 @@ awscli-cwlogs>=1.4,<1.5 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default itsdangerous==0.24 # pyup: <1.0.0 -git+https://github.com/alphagov/notifications-utils.git@36.3.3#egg=notifications-utils==36.3.3 +git+https://github.com/alphagov/notifications-utils.git@36.5.0#egg=notifications-utils==36.5.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/requirements.txt b/requirements.txt index f6d903e30..6a588ba67 100644 --- a/requirements.txt +++ b/requirements.txt @@ -31,21 +31,21 @@ awscli-cwlogs>=1.4,<1.5 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default itsdangerous==0.24 # pyup: <1.0.0 -git+https://github.com/alphagov/notifications-utils.git@36.3.3#egg=notifications-utils==36.3.3 +git+https://github.com/alphagov/notifications-utils.git@36.5.0#egg=notifications-utils==36.5.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 ## The following requirements were added by pip freeze: -alembic==1.3.2 +alembic==1.3.3 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.16.310 +awscli==1.17.8 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.0 boto3==1.10.38 -botocore==1.13.46 +botocore==1.14.8 certifi==2019.11.28 chardet==3.0.4 Click==7.0 @@ -56,21 +56,21 @@ flask-redis==0.4.0 future==0.18.2 greenlet==0.4.15 idna==2.8 -importlib-metadata==1.3.0 +importlib-metadata==1.4.0 Jinja2==2.10.3 jmespath==0.9.4 kombu==3.0.37 -Mako==1.1.0 +Mako==1.1.1 MarkupSafe==1.1.1 mistune==0.8.4 monotonic==1.5 -more-itertools==8.0.2 +more-itertools==8.1.0 orderedset==2.0.1 phonenumbers==8.11.1 pyasn1==0.4.8 pycparser==2.19 PyPDF2==1.26.0 -pyrsistent==0.15.6 +pyrsistent==0.15.7 python-dateutil==2.8.1 python-editor==1.0.4 python-json-logger==0.1.11 @@ -79,11 +79,11 @@ PyYAML==5.2 redis==3.3.11 requests==2.22.0 rsa==3.4.2 -s3transfer==0.2.1 -six==1.13.0 +s3transfer==0.3.2 +six==1.14.0 smartypants==2.0.1 statsd==3.3.0 -urllib3==1.25.7 +urllib3==1.25.8 webencodings==0.5.1 Werkzeug==0.16.0 -zipp==0.6.0 +zipp==2.0.1 From 64c2061baad10300df75c7e3d7887d115e086b18 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 23 Jan 2020 17:36:32 +0000 Subject: [PATCH 2/4] Use encryption module from utils Now that the encryption module has been moved from this app to utils, we can remove it from here (along with its tests) and import it from utils instead. This also renames the `encryption.py` file to `hashing.py`, since it no longer contains the encryption class. --- app/__init__.py | 2 +- app/encryption.py | 24 ------------------- app/hashing.py | 10 ++++++++ app/models.py | 2 +- .../versions/0025_notify_service_data.py | 2 +- tests/app/test_encryption.py | 20 ---------------- 6 files changed, 13 insertions(+), 47 deletions(-) delete mode 100644 app/encryption.py create mode 100644 app/hashing.py delete mode 100644 tests/app/test_encryption.py diff --git a/app/__init__.py b/app/__init__.py index 71ff052cb..aa106d4b3 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -11,6 +11,7 @@ from time import monotonic from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient from notifications_utils.clients.statsd.statsd_client import StatsdClient from notifications_utils.clients.redis.redis_client import RedisClient +from notifications_utils.clients.encryption.encryption_client import Encryption from notifications_utils import logging, request_helper from werkzeug.exceptions import HTTPException as WerkzeugHTTPException from werkzeug.local import LocalProxy @@ -22,7 +23,6 @@ from app.clients.email.aws_ses import AwsSesClient from app.clients.sms.firetext import FiretextClient from app.clients.sms.mmg import MMGClient from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient -from app.encryption import Encryption DATETIME_FORMAT_NO_TIMEZONE = "%Y-%m-%d %H:%M:%S.%f" DATETIME_FORMAT = "%Y-%m-%dT%H:%M:%S.%fZ" diff --git a/app/encryption.py b/app/encryption.py deleted file mode 100644 index 34ba6df8e..000000000 --- a/app/encryption.py +++ /dev/null @@ -1,24 +0,0 @@ -from flask_bcrypt import generate_password_hash, check_password_hash - -from itsdangerous import URLSafeSerializer - - -class Encryption: - def init_app(self, app): - self.serializer = URLSafeSerializer(app.config.get('SECRET_KEY')) - self.salt = app.config.get('DANGEROUS_SALT') - - def encrypt(self, thing_to_encrypt): - return self.serializer.dumps(thing_to_encrypt, salt=self.salt) - - def decrypt(self, thing_to_decrypt): - return self.serializer.loads(thing_to_decrypt, salt=self.salt) - - -def hashpw(password): - return generate_password_hash(password.encode('UTF-8'), 10).decode('utf-8') - - -def check_hash(password, hashed_password): - # If salt is invalid throws a 500 should add try/catch here - return check_password_hash(hashed_password, password) diff --git a/app/hashing.py b/app/hashing.py new file mode 100644 index 000000000..b8721964d --- /dev/null +++ b/app/hashing.py @@ -0,0 +1,10 @@ +from flask_bcrypt import generate_password_hash, check_password_hash + + +def hashpw(password): + return generate_password_hash(password.encode('UTF-8'), 10).decode('utf-8') + + +def check_hash(password, hashed_password): + # If salt is invalid throws a 500 should add try/catch here + return check_password_hash(hashed_password, password) diff --git a/app/models.py b/app/models.py index 7ab5c5e89..569cf0294 100644 --- a/app/models.py +++ b/app/models.py @@ -29,7 +29,7 @@ from notifications_utils.template import ( ) from notifications_utils.timezones import convert_bst_to_utc, convert_utc_to_bst -from app.encryption import ( +from app.hashing import ( hashpw, check_hash ) diff --git a/migrations/versions/0025_notify_service_data.py b/migrations/versions/0025_notify_service_data.py index 763cc0301..54abecdd8 100644 --- a/migrations/versions/0025_notify_service_data.py +++ b/migrations/versions/0025_notify_service_data.py @@ -11,7 +11,7 @@ from datetime import datetime from alembic import op -from app.encryption import hashpw +from app.hashing import hashpw import uuid revision = '0025_notify_service_data' down_revision = '0024_add_research_mode_defaults' diff --git a/tests/app/test_encryption.py b/tests/app/test_encryption.py deleted file mode 100644 index a985e04ea..000000000 --- a/tests/app/test_encryption.py +++ /dev/null @@ -1,20 +0,0 @@ -from app.encryption import Encryption - -encryption = Encryption() - - -def test_should_encrypt_content(notify_api): - encryption.init_app(notify_api) - assert encryption.encrypt("this") != "this" - - -def test_should_decrypt_content(notify_api): - encryption.init_app(notify_api) - encrypted = encryption.encrypt("this") - assert encryption.decrypt(encrypted) == "this" - - -def test_should_encrypt_json(notify_api): - encryption.init_app(notify_api) - encrypted = encryption.encrypt({"this": "that"}) - assert encryption.decrypt(encrypted) == {"this": "that"} From adf9906a963016695299de5bb5a1bd191068a915 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 24 Jan 2020 09:08:27 +0000 Subject: [PATCH 3/4] Change process_sanitised_letter to take a single encrypted arg Template preview will now send an encrypted dict containing all the args to the `process_sanitised_letter` task, so this updates the task to handle data in the new format. --- app/celery/letters_pdf_tasks.py | 26 ++++---- tests/app/celery/test_letters_pdf_tasks.py | 70 ++++++++++++---------- 2 files changed, 50 insertions(+), 46 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 3ee3822a0..6a0a72e7f 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -15,7 +15,7 @@ from celery.exceptions import MaxRetriesExceededError from notifications_utils.statsd_decorators import statsd from notifications_utils.s3 import s3upload -from app import notify_celery +from app import encryption, notify_celery from app.aws import s3 from app.config import QueueNames, TaskNames from app.dao.notifications_dao import ( @@ -302,14 +302,12 @@ def sanitise_letter(self, filename): @notify_celery.task(name='process-sanitised-letter') -def process_sanitised_letter( - page_count, - message, - invalid_pages, - validation_status, - filename, - notification_id, -): +def process_sanitised_letter(sanitise_data): + letter_details = encryption.decrypt(sanitise_data) + + filename = letter_details['filename'] + notification_id = letter_details['notification_id'] + current_app.logger.info('Processing sanitised letter with id {}'.format(notification_id)) notification = get_notification_by_id(notification_id, _raise=True) @@ -323,7 +321,7 @@ def process_sanitised_letter( try: original_pdf_object = s3.get_s3_object(current_app.config['LETTERS_SCAN_BUCKET_NAME'], filename) - if validation_status == 'failed': + if letter_details['validation_status'] == 'failed': current_app.logger.info('Processing invalid precompiled pdf with id {} (file {})'.format( notification_id, filename)) @@ -331,16 +329,16 @@ def process_sanitised_letter( notification=notification, filename=filename, scan_pdf_object=original_pdf_object, - message=message, - invalid_pages=invalid_pages, - page_count=page_count, + message=letter_details['message'], + invalid_pages=letter_details['invalid_pages'], + page_count=letter_details['page_count'], ) return current_app.logger.info('Processing valid precompiled pdf with id {} (file {})'.format( notification_id, filename)) - billable_units = get_billable_units_for_letter_page_count(page_count) + billable_units = get_billable_units_for_letter_page_count(letter_details['page_count']) is_test_key = notification.key_type == KEY_TYPE_TEST move_sanitised_letter_to_test_or_live_pdf_bucket(filename, is_test_key, notification.created_at) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 4e6ade009..bc280dce7 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -13,6 +13,7 @@ from celery.exceptions import MaxRetriesExceededError, Retry from requests import RequestException from sqlalchemy.orm.exc import NoResultFound +from app import encryption from app.errors import VirusScanError from app.exceptions import NotificationTechnicalFailureException from app.celery.letters_pdf_tasks import ( @@ -773,14 +774,15 @@ def test_process_sanitised_letter_with_valid_letter( sample_letter_notification.billable_units = 1 sample_letter_notification.created_at = datetime(2018, 7, 1, 12) - process_sanitised_letter( - page_count=2, - message=None, - invalid_pages=None, - validation_status='passed', - filename=filename, - notification_id=str(sample_letter_notification.id) - ) + encrypted_data = encryption.encrypt({ + 'page_count': 2, + 'message': None, + 'invalid_pages': None, + 'validation_status': 'passed', + 'filename': filename, + 'notification_id': str(sample_letter_notification.id) + }) + process_sanitised_letter(encrypted_data) assert sample_letter_notification.status == expected_status assert sample_letter_notification.billable_units == 1 @@ -815,14 +817,15 @@ def test_process_sanitised_letter_with_invalid_letter(sample_letter_notification sample_letter_notification.billable_units = 1 sample_letter_notification.created_at = datetime(2018, 7, 1, 12) - process_sanitised_letter( - page_count=2, - message='content-outside-printable-area', - invalid_pages=[1], - validation_status='failed', - filename=filename, - notification_id=str(sample_letter_notification.id) - ) + encrypted_data = encryption.encrypt({ + 'page_count': 2, + 'message': 'content-outside-printable-area', + 'invalid_pages': [1], + 'validation_status': 'failed', + 'filename': filename, + 'notification_id': str(sample_letter_notification.id) + }) + process_sanitised_letter(encrypted_data) assert sample_letter_notification.status == NOTIFICATION_VALIDATION_FAILED assert sample_letter_notification.billable_units == 0 @@ -842,14 +845,15 @@ def test_process_sanitised_letter_when_letter_status_is_not_pending_virus_scan( mock_s3 = mocker.patch('app.celery.letters_pdf_tasks.s3') sample_letter_notification.status = NOTIFICATION_CREATED - process_sanitised_letter( - page_count=2, - message=None, - invalid_pages=None, - validation_status='passed', - filename='NOTIFY.{}'.format(sample_letter_notification.reference), - notification_id=str(sample_letter_notification.id) - ) + encrypted_data = encryption.encrypt({ + 'page_count': 2, + 'message': None, + 'invalid_pages': None, + 'validation_status': 'passed', + 'filename': 'NOTIFY.{}'.format(sample_letter_notification.reference), + 'notification_id': str(sample_letter_notification.id) + }) + process_sanitised_letter(encrypted_data) assert not mock_s3.called @@ -861,15 +865,17 @@ def test_process_sanitised_letter_puts_letter_into_tech_failure_for_boto_errors( mocker.patch('app.celery.letters_pdf_tasks.s3.get_s3_object', side_effect=ClientError({}, 'operation_name')) sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK + encrypted_data = encryption.encrypt({ + 'page_count': 2, + 'message': None, + 'invalid_pages': None, + 'validation_status': 'passed', + 'filename': 'NOTIFY.{}'.format(sample_letter_notification.reference), + 'notification_id': str(sample_letter_notification.id) + }) + with pytest.raises(NotificationTechnicalFailureException): - process_sanitised_letter( - page_count=2, - message=None, - invalid_pages=None, - validation_status='passed', - filename='NOTIFY.{}'.format(sample_letter_notification.reference), - notification_id=str(sample_letter_notification.id) - ) + process_sanitised_letter(encrypted_data) assert sample_letter_notification.status == NOTIFICATION_TECHNICAL_FAILURE From 35e39bcfa882161571e2d6cc8d6ea15cc6cefd93 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 24 Jan 2020 10:15:19 +0000 Subject: [PATCH 4/4] Save recipient address in process_sanitised_letter task If the letter passed sanitisation, the recipient address will be returned from template preview, so we want to save this as the `to` field of the notification. --- app/celery/letters_pdf_tasks.py | 3 ++- tests/app/celery/test_letters_pdf_tasks.py | 13 +++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 6a0a72e7f..6f31d2f91 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -347,7 +347,8 @@ def process_sanitised_letter(sanitise_data): update_letter_pdf_status( reference=notification.reference, status=NOTIFICATION_DELIVERED if is_test_key else NOTIFICATION_CREATED, - billable_units=billable_units + billable_units=billable_units, + recipient_address=letter_details['address'] ) except BotoClientError: diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index bc280dce7..58568d52e 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -780,12 +780,14 @@ def test_process_sanitised_letter_with_valid_letter( 'invalid_pages': None, 'validation_status': 'passed', 'filename': filename, - 'notification_id': str(sample_letter_notification.id) + 'notification_id': str(sample_letter_notification.id), + 'address': 'A. User\nThe house on the corner' }) process_sanitised_letter(encrypted_data) assert sample_letter_notification.status == expected_status assert sample_letter_notification.billable_units == 1 + assert sample_letter_notification.to == 'A. User\nThe house on the corner' assert not [x for x in scan_bucket.objects.all()] assert not [x for x in template_preview_bucket.objects.all()] @@ -823,7 +825,8 @@ def test_process_sanitised_letter_with_invalid_letter(sample_letter_notification 'invalid_pages': [1], 'validation_status': 'failed', 'filename': filename, - 'notification_id': str(sample_letter_notification.id) + 'notification_id': str(sample_letter_notification.id), + 'address': None, }) process_sanitised_letter(encrypted_data) @@ -851,7 +854,8 @@ def test_process_sanitised_letter_when_letter_status_is_not_pending_virus_scan( 'invalid_pages': None, 'validation_status': 'passed', 'filename': 'NOTIFY.{}'.format(sample_letter_notification.reference), - 'notification_id': str(sample_letter_notification.id) + 'notification_id': str(sample_letter_notification.id), + 'address': None }) process_sanitised_letter(encrypted_data) @@ -871,7 +875,8 @@ def test_process_sanitised_letter_puts_letter_into_tech_failure_for_boto_errors( 'invalid_pages': None, 'validation_status': 'passed', 'filename': 'NOTIFY.{}'.format(sample_letter_notification.reference), - 'notification_id': str(sample_letter_notification.id) + 'notification_id': str(sample_letter_notification.id), + 'address': None }) with pytest.raises(NotificationTechnicalFailureException):