From 615ea6a98aefec839423f65ee50518de4d15f744 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 10 May 2019 15:32:24 +0100 Subject: [PATCH 1/5] Send verifcation email to a new reply-to email address --- app/config.py | 1 + app/service/rest.py | 32 ++++++++++++++++++++++++++++++-- tests/app/conftest.py | 14 ++++++++++++++ tests/app/service/test_rest.py | 21 +++++++++++++++++++++ 4 files changed, 66 insertions(+), 2 deletions(-) diff --git a/app/config.py b/app/config.py index f204885b4..b2970c1e4 100644 --- a/app/config.py +++ b/app/config.py @@ -150,6 +150,7 @@ class Config(object): ORGANISATION_INVITATION_EMAIL_TEMPLATE_ID = '203566f0-d835-47c5-aa06-932439c86573' TEAM_MEMBER_EDIT_EMAIL_TEMPLATE_ID = 'c73f1d71-4049-46d5-a647-d013bdeca3f0' TEAM_MEMBER_EDIT_MOBILE_TEMPLATE_ID = '8a31520f-4751-4789-8ea1-fe54496725eb' + REPLY_TO_EMAIL_ADDRESS_VERIFICATION_TEMPLATE_ID = '2d1ff26c-4d20-4cb2-8918-a0bec4002c9e' BROKER_URL = 'sqs://' BROKER_TRANSPORT_OPTIONS = { diff --git a/app/service/rest.py b/app/service/rest.py index 8b2be1818..6bf76763a 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -12,6 +12,7 @@ from notifications_utils.timezones import convert_utc_to_bst from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound +from app.config import QueueNames from app.dao import notifications_dao from app.dao.dao_utils import dao_rollback from app.dao.date_util import get_financial_year @@ -77,13 +78,17 @@ from app.dao.service_letter_contact_dao import ( add_letter_contact_for_service, update_letter_contact ) +from app.dao.templates_dao import dao_get_template_by_id from app.dao.users_dao import get_user_by_id from app.errors import ( InvalidRequest, register_errors ) from app.letters.utils import letter_print_day -from app.models import LETTER_TYPE, NOTIFICATION_CANCELLED, Permission, Service, EmailBranding, LetterBranding +from app.models import ( + KEY_TYPE_NORMAL, LETTER_TYPE, NOTIFICATION_CANCELLED, Permission, Service, EmailBranding, LetterBranding +) +from app.notifications.process_notifications import persist_notification, send_notification_to_queue from app.schema_validation import validate from app.service import statistics from app.service.service_data_retention_schema import ( @@ -103,7 +108,8 @@ from app.schemas import ( api_key_schema, notification_with_template_schema, notifications_filter_schema, - detailed_service_schema + detailed_service_schema, + email_data_request_schema ) from app.user.users_schema import post_set_permissions_schema from app.utils import pagination_links @@ -644,6 +650,28 @@ def get_email_reply_to_address(service_id, reply_to_id): return jsonify(result.serialize()), 200 +@service_blueprint.route('/email-reply-to/verify', methods=['POST']) +def verify_new_service_reply_to_email_address(): + email_address, errors = email_data_request_schema.load(request.get_json()) + template = dao_get_template_by_id(current_app.config['REPLY_TO_EMAIL_ADDRESS_VERIFICATION_TEMPLATE_ID']) + service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) + saved_notification = persist_notification( + template_id=template.id, + template_version=template.version, + recipient=email_address["email"], + service=service, + personalisation='', + notification_type=template.template_type, + api_key_id=None, + key_type=KEY_TYPE_NORMAL, + reply_to_text=service.get_default_reply_to_email_address() + ) + + send_notification_to_queue(saved_notification, False, queue=QueueNames.NOTIFY) + + return jsonify(data={"id": saved_notification.id}), 201 + + @service_blueprint.route('//email-reply-to', methods=['POST']) def add_service_reply_to_email_address(service_id): # validate the service exists, throws ResultNotFound exception. diff --git a/tests/app/conftest.py b/tests/app/conftest.py index cbb113af3..e1e98605d 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -951,6 +951,20 @@ def password_reset_email_template(notify_db, ) +@pytest.fixture(scope='function') +def verify_reply_to_address_email_template(notify_db, notify_db_session): + service, user = notify_service(notify_db, notify_db_session) + + return create_custom_template( + service=service, + user=user, + template_config_name='REPLY_TO_EMAIL_ADDRESS_VERIFICATION_TEMPLATE_ID', + content="Hi,This address has been provided as the reply-to email address so we are verifying if it's working", + subject='Your GOV.UK Notify reply-to email address', + template_type='email' + ) + + @pytest.fixture(scope='function') def team_member_email_edit_template(notify_db, notify_db_session): service, user = notify_service(notify_db, notify_db_session) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index f57d523e2..3d630ad82 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2491,6 +2491,27 @@ def test_get_email_reply_to_addresses_with_multiple_email_addresses(client, noti assert not json_response[1]['updated_at'] +@freeze_time("2016-01-01 11:09:00.061258") +def test_verify_new_service_reply_to_email_address_should_send_verification_email( + client, sample_user, mocker, verify_reply_to_address_email_template +): + mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + data = json.dumps({'email': 'reply-here@example.gov.uk'}) + auth_header = create_authorization_header() + notify_service = verify_reply_to_address_email_template.service + response = client.post( + url_for('service.verify_new_service_reply_to_email_address'), + data=data, + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201 + notification = Notification.query.first() + json_response = json.loads(response.get_data(as_text=True)) + assert json_response["data"] == {"id": str(notification.id)} + mocked.assert_called_once_with([str(notification.id)], queue="notify-internal-tasks") + assert notification.reply_to_text == notify_service.get_default_reply_to_email_address() + + def test_add_service_reply_to_email_address(client, sample_service): data = json.dumps({"email_address": "new@reply.com", "is_default": True}) response = client.post('/service/{}/email-reply-to'.format(sample_service.id), From 3c3dde635b3c70031c5bd4298f9648b33b71f13f Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 15 May 2019 15:23:27 +0100 Subject: [PATCH 2/5] Prevent service from adding duplicate reply-to addresses Check for duplicate reply-to email address has been added on: -verification endpoint, so we do not send the verifying notification needlessly - add reply-to email address and update reply-to email address endpoints, as those can be hit multiple times after the email address has been verified (so the same email address could end up being added multiple times). EDIT: this has now been prevented on admin app, but it's better to retain double-check for safety. --- app/service/rest.py | 21 ++++++++++++---- tests/app/service/test_rest.py | 44 +++++++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 6bf76763a..7c1b3fb0b 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -650,21 +650,22 @@ def get_email_reply_to_address(service_id, reply_to_id): return jsonify(result.serialize()), 200 -@service_blueprint.route('/email-reply-to/verify', methods=['POST']) -def verify_new_service_reply_to_email_address(): +@service_blueprint.route('//email-reply-to/verify', methods=['POST']) +def verify_new_service_reply_to_email_address(service_id): email_address, errors = email_data_request_schema.load(request.get_json()) + check_if_reply_to_address_already_in_use(service_id, email_address["email"]) template = dao_get_template_by_id(current_app.config['REPLY_TO_EMAIL_ADDRESS_VERIFICATION_TEMPLATE_ID']) - service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) + notify_service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) saved_notification = persist_notification( template_id=template.id, template_version=template.version, recipient=email_address["email"], - service=service, + service=notify_service, personalisation='', notification_type=template.template_type, api_key_id=None, key_type=KEY_TYPE_NORMAL, - reply_to_text=service.get_default_reply_to_email_address() + reply_to_text=notify_service.get_default_reply_to_email_address() ) send_notification_to_queue(saved_notification, False, queue=QueueNames.NOTIFY) @@ -677,6 +678,7 @@ def add_service_reply_to_email_address(service_id): # validate the service exists, throws ResultNotFound exception. dao_fetch_service_by_id(service_id) form = validate(request.get_json(), add_service_email_reply_to_request) + check_if_reply_to_address_already_in_use(service_id, form['email_address']) new_reply_to = add_reply_to_email_address_for_service(service_id=service_id, email_address=form['email_address'], is_default=form.get('is_default', True)) @@ -688,6 +690,7 @@ def update_service_reply_to_email_address(service_id, reply_to_email_id): # validate the service exists, throws ResultNotFound exception. dao_fetch_service_by_id(service_id) form = validate(request.get_json(), add_service_email_reply_to_request) + check_if_reply_to_address_already_in_use(service_id, form['email_address']) new_reply_to = update_reply_to_email_address(service_id=service_id, reply_to_id=reply_to_email_id, email_address=form['email_address'], @@ -901,3 +904,11 @@ def check_request_args(request): if errors: raise InvalidRequest(errors, status_code=400) return service_id, name, email_from + + +def check_if_reply_to_address_already_in_use(service_id, email_address): + existing_reply_to_addresses = dao_get_reply_to_by_service_id(service_id) + if email_address in [i.email_address for i in existing_reply_to_addresses]: + raise InvalidRequest( + "Your service already uses '{}' as an email reply-to address.".format(email_address), status_code=400 + ) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 3d630ad82..ecde5f097 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2495,12 +2495,13 @@ def test_get_email_reply_to_addresses_with_multiple_email_addresses(client, noti def test_verify_new_service_reply_to_email_address_should_send_verification_email( client, sample_user, mocker, verify_reply_to_address_email_template ): + service = create_service() mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') data = json.dumps({'email': 'reply-here@example.gov.uk'}) auth_header = create_authorization_header() notify_service = verify_reply_to_address_email_template.service response = client.post( - url_for('service.verify_new_service_reply_to_email_address'), + url_for('service.verify_new_service_reply_to_email_address', service_id=service.id), data=data, headers=[('Content-Type', 'application/json'), auth_header]) @@ -2512,6 +2513,21 @@ def test_verify_new_service_reply_to_email_address_should_send_verification_emai assert notification.reply_to_text == notify_service.get_default_reply_to_email_address() +def test_verify_new_service_reply_to_email_address_doesnt_allow_duplicates(client, sample_user, mocker): + data = json.dumps({'email': 'reply-here@example.gov.uk'}) + service = create_service() + create_reply_to_email(service, 'reply-here@example.gov.uk') + auth_header = create_authorization_header() + response = client.post( + url_for('service.verify_new_service_reply_to_email_address', service_id=service.id), + data=data, + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 400 + assert response.json[ + "message" + ] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." + + def test_add_service_reply_to_email_address(client, sample_service): data = json.dumps({"email_address": "new@reply.com", "is_default": True}) response = client.post('/service/{}/email-reply-to'.format(sample_service.id), @@ -2525,6 +2541,20 @@ def test_add_service_reply_to_email_address(client, sample_service): assert json_resp['data'] == results[0].serialize() +def test_add_service_reply_to_email_address_doesnt_allow_duplicates(client, sample_user, mocker): + data = json.dumps({"email_address": "reply-here@example.gov.uk", "is_default": True}) + service = create_service() + create_reply_to_email(service, 'reply-here@example.gov.uk') + auth_header = create_authorization_header() + response = client.post('/service/{}/email-reply-to'.format(service.id), + data=data, + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 400 + assert response.json[ + "message" + ] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." + + def test_add_service_reply_to_email_address_can_add_multiple_addresses(client, sample_service): data = json.dumps({"email_address": "first@reply.com", "is_default": True}) client.post('/service/{}/email-reply-to'.format(sample_service.id), @@ -2580,6 +2610,18 @@ def test_update_service_reply_to_email_address(client, sample_service): assert json_resp['data'] == results[0].serialize() +def test_update_service_reply_to_email_address_doesnt_allow_duplicates(client, sample_user, mocker): + service = create_service() + original_reply_to = create_reply_to_email(service=service, email_address="some@email.com") + data = json.dumps({"email_address": "changed@reply.com", "is_default": True}) + create_reply_to_email(service, 'changed@reply.com') + response = client.post('/service/{}/email-reply-to/{}'.format(service.id, original_reply_to.id), + data=data, + headers=[('Content-Type', 'application/json'), create_authorization_header()]) + assert response.status_code == 400 + assert response.json["message"] == "Your service already uses 'changed@reply.com' as an email reply-to address." + + def test_update_service_reply_to_email_address_returns_400_when_no_default(client, sample_service): original_reply_to = create_reply_to_email(service=sample_service, email_address="some@email.com") data = json.dumps({"email_address": "changed@reply.com", "is_default": False}) From 5692a8596d81684ab8dc71ccb3892141bb988816 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 22 May 2019 16:07:27 +0100 Subject: [PATCH 3/5] Code refactors and corrections, details below: Change method name to be more relevant Check if verification notification we send is a correct one pass in notify db session for tests instead of sample_user Refactor tests by using admin_request instead of client Refactor all tests for affected reply-to endpoints for good measure Allow overwriting reply-to address with the same address Skip checking for duplicates if it's an reply-to email update Fix refactored tests Verify duplicates exception not needed --- app/service/rest.py | 5 +- tests/app/service/test_rest.py | 211 +++++++++++++++++---------------- 2 files changed, 108 insertions(+), 108 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 7c1b3fb0b..e3f627cb6 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -651,7 +651,7 @@ def get_email_reply_to_address(service_id, reply_to_id): @service_blueprint.route('//email-reply-to/verify', methods=['POST']) -def verify_new_service_reply_to_email_address(service_id): +def verify_reply_to_email_address(service_id): email_address, errors = email_data_request_schema.load(request.get_json()) check_if_reply_to_address_already_in_use(service_id, email_address["email"]) template = dao_get_template_by_id(current_app.config['REPLY_TO_EMAIL_ADDRESS_VERIFICATION_TEMPLATE_ID']) @@ -690,7 +690,6 @@ def update_service_reply_to_email_address(service_id, reply_to_email_id): # validate the service exists, throws ResultNotFound exception. dao_fetch_service_by_id(service_id) form = validate(request.get_json(), add_service_email_reply_to_request) - check_if_reply_to_address_already_in_use(service_id, form['email_address']) new_reply_to = update_reply_to_email_address(service_id=service_id, reply_to_id=reply_to_email_id, email_address=form['email_address'], @@ -910,5 +909,5 @@ def check_if_reply_to_address_already_in_use(service_id, email_address): existing_reply_to_addresses = dao_get_reply_to_by_service_id(service_id) if email_address in [i.email_address for i in existing_reply_to_addresses]: raise InvalidRequest( - "Your service already uses '{}' as an email reply-to address.".format(email_address), status_code=400 + "Your service already uses ‘{}’ as an email reply-to address.".format(email_address), status_code=400 ) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index ecde5f097..6121d94a1 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2492,157 +2492,158 @@ def test_get_email_reply_to_addresses_with_multiple_email_addresses(client, noti @freeze_time("2016-01-01 11:09:00.061258") -def test_verify_new_service_reply_to_email_address_should_send_verification_email( - client, sample_user, mocker, verify_reply_to_address_email_template +def test_verify_reply_to_email_address_should_send_verification_email( + admin_request, notify_db, notify_db_session, mocker, verify_reply_to_address_email_template ): service = create_service() mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - data = json.dumps({'email': 'reply-here@example.gov.uk'}) - auth_header = create_authorization_header() + data = {'email': 'reply-here@example.gov.uk'} notify_service = verify_reply_to_address_email_template.service - response = client.post( - url_for('service.verify_new_service_reply_to_email_address', service_id=service.id), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) + response = admin_request.post( + 'service.verify_reply_to_email_address', + service_id=service.id, + _data=data, + _expected_status=201 + ) - assert response.status_code == 201 notification = Notification.query.first() - json_response = json.loads(response.get_data(as_text=True)) - assert json_response["data"] == {"id": str(notification.id)} + assert notification.template_id == verify_reply_to_address_email_template.id + assert response["data"] == {"id": str(notification.id)} mocked.assert_called_once_with([str(notification.id)], queue="notify-internal-tasks") assert notification.reply_to_text == notify_service.get_default_reply_to_email_address() -def test_verify_new_service_reply_to_email_address_doesnt_allow_duplicates(client, sample_user, mocker): - data = json.dumps({'email': 'reply-here@example.gov.uk'}) +def test_verify_reply_to_email_address_doesnt_allow_duplicates(admin_request, notify_db, notify_db_session, mocker): + data = {'email': 'reply-here@example.gov.uk'} service = create_service() create_reply_to_email(service, 'reply-here@example.gov.uk') - auth_header = create_authorization_header() - response = client.post( - url_for('service.verify_new_service_reply_to_email_address', service_id=service.id), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 400 - assert response.json[ - "message" - ] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." + response = admin_request.post( + 'service.verify_reply_to_email_address', + service_id=service.id, + _data=data, + _expected_status=400 + ) + assert response["message"] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." -def test_add_service_reply_to_email_address(client, sample_service): - data = json.dumps({"email_address": "new@reply.com", "is_default": True}) - response = client.post('/service/{}/email-reply-to'.format(sample_service.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) +def test_add_service_reply_to_email_address(admin_request, sample_service): + data = {"email_address": "new@reply.com", "is_default": True} + response = admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=sample_service.id, + _data=data, + _expected_status=201 + ) - assert response.status_code == 201 - json_resp = json.loads(response.get_data(as_text=True)) results = ServiceEmailReplyTo.query.all() assert len(results) == 1 - assert json_resp['data'] == results[0].serialize() + assert response['data'] == results[0].serialize() -def test_add_service_reply_to_email_address_doesnt_allow_duplicates(client, sample_user, mocker): - data = json.dumps({"email_address": "reply-here@example.gov.uk", "is_default": True}) +def test_add_service_reply_to_email_address_doesnt_allow_duplicates( + admin_request, notify_db, notify_db_session, mocker +): + data = {"email_address": "reply-here@example.gov.uk", "is_default": True} service = create_service() create_reply_to_email(service, 'reply-here@example.gov.uk') - auth_header = create_authorization_header() - response = client.post('/service/{}/email-reply-to'.format(service.id), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 400 - assert response.json[ - "message" - ] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." + response = admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=service.id, + _data=data, + _expected_status=400 + ) + assert response["message"] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." -def test_add_service_reply_to_email_address_can_add_multiple_addresses(client, sample_service): - data = json.dumps({"email_address": "first@reply.com", "is_default": True}) - client.post('/service/{}/email-reply-to'.format(sample_service.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - - second = json.dumps({"email_address": "second@reply.com", "is_default": True}) - response = client.post('/service/{}/email-reply-to'.format(sample_service.id), - data=second, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - assert response.status_code == 201 - json_resp = json.loads(response.get_data(as_text=True)) +def test_add_service_reply_to_email_address_can_add_multiple_addresses(admin_request, sample_service): + data = {"email_address": "first@reply.com", "is_default": True} + admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=sample_service.id, + _data=data, + _expected_status=201 + ) + second = {"email_address": "second@reply.com", "is_default": True} + response = admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=sample_service.id, + _data=second, + _expected_status=201 + ) results = ServiceEmailReplyTo.query.all() assert len(results) == 2 default = [x for x in results if x.is_default] - assert json_resp['data'] == default[0].serialize() + assert response['data'] == default[0].serialize() first_reply_to_not_default = [x for x in results if not x.is_default] assert first_reply_to_not_default[0].email_address == 'first@reply.com' -def test_add_service_reply_to_email_address_raise_exception_if_no_default(client, sample_service): - data = json.dumps({"email_address": "first@reply.com", "is_default": False}) - response = client.post('/service/{}/email-reply-to'.format(sample_service.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - assert response.status_code == 400 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['message'] == 'You must have at least one reply to email address as the default.' +def test_add_service_reply_to_email_address_raise_exception_if_no_default(admin_request, sample_service): + data = {"email_address": "first@reply.com", "is_default": False} + response = admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=sample_service.id, + _data=data, + _expected_status=400 + ) + assert response['message'] == 'You must have at least one reply to email address as the default.' -def test_add_service_reply_to_email_address_404s_when_invalid_service_id(client, notify_db, notify_db_session): - response = client.post('/service/{}/email-reply-to'.format(uuid.uuid4()), - data={}, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) +def test_add_service_reply_to_email_address_404s_when_invalid_service_id(admin_request, notify_db, notify_db_session): + response = admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=uuid.uuid4(), + _data={}, + _expected_status=404 + ) - assert response.status_code == 404 - result = json.loads(response.get_data(as_text=True)) - assert result['result'] == 'error' - assert result['message'] == 'No result found' + assert response['result'] == 'error' + assert response['message'] == 'No result found' -def test_update_service_reply_to_email_address(client, sample_service): +def test_update_service_reply_to_email_address(admin_request, sample_service): original_reply_to = create_reply_to_email(service=sample_service, email_address="some@email.com") - data = json.dumps({"email_address": "changed@reply.com", "is_default": True}) - response = client.post('/service/{}/email-reply-to/{}'.format(sample_service.id, original_reply_to.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) + data = {"email_address": "changed@reply.com", "is_default": True} + response = admin_request.post( + 'service.update_service_reply_to_email_address', + service_id=sample_service.id, + reply_to_email_id=original_reply_to.id, + _data=data, + _expected_status=200 + ) - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) results = ServiceEmailReplyTo.query.all() assert len(results) == 1 - assert json_resp['data'] == results[0].serialize() + assert response['data'] == results[0].serialize() -def test_update_service_reply_to_email_address_doesnt_allow_duplicates(client, sample_user, mocker): - service = create_service() - original_reply_to = create_reply_to_email(service=service, email_address="some@email.com") - data = json.dumps({"email_address": "changed@reply.com", "is_default": True}) - create_reply_to_email(service, 'changed@reply.com') - response = client.post('/service/{}/email-reply-to/{}'.format(service.id, original_reply_to.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - assert response.status_code == 400 - assert response.json["message"] == "Your service already uses 'changed@reply.com' as an email reply-to address." - - -def test_update_service_reply_to_email_address_returns_400_when_no_default(client, sample_service): +def test_update_service_reply_to_email_address_returns_400_when_no_default(admin_request, sample_service): original_reply_to = create_reply_to_email(service=sample_service, email_address="some@email.com") - data = json.dumps({"email_address": "changed@reply.com", "is_default": False}) - response = client.post('/service/{}/email-reply-to/{}'.format(sample_service.id, original_reply_to.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) + data = {"email_address": "changed@reply.com", "is_default": False} + response = admin_request.post( + 'service.update_service_reply_to_email_address', + service_id=sample_service.id, + reply_to_email_id=original_reply_to.id, + _data=data, + _expected_status=400 + ) - assert response.status_code == 400 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['message'] == 'You must have at least one reply to email address as the default.' + assert response['message'] == 'You must have at least one reply to email address as the default.' -def test_update_service_reply_to_email_address_404s_when_invalid_service_id(client, notify_db, notify_db_session): - response = client.post('/service/{}/email-reply-to/{}'.format(uuid.uuid4(), uuid.uuid4()), - data={}, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) +def test_update_service_reply_to_email_address_404s_when_invalid_service_id( + admin_request, notify_db, notify_db_session +): + response = admin_request.post( + 'service.update_service_reply_to_email_address', + service_id=uuid.uuid4(), + reply_to_email_id=uuid.uuid4(), + _data={}, + _expected_status=404 + ) - assert response.status_code == 404 - result = json.loads(response.get_data(as_text=True)) - assert result['result'] == 'error' - assert result['message'] == 'No result found' + assert response['result'] == 'error' + assert response['message'] == 'No result found' def test_delete_service_reply_to_email_address_archives_an_email_reply_to( From 5f1f688c7baa65654598267626b2c11bf01c0e84 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 22 May 2019 17:30:35 +0100 Subject: [PATCH 4/5] Create template to verify service email reply-to addresses So that template with the same ID is present on all environments --- app/config.py | 2 +- .../versions/0294_add_verify_reply_to_.py | 82 +++++++++++++++++++ tests/app/service/test_rest.py | 5 +- 3 files changed, 85 insertions(+), 4 deletions(-) create mode 100644 migrations/versions/0294_add_verify_reply_to_.py diff --git a/app/config.py b/app/config.py index b2970c1e4..50889727a 100644 --- a/app/config.py +++ b/app/config.py @@ -150,7 +150,7 @@ class Config(object): ORGANISATION_INVITATION_EMAIL_TEMPLATE_ID = '203566f0-d835-47c5-aa06-932439c86573' TEAM_MEMBER_EDIT_EMAIL_TEMPLATE_ID = 'c73f1d71-4049-46d5-a647-d013bdeca3f0' TEAM_MEMBER_EDIT_MOBILE_TEMPLATE_ID = '8a31520f-4751-4789-8ea1-fe54496725eb' - REPLY_TO_EMAIL_ADDRESS_VERIFICATION_TEMPLATE_ID = '2d1ff26c-4d20-4cb2-8918-a0bec4002c9e' + REPLY_TO_EMAIL_ADDRESS_VERIFICATION_TEMPLATE_ID = 'a42f1d17-9404-46d5-a647-d013bdfca3e1' BROKER_URL = 'sqs://' BROKER_TRANSPORT_OPTIONS = { diff --git a/migrations/versions/0294_add_verify_reply_to_.py b/migrations/versions/0294_add_verify_reply_to_.py new file mode 100644 index 000000000..1bb029854 --- /dev/null +++ b/migrations/versions/0294_add_verify_reply_to_.py @@ -0,0 +1,82 @@ +""" + +Revision ID: 0294_add_verify_reply_to +Revises: 0293_drop_complaint_fk +Create Date: 2019-05-22 16:58:52.929661 + +""" +from datetime import datetime + +from alembic import op +from flask import current_app + + +revision = '0294_add_verify_reply_to' +down_revision = '0293_drop_complaint_fk' + +email_template_id = "a42f1d17-9404-46d5-a647-d013bdfca3e1" + + +def upgrade(): + template_insert = """ + INSERT INTO templates (id, name, template_type, created_at, content, archived, service_id, subject, + created_by_id, version, process_type, hidden) + VALUES ('{}', '{}', '{}', '{}', '{}', False, '{}', '{}', '{}', 1, '{}', false) + """ + template_history_insert = """ + INSERT INTO templates_history (id, name, template_type, created_at, content, archived, service_id, subject, + created_by_id, version, process_type, hidden) + VALUES ('{}', '{}', '{}', '{}', '{}', False, '{}', '{}', '{}', 1, '{}', false) + """ + + email_template_content = '\n'.join([ + "Hi,", + "", + "This address has been provided as a reply-to email address for a GOV.​UK Notify account.", + "Any replies from users to emails they receive through GOV.​UK Notify will come back to this email address.", + "", + "This is just a quick check to make sure the address is valid.", + "", + "No need to reply.", + "", + "Thanks", + "", + "GOV.​UK Notify team", + "https://www.gov.uk/notify" + ]) + + email_template_name = "Verify email reply-to address for a service" + email_template_subject = 'Your GOV.UK Notify reply-to email address' + + op.execute( + template_history_insert.format( + email_template_id, + email_template_name, + 'email', + datetime.utcnow(), + email_template_content, + current_app.config['NOTIFY_SERVICE_ID'], + email_template_subject, + current_app.config['NOTIFY_USER_ID'], + 'normal' + ) + ) + + op.execute( + template_insert.format( + email_template_id, + email_template_name, + 'email', + datetime.utcnow(), + email_template_content, + current_app.config['NOTIFY_SERVICE_ID'], + email_template_subject, + current_app.config['NOTIFY_USER_ID'], + 'normal' + ) + ) + + +def downgrade(): + op.execute("DELETE FROM templates_history WHERE id = '{}'".format(email_template_id)) + op.execute("DELETE FROM templates WHERE id = '{}'".format(email_template_id)) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 6121d94a1..7afb7786f 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2491,7 +2491,6 @@ def test_get_email_reply_to_addresses_with_multiple_email_addresses(client, noti assert not json_response[1]['updated_at'] -@freeze_time("2016-01-01 11:09:00.061258") def test_verify_reply_to_email_address_should_send_verification_email( admin_request, notify_db, notify_db_session, mocker, verify_reply_to_address_email_template ): @@ -2523,7 +2522,7 @@ def test_verify_reply_to_email_address_doesnt_allow_duplicates(admin_request, no _data=data, _expected_status=400 ) - assert response["message"] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." + assert response["message"] == "Your service already uses ‘reply-here@example.gov.uk’ as an email reply-to address." def test_add_service_reply_to_email_address(admin_request, sample_service): @@ -2552,7 +2551,7 @@ def test_add_service_reply_to_email_address_doesnt_allow_duplicates( _data=data, _expected_status=400 ) - assert response["message"] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." + assert response["message"] == "Your service already uses ‘reply-here@example.gov.uk’ as an email reply-to address." def test_add_service_reply_to_email_address_can_add_multiple_addresses(admin_request, sample_service): From c4d20667a622bdbfdc78267453f9f07443ddac88 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 28 May 2019 15:25:06 +0100 Subject: [PATCH 5/5] Add notification deletion to downgrade to respect foreign key constraints --- migrations/versions/0294_add_verify_reply_to_.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/migrations/versions/0294_add_verify_reply_to_.py b/migrations/versions/0294_add_verify_reply_to_.py index 1bb029854..8852cab16 100644 --- a/migrations/versions/0294_add_verify_reply_to_.py +++ b/migrations/versions/0294_add_verify_reply_to_.py @@ -78,5 +78,7 @@ def upgrade(): def downgrade(): - op.execute("DELETE FROM templates_history WHERE id = '{}'".format(email_template_id)) + op.execute("DELETE FROM notifications WHERE template_id = '{}'".format(email_template_id)) + op.execute("DELETE FROM notification_history WHERE template_id = '{}'".format(email_template_id)) op.execute("DELETE FROM templates WHERE id = '{}'".format(email_template_id)) + op.execute("DELETE FROM templates_history WHERE id = '{}'".format(email_template_id))