From 3d0df9b5a766bfb0828f89291eb68adac5d7add7 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 20 Sep 2016 15:41:53 +0100 Subject: [PATCH 01/19] add new service whitelist table services can have a whitelist of phone numbers and email addresses that they can send to in addition to team members when in trial mode. email_address and mobile_number are nullable and app level checks will be in place to prevent inserting blank rows. they have a created_at date so that we can [potentially] delete them a week later to avoid keeping personally identifying data any longer than necessary --- app/models.py | 11 +++++++ migrations/versions/0055_service_whitelist.py | 31 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 migrations/versions/0055_service_whitelist.py diff --git a/app/models.py b/app/models.py index 91294c15d..fac1458cb 100644 --- a/app/models.py +++ b/app/models.py @@ -132,6 +132,17 @@ class Service(db.Model, Versioned): ) +class ServiceWhitelist(db.Model): + __tablename__ = 'service_whitelist' + + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False) + service = db.relationship('Service', backref='whitelist') + email_address = db.Column(db.String(255), nullable=True) + mobile_number = db.Column(db.String, nullable=True) + created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow) + + class ApiKey(db.Model, Versioned): __tablename__ = 'api_keys' diff --git a/migrations/versions/0055_service_whitelist.py b/migrations/versions/0055_service_whitelist.py new file mode 100644 index 000000000..b2457833f --- /dev/null +++ b/migrations/versions/0055_service_whitelist.py @@ -0,0 +1,31 @@ +"""add service whitelist table + +Revision ID: 0055_service_whitelist +Revises: 0054_perform_drop_status_column +Create Date: 2016-09-20 12:12:30.838095 + +""" + +# revision identifiers, used by Alembic. +revision = '0055_service_whitelist' +down_revision = '0054_perform_drop_status_column' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + op.create_table('service_whitelist', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('email_address', sa.String(length=255), nullable=True), + sa.Column('mobile_number', sa.String(), nullable=True), + sa.Column('created_at', sa.DateTime(), nullable=True), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_service_whitelist_service_id'), 'service_whitelist', ['service_id'], unique=False) + + +def downgrade(): + op.drop_table('service_whitelist') From 203936fa84d69da6e8c2fea594b98a362c9ec770 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 20 Sep 2016 17:35:15 +0100 Subject: [PATCH 02/19] add GET/POST rest endpoints for whitelist GET //whitelist returns all whitelisted contacts for a service, separated into two lists POST //whitelist removes all existing whitelisted contacts, and replaces them with the provided new entries (todo: dao work + tests) --- app/dao/service_whitelist_dao.py | 11 +++++++++++ app/service/rest.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 app/dao/service_whitelist_dao.py diff --git a/app/dao/service_whitelist_dao.py b/app/dao/service_whitelist_dao.py new file mode 100644 index 000000000..359ad0bdc --- /dev/null +++ b/app/dao/service_whitelist_dao.py @@ -0,0 +1,11 @@ +from sqlalchemy import or_ + +from app import db +from app.models import ServiceWhitelist + +def dao_fetch_service_whitelist(service_id): + return ServiceWhitelist.query().filter(ServiceWhitelist.service_id == service_id).all() + + +def dao_add_whitelisted_contact(obj): + db.session.add(obj) diff --git a/app/service/rest.py b/app/service/rest.py index a6adebdd2..7dce1e19a 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -26,6 +26,10 @@ from app.dao.services_dao import ( dao_fetch_weekly_historical_stats_for_service, dao_fetch_todays_stats_for_all_services ) +from app.dao.service_whitelist_dao import ( + dao_fetch_service_whitelist, + dao_add_whitelisted_contact +) from app.dao import notifications_dao from app.dao.provider_statistics_dao import get_fragment_count from app.dao.users_dao import get_model_users @@ -45,6 +49,7 @@ from app.errors import ( ) from app.service import statistics + service_blueprint = Blueprint('service', __name__) register_errors(service_blueprint) @@ -266,3 +271,27 @@ def get_detailed_services(): service.statistics = statistics.create_zeroed_stats_dicts() return detailed_service_schema.dump(services.values(), many=True).data + + +@service_blueprint.route('//whitelist', methods=['GET']) +def get_whitelist(service_id): + whitelist = dao_fetch_service_whitelist(service_id) + + return { + 'emails': [ + {'id': item.id, 'email_address': item.email_address} + for item in whitelist if item.email_address is not None + ], + 'mobile_numbers': [ + {'id': item.id, 'mobile_number': item.mobile_number} + for item in whitelist if item.mobile_number is not None + ] + } + + +@service_blueprint.route('//whitelist', methods=['POST']) +def update_whitelist(service_id): + # todo: make this transactional + dao_remove_service_whitelist(service_id) + for contact in request.get_json(): + dao_add_whitelisted_contact(ServiceWhitelist.from_string(contact)) From 5c5ca266303ae7c87cbf4f70c3adddd6ca896f7c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 22 Sep 2016 11:56:26 +0100 Subject: [PATCH 03/19] Add ServiceWhiteList model with corresponding tests --- app/models.py | 24 +++++++++++++++++++++++- tests/app/test_model.py | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/app/models.py b/app/models.py index fac1458cb..bf6bf2394 100644 --- a/app/models.py +++ b/app/models.py @@ -1,11 +1,18 @@ import uuid import datetime + from sqlalchemy.dialects.postgresql import ( UUID, JSON ) -from sqlalchemy import UniqueConstraint, text, ForeignKeyConstraint, and_ +from sqlalchemy import UniqueConstraint, and_ from sqlalchemy.orm import foreign, remote +from notifications_utils.recipients import ( + validate_email_address, + validate_phone_number, + InvalidPhoneError, + InvalidEmailError +) from app.encryption import ( hashpw, @@ -142,6 +149,20 @@ class ServiceWhitelist(db.Model): mobile_number = db.Column(db.String, nullable=True) created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow) + @classmethod + def from_string(cls, service_id, contact): + instance = cls(service_id=service_id) + try: + validate_email_address(contact) + instance.email_address = contact + except InvalidEmailError: + try: + validate_phone_number(contact) + instance.mobile_number = contact + except InvalidPhoneError: + raise ValueError("Invalid contact: {}".format(contact)) + + return instance class ApiKey(db.Model, Versioned): __tablename__ = 'api_keys' @@ -307,6 +328,7 @@ class ProviderDetails(db.Model): priority = db.Column(db.Integer, nullable=False) notification_type = db.Column(notification_types, nullable=False) active = db.Column(db.Boolean, default=False) + blah = db.Column JOB_STATUS_PENDING = 'pending' diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 04385a6b6..278d619bc 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -1,7 +1,9 @@ from datetime import datetime +import pytest + from app import DATETIME_FORMAT -from app.models import Notification +from app.models import Notification, ServiceWhitelist def test_should_build_notification_from_minimal_set_of_api_derived_params(notify_api): @@ -70,3 +72,33 @@ def test_should_build_notification_from_full_set_of_api_derived_params(notify_ap assert notification.notification_type == 'SMS' assert notification.api_key_id == 'api_key_id' assert notification.key_type == 'key_type' + + +@pytest.mark.parametrize('mobile_number', [ + '07700 900678', + '+44 7700 900678' +]) +def test_should_build_service_whitelist_from_mobile_number(mobile_number): + service_whitelist = ServiceWhitelist.from_string('service_id', mobile_number) + + assert service_whitelist.mobile_number == mobile_number + assert service_whitelist.email_address is None + +@pytest.mark.parametrize('email_address', [ + 'test@example.com' +]) +def test_should_build_service_whitelist_from_email_address(email_address): + service_whitelist = ServiceWhitelist.from_string('service_id', email_address) + + assert service_whitelist.email_address == email_address + assert service_whitelist.mobile_number is None + + +@pytest.mark.parametrize('contact', [ + '', + '07700dsadsad', + 'gmail.com' +]) +def test_should_not_build_service_whitelist_from_invalid_contact(contact): + with pytest.raises(ValueError): + ServiceWhitelist.from_string('service_id', contact) From af0dbd14be880bea4caefd520a9e57e0ee91e895 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 22 Sep 2016 17:17:34 +0100 Subject: [PATCH 04/19] correct service_whitelist dao/rest functionality additionally added dao tests --- app/dao/service_whitelist_dao.py | 14 +++++++----- app/service/rest.py | 19 +++++++--------- tests/app/conftest.py | 16 +++++++++++++ tests/app/dao/test_service_whitelist_dao.py | 25 +++++++++++++++++++++ 4 files changed, 58 insertions(+), 16 deletions(-) create mode 100644 tests/app/dao/test_service_whitelist_dao.py diff --git a/app/dao/service_whitelist_dao.py b/app/dao/service_whitelist_dao.py index 359ad0bdc..4407cbe08 100644 --- a/app/dao/service_whitelist_dao.py +++ b/app/dao/service_whitelist_dao.py @@ -1,11 +1,15 @@ -from sqlalchemy import or_ - from app import db from app.models import ServiceWhitelist + def dao_fetch_service_whitelist(service_id): - return ServiceWhitelist.query().filter(ServiceWhitelist.service_id == service_id).all() + return ServiceWhitelist.query.filter(ServiceWhitelist.service_id == service_id).all() -def dao_add_whitelisted_contact(obj): - db.session.add(obj) +def dao_add_and_commit_whitelisted_contacts(objs): + db.session.add_all(objs) + db.session.commit() + + +def dao_remove_service_whitelist(service_id): + return ServiceWhitelist.query.filter(ServiceWhitelist.service_id == service_id).delete() diff --git a/app/service/rest.py b/app/service/rest.py index 7dce1e19a..e475839a2 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -28,7 +28,8 @@ from app.dao.services_dao import ( ) from app.dao.service_whitelist_dao import ( dao_fetch_service_whitelist, - dao_add_whitelisted_contact + dao_add_and_commit_whitelisted_contacts, + dao_remove_service_whitelist ) from app.dao import notifications_dao from app.dao.provider_statistics_dao import get_fragment_count @@ -278,14 +279,8 @@ def get_whitelist(service_id): whitelist = dao_fetch_service_whitelist(service_id) return { - 'emails': [ - {'id': item.id, 'email_address': item.email_address} - for item in whitelist if item.email_address is not None - ], - 'mobile_numbers': [ - {'id': item.id, 'mobile_number': item.mobile_number} - for item in whitelist if item.mobile_number is not None - ] + 'emails': [item.email_address for item in whitelist if item.email_address is not None], + 'mobile_numbers': [item.mobile_number for item in whitelist if item.mobile_number is not None] } @@ -293,5 +288,7 @@ def get_whitelist(service_id): def update_whitelist(service_id): # todo: make this transactional dao_remove_service_whitelist(service_id) - for contact in request.get_json(): - dao_add_whitelisted_contact(ServiceWhitelist.from_string(contact)) + + whitelist_objs = [ServiceWhitelist.from_string(service_id, contact) for contact in request.get_json()] + + dao_add_and_commit_whitelisted_contacts(whitelist_objs) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d16adcc87..0f9c66af7 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -19,6 +19,7 @@ from app.models import ( ProviderStatistics, ProviderDetails, NotificationStatistics, + ServiceWhitelist, KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM) from app.dao.users_dao import (save_model_user, create_user_code, create_secret_code) from app.dao.services_dao import (dao_create_service, dao_add_user_to_service) @@ -862,3 +863,18 @@ def already_registered_template(notify_db, template = Template(**data) db.session.add(template) return template + + +@pytest.fixture(scope='function') +def sample_service_whitelist(notify_db, notify_db_session, service=None, email_address=None, mobile_number=None): + if service is None: + service = sample_service(notify_db, notify_db_session) + + if not email_address and not mobile_number: + email_address = 'whitelisted_user@digital.gov.uk' + + whitelisted_user = ServiceWhitelist.from_string(service.id, email_address or mobile_number) + + notify_db.session.add(whitelisted_user) + notify_db.session.commit() + return whitelisted_user diff --git a/tests/app/dao/test_service_whitelist_dao.py b/tests/app/dao/test_service_whitelist_dao.py new file mode 100644 index 000000000..d399a9d4f --- /dev/null +++ b/tests/app/dao/test_service_whitelist_dao.py @@ -0,0 +1,25 @@ +import uuid + +from app.models import ServiceWhitelist +from app.dao.service_whitelist_dao import (# + dao_fetch_service_whitelist, + dao_add_and_commit_whitelisted_contacts, + dao_remove_service_whitelist +) + + +def test_fetch_service_whitelist_gets_whitelists(sample_service_whitelist): + whitelist = dao_fetch_service_whitelist(sample_service_whitelist.service_id) + assert len(whitelist) == 1 + assert whitelist[0].id == sample_service_whitelist.id + +def test_fetch_service_whitelist_ignores_other_service(sample_service_whitelist): + assert len(dao_fetch_service_whitelist(uuid.uuid4())) == 0 + +def test_add_and_commit_whitelisted_contacts_saves_data(sample_service): + whitelist = ServiceWhitelist.from_string(sample_service.id, 'foo@example.com') + dao_add_and_commit_whitelisted_contacts([whitelist]) + + db_contents = ServiceWhitelist.query.all() + assert len(db_contents) == 1 + assert db_contents[0].id == whitelist.id From 0b8c385de1706df258f864f3313d142d808d800e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 23 Sep 2016 10:45:12 +0100 Subject: [PATCH 05/19] add remove_whitelist tests --- app/service/rest.py | 1 - tests/app/conftest.py | 4 ++- tests/app/dao/test_service_whitelist_dao.py | 27 ++++++++++++++++++++- 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 25577d7bb..9679110b9 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -287,7 +287,6 @@ def get_whitelist(service_id): @service_blueprint.route('//whitelist', methods=['POST']) def update_whitelist(service_id): - # todo: make this transactional dao_remove_service_whitelist(service_id) whitelist_objs = [ServiceWhitelist.from_string(service_id, contact) for contact in request.get_json()] diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 0f9c66af7..43ac08458 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -130,9 +130,11 @@ def sample_service(notify_db, user=None, restricted=False, limit=1000, - email_from="sample.service"): + email_from=None): if user is None: user = sample_user(notify_db, notify_db_session) + if email_from is None: + email_from = service_name.lower().replace(' ', '.') data = { 'name': service_name, 'message_limit': limit, diff --git a/tests/app/dao/test_service_whitelist_dao.py b/tests/app/dao/test_service_whitelist_dao.py index 8b429d249..2fdd7ae6f 100644 --- a/tests/app/dao/test_service_whitelist_dao.py +++ b/tests/app/dao/test_service_whitelist_dao.py @@ -2,12 +2,13 @@ import uuid from app.models import ServiceWhitelist from app.dao.service_whitelist_dao import ( - dao_fetch_service_whitelist, dao_add_and_commit_whitelisted_contacts, dao_remove_service_whitelist ) +from tests.app.conftest import sample_service as create_service + def test_fetch_service_whitelist_gets_whitelists(sample_service_whitelist): whitelist = dao_fetch_service_whitelist(sample_service_whitelist.service_id) @@ -21,8 +22,32 @@ def test_fetch_service_whitelist_ignores_other_service(sample_service_whitelist) def test_add_and_commit_whitelisted_contacts_saves_data(sample_service): whitelist = ServiceWhitelist.from_string(sample_service.id, 'foo@example.com') + dao_add_and_commit_whitelisted_contacts([whitelist]) db_contents = ServiceWhitelist.query.all() assert len(db_contents) == 1 assert db_contents[0].id == whitelist.id + + +def test_remove_service_whitelist_only_removes_for_my_service(notify_db, notify_db_session): + service_1 = create_service(notify_db, notify_db_session, service_name="service 1") + service_2 = create_service(notify_db, notify_db_session, service_name="service 2") + dao_add_and_commit_whitelisted_contacts([ + ServiceWhitelist.from_string(service_1.id, 'service1@example.com'), + ServiceWhitelist.from_string(service_2.id, 'service2@example.com') + ]) + + dao_remove_service_whitelist(service_1.id) + + assert service_1.whitelist == [] + assert len(service_2.whitelist) == 1 + + +def test_remove_service_whitelist_does_not_commit(notify_db, sample_service_whitelist): + dao_remove_service_whitelist(sample_service_whitelist.service_id) + + # since dao_remove_service_whitelist doesn't commit, we can still rollback its changes + notify_db.session.rollback() + + assert ServiceWhitelist.query.count() == 1 From c475bd03ced8a1188bd4226345c5929c68d587e6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 23 Sep 2016 12:21:00 +0100 Subject: [PATCH 06/19] improved service whitelist endpoints * changed POST to PUT - we are modifiying an already present resource * improved error handling on PUT - return 400 if bad - rollback the delete of the previous whitelist on error * return 204 if PUT succeeds ( NO CONTENT ) --- app/dao/dao_utils.py | 5 +++++ app/models.py | 2 +- app/service/rest.py | 26 ++++++++++++++++++-------- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py index d5ec27fb5..bdaafc074 100644 --- a/app/dao/dao_utils.py +++ b/app/dao/dao_utils.py @@ -1,6 +1,7 @@ import itertools from functools import wraps, partial +from app import db from app.history_meta import create_history @@ -35,3 +36,7 @@ def version_class(model_class, history_cls=None): db.session.add(h_obj) return record_version return versioned + + +def dao_rollback(): + db.session.rollback() diff --git a/app/models.py b/app/models.py index b604a1155..105b4e93b 100644 --- a/app/models.py +++ b/app/models.py @@ -160,7 +160,7 @@ class ServiceWhitelist(db.Model): validate_phone_number(contact) instance.mobile_number = contact except InvalidPhoneError: - raise ValueError("Invalid contact: {}".format(contact)) + raise ValueError('Invalid whitelist: "{}"'.format(contact)) return instance diff --git a/app/service/rest.py b/app/service/rest.py index 9679110b9..192787de6 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -8,6 +8,7 @@ from flask import ( ) from sqlalchemy.orm.exc import NoResultFound +from app.dao.dao_utils import dao_rollback from app.dao.api_key_dao import ( save_model_api_key, get_model_api_keys, @@ -49,6 +50,7 @@ from app.errors import ( InvalidRequest ) from app.service import statistics +from app.models import ServiceWhitelist service_blueprint = Blueprint('service', __name__) @@ -279,16 +281,24 @@ def get_detailed_services(): def get_whitelist(service_id): whitelist = dao_fetch_service_whitelist(service_id) - return { - 'emails': [item.email_address for item in whitelist if item.email_address is not None], - 'mobile_numbers': [item.mobile_number for item in whitelist if item.mobile_number is not None] - } + return jsonify( + email_addresses=[item.email_address for item in whitelist if item.email_address is not None], + mobile_numbers=[item.mobile_number for item in whitelist if item.mobile_number is not None] + ) -@service_blueprint.route('//whitelist', methods=['POST']) +@service_blueprint.route('//whitelist', methods=['PUT']) def update_whitelist(service_id): + # doesn't commit so if there are any errors, we preserve old values in db dao_remove_service_whitelist(service_id) - whitelist_objs = [ServiceWhitelist.from_string(service_id, contact) for contact in request.get_json()] - - dao_add_and_commit_whitelisted_contacts(whitelist_objs) + try: + whitelist_objs = [ServiceWhitelist.from_string(service_id, contact) for contact in request.get_json()] + except ValueError as e: + current_app.logger.exception(e) + dao_rollback() + msg = '{} is not a valid email address or phone number'.format(str(e)) + return jsonify(result='error', message=msg), 400 + else: + dao_add_and_commit_whitelisted_contacts(whitelist_objs) + return '', 204 From eedc1f2093a6e8b5fb2593f5925dad227ee4c1f2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 23 Sep 2016 12:24:53 +0100 Subject: [PATCH 07/19] tests for service whitelist rest --- tests/app/service/test_service_whitelist.py | 87 +++++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 tests/app/service/test_service_whitelist.py diff --git a/tests/app/service/test_service_whitelist.py b/tests/app/service/test_service_whitelist.py new file mode 100644 index 000000000..e7a7ef91d --- /dev/null +++ b/tests/app/service/test_service_whitelist.py @@ -0,0 +1,87 @@ +import uuid +import json + +from tests import create_authorization_header + +from app.models import ServiceWhitelist +from app.dao.service_whitelist_dao import dao_add_and_commit_whitelisted_contacts + + +def test_get_whitelist_returns_data(client, sample_service_whitelist): + service_id = sample_service_whitelist.service_id + + response = client.get('service/{}/whitelist'.format(service_id), headers=[create_authorization_header()]) + + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == { + 'email_addresses': [sample_service_whitelist.email_address], + 'mobile_numbers': [] + } + + +def test_get_whitelist_separates_emails_and_phones(client, sample_service): + dao_add_and_commit_whitelisted_contacts([ + ServiceWhitelist.from_string(sample_service.id, 'service@example.com'), + ServiceWhitelist.from_string(sample_service.id, '07123456789') + ]) + + response = client.get('service/{}/whitelist'.format(sample_service.id), headers=[create_authorization_header()]) + + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == { + 'email_addresses': ['service@example.com'], + 'mobile_numbers': ['07123456789'] + } + + +def test_get_whitelist_404s_with_unknown_service_id(client): + path = 'service/{}/api-keys'.format(uuid.uuid4()) + + response = client.get(path, headers=[create_authorization_header()]) + + assert response.status_code == 404 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'No result found' + + +def test_get_whitelist_returns_no_data(client, sample_service): + path = 'service/{}/whitelist'.format(sample_service.id) + + response = client.get(path, headers=[create_authorization_header()]) + + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == {'email_addresses': [], 'mobile_numbers': []} + + +def test_update_whitelist_replaces_old_whitelist(client, sample_service_whitelist): + data = ['foo@bar.com', '07123456789'] + + response = client.put( + 'service/{}/whitelist'.format(sample_service_whitelist.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + + assert response.status_code == 204 + whitelist = ServiceWhitelist.query.order_by(ServiceWhitelist.email_address).all() + assert len(whitelist) == 2 + assert whitelist[0].email_address == 'foo@bar.com' + assert whitelist[1].mobile_number == '07123456789' + + +def test_update_whitelist_doesnt_remove_old_whitelist_if_error(client, sample_service_whitelist): + data = [''] + response = client.put( + 'service/{}/whitelist'.format(sample_service_whitelist.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + + assert response.status_code == 400 + assert json.loads(response.get_data(as_text=True)) == { + 'result': 'error', + 'message': 'Invalid whitelist: "" is not a valid email address or phone number' + } + whitelist = ServiceWhitelist.query.one() + assert whitelist.id == sample_service_whitelist.id From 005e8a9c3a666c340dc9ae002c42d2f4bd392663 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 27 Sep 2016 13:43:42 +0100 Subject: [PATCH 08/19] Change data model to use single recipient address and type columns --- migrations/versions/0055_service_whitelist.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/migrations/versions/0055_service_whitelist.py b/migrations/versions/0055_service_whitelist.py index b2457833f..812cda7ba 100644 --- a/migrations/versions/0055_service_whitelist.py +++ b/migrations/versions/0055_service_whitelist.py @@ -18,8 +18,8 @@ def upgrade(): op.create_table('service_whitelist', sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=False), - sa.Column('email_address', sa.String(length=255), nullable=True), - sa.Column('mobile_number', sa.String(), nullable=True), + sa.Column('recipient_type', sa.Enum('mobile', 'email', name='recipient_type'), nullable=False), + sa.Column('recipient', sa.String(length=255), nullable=True), sa.Column('created_at', sa.DateTime(), nullable=True), sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), sa.PrimaryKeyConstraint('id') From 8184eff15abf553b06403cd6de11fccdc6a8126f Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 27 Sep 2016 13:44:29 +0100 Subject: [PATCH 09/19] Update ServiceWhitelist to conform to new data model --- app/models.py | 43 +++++++++++++++++++++++++++-------------- tests/app/test_model.py | 27 +++++++++++++------------- 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/app/models.py b/app/models.py index 105b4e93b..fb530ea3b 100644 --- a/app/models.py +++ b/app/models.py @@ -138,6 +138,11 @@ class Service(db.Model, Versioned): default=BRANDING_GOVUK ) +MOBILE_TYPE = 'mobile' +EMAIL_TYPE = 'email' + +WHITELIST_RECIPIENT_TYPE = [MOBILE_TYPE, EMAIL_TYPE] +whitelist_recipient_types = db.Enum(*WHITELIST_RECIPIENT_TYPE, name='recipient_type') class ServiceWhitelist(db.Model): __tablename__ = 'service_whitelist' @@ -145,24 +150,33 @@ class ServiceWhitelist(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False) service = db.relationship('Service', backref='whitelist') - email_address = db.Column(db.String(255), nullable=True) - mobile_number = db.Column(db.String, nullable=True) + recipient_type = db.Column(whitelist_recipient_types, nullable=False) + recipient = db.Column(db.String(255), nullable=False) created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow) @classmethod - def from_string(cls, service_id, contact): - instance = cls(service_id=service_id) - try: - validate_email_address(contact) - instance.email_address = contact - except InvalidEmailError: - try: - validate_phone_number(contact) - instance.mobile_number = contact - except InvalidPhoneError: - raise ValueError('Invalid whitelist: "{}"'.format(contact)) + def from_string(cls, service_id, recipient_type, recipient): + instance = cls(service_id=service_id, recipient_type=recipient_type) - return instance + try: + if recipient_type == MOBILE_TYPE: + validate_phone_number(recipient) + instance.recipient = recipient + elif recipient_type == EMAIL_TYPE: + validate_email_address(recipient) + instance.recipient = recipient + else: + raise ValueError('Invalid recipient type') + except InvalidPhoneError: + raise ValueError('Invalid whitelist: "{}"'.format(recipient)) + except InvalidEmailError: + raise ValueError('Invalid whitelist: "{}"'.format(recipient)) + else: + return instance + + def __repr__(self): + return 'Recipient {} of type: {}'.format(self.recipient, + self.recipient_type) class ApiKey(db.Model, Versioned): @@ -329,7 +343,6 @@ class ProviderDetails(db.Model): priority = db.Column(db.Integer, nullable=False) notification_type = db.Column(notification_types, nullable=False) active = db.Column(db.Boolean, default=False) - blah = db.Column JOB_STATUS_PENDING = 'pending' diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 13bc06b43..78827235a 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -3,7 +3,10 @@ from datetime import datetime import pytest from app import DATETIME_FORMAT -from app.models import Notification, ServiceWhitelist +from app.models import ( + Notification, + ServiceWhitelist, + MOBILE_TYPE, EMAIL_TYPE) def test_should_build_notification_from_minimal_set_of_api_derived_params(notify_api): @@ -79,27 +82,25 @@ def test_should_build_notification_from_full_set_of_api_derived_params(notify_ap '+44 7700 900678' ]) def test_should_build_service_whitelist_from_mobile_number(mobile_number): - service_whitelist = ServiceWhitelist.from_string('service_id', mobile_number) + service_whitelist = ServiceWhitelist.from_string('service_id', MOBILE_TYPE, mobile_number) - assert service_whitelist.mobile_number == mobile_number - assert service_whitelist.email_address is None + assert service_whitelist.recipient == mobile_number @pytest.mark.parametrize('email_address', [ 'test@example.com' ]) def test_should_build_service_whitelist_from_email_address(email_address): - service_whitelist = ServiceWhitelist.from_string('service_id', email_address) + service_whitelist = ServiceWhitelist.from_string('service_id', EMAIL_TYPE, email_address) - assert service_whitelist.email_address == email_address - assert service_whitelist.mobile_number is None + assert service_whitelist.recipient == email_address -@pytest.mark.parametrize('contact', [ - '', - '07700dsadsad', - 'gmail.com' +@pytest.mark.parametrize('contact, recipient_type', [ + ('', None), + ('07700dsadsad', MOBILE_TYPE), + ('gmail.com', EMAIL_TYPE) ]) -def test_should_not_build_service_whitelist_from_invalid_contact(contact): +def test_should_not_build_service_whitelist_from_invalid_contact(recipient_type, contact): with pytest.raises(ValueError): - ServiceWhitelist.from_string('service_id', contact) + ServiceWhitelist.from_string('service_id', recipient_type, contact) From fbf266be87c41604aea5b5930fad01aaeca01463 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 27 Sep 2016 13:45:36 +0100 Subject: [PATCH 10/19] Update ServiceWhitelist dao to conform to new data model --- app/dao/service_whitelist_dao.py | 8 +++++--- tests/app/dao/test_service_whitelist_dao.py | 11 +++++++---- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/app/dao/service_whitelist_dao.py b/app/dao/service_whitelist_dao.py index 4407cbe08..b6874d8de 100644 --- a/app/dao/service_whitelist_dao.py +++ b/app/dao/service_whitelist_dao.py @@ -1,9 +1,10 @@ from app import db -from app.models import ServiceWhitelist +from app.models import Service, ServiceWhitelist def dao_fetch_service_whitelist(service_id): - return ServiceWhitelist.query.filter(ServiceWhitelist.service_id == service_id).all() + return ServiceWhitelist.query.filter( + ServiceWhitelist.service_id == service_id).all() def dao_add_and_commit_whitelisted_contacts(objs): @@ -12,4 +13,5 @@ def dao_add_and_commit_whitelisted_contacts(objs): def dao_remove_service_whitelist(service_id): - return ServiceWhitelist.query.filter(ServiceWhitelist.service_id == service_id).delete() + return ServiceWhitelist.query.filter( + ServiceWhitelist.service_id == service_id).delete() diff --git a/tests/app/dao/test_service_whitelist_dao.py b/tests/app/dao/test_service_whitelist_dao.py index 2fdd7ae6f..49a225136 100644 --- a/tests/app/dao/test_service_whitelist_dao.py +++ b/tests/app/dao/test_service_whitelist_dao.py @@ -1,6 +1,9 @@ import uuid -from app.models import ServiceWhitelist +from app.models import ( + ServiceWhitelist, + MOBILE_TYPE, EMAIL_TYPE) + from app.dao.service_whitelist_dao import ( dao_fetch_service_whitelist, dao_add_and_commit_whitelisted_contacts, @@ -21,7 +24,7 @@ def test_fetch_service_whitelist_ignores_other_service(sample_service_whitelist) def test_add_and_commit_whitelisted_contacts_saves_data(sample_service): - whitelist = ServiceWhitelist.from_string(sample_service.id, 'foo@example.com') + whitelist = ServiceWhitelist.from_string(sample_service.id, EMAIL_TYPE, 'foo@example.com') dao_add_and_commit_whitelisted_contacts([whitelist]) @@ -34,8 +37,8 @@ def test_remove_service_whitelist_only_removes_for_my_service(notify_db, notify_ service_1 = create_service(notify_db, notify_db_session, service_name="service 1") service_2 = create_service(notify_db, notify_db_session, service_name="service 2") dao_add_and_commit_whitelisted_contacts([ - ServiceWhitelist.from_string(service_1.id, 'service1@example.com'), - ServiceWhitelist.from_string(service_2.id, 'service2@example.com') + ServiceWhitelist.from_string(service_1.id, EMAIL_TYPE, 'service1@example.com'), + ServiceWhitelist.from_string(service_2.id, EMAIL_TYPE, 'service2@example.com') ]) dao_remove_service_whitelist(service_1.id) From f29c6c0bb217eb27c8ff5292f6189e162645eea1 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 27 Sep 2016 13:47:53 +0100 Subject: [PATCH 11/19] Update to return 404 if invalid service id --- app/service/rest.py | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 192787de6..966af79f1 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -50,7 +50,9 @@ from app.errors import ( InvalidRequest ) from app.service import statistics -from app.models import ServiceWhitelist +from app.models import ( + ServiceWhitelist, + MOBILE_TYPE, EMAIL_TYPE) service_blueprint = Blueprint('service', __name__) @@ -279,11 +281,17 @@ def get_detailed_services(): @service_blueprint.route('//whitelist', methods=['GET']) def get_whitelist(service_id): - whitelist = dao_fetch_service_whitelist(service_id) + service = dao_fetch_service_by_id(service_id) + if not service: + raise InvalidRequest("Service does not exist", status_code=404) + + whitelist = dao_fetch_service_whitelist(service.id) return jsonify( - email_addresses=[item.email_address for item in whitelist if item.email_address is not None], - mobile_numbers=[item.mobile_number for item in whitelist if item.mobile_number is not None] + email_addresses=[item.recipient for item in whitelist + if item.recipient_type == EMAIL_TYPE], + phone_numbers=[item.recipient for item in whitelist + if item.recipient_type == MOBILE_TYPE] ) @@ -291,9 +299,12 @@ def get_whitelist(service_id): def update_whitelist(service_id): # doesn't commit so if there are any errors, we preserve old values in db dao_remove_service_whitelist(service_id) - try: - whitelist_objs = [ServiceWhitelist.from_string(service_id, contact) for contact in request.get_json()] + whitelist_objs = itertools.chain( + [ServiceWhitelist.from_string(service_id, MOBILE_TYPE, recipient) + for recipient in request.get_json().get('phone_numbers')], + [ServiceWhitelist.from_string(service_id, EMAIL_TYPE, recipient) + for recipient in request.get_json().get('email_addresses')]) except ValueError as e: current_app.logger.exception(e) dao_rollback() From 7b0cbca89b1c0f9a6a7cec3a902947df2d25f164 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 27 Sep 2016 13:48:51 +0100 Subject: [PATCH 12/19] Enable sending to whitelist users if using a normal api key --- app/notifications/rest.py | 6 +- .../rest/test_send_notification.py | 239 +++++++++++++++++- 2 files changed, 242 insertions(+), 3 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index d47f9f27c..b5dad7427 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -252,11 +252,13 @@ def send_notification(notification_type): service.restricted or api_user.key_type == KEY_TYPE_TEAM, not allowed_to_send_to( notification['to'], - itertools.chain.from_iterable( - [user.mobile_number, user.email_address] for user in service.users + itertools.chain( + itertools.chain.from_iterable([user.mobile_number, user.email_address] for user in service.users), + ([member.recipient for member in service.whitelist]) if api_user.key_type == KEY_TYPE_NORMAL else iter([]) ) ) )): + if (api_user.key_type == KEY_TYPE_TEAM): message = 'Can’t send to this recipient using a team-only API key' else: diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 0b5e5eaf0..f6a0145de 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -21,7 +21,9 @@ from tests.app.conftest import ( sample_notification as create_sample_notification, sample_service as create_sample_service, sample_email_template as create_sample_email_template, - sample_template as create_sample_template + sample_template as create_sample_template, + sample_service_whitelist as create_sample_service_whitelist, + sample_api_key as create_sample_api_key ) @@ -999,3 +1001,238 @@ def test_should_not_persist_notification_or_send_sms_if_simulated_number( assert response.status_code == 201 apply_async.assert_not_called() assert Notification.query.count() == 0 + + +@pytest.mark.parametrize('to_sms', ['07827992635']) +def test_should_not_send_sms_to_non_whitelist_recipient_in_trial_mode_with_live_key( + client, + notify_db, + notify_db_session, + to_sms, + mocker): + apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') + service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service) + sms_template = create_sample_template(notify_db, notify_db_session, service=service) + + assert service_whitelist.service_id == service.id + assert to_sms not in [member.recipient for member in service.whitelist] + + create_sample_notification(notify_db, notify_db_session, template=sms_template, service=service) + + data = { + 'to': to_sms, + 'template': str(sms_template.id) + } + + live_key = create_sample_api_key(notify_db, notify_db_session, service) + auth_header = create_jwt_token(secret=live_key.unsigned_secret, client_id=str(live_key.service_id)) + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + + expected_response_message = ( + 'Can’t send to this recipient when service is in trial mode ' + '– see https://www.notifications.service.gov.uk/trial-mode' + ) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert expected_response_message in json_resp['message']['to'] + apply_async.assert_not_called() + +@pytest.mark.parametrize('to_email', ['non_whitelist_recipient@mail.com']) +def test_should_not_send_email_to_non_whitelist_recipient_in_trial_mode_with_live_key( + client, + notify_db, + notify_db_session, + to_email, + mocker): + apply_async = mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') + service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service) + email_template = create_sample_email_template(notify_db, notify_db_session, service=service) + + assert service_whitelist.service_id == service.id + assert to_email not in [member.recipient for member in service.whitelist] + + create_sample_notification(notify_db, notify_db_session, template=email_template, service=service) + + data = { + 'to': to_email, + 'template': str(email_template.id) + } + + live_key = create_sample_api_key(notify_db, notify_db_session, service) + auth_header = create_jwt_token(secret=live_key.unsigned_secret, client_id=str(live_key.service_id)) + + response = client.post( + path='/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + + expected_response_message = ( + 'Can’t send to this recipient when service is in trial mode ' + '– see https://www.notifications.service.gov.uk/trial-mode' + ) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert expected_response_message in json_resp['message']['to'] + apply_async.assert_not_called() + + +@pytest.mark.parametrize('to_sms', ['07827992635']) +def test_should_not_send_sms_to_whitelist_recipient_in_trial_mode_with_team_key( + client, + notify_db, + notify_db_session, + to_sms, + mocker): + apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') + service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service, mobile_number=to_sms) + sms_template = create_sample_template(notify_db, notify_db_session, service=service) + + assert service_whitelist.service_id == service.id + assert to_sms in [member.recipient for member in service.whitelist] + + create_sample_notification(notify_db, notify_db_session, template=sms_template, service=service) + + data = { + 'to': to_sms, + 'template': str(sms_template.id) + } + + team_key = create_sample_api_key(notify_db, notify_db_session, service, key_type=KEY_TYPE_TEAM) + auth_header = create_jwt_token(secret=team_key.unsigned_secret, client_id=str(team_key.service_id)) + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert 'Can’t send to this recipient using a team-only API key' in json_resp['message']['to'] + apply_async.assert_not_called() + + +@pytest.mark.parametrize('to_email', ['non_whitelist_recipient@mail.com']) +def test_should_not_send_email_to_whitelist_recipient_in_trial_mode_with_team_key( + client, + notify_db, + notify_db_session, + to_email, + mocker): + apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') + service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service, email_address=to_email) + email_template = create_sample_email_template(notify_db, notify_db_session, service=service) + + assert service_whitelist.service_id == service.id + assert to_email in [member.recipient for member in service.whitelist] + + create_sample_notification(notify_db, notify_db_session, template=email_template, service=service) + + data = { + 'to': to_email, + 'template': str(email_template.id) + } + + team_key = create_sample_api_key(notify_db, notify_db_session, service, key_type=KEY_TYPE_TEAM) + auth_header = create_jwt_token(secret=team_key.unsigned_secret, client_id=str(team_key.service_id)) + + response = client.post( + path='/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert 'Can’t send to this recipient using a team-only API key' in json_resp['message']['to'] + apply_async.assert_not_called() + + +@pytest.mark.parametrize('to_sms', ['07123123123']) +def test_should_send_sms_to_whitelist_recipient_in_trial_mode_with_live_key( + client, + notify_db, + notify_db_session, + to_sms, + mocker): + apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') + + service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service, mobile_number=to_sms) + sms_template = create_sample_template(notify_db, notify_db_session, service=service) + + assert service_whitelist.service_id == service.id + assert to_sms in [member.recipient for member in service.whitelist] + + create_sample_notification(notify_db, notify_db_session, template=sms_template, service=service) + + data = { + 'to': to_sms, + 'template': str(sms_template.id) + } + + sample_live_key = create_sample_api_key(notify_db, notify_db_session, service) + auth_header = create_jwt_token(secret=sample_live_key.unsigned_secret, client_id=str(sample_live_key.service_id)) + + response = client.post( + path='/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 201 + assert json_resp['data']['notification']['id'] + assert json_resp['data']['body'] == sms_template.content + assert json_resp['data']['template_version'] == sms_template.version + apply_async.called + + +@pytest.mark.parametrize('to_email', ['whitelist_recipient@mail.com']) +def test_should_send_email_to_whitelist_recipient_in_trial_mode_with_live_key( + client, + notify_db, + notify_db_session, + to_email, + mocker): + apply_async = mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') + + service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service, email_address=to_email) + email_template = create_sample_email_template(notify_db, notify_db_session, service=service) + + assert service_whitelist.service_id == service.id + assert to_email in [member.recipient for member in service.whitelist] + + create_sample_notification(notify_db, notify_db_session, template=email_template, service=service) + + data = { + 'to': to_email, + 'template': str(email_template.id) + } + + sample_live_key = create_sample_api_key(notify_db, notify_db_session, service) + auth_header = create_jwt_token(secret=sample_live_key.unsigned_secret, client_id=str(sample_live_key.service_id)) + + response = client.post( + path='/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 201 + assert json_resp['data']['notification']['id'] + assert json_resp['data']['body'] == email_template.content + assert json_resp['data']['template_version'] == email_template.version + apply_async.called From 3b592c861960564535c3ffba323129d92b114bcb Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 27 Sep 2016 13:50:23 +0100 Subject: [PATCH 13/19] Update creating a sample service whitelist based on new data model --- tests/app/conftest.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 43ac08458..217ea4df2 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -20,7 +20,8 @@ from app.models import ( ProviderDetails, NotificationStatistics, ServiceWhitelist, - KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM) + KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM, + MOBILE_TYPE, EMAIL_TYPE) from app.dao.users_dao import (save_model_user, create_user_code, create_secret_code) from app.dao.services_dao import (dao_create_service, dao_add_user_to_service) from app.dao.templates_dao import dao_create_template @@ -872,10 +873,12 @@ def sample_service_whitelist(notify_db, notify_db_session, service=None, email_a if service is None: service = sample_service(notify_db, notify_db_session) - if not email_address and not mobile_number: - email_address = 'whitelisted_user@digital.gov.uk' - - whitelisted_user = ServiceWhitelist.from_string(service.id, email_address or mobile_number) + if email_address: + whitelisted_user = ServiceWhitelist.from_string(service.id, EMAIL_TYPE, email_address) + elif mobile_number: + whitelisted_user = ServiceWhitelist.from_string(service.id, MOBILE_TYPE, mobile_number) + else: + whitelisted_user = ServiceWhitelist.from_string(service.id, EMAIL_TYPE, 'whitelisted_user@digital.gov.uk') notify_db.session.add(whitelisted_user) notify_db.session.commit() From 99cfa7bae25c80d131b3feee32f84d4a505d7c42 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 27 Sep 2016 13:51:43 +0100 Subject: [PATCH 14/19] Fix url to get from whitelist endpoint --- tests/app/service/test_service_whitelist.py | 96 +++++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 tests/app/service/test_service_whitelist.py diff --git a/tests/app/service/test_service_whitelist.py b/tests/app/service/test_service_whitelist.py new file mode 100644 index 000000000..953a13c52 --- /dev/null +++ b/tests/app/service/test_service_whitelist.py @@ -0,0 +1,96 @@ +import uuid +import json + +from tests import create_authorization_header + +from app.models import ( + ServiceWhitelist, + MOBILE_TYPE, EMAIL_TYPE) + +from app.dao.service_whitelist_dao import dao_add_and_commit_whitelisted_contacts + + +def test_get_whitelist_returns_data(client, sample_service_whitelist): + service_id = sample_service_whitelist.service_id + + response = client.get('service/{}/whitelist'.format(service_id), headers=[create_authorization_header()]) + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == { + 'email_addresses': [sample_service_whitelist.recipient], + 'phone_numbers': [] + } + + +def test_get_whitelist_separates_emails_and_phones(client, sample_service): + dao_add_and_commit_whitelisted_contacts([ + ServiceWhitelist.from_string(sample_service.id, EMAIL_TYPE, 'service@example.com'), + ServiceWhitelist.from_string(sample_service.id, MOBILE_TYPE, '07123456789') + ]) + + response = client.get('service/{}/whitelist'.format(sample_service.id), headers=[create_authorization_header()]) + + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == { + 'email_addresses': ['service@example.com'], + 'phone_numbers': ['07123456789'] + } + + +def test_get_whitelist_404s_with_unknown_service_id(client): + path = 'service/{}/whitelist'.format(uuid.uuid4()) + + response = client.get(path, headers=[create_authorization_header()]) + assert response.status_code == 404 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'No result found' + + +def test_get_whitelist_returns_no_data(client, sample_service): + path = 'service/{}/whitelist'.format(sample_service.id) + + response = client.get(path, headers=[create_authorization_header()]) + + assert response.status_code == 200 + assert json.loads(response.get_data(as_text=True)) == {'email_addresses': [], 'phone_numbers': []} + + +def test_update_whitelist_replaces_old_whitelist(client, sample_service_whitelist): + data = { + 'email_addresses': ['foo@bar.com'], + 'phone_numbers': ['07123456789'] + } + + response = client.put( + 'service/{}/whitelist'.format(sample_service_whitelist.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + + assert response.status_code == 204 + whitelist = ServiceWhitelist.query.all() + assert len(whitelist) == 2 + assert whitelist[0].recipient == '07123456789' + assert whitelist[1].recipient == 'foo@bar.com' + + +def test_update_whitelist_doesnt_remove_old_whitelist_if_error(client, sample_service_whitelist): + + data = { + 'email_addresses': [''], + 'phone_numbers': ['07123456789'] + } + + response = client.put( + 'service/{}/whitelist'.format(sample_service_whitelist.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), create_authorization_header()] + ) + + assert response.status_code == 400 + assert json.loads(response.get_data(as_text=True)) == { + 'result': 'error', + 'message': 'Invalid whitelist: "" is not a valid email address or phone number' + } + whitelist = ServiceWhitelist.query.one() + assert whitelist.id == sample_service_whitelist.id From af2cbaa9c55ab7db93c8578f85b4a1a9d6b52456 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Tue, 27 Sep 2016 14:16:35 +0100 Subject: [PATCH 15/19] Fix PEP8 issues --- app/models.py | 1 + app/notifications/rest.py | 3 +- .../rest/test_send_notification.py | 79 +++++++++---------- tests/app/service/test_service_whitelist.py | 2 + 4 files changed, 44 insertions(+), 41 deletions(-) diff --git a/app/models.py b/app/models.py index fb530ea3b..b39de532d 100644 --- a/app/models.py +++ b/app/models.py @@ -144,6 +144,7 @@ EMAIL_TYPE = 'email' WHITELIST_RECIPIENT_TYPE = [MOBILE_TYPE, EMAIL_TYPE] whitelist_recipient_types = db.Enum(*WHITELIST_RECIPIENT_TYPE, name='recipient_type') + class ServiceWhitelist(db.Model): __tablename__ = 'service_whitelist' diff --git a/app/notifications/rest.py b/app/notifications/rest.py index b5dad7427..269da9bef 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -254,7 +254,8 @@ def send_notification(notification_type): notification['to'], itertools.chain( itertools.chain.from_iterable([user.mobile_number, user.email_address] for user in service.users), - ([member.recipient for member in service.whitelist]) if api_user.key_type == KEY_TYPE_NORMAL else iter([]) + ([member.recipient for member in service.whitelist]) + if api_user.key_type == KEY_TYPE_NORMAL else iter([]) ) ) )): diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index f6a0145de..56fcfad33 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -1004,12 +1004,11 @@ def test_should_not_persist_notification_or_send_sms_if_simulated_number( @pytest.mark.parametrize('to_sms', ['07827992635']) -def test_should_not_send_sms_to_non_whitelist_recipient_in_trial_mode_with_live_key( - client, - notify_db, - notify_db_session, - to_sms, - mocker): +def test_should_not_send_sms_to_non_whitelist_recipient_in_trial_mode_with_live_key(client, + notify_db, + notify_db_session, + to_sms, + mocker): apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service) @@ -1044,13 +1043,13 @@ def test_should_not_send_sms_to_non_whitelist_recipient_in_trial_mode_with_live_ assert expected_response_message in json_resp['message']['to'] apply_async.assert_not_called() + @pytest.mark.parametrize('to_email', ['non_whitelist_recipient@mail.com']) -def test_should_not_send_email_to_non_whitelist_recipient_in_trial_mode_with_live_key( - client, - notify_db, - notify_db_session, - to_email, - mocker): +def test_should_not_send_email_to_non_whitelist_recipient_in_trial_mode_with_live_key(client, + notify_db, + notify_db_session, + to_email, + mocker): apply_async = mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service) @@ -1087,15 +1086,15 @@ def test_should_not_send_email_to_non_whitelist_recipient_in_trial_mode_with_liv @pytest.mark.parametrize('to_sms', ['07827992635']) -def test_should_not_send_sms_to_whitelist_recipient_in_trial_mode_with_team_key( - client, - notify_db, - notify_db_session, - to_sms, - mocker): +def test_should_not_send_sms_to_whitelist_recipient_in_trial_mode_with_team_key(client, + notify_db, + notify_db_session, + to_sms, + mocker): apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) - service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service, mobile_number=to_sms) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, + service=service, mobile_number=to_sms) sms_template = create_sample_template(notify_db, notify_db_session, service=service) assert service_whitelist.service_id == service.id @@ -1124,15 +1123,15 @@ def test_should_not_send_sms_to_whitelist_recipient_in_trial_mode_with_team_key( @pytest.mark.parametrize('to_email', ['non_whitelist_recipient@mail.com']) -def test_should_not_send_email_to_whitelist_recipient_in_trial_mode_with_team_key( - client, - notify_db, - notify_db_session, - to_email, - mocker): +def test_should_not_send_email_to_whitelist_recipient_in_trial_mode_with_team_key(client, + notify_db, + notify_db_session, + to_email, + mocker): apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) - service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service, email_address=to_email) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, + service=service, email_address=to_email) email_template = create_sample_email_template(notify_db, notify_db_session, service=service) assert service_whitelist.service_id == service.id @@ -1161,16 +1160,16 @@ def test_should_not_send_email_to_whitelist_recipient_in_trial_mode_with_team_ke @pytest.mark.parametrize('to_sms', ['07123123123']) -def test_should_send_sms_to_whitelist_recipient_in_trial_mode_with_live_key( - client, - notify_db, - notify_db_session, - to_sms, - mocker): +def test_should_send_sms_to_whitelist_recipient_in_trial_mode_with_live_key(client, + notify_db, + notify_db_session, + to_sms, + mocker): apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) - service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service, mobile_number=to_sms) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, + service=service, mobile_number=to_sms) sms_template = create_sample_template(notify_db, notify_db_session, service=service) assert service_whitelist.service_id == service.id @@ -1200,16 +1199,16 @@ def test_should_send_sms_to_whitelist_recipient_in_trial_mode_with_live_key( @pytest.mark.parametrize('to_email', ['whitelist_recipient@mail.com']) -def test_should_send_email_to_whitelist_recipient_in_trial_mode_with_live_key( - client, - notify_db, - notify_db_session, - to_email, - mocker): +def test_should_send_email_to_whitelist_recipient_in_trial_mode_with_live_key(client, + notify_db, + notify_db_session, + to_email, + mocker): apply_async = mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) - service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service, email_address=to_email) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, + service=service, email_address=to_email) email_template = create_sample_email_template(notify_db, notify_db_session, service=service) assert service_whitelist.service_id == service.id diff --git a/tests/app/service/test_service_whitelist.py b/tests/app/service/test_service_whitelist.py index 1eba735fa..23f6a3aa2 100644 --- a/tests/app/service/test_service_whitelist.py +++ b/tests/app/service/test_service_whitelist.py @@ -20,6 +20,7 @@ def test_get_whitelist_returns_data(client, sample_service_whitelist): 'phone_numbers': [] } + def test_get_whitelist_separates_emails_and_phones(client, sample_service): dao_add_and_commit_whitelisted_contacts([ ServiceWhitelist.from_string(sample_service.id, EMAIL_TYPE, 'service@example.com'), @@ -71,6 +72,7 @@ def test_update_whitelist_replaces_old_whitelist(client, sample_service_whitelis assert whitelist[0].recipient == '07123456789' assert whitelist[1].recipient == 'foo@bar.com' + def test_update_whitelist_doesnt_remove_old_whitelist_if_error(client, sample_service_whitelist): data = { From 1222a6ddf3b358744988bc387cddf8e33fe9c44f Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 28 Sep 2016 10:16:10 +0100 Subject: [PATCH 16/19] Refactor to increase readability for getting whitelist objects --- app/service/rest.py | 22 ++++++++-------------- app/service/utils.py | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 14 deletions(-) create mode 100644 app/service/utils.py diff --git a/app/service/rest.py b/app/service/rest.py index 1da828067..7aa823f80 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -35,6 +35,12 @@ from app.dao.service_whitelist_dao import ( from app.dao import notifications_dao from app.dao.provider_statistics_dao import get_fragment_count from app.dao.users_dao import get_model_users +from app.errors import ( + register_errors, + InvalidRequest +) +from app.service import statistics +from app.service.utils import get_whitelist_objects from app.schemas import ( service_schema, api_key_schema, @@ -45,15 +51,6 @@ from app.schemas import ( detailed_service_schema ) from app.utils import pagination_links -from app.errors import ( - register_errors, - InvalidRequest -) -from app.service import statistics -from app.models import ( - ServiceWhitelist, - MOBILE_TYPE, EMAIL_TYPE) - service_blueprint = Blueprint('service', __name__) register_errors(service_blueprint) @@ -284,6 +281,7 @@ def get_detailed_services(): @service_blueprint.route('//whitelist', methods=['GET']) def get_whitelist(service_id): + from app.models import (EMAIL_TYPE, MOBILE_TYPE) service = dao_fetch_service_by_id(service_id) if not service: @@ -303,11 +301,7 @@ def update_whitelist(service_id): # doesn't commit so if there are any errors, we preserve old values in db dao_remove_service_whitelist(service_id) try: - whitelist_objs = itertools.chain( - [ServiceWhitelist.from_string(service_id, MOBILE_TYPE, recipient) - for recipient in request.get_json().get('phone_numbers')], - [ServiceWhitelist.from_string(service_id, EMAIL_TYPE, recipient) - for recipient in request.get_json().get('email_addresses')]) + whitelist_objs = get_whitelist_objects(service_id, request.get_json()) except ValueError as e: current_app.logger.exception(e) dao_rollback() diff --git a/app/service/utils.py b/app/service/utils.py new file mode 100644 index 000000000..e8ca49dc3 --- /dev/null +++ b/app/service/utils.py @@ -0,0 +1,17 @@ +from app.models import ( + ServiceWhitelist, + MOBILE_TYPE, EMAIL_TYPE) + + +def get_recipients_from_request(request_json, key, type): + return [(type, recipient) for recipient in request_json.get(key)] + + +def get_whitelist_objects(service_id, request_json): + return [ + ServiceWhitelist.from_string(service_id, type, recipient) + for type, recipient in ( + get_recipients_from_request(request_json, 'phone_numbers', MOBILE_TYPE) + + get_recipients_from_request(request_json, 'email_addresses', EMAIL_TYPE) + ) + ] From db608a05d25330d5e2ecbd7d0bb0c4503550407f Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 28 Sep 2016 17:00:17 +0100 Subject: [PATCH 17/19] Refactor sending elegibility function and update across files --- app/celery/tasks.py | 17 ++-------------- app/notifications/rest.py | 18 +++-------------- app/service/utils.py | 42 ++++++++++++++++++++++++++++++++++++--- 3 files changed, 44 insertions(+), 33 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index eee951dfd..a9516b646 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,10 +1,8 @@ -import itertools from datetime import (datetime) from flask import current_app from notifications_utils.recipients import ( - RecipientCSV, - allowed_to_send_to + RecipientCSV ) from notifications_utils.template import Template from sqlalchemy.exc import SQLAlchemyError @@ -31,6 +29,7 @@ from app.models import ( KEY_TYPE_NORMAL, KEY_TYPE_TEST ) +from app.service.utils import service_allowed_to_send_to from app.statsd_decorators import statsd @@ -181,15 +180,3 @@ def send_email(self, service_id, "RETRY FAILED: task send_email failed for notification {}".format(notification.id), e ) - - -def service_allowed_to_send_to(recipient, service, key_type): - if not service.restricted or key_type == KEY_TYPE_TEST: - return True - - return allowed_to_send_to( - recipient, - itertools.chain.from_iterable( - [user.mobile_number, user.email_address] for user in service.users - ) - ) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 48172aedc..e3362c10d 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -9,7 +9,7 @@ from flask import ( json ) -from notifications_utils.recipients import allowed_to_send_to, first_column_heading +from notifications_utils.recipients import first_column_heading from notifications_utils.template import Template from notifications_utils.renderers import PassThrough from app.clients.email.aws_ses import get_aws_responses @@ -27,6 +27,7 @@ from app.notifications.process_client_response import ( validate_callback_data, process_sms_client_response ) +from app.service.utils import service_allowed_to_send_to from app.schemas import ( email_notification_schema, sms_template_notification_schema, @@ -252,19 +253,7 @@ def send_notification(notification_type): errors = {'content': [message]} raise InvalidRequest(errors, status_code=400) - if all(( - api_user.key_type != KEY_TYPE_TEST, - service.restricted or api_user.key_type == KEY_TYPE_TEAM, - not allowed_to_send_to( - notification['to'], - itertools.chain( - itertools.chain.from_iterable([user.mobile_number, user.email_address] for user in service.users), - ([member.recipient for member in service.whitelist]) - if api_user.key_type == KEY_TYPE_NORMAL else iter([]) - ) - ) - )): - + if not service_allowed_to_send_to(notification['to'], service, api_user.key_type): if (api_user.key_type == KEY_TYPE_TEAM): message = 'Can’t send to this recipient using a team-only API key' else: @@ -279,7 +268,6 @@ def send_notification(notification_type): notification_id = create_uuid() notification.update({"template_version": template.version}) - if not _simulated_recipient(notification['to'], notification_type): persist_notification( service, diff --git a/app/service/utils.py b/app/service/utils.py index e8ca49dc3..17ce89071 100644 --- a/app/service/utils.py +++ b/app/service/utils.py @@ -1,6 +1,11 @@ +import itertools + from app.models import ( ServiceWhitelist, - MOBILE_TYPE, EMAIL_TYPE) + MOBILE_TYPE, EMAIL_TYPE, + KEY_TYPE_TEST, KEY_TYPE_TEAM, KEY_TYPE_NORMAL) + +from notifications_utils.recipients import allowed_to_send_to def get_recipients_from_request(request_json, key, type): @@ -11,7 +16,38 @@ def get_whitelist_objects(service_id, request_json): return [ ServiceWhitelist.from_string(service_id, type, recipient) for type, recipient in ( - get_recipients_from_request(request_json, 'phone_numbers', MOBILE_TYPE) + - get_recipients_from_request(request_json, 'email_addresses', EMAIL_TYPE) + get_recipients_from_request(request_json, + 'phone_numbers', + MOBILE_TYPE) + + get_recipients_from_request(request_json, + 'email_addresses', + EMAIL_TYPE) ) ] + + +def service_allowed_to_send_to(recipient, service, key_type): + if key_type == KEY_TYPE_TEST: + return True + + if key_type == KEY_TYPE_NORMAL and not service.restricted: + return True + + team_members = itertools.chain.from_iterable( + [user.mobile_number, user.email_address] for user in service.users) + + if key_type == KEY_TYPE_TEAM: + return allowed_to_send_to( + recipient, + team_members + ) + + if key_type == KEY_TYPE_NORMAL and service.restricted: + whitelist_members = [member.recipient for member in service.whitelist] + return allowed_to_send_to( + recipient, + itertools.chain( + team_members, + whitelist_members + ) + ) From 8f74d9a122237de463b50034b6ca505eb9cfcc86 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 28 Sep 2016 17:02:57 +0100 Subject: [PATCH 18/19] Fix issue with test where team key does not check sending elegibility --- tests/app/celery/test_tasks.py | 73 ++++++++++++++++++++++++++++++---- 1 file changed, 66 insertions(+), 7 deletions(-) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index aa792776f..f6a039698 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,7 +1,8 @@ import uuid +import itertools +import pytest from datetime import datetime -import pytest from freezegun import freeze_time from unittest.mock import ANY from sqlalchemy.exc import SQLAlchemyError @@ -17,7 +18,7 @@ from app.celery.tasks import ( send_email ) from app.dao import jobs_dao -from app.models import Notification, KEY_TYPE_TEAM, KEY_TYPE_TEST +from app.models import Notification, KEY_TYPE_TEAM, KEY_TYPE_TEST, KEY_TYPE_NORMAL from tests.app import load_example_csv from tests.app.conftest import ( sample_service, @@ -455,8 +456,9 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ encryption.encrypt(notification), datetime.utcnow().strftime(DATETIME_FORMAT), api_key_id=str(sample_api_key.id), - key_type=KEY_TYPE_TEAM + key_type=KEY_TYPE_NORMAL ) + provider_tasks.send_sms_to_provider.apply_async.assert_called_once_with( (sample_job.service.id, notification_id), @@ -473,14 +475,71 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ assert not persisted_notification.sent_by assert persisted_notification.job_row_number == 2 assert persisted_notification.api_key_id == sample_api_key.id - assert persisted_notification.key_type == KEY_TYPE_TEAM + assert persisted_notification.key_type == KEY_TYPE_NORMAL assert persisted_notification.notification_type == 'sms' +def test_should_not_send_email_if_team_key_and_recipient_not_in_team(sample_email_template_with_placeholders, + sample_team_api_key, + mocker): + notification = _notification_json( + sample_email_template_with_placeholders, + "my_email@my_email.com", + {"name": "Jo"}, + row_number=1) + notification_id = uuid.uuid4() + + team_members = [user.email_address for user in sample_email_template_with_placeholders.service.users] + assert "my_email@my_email.com" not in team_members + + apply_async = mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') + + with freeze_time("2016-01-01 11:09:00.00000"): + now = datetime.utcnow() + + send_email( + sample_email_template_with_placeholders.service_id, + notification_id, + encryption.encrypt(notification), + now.strftime(DATETIME_FORMAT), + api_key_id=str(sample_team_api_key.id), + key_type=KEY_TYPE_TEAM + ) + + with pytest.raises(NoResultFound): + persisted_notification = Notification.query.filter_by(id=notification_id).one() + print(persisted_notification) + + apply_async.not_called() + + +def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(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) + + team_members = [user.mobile_number for user in service.users] + assert "07890 300000" not in team_members + + notification = _notification_json(template, "07700 900849") + mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') + + notification_id = uuid.uuid4() + send_sms( + service.id, + notification_id, + encryption.encrypt(notification), + datetime.utcnow().strftime(DATETIME_FORMAT) + ) + provider_tasks.send_sms_to_provider.apply_async.assert_not_called() + with pytest.raises(NoResultFound): + Notification.query.filter_by(id=notification_id).one() + + def test_should_use_email_template_and_persist(sample_email_template_with_placeholders, sample_api_key, mocker): notification = _notification_json( sample_email_template_with_placeholders, - "my_email@my_email.com", + 'my_email@my_email.com', {"name": "Jo"}, row_number=1) mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') @@ -497,7 +556,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh encryption.encrypt(notification), now.strftime(DATETIME_FORMAT), api_key_id=str(sample_api_key.id), - key_type=KEY_TYPE_TEAM + key_type=sample_api_key.key_type ) persisted_notification = Notification.query.filter_by(id=notification_id).one() @@ -516,7 +575,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.personalisation == {'name': 'Jo'} assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) assert persisted_notification.api_key_id == sample_api_key.id - assert persisted_notification.key_type == KEY_TYPE_TEAM + assert persisted_notification.key_type == KEY_TYPE_NORMAL assert persisted_notification.notification_type == 'email' From a7d42896cd22f7504108d6a825d8532ed9543663 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Wed, 28 Sep 2016 17:03:17 +0100 Subject: [PATCH 19/19] Refactor tests --- .../rest/test_send_notification.py | 246 ++++-------------- 1 file changed, 56 insertions(+), 190 deletions(-) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 4525fa9ed..db58a39d7 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -1,7 +1,7 @@ -from datetime import datetime import random import string import pytest +from datetime import datetime from flask import (json, current_app) from freezegun import freeze_time @@ -9,7 +9,7 @@ from notifications_python_client.authentication import create_jwt_token import app from app.dao import notifications_dao -from app.models import ApiKey, KEY_TYPE_TEAM, KEY_TYPE_TEST, Notification, NotificationHistory +from app.models import ApiKey, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, Notification, NotificationHistory from app.dao.templates_dao import dao_get_all_templates_for_service, dao_update_template from app.dao.services_dao import dao_update_service from app.dao.api_key_dao import save_model_api_key @@ -1000,39 +1000,51 @@ def test_should_not_persist_notification_or_send_sms_if_simulated_number( assert Notification.query.count() == 0 -@pytest.mark.parametrize('to_sms', ['07827992635']) -def test_should_not_send_sms_to_non_whitelist_recipient_in_trial_mode_with_live_key(client, - notify_db, - notify_db_session, - to_sms, - mocker): - apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') +@pytest.mark.parametrize('notification_type,to, key_type', [ + ('sms', '07827992635', KEY_TYPE_NORMAL), + ('email', 'non_whitelist_recipient@mail.com', KEY_TYPE_NORMAL), + ('sms', '07827992635', KEY_TYPE_TEAM), + ('email', 'non_whitelist_recipient@mail.com', KEY_TYPE_TEAM)]) +def test_should_not_send_notification_to_non_whitelist_recipient_in_trial_mode(client, + notify_db, + notify_db_session, + notification_type, + to, + key_type, + mocker): service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service) - sms_template = create_sample_template(notify_db, notify_db_session, service=service) + + if notification_type == 'sms': + apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') + template = create_sample_template(notify_db, notify_db_session, service=service) + + elif notification_type == 'email': + apply_async = mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') + template = create_sample_email_template(notify_db, notify_db_session, service=service) assert service_whitelist.service_id == service.id - assert to_sms not in [member.recipient for member in service.whitelist] + assert to not in [member.recipient for member in service.whitelist] - create_sample_notification(notify_db, notify_db_session, template=sms_template, service=service) + create_sample_notification(notify_db, notify_db_session, template=template, service=service) data = { - 'to': to_sms, - 'template': str(sms_template.id) + 'to': to, + 'template': str(template.id) } - live_key = create_sample_api_key(notify_db, notify_db_session, service) - auth_header = create_jwt_token(secret=live_key.unsigned_secret, client_id=str(live_key.service_id)) + api_key = create_sample_api_key(notify_db, notify_db_session, service, key_type=key_type) + auth_header = create_jwt_token(secret=api_key.unsigned_secret, client_id=str(api_key.service_id)) response = client.post( - path='/notifications/sms', + path='/notifications/{}'.format(notification_type), data=json.dumps(data), headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) expected_response_message = ( 'Can’t send to this recipient when service is in trial mode ' '– see https://www.notifications.service.gov.uk/trial-mode' - ) + ) if key_type == KEY_TYPE_NORMAL else ('Can’t send to this recipient using a team-only API key') json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 400 @@ -1041,196 +1053,50 @@ def test_should_not_send_sms_to_non_whitelist_recipient_in_trial_mode_with_live_ apply_async.assert_not_called() -@pytest.mark.parametrize('to_email', ['non_whitelist_recipient@mail.com']) -def test_should_not_send_email_to_non_whitelist_recipient_in_trial_mode_with_live_key(client, - notify_db, - notify_db_session, - to_email, - mocker): - apply_async = mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') +@pytest.mark.parametrize('notification_type,to', [ + ('sms', '07123123123'), + ('email', 'whitelist_recipient@mail.com')]) +def test_should_send_notification_to_whitelist_recipient_in_trial_mode_with_live_key(client, + notify_db, + notify_db_session, + notification_type, + to, + mocker): service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) - service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, service=service) - email_template = create_sample_email_template(notify_db, notify_db_session, service=service) + if notification_type == 'sms': + apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') + template = create_sample_template(notify_db, notify_db_session, service=service) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, + service=service, mobile_number=to) + elif notification_type == 'email': + apply_async = mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') + template = create_sample_email_template(notify_db, notify_db_session, service=service) + service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, + service=service, email_address=to) assert service_whitelist.service_id == service.id - assert to_email not in [member.recipient for member in service.whitelist] + assert to in [member.recipient for member in service.whitelist] - create_sample_notification(notify_db, notify_db_session, template=email_template, service=service) + create_sample_notification(notify_db, notify_db_session, template=template, service=service) data = { - 'to': to_email, - 'template': str(email_template.id) - } - - live_key = create_sample_api_key(notify_db, notify_db_session, service) - auth_header = create_jwt_token(secret=live_key.unsigned_secret, client_id=str(live_key.service_id)) - - response = client.post( - path='/notifications/email', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - - expected_response_message = ( - 'Can’t send to this recipient when service is in trial mode ' - '– see https://www.notifications.service.gov.uk/trial-mode' - ) - - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert expected_response_message in json_resp['message']['to'] - apply_async.assert_not_called() - - -@pytest.mark.parametrize('to_sms', ['07827992635']) -def test_should_not_send_sms_to_whitelist_recipient_in_trial_mode_with_team_key(client, - notify_db, - notify_db_session, - to_sms, - mocker): - apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') - service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) - service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, - service=service, mobile_number=to_sms) - sms_template = create_sample_template(notify_db, notify_db_session, service=service) - - assert service_whitelist.service_id == service.id - assert to_sms in [member.recipient for member in service.whitelist] - - create_sample_notification(notify_db, notify_db_session, template=sms_template, service=service) - - data = { - 'to': to_sms, - 'template': str(sms_template.id) - } - - team_key = create_sample_api_key(notify_db, notify_db_session, service, key_type=KEY_TYPE_TEAM) - auth_header = create_jwt_token(secret=team_key.unsigned_secret, client_id=str(team_key.service_id)) - - response = client.post( - path='/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert 'Can’t send to this recipient using a team-only API key' in json_resp['message']['to'] - apply_async.assert_not_called() - - -@pytest.mark.parametrize('to_email', ['non_whitelist_recipient@mail.com']) -def test_should_not_send_email_to_whitelist_recipient_in_trial_mode_with_team_key(client, - notify_db, - notify_db_session, - to_email, - mocker): - apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') - service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) - service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, - service=service, email_address=to_email) - email_template = create_sample_email_template(notify_db, notify_db_session, service=service) - - assert service_whitelist.service_id == service.id - assert to_email in [member.recipient for member in service.whitelist] - - create_sample_notification(notify_db, notify_db_session, template=email_template, service=service) - - data = { - 'to': to_email, - 'template': str(email_template.id) - } - - team_key = create_sample_api_key(notify_db, notify_db_session, service, key_type=KEY_TYPE_TEAM) - auth_header = create_jwt_token(secret=team_key.unsigned_secret, client_id=str(team_key.service_id)) - - response = client.post( - path='/notifications/email', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert 'Can’t send to this recipient using a team-only API key' in json_resp['message']['to'] - apply_async.assert_not_called() - - -@pytest.mark.parametrize('to_sms', ['07123123123']) -def test_should_send_sms_to_whitelist_recipient_in_trial_mode_with_live_key(client, - notify_db, - notify_db_session, - to_sms, - mocker): - apply_async = mocker.patch('app.celery.provider_tasks.send_sms_to_provider.apply_async') - - service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) - service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, - service=service, mobile_number=to_sms) - sms_template = create_sample_template(notify_db, notify_db_session, service=service) - - assert service_whitelist.service_id == service.id - assert to_sms in [member.recipient for member in service.whitelist] - - create_sample_notification(notify_db, notify_db_session, template=sms_template, service=service) - - data = { - 'to': to_sms, - 'template': str(sms_template.id) + 'to': to, + 'template': str(template.id) } sample_live_key = create_sample_api_key(notify_db, notify_db_session, service) auth_header = create_jwt_token(secret=sample_live_key.unsigned_secret, client_id=str(sample_live_key.service_id)) response = client.post( - path='/notifications/sms', + path='/notifications/{}'.format(notification_type), data=json.dumps(data), headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 201 assert json_resp['data']['notification']['id'] - assert json_resp['data']['body'] == sms_template.content - assert json_resp['data']['template_version'] == sms_template.version - apply_async.called - - -@pytest.mark.parametrize('to_email', ['whitelist_recipient@mail.com']) -def test_should_send_email_to_whitelist_recipient_in_trial_mode_with_live_key(client, - notify_db, - notify_db_session, - to_email, - mocker): - apply_async = mocker.patch('app.celery.provider_tasks.send_email_to_provider.apply_async') - - service = create_sample_service(notify_db, notify_db_session, limit=2, restricted=True) - service_whitelist = create_sample_service_whitelist(notify_db, notify_db_session, - service=service, email_address=to_email) - email_template = create_sample_email_template(notify_db, notify_db_session, service=service) - - assert service_whitelist.service_id == service.id - assert to_email in [member.recipient for member in service.whitelist] - - create_sample_notification(notify_db, notify_db_session, template=email_template, service=service) - - data = { - 'to': to_email, - 'template': str(email_template.id) - } - - sample_live_key = create_sample_api_key(notify_db, notify_db_session, service) - auth_header = create_jwt_token(secret=sample_live_key.unsigned_secret, client_id=str(sample_live_key.service_id)) - - response = client.post( - path='/notifications/email', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) - - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 201 - assert json_resp['data']['notification']['id'] - assert json_resp['data']['body'] == email_template.content - assert json_resp['data']['template_version'] == email_template.version + assert json_resp['data']['body'] == template.content + assert json_resp['data']['template_version'] == template.version apply_async.called