Merge pull request #159 from alphagov/validate-number

Format and validate phone number

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)

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((

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

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):

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