From 157b38532795b542db492acf26a55c428a8fee89 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sun, 6 Mar 2016 12:58:28 +0000 Subject: [PATCH] Use validation of recipients from utils This was added to utils in https://github.com/alphagov/notifications-utils/commit/5914da74f1f948e2fbcd7597b7a26476de1e2eb3 This means that: - we are doing the exact same validation in the API and admin app - we are actually validating phone numbers for the correct format (hence all the changes to the tests) --- app/schemas.py | 19 +++++++++++-------- tests/app/conftest.py | 2 +- tests/app/notifications/test_rest.py | 18 +++++++++--------- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 40cb7ff08..016cfadb7 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -6,10 +6,7 @@ from . import models from app.dao.permissions_dao import permission_dao from marshmallow import (post_load, ValidationError, validates, validates_schema) from marshmallow_sqlalchemy import field_for - -mobile_regex = re.compile("^\\+44[\\d]{10}$") - -email_regex = re.compile("(^[^@^\\s]+@[^@^\\.^\\s]+(\\.[^@^\\.^\\s]*)*\.(.+))") +from utils.recipients import validate_email_address, InvalidEmailError, validate_phone_number, InvalidPhoneError # TODO I think marshmallow provides a better integration and error handling. @@ -128,8 +125,10 @@ class SmsNotificationSchema(NotificationSchema): @validates('to') def validate_to(self, value): - if not mobile_regex.match(value): - raise ValidationError('Invalid phone number, must be of format +441234123123') + try: + validate_phone_number(value) + except InvalidPhoneError as error: + raise ValidationError('Invalid phone number: {}'.format(error)) class EmailNotificationSchema(NotificationSchema): @@ -138,7 +137,9 @@ class EmailNotificationSchema(NotificationSchema): @validates('to') def validate_to(self, value): - if not email_regex.match(value): + try: + validate_email_address(value) + except InvalidEmailError: raise ValidationError('Invalid email') @@ -174,7 +175,9 @@ class InvitedUserSchema(BaseSchema): @validates('email_address') def validate_to(self, value): - if not email_regex.match(value): + try: + validate_email_address(value) + except InvalidEmailError: raise ValidationError('Invalid email') diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d8ec894f0..679d12df8 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -277,7 +277,7 @@ def sample_notification(notify_db, if to_field: to = to_field else: - to = '+44709123456' + to = '+447700900855' data = { 'id': notification_id, diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 650cd85ec..967a74d39 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -24,7 +24,7 @@ def test_get_notification_by_id(notify_api, sample_notification): notification = json.loads(response.get_data(as_text=True))['notification'] assert notification['status'] == 'sent' assert notification['template'] == sample_notification.template.id - assert notification['to'] == '+44709123456' + assert notification['to'] == '+447700900855' assert notification['service'] == str(sample_notification.service_id) assert response.status_code == 200 @@ -63,7 +63,7 @@ def test_get_all_notifications(notify_api, sample_notification): notifications = json.loads(response.get_data(as_text=True)) assert notifications['notifications'][0]['status'] == 'sent' assert notifications['notifications'][0]['template'] == sample_notification.template.id - assert notifications['notifications'][0]['to'] == '+44709123456' + assert notifications['notifications'][0]['to'] == '+447700900855' assert notifications['notifications'][0]['service'] == str(sample_notification.service_id) assert response.status_code == 200 @@ -296,7 +296,7 @@ def test_should_reject_bad_phone_numbers(notify_api, sample_template, mocker): assert json_resp['result'] == 'error' assert len(json_resp['message'].keys()) == 1 - assert 'Invalid phone number, must be of format +441234123123' in json_resp['message']['to'] + assert 'Invalid phone number: Must not contain letters or symbols' in json_resp['message']['to'] assert response.status_code == 400 @@ -306,7 +306,7 @@ def test_send_notification_invalid_template_id(notify_api, sample_template, mock mocker.patch('app.celery.tasks.send_sms.apply_async') data = { - 'to': '+441234123123', + 'to': '+447700900855', 'template': 9999 } auth_header = create_authorization_header( @@ -337,7 +337,7 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_templat mocker.patch('app.encryption.encrypt', return_value="something_encrypted") data = { - 'to': '+441234123123', + 'to': '+447700900855', 'template': sample_template_with_placeholders.id, 'personalisation': { 'name': 'Jo' @@ -373,7 +373,7 @@ def test_send_notification_with_missing_personalisation( mocker.patch('app.celery.tasks.send_sms.apply_async') data = { - 'to': '+441234123123', + 'to': '+447700900855', 'template': sample_template_with_placeholders.id, 'personalisation': { 'foo': 'bar' @@ -405,7 +405,7 @@ def test_send_notification_with_too_much_personalisation_data( mocker.patch('app.celery.tasks.send_sms.apply_async') data = { - 'to': '+441234123123', + 'to': '+447700900855', 'template': sample_template_with_placeholders.id, 'personalisation': { 'name': 'Jo', 'foo': 'bar' @@ -439,7 +439,7 @@ def test_prevents_sending_to_any_mobile_on_restricted_service(notify_api, sample ).update( {'restricted': True} ) - invalid_mob = '+449999999999' + invalid_mob = '+447700900855' data = { 'to': invalid_mob, 'template': sample_template.id @@ -504,7 +504,7 @@ def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker mocker.patch('app.encryption.encrypt', return_value="something_encrypted") data = { - 'to': '+441234123123', + 'to': '+447700900855', 'template': sample_template.id }