From eef6d80ae2fe216524d83a494766be7b6b6b6804 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 5 Apr 2016 14:51:55 +0100 Subject: [PATCH] Catch sending to restricted recipients in Celery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Celery `send_sms` and `send_email` tasks _could_ assume that all the recipients it gets are safe, because they have been checked either: - when the admin app processes the CSV - in the `/notifications/sms|email` endpoint *However*, it’s probably safer to make the check again when the Celery task run and passes the message off to the third party. This also means that changing a service _back_ to restricted would have an effect on messages that were queued, as well as all subsequent messages. This commit: - restores the test that was removed here: https://github.com/alphagov/notifications-api/commit/e56aee5d1d77142b78434f3faeddfd2cb1a6e9bf#diff-e5627619e387fccc04783c32a23e6859L346 - adds checks back into the Celery tasks for sending email and SMS, using the `allowed_to_send_to` function that was introduced into utils in https://github.com/alphagov/notifications-utils/pull/16 --- app/celery/tasks.py | 45 +++++++++++++++++++++++++++++++--- tests/app/celery/test_tasks.py | 25 +++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d41271e06..260b93b08 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,3 +1,4 @@ +import itertools from datetime import datetime from flask import current_app @@ -12,7 +13,8 @@ from utils.template import Template from utils.recipients import ( RecipientCSV, - validate_and_format_phone_number + validate_and_format_phone_number, + allowed_to_send_to ) from app import ( @@ -188,9 +190,16 @@ def process_job(job_id): def send_sms(service_id, notification_id, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) - client = firetext_client + restricted = False + + if not service_allowed_to_send_to(notification['to'], service): + current_app.logger.info( + "SMS {} failed as restricted service".format(notification_id) + ) + restricted = True + try: sent_at = datetime.utcnow() notification_db_object = Notification( @@ -199,7 +208,7 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): to=notification['to'], service_id=service_id, job_id=notification.get('job', None), - status='sent', + status='failed' if restricted else 'sent', created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, sent_by=client.get_name() @@ -207,6 +216,9 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): dao_create_notification(notification_db_object, TEMPLATE_TYPE_SMS) + if restricted: + return + try: template = Template( dao_get_template_by_id(notification['template']).__dict__, @@ -238,6 +250,15 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): def send_email(service_id, notification_id, subject, from_address, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) client = aws_ses_client + service = dao_fetch_service_by_id(service_id) + + restricted = False + + if not service_allowed_to_send_to(notification['to'], service): + current_app.logger.info( + "Email {} failed as restricted service".format(notification_id) + ) + restricted = True try: sent_at = datetime.utcnow() @@ -247,13 +268,16 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not to=notification['to'], service_id=service_id, job_id=notification.get('job', None), - status='sent', + status='failed' if restricted else 'sent', created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, sent_by=client.get_name() ) dao_create_notification(notification_db_object, TEMPLATE_TYPE_EMAIL) + if restricted: + return + try: template = Template( dao_get_template_by_id(notification['template']).__dict__, @@ -388,3 +412,16 @@ def email_registration_verification(encrypted_verification_message): url=verification_message['url'])) except AwsSesClientException as e: current_app.logger.exception(e) + + +def service_allowed_to_send_to(recipient, service): + + if not service.restricted: + return True + + return allowed_to_send_to( + recipient, + itertools.chain.from_iterable( + [user.mobile_number, user.email_address] for user in service.users + ) + ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 39a755a71..767a4cb5c 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -343,6 +343,31 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif ) +def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): + user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205") + service = sample_service(notify_db, notify_db_session, user=user, restricted=True) + template = sample_template(notify_db, notify_db_session, service=service) + + notification = { + "template": template.id, + "to": "07700 900849" + } + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.firetext_client.send_sms') + mocker.patch('app.firetext_client.get_name', return_value="firetext") + + notification_id = uuid.uuid4() + now = datetime.utcnow() + send_sms( + service.id, + notification_id, + "encrypted-in-reality", + now.strftime(DATETIME_FORMAT) + ) + + firetext_client.send_sms.assert_not_called() + + def test_should_send_email_if_restricted_service_and_valid_email(notify_db, notify_db_session, mocker): user = sample_user(notify_db, notify_db_session, email="test@restricted.com") service = sample_service(notify_db, notify_db_session, user=user, restricted=True)