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