From 4268f8453be6961a625369d0b6328173f39f63fc Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 16 Mar 2016 13:33:12 +0000 Subject: [PATCH] Use the same validation in the endpoint and the task to validate the phone number is ok to send to. Format the phone number before sending it to the sms provider. --- app/celery/tasks.py | 23 ++++-------- app/notifications/rest.py | 5 +-- app/validation.py | 15 ++++++++ tests/app/celery/test_tasks.py | 40 ++++++++++++--------- tests/app/test_validation.py | 65 ++++++++++++++++++++++++++++++++++ 5 files changed, 112 insertions(+), 36 deletions(-) create mode 100644 app/validation.py create mode 100644 tests/app/test_validation.py diff --git a/app/celery/tasks.py b/app/celery/tasks.py index ff952d6e2..94fb59bc4 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -25,7 +25,8 @@ from sqlalchemy.exc import SQLAlchemyError from app.aws import s3 from datetime import datetime from utils.template import Template -from utils.recipients import RecipientCSV, validate_phone_number, format_phone_number +from utils.recipients import RecipientCSV, format_phone_number, validate_phone_number +from app.validation import (allowed_send_to_email, allowed_send_to_number) @notify_celery.task(name="delete-verify-codes") @@ -204,7 +205,7 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): ) client.send_sms( - to=notification['to'], + to=format_phone_number(validate_phone_number(notification['to'])), content=template.replaced, reference=str(notification_id) ) @@ -223,20 +224,6 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): current_app.logger.debug(e) -def allowed_send_to_number(service, to): - if service.restricted and format_phone_number(validate_phone_number(to)) not in [ - format_phone_number(validate_phone_number(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) @@ -300,7 +287,9 @@ def send_sms_code(encrypted_verification): verification_message = encryption.decrypt(encrypted_verification) try: firetext_client.send_sms( - verification_message['to'], verification_message['secret_code'], 'send-sms-code' + format_phone_number(validate_phone_number(verification_message['to'])), + verification_message['secret_code'], + 'send-sms-code' ) except FiretextClientException as e: current_app.logger.exception(e) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 594ba5613..03869cfd3 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -26,6 +26,7 @@ from app.schemas import ( notification_status_schema ) from app.celery.tasks import send_sms, send_email +from app.validation import allowed_send_to_number, allowed_send_to_email notifications = Blueprint('notifications', __name__) @@ -320,7 +321,7 @@ def send_notification(notification_type): notification_id = create_uuid() if notification_type == 'sms': - if service.restricted and notification['to'] not in [user.mobile_number for user in service.users]: + if not allowed_send_to_number(service, notification['to']): return jsonify( result="error", message={'to': ['Invalid phone number for restricted service']}), 400 send_sms.apply_async(( @@ -330,7 +331,7 @@ def send_notification(notification_type): datetime.utcnow().strftime(DATETIME_FORMAT) ), queue='sms') else: - if service.restricted and notification['to'] not in [user.email_address for user in service.users]: + if not allowed_send_to_email(service, notification['to']): return jsonify( result="error", message={'to': ['Email address not permitted for restricted service']}), 400 send_email.apply_async(( diff --git a/app/validation.py b/app/validation.py new file mode 100644 index 000000000..49a6b0664 --- /dev/null +++ b/app/validation.py @@ -0,0 +1,15 @@ +from utils.recipients import format_phone_number, validate_phone_number + + +def allowed_send_to_number(service, to): + if service.restricted and format_phone_number(validate_phone_number(to)) not in [ + format_phone_number(validate_phone_number(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 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index e2f334697..737f08864 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,6 +1,8 @@ import uuid import pytest from flask import current_app +from utils.recipients import validate_phone_number, format_phone_number + from app.celery.tasks import ( send_sms, send_sms_code, @@ -248,7 +250,7 @@ def test_should_process_all_sms_job(sample_job, sample_job_with_placeholdered_te def test_should_send_template_to_correct_sms_provider_and_persist(sample_template_with_placeholders, mocker): notification = { "template": sample_template_with_placeholders.id, - "to": "+441234123123", + "to": "+447234123123", "personalisation": {"name": "Jo"} } mocker.patch('app.encryption.decrypt', return_value=notification) @@ -265,7 +267,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat ) firetext_client.send_sms.assert_called_once_with( - to="+441234123123", + to=format_phone_number(validate_phone_number("+447234123123")), content="Sample service: Hello Jo", reference=str(notification_id) ) @@ -273,7 +275,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat sample_template_with_placeholders.service_id, notification_id ) assert persisted_notification.id == notification_id - assert persisted_notification.to == '+441234123123' + assert persisted_notification.to == '+447234123123' assert persisted_notification.template_id == sample_template_with_placeholders.id assert persisted_notification.status == 'sent' assert persisted_notification.created_at == now @@ -285,7 +287,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat def test_should_send_sms_without_personalisation(sample_template, mocker): notification = { "template": sample_template.id, - "to": "+441234123123" + "to": "+447234123123" } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.firetext_client.send_sms') @@ -301,7 +303,7 @@ def test_should_send_sms_without_personalisation(sample_template, mocker): ) firetext_client.send_sms.assert_called_once_with( - to="+441234123123", + to=format_phone_number(validate_phone_number("+447234123123")), content="Sample service: This is a template", reference=str(notification_id) ) @@ -330,7 +332,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif ) firetext_client.send_sms.assert_called_once_with( - to="+447700900890", + to=format_phone_number(validate_phone_number("+447700900890")), content="Sample service: This is a template", reference=str(notification_id) ) @@ -396,7 +398,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa notification = { "template": sample_job.template.id, "job": sample_job.id, - "to": "+441234123123" + "to": "+447234123123" } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.firetext_client.send_sms') @@ -411,13 +413,13 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa now.strftime(DATETIME_FORMAT) ) firetext_client.send_sms.assert_called_once_with( - to="+441234123123", + to=format_phone_number(validate_phone_number("+447234123123")), content="Sample service: This is a template", reference=str(notification_id) ) persisted_notification = notifications_dao.get_notification(sample_job.template.service_id, notification_id) assert persisted_notification.id == notification_id - assert persisted_notification.to == '+441234123123' + assert persisted_notification.to == '+447234123123' assert persisted_notification.job_id == sample_job.id assert persisted_notification.template_id == sample_job.template.id assert persisted_notification.status == 'sent' @@ -520,7 +522,7 @@ def test_should_use_email_template_and_persist_without_personalisation( def test_should_persist_notification_as_failed_if_sms_client_fails(sample_template, mocker): notification = { "template": sample_template.id, - "to": "+441234123123" + "to": "+447234123123" } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.firetext_client.send_sms', side_effect=FiretextClientException(firetext_error())) @@ -536,13 +538,13 @@ def test_should_persist_notification_as_failed_if_sms_client_fails(sample_templa now.strftime(DATETIME_FORMAT) ) firetext_client.send_sms.assert_called_once_with( - to="+441234123123", + to=format_phone_number(validate_phone_number("+447234123123")), content="Sample service: This is a template", reference=str(notification_id) ) persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) assert persisted_notification.id == notification_id - assert persisted_notification.to == '+441234123123' + assert persisted_notification.to == '+447234123123' assert persisted_notification.template_id == sample_template.id assert persisted_notification.status == 'failed' assert persisted_notification.created_at == now @@ -590,7 +592,7 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai def test_should_not_send_sms_if_db_peristance_failed(sample_template, mocker): notification = { "template": sample_template.id, - "to": "+441234123123" + "to": "+447234123123" } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.firetext_client.send_sms') @@ -638,24 +640,28 @@ def test_should_not_send_email_if_db_peristance_failed(sample_email_template, mo def test_should_send_sms_code(mocker): - notification = {'to': '+441234123123', + notification = {'to': '+447234123123', 'secret_code': '12345'} encrypted_notification = encryption.encrypt(notification) mocker.patch('app.firetext_client.send_sms') send_sms_code(encrypted_notification) - firetext_client.send_sms.assert_called_once_with(notification['to'], notification['secret_code'], 'send-sms-code') + firetext_client.send_sms.assert_called_once_with(format_phone_number(validate_phone_number(notification['to'])), + notification['secret_code'], + 'send-sms-code') def test_should_throw_firetext_client_exception(mocker): - notification = {'to': '+441234123123', + notification = {'to': '+447234123123', 'secret_code': '12345'} encrypted_notification = encryption.encrypt(notification) mocker.patch('app.firetext_client.send_sms', side_effect=FiretextClientException(firetext_error())) send_sms_code(encrypted_notification) - firetext_client.send_sms.assert_called_once_with(notification['to'], notification['secret_code'], 'send-sms-code') + firetext_client.send_sms.assert_called_once_with(format_phone_number(validate_phone_number(notification['to'])), + notification['secret_code'], + 'send-sms-code') def test_should_send_email_code(mocker): diff --git a/tests/app/test_validation.py b/tests/app/test_validation.py new file mode 100644 index 000000000..9c822533d --- /dev/null +++ b/tests/app/test_validation.py @@ -0,0 +1,65 @@ +from app.models import User, Service +from app.validation import allowed_send_to_number, allowed_send_to_email + + +def test_allowed_send_to_number_returns_true_for_restricted_service_with_same_number(): + mobile_number = '07524609792' + service = _create_service_data(mobile_number) + assert allowed_send_to_number(service, mobile_number) + + +def test_allowed_send_to_number_returns_false_for_restricted_service_with_different_number(): + mobile_number = '00447524609792' + service = _create_service_data(mobile_number) + assert not allowed_send_to_number(service, '+447344609793') + + +def test_allowed_send_to_number_returns_true_for_unrestricted_service_with_different_number(): + mobile_number = '+447524609792' + service = _create_service_data(mobile_number, False) + assert allowed_send_to_number(service, '+447344609793') + + +def test_allowed_send_to_email__returns_true_for_restricted_service_with_same_email(): + email = 'testing@it.gov.uk' + service = _create_service_data(email_address=email) + assert allowed_send_to_email(service, email) + + +def test_allowed_send_to_email__returns_false_for_restricted_service_with_different_email(): + email = 'testing@it.gov.uk' + service = _create_service_data(email_address=email) + assert not allowed_send_to_email(service, 'another@it.gov.uk') + + +def test_allowed_send_to_email__returns_false_for_restricted_service_with_different_email(): + email = 'testing@it.gov.uk' + service = _create_service_data(email_address=email) + assert not allowed_send_to_email(service, 'another@it.gov.uk') + + +def test_allowed_send_to_email__returns_true_for_unrestricted_service_with_different_email(): + email = 'testing@it.gov.uk' + service = _create_service_data(email_address=email, restricted=False) + assert allowed_send_to_number(service, 'another@it.gov.uk') + + +def _create_service_data(mobile_number='+447524609792', restricted=True, email_address='test_user@it.gov.uk'): + usr = { + 'name': 'Test User', + 'email_address': email_address, + 'password': 'password', + 'mobile_number': mobile_number, + 'state': 'active' + } + user = User(**usr) + data = { + 'name': 'Test service', + 'limit': 10, + 'active': False, + 'restricted': restricted, + 'email_from': 'test_service@it.gov.uk' + } + service = Service(**data) + service.users = [user] + return service