From d6cf15469fe7c7561eff703a96b9bf83a0b6e7be Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 11 Mar 2016 13:11:10 +0000 Subject: [PATCH] Be agnostic about format when comparing phone #s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a service is in restricted mode then a user can’t send messages to anyone other than themselves and members of their team. To do this the API has to compare the numbers they are sending to with those of their team members. It will (falsely) say the numbers do not match if they are in a different format, eg 07700 900849 vs +447700900849 This commit uses the code we use elsewhere for formatting phone numbers to make sure that both numbers are in a consistent format before doing a comparison. I have a strong preference for doing it this way, rather than formatting numbers before we store them: 1. https://en.wikipedia.org/wiki/Robustness_principle 2. It’s confusing to a user to see their own phone number formatted in a different way to that which they entered it, and the alternative, storing the phone number in two different formats is grim --- app/celery/tasks.py | 6 ++++-- tests/app/celery/test_tasks.py | 10 +++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index b3f090127..a3bc23f5a 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -24,7 +24,7 @@ from sqlalchemy.exc import SQLAlchemyError from app.aws import s3 from datetime import datetime from utils.template import Template -from utils.recipients import RecipientCSV +from utils.recipients import RecipientCSV, validate_phone_number, format_phone_number @notify_celery.task(name="delete-verify-codes") @@ -219,7 +219,9 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): def allowed_send_to_number(service, to): - if service.restricted and to not in [user.mobile_number for user in service.users]: + 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 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index bb2a7bb3b..b229a5f37 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -296,13 +296,13 @@ def test_should_send_sms_without_personalisation(sample_template, mocker): 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") + user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900890") 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" + "to": "+447700900890" # The user’s own number, but in a different format } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.firetext_client.send_sms') @@ -317,17 +317,17 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif now.strftime(DATETIME_FORMAT) ) - firetext_client.send_sms.assert_called_once_with("+441234123123", "Sample service: This is a template") + firetext_client.send_sms.assert_called_once_with("+447700900890", "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") + 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": "+440000000000" + "to": "07700 900849" } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.firetext_client.send_sms')