From 800afc9e4407b42dfff190e245e5a90f3700e97b Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 3 Mar 2016 12:05:18 +0000 Subject: [PATCH] Ensure restricted service are respected by tasks: This is checked on 3rd party API calls, but jobs (CSV files) were able expected to only allow valid files. Change in tack means we want to have restricted notification failures reported in the UI. --- app/celery/tasks.py | 116 ++++++++++++++++++++++----------- app/models.py | 2 +- tests/app/celery/test_tasks.py | 88 +++++++++++++++++++++++++ tests/app/conftest.py | 8 ++- 4 files changed, 171 insertions(+), 43 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 873159e7c..d4388a08c 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -66,16 +66,20 @@ 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) - template = Template( - dao_get_template_by_id(notification['template']).__dict__, - values=notification.get('personalisation', {}), - prefix=service.name, - drop_values={first_column_heading['sms']} - ) client = firetext_client try: + status = 'sent' + can_send = True + + if not allowed_send_to_number(service, notification['to']): + current_app.logger.info( + "SMS {} failed as restricted service".format(notification_id) + ) + status = 'failed' + can_send = False + sent_at = datetime.utcnow() notification_db_object = Notification( id=notification_id, @@ -83,42 +87,69 @@ 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=status, created_at=created_at, sent_at=sent_at, sent_by=client.get_name() ) + dao_create_notification(notification_db_object) - try: - client.send_sms( - notification['to'], - template.replaced - ) - except FiretextClientException as e: - current_app.logger.exception(e) - notification_db_object.status = 'failed' - dao_update_notification(notification_db_object) + if can_send: + try: + template = Template( + dao_get_template_by_id(notification['template']).__dict__, + values=notification.get('personalisation', {}), + prefix=service.name, + drop_values={first_column_heading['sms']} + ) - current_app.logger.info( - "SMS {} created at {} sent at {}".format(notification_id, created_at, sent_at) - ) + client.send_sms( + notification['to'], + template.replaced + ) + except FiretextClientException as e: + current_app.logger.exception(e) + notification_db_object.status = 'failed' + dao_update_notification(notification_db_object) + + current_app.logger.info( + "SMS {} created at {} sent at {}".format(notification_id, created_at, sent_at) + ) except SQLAlchemyError as e: current_app.logger.debug(e) +def allowed_send_to_number(service, to): + if service.restricted and to not in [user.mobile_number for user in service.users]: + return False + return True + + +def allowed_send_to_email(service, to): + if service.restricted and to not in [user.email_address for user in service.users]: + return False + return True + + @notify_celery.task(name="send-email") def send_email(service_id, notification_id, subject, from_address, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) - template = Template( - dao_get_template_by_id(notification['template']).__dict__, - values=notification.get('personalisation', {}), - drop_values={first_column_heading['email']} - ) + service = dao_fetch_service_by_id(service_id) client = aws_ses_client try: + status = 'sent' + can_send = True + + if not allowed_send_to_email(service, notification['to']): + current_app.logger.info( + "Email {} failed as restricted service".format(notification_id) + ) + status = 'failed' + can_send = False + sent_at = datetime.utcnow() notification_db_object = Notification( id=notification_id, @@ -126,28 +157,35 @@ 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=status, created_at=created_at, sent_at=sent_at, sent_by=client.get_name() ) dao_create_notification(notification_db_object) - try: - client.send_email( - from_address, - notification['to'], - subject, - template.replaced - ) - except AwsSesClientException as e: - current_app.logger.debug(e) - notification_db_object.status = 'failed' - dao_update_notification(notification_db_object) + if can_send: + try: + template = Template( + dao_get_template_by_id(notification['template']).__dict__, + values=notification.get('personalisation', {}), + drop_values={first_column_heading['email']} + ) - current_app.logger.info( - "Email {} created at {} sent at {}".format(notification_id, created_at, sent_at) - ) + client.send_email( + from_address, + notification['to'], + subject, + template.replaced + ) + except AwsSesClientException as e: + current_app.logger.debug(e) + notification_db_object.status = 'failed' + dao_update_notification(notification_db_object) + + current_app.logger.info( + "Email {} created at {} sent at {}".format(notification_id, created_at, sent_at) + ) except SQLAlchemyError as e: current_app.logger.debug(e) diff --git a/app/models.py b/app/models.py index c2bbf4765..836938945 100644 --- a/app/models.py +++ b/app/models.py @@ -207,7 +207,7 @@ class VerifyCode(db.Model): return check_hash(cde, self._code) -NOTIFICATION_STATUS_TYPES = ['sent', 'failed'] +NOTIFICATION_STATUS_TYPES = ['sent', 'failed', 'failed - restricted service'] class Notification(db.Model): diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 9f5fea5ed..6ee7c5391 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -13,6 +13,11 @@ from app.celery import tasks from tests.app import load_example_csv from datetime import datetime, timedelta from freezegun import freeze_time +from tests.app.conftest import ( + sample_service, + sample_user, + sample_template +) @freeze_time("2016-01-01 11:09:00.061258") @@ -139,6 +144,89 @@ def test_should_send_sms_without_personalisation(sample_template, mocker): firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: This is a template") +def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker): + + user = sample_user(notify_db, notify_db_session, mobile_numnber="+441234123123") + 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": "+441234123123" + } + 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 + ) + + firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: This is a template") + + +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="+441234123123") + 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": "+440000000000" + } + 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 + ) + + 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) + template = sample_template(notify_db, notify_db_session, service=service) + + notification = { + "template": template.id, + "to": "test@restricted.com" + } + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.aws_ses_client.send_email') + + notification_id = uuid.uuid4() + now = datetime.utcnow() + send_email( + service.id, + notification_id, + 'subject', + 'email_from', + "encrypted-in-reality", + now) + + aws_ses_client.send_email.assert_called_once_with( + "email_from", + "test@restricted.com", + "subject", + template.content + ) + + def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sample_job, mocker): notification = { "template": sample_job.template.id, diff --git a/tests/app/conftest.py b/tests/app/conftest.py index eff3a56b0..d8ec894f0 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -42,12 +42,13 @@ def service_factory(notify_db, notify_db_session): @pytest.fixture(scope='function') def sample_user(notify_db, notify_db_session, + mobile_numnber="+447700900986", email="notify@digital.cabinet-office.gov.uk"): data = { 'name': 'Test User', 'email_address': email, 'password': 'password', - 'mobile_number': '+447700900986', + 'mobile_number': mobile_numnber, 'state': 'active' } usr = User.query.filter_by(email_address=email).first() @@ -99,14 +100,15 @@ def sample_sms_code(notify_db, def sample_service(notify_db, notify_db_session, service_name="Sample service", - user=None): + user=None, + restricted=False): if user is None: user = sample_user(notify_db, notify_db_session) data = { 'name': service_name, 'limit': 1000, 'active': False, - 'restricted': False, + 'restricted': restricted, 'email_from': email_safe(service_name) } service = Service.query.filter_by(name=service_name).first()