diff --git a/app/celery/tasks.py b/app/celery/tasks.py index fd8ccb3a9..d41271e06 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -49,11 +49,6 @@ from app.models import ( TEMPLATE_TYPE_SMS ) -from app.validation import ( - allowed_send_to_email, - allowed_send_to_number -) - @notify_celery.task(name="delete-verify-codes") def delete_verify_codes(): @@ -197,16 +192,6 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): client = firetext_client try: - status = 'sent' - can_send = True - - if not allowed_send_to_number(service, notification['to']): - current_app.logger.info( - "SMS {} failed as restricted service".format(notification_id) - ) - status = 'failed' - can_send = False - sent_at = datetime.utcnow() notification_db_object = Notification( id=notification_id, @@ -214,7 +199,7 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): to=notification['to'], service_id=service_id, job_id=notification.get('job', None), - status=status, + status='sent', created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, sent_by=client.get_name() @@ -222,30 +207,29 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): dao_create_notification(notification_db_object, TEMPLATE_TYPE_SMS) - if can_send: - try: - template = Template( - dao_get_template_by_id(notification['template']).__dict__, - values=notification.get('personalisation', {}), - prefix=service.name - ) - - client.send_sms( - to=validate_and_format_phone_number(notification['to']), - content=template.replaced, - reference=str(notification_id) - ) - except FiretextClientException as e: - current_app.logger.error( - "SMS notification {} failed".format(notification_id) - ) - current_app.logger.exception(e) - notification_db_object.status = 'failed' - dao_update_notification(notification_db_object) - - current_app.logger.info( - "SMS {} created at {} sent at {}".format(notification_id, created_at, sent_at) + try: + template = Template( + dao_get_template_by_id(notification['template']).__dict__, + values=notification.get('personalisation', {}), + prefix=service.name ) + + client.send_sms( + to=validate_and_format_phone_number(notification['to']), + content=template.replaced, + reference=str(notification_id) + ) + except FiretextClientException as e: + current_app.logger.error( + "SMS notification {} failed".format(notification_id) + ) + current_app.logger.exception(e) + notification_db_object.status = 'failed' + dao_update_notification(notification_db_object) + + current_app.logger.info( + "SMS {} created at {} sent at {}".format(notification_id, created_at, sent_at) + ) except SQLAlchemyError as e: current_app.logger.debug(e) @@ -253,21 +237,9 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): @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) - service = dao_fetch_service_by_id(service_id) - client = aws_ses_client try: - status = 'sent' - can_send = True - - if not allowed_send_to_email(service, notification['to']): - current_app.logger.info( - "Email {} failed as restricted service".format(notification_id) - ) - status = 'failed' - can_send = False - sent_at = datetime.utcnow() notification_db_object = Notification( id=notification_id, @@ -275,36 +247,35 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not to=notification['to'], service_id=service_id, job_id=notification.get('job', None), - status=status, + status='sent', created_at=datetime.strptime(created_at, DATETIME_FORMAT), sent_at=sent_at, sent_by=client.get_name() ) dao_create_notification(notification_db_object, TEMPLATE_TYPE_EMAIL) - if can_send: - try: - template = Template( - dao_get_template_by_id(notification['template']).__dict__, - values=notification.get('personalisation', {}) - ) - - reference = client.send_email( - from_address, - notification['to'], - subject, - body=template.replaced, - html_body=template.as_HTML_email, - ) - update_notification_reference_by_id(notification_id, reference) - except AwsSesClientException as e: - current_app.logger.exception(e) - notification_db_object.status = 'failed' - dao_update_notification(notification_db_object) - - current_app.logger.info( - "Email {} created at {} sent at {}".format(notification_id, created_at, sent_at) + try: + template = Template( + dao_get_template_by_id(notification['template']).__dict__, + values=notification.get('personalisation', {}) ) + + reference = client.send_email( + from_address, + notification['to'], + subject, + body=template.replaced, + html_body=template.as_HTML_email, + ) + update_notification_reference_by_id(notification_id, reference) + except AwsSesClientException as e: + current_app.logger.exception(e) + notification_db_object.status = 'failed' + dao_update_notification(notification_db_object) + + current_app.logger.info( + "Email {} created at {} sent at {}".format(notification_id, created_at, sent_at) + ) except SQLAlchemyError as e: current_app.logger.debug(e) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 3e536b4ed..edd4a4e51 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -1,5 +1,6 @@ from datetime import datetime import uuid +import itertools from flask import ( Blueprint, @@ -11,6 +12,7 @@ from flask import ( ) from utils.template import Template +from utils.recipients import allowed_to_send_to, first_column_heading from app.clients.sms.firetext import FiretextResponses from app.clients.email.aws_ses import AwsSesResponses from app import api_user, encryption, create_uuid, DATETIME_FORMAT, DATE_FORMAT @@ -28,7 +30,6 @@ from app.schemas import ( notifications_filter_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__) @@ -356,12 +357,21 @@ def send_notification(notification_type): } ), 400 + if service.restricted and not allowed_to_send_to( + notification['to'], + itertools.chain.from_iterable( + [user.mobile_number, user.email_address] for user in service.users + ) + ): + return jsonify( + result="error", message={ + 'to': ['Invalid {} for restricted service'.format(first_column_heading[notification_type])] + } + ), 400 + notification_id = create_uuid() if notification_type == 'sms': - 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(( service_id, notification_id, @@ -369,9 +379,6 @@ def send_notification(notification_type): datetime.utcnow().strftime(DATETIME_FORMAT) ), queue='sms') else: - 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(( service_id, notification_id, diff --git a/app/validation.py b/app/validation.py deleted file mode 100644 index 49a6b0664..000000000 --- a/app/validation.py +++ /dev/null @@ -1,15 +0,0 @@ -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/requirements.txt b/requirements.txt index 3f4ed5ceb..7edbd4718 100644 --- a/requirements.txt +++ b/requirements.txt @@ -21,4 +21,4 @@ monotonic==0.3 git+https://github.com/alphagov/notifications-python-client.git@0.2.6#egg=notifications-python-client==0.2.6 -git+https://github.com/alphagov/notifications-utils.git@3.1.3#egg=notifications-utils==3.1.3 +git+https://github.com/alphagov/notifications-utils.git@3.2.0#egg=notifications-utils==3.2.0 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 767a4cb5c..39a755a71 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -343,31 +343,6 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif ) -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="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": "07700 900849" - } - mocker.patch('app.encryption.decrypt', return_value=notification) - mocker.patch('app.firetext_client.send_sms') - mocker.patch('app.firetext_client.get_name', return_value="firetext") - - notification_id = uuid.uuid4() - now = datetime.utcnow() - send_sms( - service.id, - notification_id, - "encrypted-in-reality", - now.strftime(DATETIME_FORMAT) - ) - - firetext_client.send_sms.assert_not_called() - - def test_should_send_email_if_restricted_service_and_valid_email(notify_db, notify_db_session, mocker): user = sample_user(notify_db, notify_db_session, email="test@restricted.com") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 4882ba420..5f55059a1 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -879,7 +879,7 @@ def test_should_not_send_email_if_restricted_and_not_a_service_user(notify_api, app.celery.tasks.send_email.apply_async.assert_not_called() assert response.status_code == 400 - assert 'Email address not permitted for restricted service' in json_resp['message']['to'] + assert 'Invalid email address for restricted service' in json_resp['message']['to'] def test_should_not_send_email_for_job_if_restricted_and_not_a_service_user( @@ -915,7 +915,7 @@ def test_should_not_send_email_for_job_if_restricted_and_not_a_service_user( app.celery.tasks.send_email.apply_async.assert_not_called() assert response.status_code == 400 - assert 'Email address not permitted for restricted service' in json_resp['message']['to'] + assert 'Invalid email address for restricted service' in json_resp['message']['to'] @freeze_time("2016-01-01 11:09:00.061258") diff --git a/tests/app/test_validation.py b/tests/app/test_validation.py deleted file mode 100644 index 9c822533d..000000000 --- a/tests/app/test_validation.py +++ /dev/null @@ -1,65 +0,0 @@ -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