From b0bb0b97802c2ba28ea66dec56f58a4b064444cc Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 16 Jun 2020 15:52:14 +0100 Subject: [PATCH 1/2] Make sure people without international sms permission can send to crown dependencies --- app/notifications/rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index ed1c4a871..e53dffd0a 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -163,7 +163,7 @@ def get_notification_return_data(notification_id, notification, template): def _service_can_send_internationally(service, number): international_phone_info = get_international_phone_info(number) - if international_phone_info.international and \ + if (international_phone_info.international and international_phone_info.country_prefix != '44') and \ INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]: raise InvalidRequest( {'to': ["Cannot send to international mobile numbers"]}, From ef9f3c1e5fb15cce9c30e5224901baf951654ab4 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 16 Jun 2020 17:55:02 +0100 Subject: [PATCH 2/2] Make sure we check if a service can send to number harmoniously We were checking this separately in two places in the code. Now we will have this logic in one place, in validators. Also pull in utils version that recognises crown depenency numbers as international. --- app/notifications/rest.py | 19 +++-------- app/notifications/validators.py | 18 +++++++--- requirements-app.txt | 2 +- requirements.txt | 6 ++-- .../rest/test_send_notification.py | 33 +++++++++++++++++-- .../test_process_notification.py | 2 +- 6 files changed, 52 insertions(+), 28 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index e53dffd0a..692c960fd 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -15,7 +15,7 @@ from app.errors import ( InvalidRequest ) from app.models import ( - EMAIL_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE, + EMAIL_TYPE, SMS_TYPE, KEY_TYPE_TEAM, PRIORITY, LETTER_TYPE) from app.notifications.process_notifications import ( @@ -24,9 +24,10 @@ from app.notifications.process_notifications import ( simulated_recipient ) from app.notifications.validators import ( + check_if_service_can_send_to_number, check_rate_limiting, service_has_permission, - validate_template + validate_template, ) from app.schemas import ( email_notification_schema, @@ -38,7 +39,6 @@ from app.service.utils import service_allowed_to_send_to from app.utils import pagination_links, get_public_notify_type_text from notifications_utils import SMS_CHAR_COUNT_LIMIT -from notifications_utils.recipients import get_international_phone_info notifications = Blueprint('notifications', __name__) @@ -115,7 +115,7 @@ def send_notification(notification_type): ) if notification_type == SMS_TYPE: - _service_can_send_internationally(authenticated_service, notification_form['to']) + check_if_service_can_send_to_number(authenticated_service, notification_form['to']) # Do not persist or send notification to the queue if it is a simulated recipient simulated = simulated_recipient(notification_form['to'], notification_type) notification_model = persist_notification(template_id=template.id, @@ -160,17 +160,6 @@ def get_notification_return_data(notification_id, notification, template): return output -def _service_can_send_internationally(service, number): - international_phone_info = get_international_phone_info(number) - - if (international_phone_info.international and international_phone_info.country_prefix != '44') and \ - INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]: - raise InvalidRequest( - {'to': ["Cannot send to international mobile numbers"]}, - status_code=400 - ) - - def _service_allowed_to_send_to(notification, service): if not service_allowed_to_send_to(notification['to'], service, api_user.key_type): if api_user.key_type == KEY_TYPE_TEAM: diff --git a/app/notifications/validators.py b/app/notifications/validators.py index fb5880064..32aaa3b9c 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -120,11 +120,7 @@ def validate_and_format_recipient(send_to, key_type, service, notification_type, service_can_send_to_recipient(send_to, key_type, service, allow_whitelisted_recipients) if notification_type == SMS_TYPE: - international_phone_info = get_international_phone_info(send_to) - - if international_phone_info.international and \ - INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]: - raise BadRequestError(message="Cannot send to international mobile numbers") + international_phone_info = check_if_service_can_send_to_number(service, send_to) return validate_and_format_phone_number( number=send_to, @@ -134,6 +130,18 @@ def validate_and_format_recipient(send_to, key_type, service, notification_type, return validate_and_format_email_address(email_address=send_to) +def check_if_service_can_send_to_number(service, number): + international_phone_info = get_international_phone_info(number) + + if ( + # if number is international and not a crown dependency + international_phone_info.international and international_phone_info.country_prefix != '44' + ) and INTERNATIONAL_SMS_TYPE not in [p.permission for p in service.permissions]: + raise BadRequestError(message="Cannot send to international mobile numbers") + else: + return international_phone_info + + def check_content_char_count(template_with_content): if template_with_content.is_message_too_long(): message = f"Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. " \ diff --git a/requirements-app.txt b/requirements-app.txt index d5834cb4b..f159a35ce 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -26,7 +26,7 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@39.6.0#egg=notifications-utils==39.6.0 +git+https://github.com/alphagov/notifications-utils.git@39.7.0#egg=notifications-utils==39.7.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.7.1 diff --git a/requirements.txt b/requirements.txt index 9d8a5bb82..1fc092b09 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,7 +28,7 @@ notifications-python-client==5.5.1 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@39.6.0#egg=notifications-utils==39.6.0 +git+https://github.com/alphagov/notifications-utils.git@39.7.0#egg=notifications-utils==39.7.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.7.1 @@ -39,14 +39,14 @@ alembic==1.4.2 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.18.82 +awscli==1.18.83 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.17.5 +botocore==1.17.6 cachetools==4.1.0 certifi==2020.4.5.2 chardet==3.0.4 diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index d5be60713..92c31f555 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -1051,7 +1051,9 @@ def test_should_allow_store_original_number_on_sms_notification(client, sample_t assert '+(44) 7700-900 855' == notifications[0].to -def test_should_not_allow_international_number_on_sms_notification(client, sample_template, mocker): +def test_should_not_allow_sending_to_international_number_without_international_permission( + client, sample_template, mocker +): mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') data = { @@ -1070,10 +1072,35 @@ def test_should_not_allow_international_number_on_sms_notification(client, sampl assert response.status_code == 400 error_json = json.loads(response.get_data(as_text=True)) assert error_json['result'] == 'error' - assert error_json['message']['to'][0] == 'Cannot send to international mobile numbers' + assert error_json['message'] == 'Cannot send to international mobile numbers' -def test_should_allow_international_number_on_sms_notification(client, sample_service_full_permissions, mocker): +def test_should_allow_sending_to_crown_dependency_number_without_international_permission( + client, mocker, notify_db_session +): + service = create_service() + + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + template = create_template(service) + + data = { + 'to': '07700-900-123', + 'template': str(template.id) + } + + auth_header = create_authorization_header(service_id=service.id) + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201 + + +def test_should_allow_sending_to_international_number_with_international_permission( + client, sample_service_full_permissions, mocker +): mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') template = create_template(sample_service_full_permissions) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 52f5fbfea..5fe5a513a 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -356,7 +356,7 @@ def test_simulated_recipient(notify_api, to_address, notification_type, expected @pytest.mark.parametrize('recipient, expected_international, expected_prefix, expected_units', [ ('7900900123', False, '44', 1), # UK ('+447900900123', False, '44', 1), # UK - ('07700900222', False, '44', 1), # UK + ('07700900222', True, '44', 1), # UK (Jersey) ('73122345678', True, '7', 1), # Russia ('360623400400', True, '36', 3)] # Hungary )