From 01cf175cb2a2d9ec15885a0fcdf8c190516e860d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 17 Jan 2018 15:20:04 +0000 Subject: [PATCH 1/2] =?UTF-8?q?Don=E2=80=99t=20respect=20the=20whitelist?= =?UTF-8?q?=20for=20one=20off=20sending?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The whitelist was built to help developers and designers making prototypes to do realistic usability testing of them, without having to go through the whole go live process. These users are sending messages using the API. The whitelist wasn’t made available to users uploading spreadsheets. The users sending one off messages are similar to those uploading spreadsheets, not those using the API. Therefore they shouldn’t be able to use the whitelist to expand the range of recipients they can send to. Passing the argument through three methods doesn’t feel that great, but can’t think of a better way without major refactoring… --- app/notifications/validators.py | 8 +++---- app/service/send_notification.py | 3 ++- app/service/utils.py | 3 ++- tests/app/notifications/test_validators.py | 23 +++++++++++++++++++ .../service/test_send_one_off_notification.py | 20 +++++++++++++--- 5 files changed, 48 insertions(+), 9 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 85909c5a2..c25d75aea 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -67,8 +67,8 @@ def check_template_is_active(template): message="Template has been deleted") -def service_can_send_to_recipient(send_to, key_type, service): - if not service_allowed_to_send_to(send_to, service, key_type): +def service_can_send_to_recipient(send_to, key_type, service, allow_whitelisted_recipients=True): + if not service_allowed_to_send_to(send_to, service, key_type, allow_whitelisted_recipients): if key_type == KEY_TYPE_TEAM: message = 'Can’t send to this recipient using a team-only API key' else: @@ -97,11 +97,11 @@ def check_service_can_schedule_notification(permissions, scheduled_for): raise BadRequestError(message="Cannot schedule notifications (this feature is invite-only)") -def validate_and_format_recipient(send_to, key_type, service, notification_type): +def validate_and_format_recipient(send_to, key_type, service, notification_type, allow_whitelisted_recipients=True): if send_to is None: raise BadRequestError(message="Recipient can't be empty") - service_can_send_to_recipient(send_to, key_type, service) + 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) diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 6f630d74b..dbb4bd4fc 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -49,7 +49,8 @@ def send_one_off_notification(service_id, post_data): send_to=post_data['to'], key_type=KEY_TYPE_NORMAL, service=service, - notification_type=template.template_type + notification_type=template.template_type, + allow_whitelisted_recipients=False, ) validate_created_by(service, post_data['created_by']) diff --git a/app/service/utils.py b/app/service/utils.py index d25c6b7de..16521fbbd 100644 --- a/app/service/utils.py +++ b/app/service/utils.py @@ -26,7 +26,7 @@ def get_whitelist_objects(service_id, request_json): ] -def service_allowed_to_send_to(recipient, service, key_type): +def service_allowed_to_send_to(recipient, service, key_type, allow_whitelisted_recipients=True): if key_type == KEY_TYPE_TEST: return True @@ -38,6 +38,7 @@ def service_allowed_to_send_to(recipient, service, key_type): ) whitelist_members = [ member.recipient for member in service.whitelist + if allow_whitelisted_recipients ] if ( diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 1eb712f2e..aead9c5f1 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -213,6 +213,29 @@ def test_service_can_send_to_recipient_passes_for_whitelisted_recipient_passes(n sample_service) is None +@pytest.mark.parametrize('recipient', [ + {"email_address": "some_other_email@test.com"}, + {"mobile_number": "07513332413"}, +]) +def test_service_can_send_to_recipient_fails_when_ignoring_whitelist( + notify_db, + notify_db_session, + sample_service, + recipient, +): + sample_service_whitelist(notify_db, notify_db_session, **recipient) + with pytest.raises(BadRequestError) as exec_info: + service_can_send_to_recipient( + next(iter(recipient.values())), + 'team', + sample_service, + allow_whitelisted_recipients=False, + ) + assert exec_info.value.status_code == 400 + assert exec_info.value.message == 'Can’t send to this recipient using a team-only API key' + assert exec_info.value.fields == [] + + @pytest.mark.parametrize('recipient', ['07513332413', 'some_other_email@test.com']) @pytest.mark.parametrize('key_type, error_message', [('team', 'Can’t send to this recipient using a team-only API key'), diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index cbbe8f7dd..5a88e107d 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -7,12 +7,15 @@ from sqlalchemy.exc import SQLAlchemyError from app.v2.errors import BadRequestError, TooManyRequestsError from app.config import QueueNames +from app.dao.service_whitelist_dao import dao_add_and_commit_whitelisted_contacts from app.service.send_notification import send_one_off_notification from app.models import ( KEY_TYPE_NORMAL, + MOBILE_TYPE, PRIORITY, SMS_TYPE, - Notification + Notification, + ServiceWhitelist, ) from tests.app.db import ( @@ -137,13 +140,24 @@ def test_send_one_off_notification_raises_if_invalid_recipient(notify_db_session send_one_off_notification(service.id, post_data) -def test_send_one_off_notification_raises_if_cant_send_to_recipient(notify_db_session): +@pytest.mark.parametrize('recipient', [ + '07700 900 001', # not in team or whitelist + '07700900123', # in whitelist + '+447700-900-123', # in whitelist in different format +]) +def test_send_one_off_notification_raises_if_cant_send_to_recipient( + notify_db_session, + recipient, +): service = create_service(restricted=True) template = create_template(service=service) + dao_add_and_commit_whitelisted_contacts([ + ServiceWhitelist.from_string(service.id, MOBILE_TYPE, '07700900123'), + ]) post_data = { 'template_id': str(template.id), - 'to': '07700 900 001', + 'to': recipient, 'created_by': str(service.created_by_id) } From c6e4aee80a730a059a92d6f470a536e8f586d164 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Jan 2018 11:33:20 +0000 Subject: [PATCH 2/2] Be stricter about length of phone numbers Brings in: - [x] https://github.com/alphagov/notifications-utils/pull/319 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index a36d0099b..8a3e83605 100644 --- a/requirements.txt +++ b/requirements.txt @@ -25,6 +25,6 @@ notifications-python-client==4.7.1 awscli==1.14.25 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@23.5.0#egg=notifications-utils==23.5.0 +git+https://github.com/alphagov/notifications-utils.git@23.5.2#egg=notifications-utils==23.5.2 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3