From 5ad6bc76210ab690cc302b9d503924bc58b482e6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 16 Oct 2017 17:29:45 +0100 Subject: [PATCH] ensure emails are formatted before sending we allow some invalid to addresses - for example, phone numbers with spaces or brackets - in the database. This is so that users can match up their data in a format that they expect (since they passed it in). When we send SMS, we strip this formatting just before sending - but we weren't with email. This commit changes that and adds some tests. It also adds formatting for reply_to addresses. We should never expect invalid reply_to email addresses in our data, but just in case, lets validate them here. Also, bump requirements.txt to capture some more email validation --- app/delivery/send_to_providers.py | 7 ++- requirements.txt | 2 +- tests/app/db.py | 5 +- tests/app/delivery/test_send_to_providers.py | 58 ++++++++++++++++++++ 4 files changed, 67 insertions(+), 5 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 4dd2892cb..808cd2cdb 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -3,7 +3,8 @@ from datetime import datetime from flask import current_app from notifications_utils.recipients import ( - validate_and_format_phone_number + validate_and_format_phone_number, + validate_and_format_email_address ) from notifications_utils.template import HTMLEmailTemplate, PlainTextEmailTemplate, SMSMessageTemplate @@ -130,11 +131,11 @@ def send_email_to_provider(notification): reference = provider.send_email( from_address, - notification.to, + validate_and_format_email_address(notification.to), plain_text_email.subject, body=str(plain_text_email), html_body=str(html_email), - reply_to_address=email_reply_to, + reply_to_address=validate_and_format_email_address(email_reply_to) if email_reply_to else None, ) notification.reference = reference update_notification(notification, provider) diff --git a/requirements.txt b/requirements.txt index 6395840ba..a356d50e3 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,6 +26,6 @@ notifications-python-client==4.4.0 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@21.3.1#egg=notifications-utils==21.3.1 +git+https://github.com/alphagov/notifications-utils.git@21.4.1#egg=notifications-utils==21.4.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/db.py b/tests/app/db.py index 20eb3b91e..518f0cbdd 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -109,7 +109,7 @@ def create_notification( template, job=None, job_row_number=None, - to_field='+447700900855', + to_field=None, status='created', reference=None, created_at=None, @@ -131,6 +131,9 @@ def create_notification( if created_at is None: created_at = datetime.utcnow() + if to_field is None: + to_field = '+447700900855' if template.template_type == SMS_TYPE else 'test@example.com' + if status != 'created': sent_at = sent_at or datetime.utcnow() updated_at = updated_at or datetime.utcnow() diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 00b75f873..37b5f2ffb 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -781,3 +781,61 @@ def test_send_email_to_provider_get_linked_email_reply_to_create_service_email_a html_body=ANY, reply_to_address=reply_to.email_address ) + + +def test_send_email_to_provider_should_format_reply_to_email_address( + sample_service, + sample_email_template, + mocker): + mocker.patch('app.aws_ses_client.send_email', return_value='reference') + mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') + + db_notification = create_notification(template=sample_email_template) + + reply_to = create_reply_to_email_for_notification( + db_notification.id, + sample_service, + "test@test.com\t" + ) + + send_to_providers.send_email_to_provider( + db_notification, + ) + + app.aws_ses_client.send_email.assert_called_once_with( + ANY, + ANY, + ANY, + body=ANY, + html_body=ANY, + reply_to_address="test@test.com" + ) + + +def test_send_sms_to_provider_should_format_phone_number(sample_notification, mocker): + sample_notification.to = '+44 (7123) 123-123' + send_mock = mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') + + send_to_providers.send_sms_to_provider(sample_notification) + + assert send_mock.call_args[1]['to'] == '447123123123' + + +def test_send_email_to_provider_should_format_email_address(sample_email_notification, mocker): + sample_email_notification.to = 'test@example.com\t' + send_mock = mocker.patch('app.aws_ses_client.send_email', return_value='reference') + mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') + + send_to_providers.send_email_to_provider(sample_email_notification) + + # to_addresses + send_mock.assert_called_once_with( + ANY, + # to_addresses + 'test@example.com', + ANY, + body=ANY, + html_body=ANY, + reply_to_address=ANY, + )