From 3128b5424d30db2f0f420dd23850acd9b764fae1 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 30 Oct 2017 16:59:24 +0000 Subject: [PATCH 01/41] make sure load_service_before_request handles 404s if it 404s, because the service id doesn't exist, then it should die gracefully (showing a 404 error page), rather than what it currently does, which is die kicking and screaming with a 500 --- app/__init__.py | 19 ++++++++++++++++--- tests/app/main/test_errorhandlers.py | 15 +++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 34aa8523e..c27a4db36 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -404,10 +404,23 @@ def load_service_before_request(): if '/static/' in request.url: _request_ctx_stack.top.service = None return - service_id = request.view_args.get('service_id', session.get('service_id')) if request.view_args \ - else session.get('service_id') if _request_ctx_stack.top is not None: - _request_ctx_stack.top.service = service_api_client.get_service(service_id)['data'] if service_id else None + _request_ctx_stack.top.service = None + + if request.view_args: + service_id = request.view_args.get('service_id', session.get('service_id')) + else: + service_id = session.get('service_id') + + if service_id: + try: + _request_ctx_stack.top.service = service_api_client.get_service(service_id)['data'] + except HTTPError as exc: + # if service id isn't real, then 404 rather than 500ing later because we expect service to be set + if exc.status_code == 404: + abort(404) + else: + raise def save_service_after_request(response): diff --git a/tests/app/main/test_errorhandlers.py b/tests/app/main/test_errorhandlers.py index f9185a1d1..62848a5ff 100644 --- a/tests/app/main/test_errorhandlers.py +++ b/tests/app/main/test_errorhandlers.py @@ -1,4 +1,6 @@ +from flask import Response from bs4 import BeautifulSoup +from notifications_python_client.errors import HTTPError def test_bad_url_returns_page_not_found(client): @@ -6,3 +8,16 @@ def test_bad_url_returns_page_not_found(client): assert response.status_code == 404 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.string.strip() == 'Page could not be found' + + +def test_load_service_before_request_handles_404(client_request, mocker): + exc = HTTPError(Response(status=404), 'Not found') + get_service = mocker.patch('app.service_api_client.get_service', side_effect=exc) + + client_request.get( + 'main.service_dashboard', + service_id='00000000-0000-0000-0000-000000000000', + _expected_status=404 + ) + + get_service.assert_called_once_with('00000000-0000-0000-0000-000000000000') From 061ef3dddc6f38b5c80901e2942f240b1a36f63e Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Tue, 31 Oct 2017 12:27:34 +0000 Subject: [PATCH 02/41] 98 to 100 services..... woo hoo.... Also corrected the org count to 44 as HM Passports Office isn't a separate org from Home Office. --- app/templates/views/signedout.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index a9d05fa34..e45a032eb 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -116,12 +116,12 @@
-
98
+
100
services
-
45
+
44
organisations
From 57ca2b48eeac356048523e4f68e6f9abc61e1570 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sat, 28 Oct 2017 18:08:18 +0100 Subject: [PATCH 03/41] Add extra letter spacing to phone number search This is another place where you might be transcribing a phone number and having it spaced out will make it easier for you to spot errors. --- app/templates/views/notifications.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index 40bf914a9..b76641b34 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -25,7 +25,7 @@ action="{{ url_for('.view_notifications', service_id=current_service.id, message_type=message_type) }}" class="grid-row" > -
+
{{ textbox( search_form.to, width='1-1', From fafb8dc75b14116106d1708a1c22a0c07405963a Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Wed, 1 Nov 2017 14:50:15 +0000 Subject: [PATCH 04/41] 100-102 for GOV.UK Email and SSCSA --- app/templates/views/signedout.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index e45a032eb..607352fb7 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -116,7 +116,7 @@
-
100
+
102
services
From aff9d473233e6e3dbeb21a4f68092651ef0670f8 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 1 Nov 2017 14:39:14 +0000 Subject: [PATCH 05/41] don't hit API when checking new account email-token we currently store new account email verify tokens in the database, and check against that to work out if they've expired. But we don't need to do that, tokens have their own timing mechanism. So lets just use that, and free up the database to do other things. Also, standardised the forgot password, change email, and new account email verification timeouts to all be an hour, from the config val 'EMAIL_EXPIRY_SECONDS' --- app/config.py | 3 +- app/main/views/new_password.py | 8 ++-- app/main/views/verify.py | 50 ++++++++++------------- tests/app/main/views/test_new_password.py | 13 +++--- tests/app/main/views/test_verify.py | 37 ++++------------- 5 files changed, 41 insertions(+), 70 deletions(-) diff --git a/app/config.py b/app/config.py index 2cd1bc3fb..dce3d4104 100644 --- a/app/config.py +++ b/app/config.py @@ -41,7 +41,7 @@ class Config(object): 'local': 25000, 'nhs': 25000, } - EMAIL_EXPIRY_SECONDS = 3600 * 24 * 7 # one week + EMAIL_EXPIRY_SECONDS = 3600 # 1 hour HEADER_COLOUR = '#FFBF47' # $yellow HTTP_PROTOCOL = 'http' MAX_FAILED_LOGIN_COUNT = 10 @@ -56,7 +56,6 @@ class Config(object): SHOW_STYLEGUIDE = True # TODO: move to utils SMS_CHAR_COUNT_LIMIT = 459 - TOKEN_MAX_AGE_SECONDS = 3600 WTF_CSRF_ENABLED = True WTF_CSRF_TIME_LIMIT = None CSV_UPLOAD_BUCKET_NAME = 'local-notifications-csv-upload' diff --git a/app/main/views/new_password.py b/app/main/views/new_password.py index c615c5ea8..84f5051d8 100644 --- a/app/main/views/new_password.py +++ b/app/main/views/new_password.py @@ -1,20 +1,20 @@ +from datetime import datetime import json from flask import (render_template, url_for, redirect, flash, session, current_app) from itsdangerous import SignatureExpired +from notifications_utils.url_safe_token import check_token +from app import user_api_client from app.main import main from app.main.forms import NewPasswordForm -from datetime import datetime -from app import user_api_client @main.route('/new-password/', methods=['GET', 'POST']) def new_password(token): - from notifications_utils.url_safe_token import check_token try: token_data = check_token(token, current_app.config['SECRET_KEY'], current_app.config['DANGEROUS_SALT'], - current_app.config['TOKEN_MAX_AGE_SECONDS']) + current_app.config['EMAIL_EXPIRY_SECONDS']) except SignatureExpired: flash('The link in the email we sent you has expired. Enter your email address to resend.') return redirect(url_for('.forgot_password')) diff --git a/app/main/views/verify.py b/app/main/views/verify.py index a3163a538..e94af163e 100644 --- a/app/main/views/verify.py +++ b/app/main/views/verify.py @@ -50,34 +50,26 @@ def verify(): @main.route('/verify-email/') def verify_email(token): try: - token_data = check_token(token, - current_app.config['SECRET_KEY'], - current_app.config['DANGEROUS_SALT'], - current_app.config['EMAIL_EXPIRY_SECONDS']) - - token_data = json.loads(token_data) - verified = user_api_client.check_verify_code(token_data['user_id'], token_data['secret_code'], 'email') - user = user_api_client.get_user(token_data['user_id']) - if not user: - abort(404) - - if user.is_active: - flash("That verification link has expired.") - return redirect(url_for('main.sign_in')) - - session['user_details'] = {"email": user.email_address, "id": user.id} - if verified[0]: - user_api_client.send_verify_code(user.id, 'sms', user.mobile_number) - return redirect('verify') - else: - if verified[1] == 'Code has expired': - flash("The link in the email we sent you has expired. We've sent you a new one.") - return redirect(url_for('main.resend_email_verification')) - else: - message = "There was a problem verifying your account. Error message: '{}'".format(verified[1]) - flash(message) - return redirect(url_for('main.index')) - + token_data = check_token( + token, + current_app.config['SECRET_KEY'], + current_app.config['DANGEROUS_SALT'], + current_app.config['EMAIL_EXPIRY_SECONDS'] + ) except SignatureExpired: - flash('The link in the email we sent you has expired') + flash("The link in the email we sent you has expired. We've sent you a new one.") return redirect(url_for('main.resend_email_verification')) + + # token contains json blob of format: {'user_id': '...', 'secret_code': '...'} (secret_code is unused) + token_data = json.loads(token_data) + user = user_api_client.get_user(token_data['user_id']) + if not user: + abort(404) + + if user.is_active: + flash("That verification link has expired.") + return redirect(url_for('main.sign_in')) + + session['user_details'] = {"email": user.email_address, "id": user.id} + user_api_client.send_verify_code(user.id, 'sms', user.mobile_number) + return redirect('verify') diff --git a/tests/app/main/views/test_new_password.py b/tests/app/main/views/test_new_password.py index e18601970..6efbeec87 100644 --- a/tests/app/main/views/test_new_password.py +++ b/tests/app/main/views/test_new_password.py @@ -1,6 +1,7 @@ import json from datetime import datetime +from itsdangerous import SignatureExpired from flask import url_for from notifications_utils.url_safe_token import generate_token @@ -69,13 +70,13 @@ def test_should_redirect_index_if_user_has_already_changed_password( def test_should_redirect_to_forgot_password_with_flash_message_when_token_is_expired( app_, client, - mock_get_user_by_email_request_password_reset, mock_login, + mocker ): - app_.config['TOKEN_MAX_AGE_SECONDS'] = -1000 - user = mock_get_user_by_email_request_password_reset.return_value - token = generate_token(user.email_address, app_.config['SECRET_KEY'], app_.config['DANGEROUS_SALT']) - response = client.post(url_for('.new_password', token=token), data={'new_password': 'a-new_password'}) + mocker.patch('app.main.views.new_password.check_token', side_effect=SignatureExpired('expired')) + token = generate_token('foo@bar.com', app_.config['SECRET_KEY'], app_.config['DANGEROUS_SALT']) + + response = client.get(url_for('.new_password', token=token)) + assert response.status_code == 302 assert response.location == url_for('.forgot_password', _external=True) - app_.config['TOKEN_MAX_AGE_SECONDS'] = 3600 diff --git a/tests/app/main/views/test_verify.py b/tests/app/main/views/test_verify.py index ce27250b3..5cda6794f 100644 --- a/tests/app/main/views/test_verify.py +++ b/tests/app/main/views/test_verify.py @@ -1,6 +1,7 @@ import uuid import json +from itsdangerous import SignatureExpired from flask import url_for from bs4 import BeautifulSoup @@ -97,7 +98,7 @@ def test_verify_email_redirects_to_verify_if_token_valid( mock_send_verify_code, mock_check_verify_code, ): - token_data = {"user_id": api_user_pending.id, "secret_code": 12345} + token_data = {"user_id": api_user_pending.id, "secret_code": 'UNUSED'} mocker.patch('app.main.views.verify.check_token', return_value=json.dumps(token_data)) with client.session_transaction() as session: @@ -108,39 +109,20 @@ def test_verify_email_redirects_to_verify_if_token_valid( assert response.status_code == 302 assert response.location == url_for('main.verify', _external=True) + assert not mock_check_verify_code.called + mock_send_verify_code.assert_called_once_with(api_user_pending.id, 'sms', api_user_pending.mobile_number) + + with client.session_transaction() as session: + assert session['user_details'] == {'email': api_user_pending.email_address, 'id': api_user_pending.id} + def test_verify_email_redirects_to_email_sent_if_token_expired( client, mocker, api_user_pending, - mock_check_verify_code, ): - from itsdangerous import SignatureExpired mocker.patch('app.main.views.verify.check_token', side_effect=SignatureExpired('expired')) - with client.session_transaction() as session: - session['user_details'] = {'email_address': api_user_pending.email_address, 'id': api_user_pending.id} - - response = client.get(url_for('main.verify_email', token='notreal')) - - assert response.status_code == 302 - assert response.location == url_for('main.resend_email_verification', _external=True) - - -def test_verify_email_redirects_to_email_sent_if_token_used( - client, - mocker, - api_user_pending, - mock_get_user_pending, - mock_send_verify_code, - mock_check_verify_code_code_expired, -): - from itsdangerous import SignatureExpired - mocker.patch('app.main.views.verify.check_token', side_effect=SignatureExpired('expired')) - - with client.session_transaction() as session: - session['user_details'] = {'email_address': api_user_pending.email_address, 'id': api_user_pending.id} - response = client.get(url_for('main.verify_email', token='notreal')) assert response.status_code == 302 @@ -158,9 +140,6 @@ def test_verify_email_redirects_to_sign_in_if_user_active( token_data = {"user_id": api_user_active.id, "secret_code": 12345} mocker.patch('app.main.views.verify.check_token', return_value=json.dumps(token_data)) - with client.session_transaction() as session: - session['user_details'] = {'email_address': api_user_active.email_address, 'id': api_user_active.id} - response = client.get(url_for('main.verify_email', token='notreal'), follow_redirects=True) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.text == 'Sign in' From 19f731ec0754bfb8e36b61eff06c576afa59c802 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 1 Nov 2017 15:47:05 +0000 Subject: [PATCH 06/41] add error handler that catches invalid tokens, and returns 404 --- app/__init__.py | 10 +++++++++- tests/app/main/test_errorhandlers.py | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/app/__init__.py b/app/__init__.py index 34aa8523e..ce425152b 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -5,6 +5,7 @@ from time import monotonic import itertools import ago +from itsdangerous import BadSignature from flask import ( Flask, session, @@ -13,7 +14,8 @@ from flask import ( current_app, request, g, - url_for + url_for, + flash ) from flask._compat import string_types from flask.globals import _lookup_req_object, _request_ctx_stack @@ -492,6 +494,12 @@ def register_errorhandlers(application): raise error return _error_response(500) + @application.errorhandler(BadSignature) + def handle_bad_token(error): + # if someone has a malformed token + flash('There’s something wrong with the link you’ve used.') + return _error_response(404) + def setup_event_handlers(): from flask_login import user_logged_in diff --git a/tests/app/main/test_errorhandlers.py b/tests/app/main/test_errorhandlers.py index f9185a1d1..92d712259 100644 --- a/tests/app/main/test_errorhandlers.py +++ b/tests/app/main/test_errorhandlers.py @@ -1,3 +1,4 @@ +import pytest from bs4 import BeautifulSoup @@ -6,3 +7,19 @@ def test_bad_url_returns_page_not_found(client): assert response.status_code == 404 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.string.strip() == 'Page could not be found' + + +@pytest.mark.parametrize('url', [ + '/invitation/MALFORMED_TOKEN', + '/new-password/MALFORMED_TOKEN', + '/user-profile/email/confirm/MALFORMED_TOKEN', + '/verify-email/MALFORMED_TOKEN' +]) +def test_malformed_token_returns_page_not_found(client, url): + response = client.get(url) + + assert response.status_code == 404 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Page could not be found' + flash_banner = page.find('div', class_='banner-dangerous').string.strip() + assert flash_banner == "There’s something wrong with the link you’ve used." From 9eb5e6a532c5d7438ec36fc5458644450d45a444 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 1 Nov 2017 16:02:05 +0000 Subject: [PATCH 07/41] make sure invite tokens still check token on admin for error handler to kick in --- app/__init__.py | 2 +- app/main/views/invites.py | 15 ++++++++++----- tests/app/main/test_errorhandlers.py | 4 ++-- tests/app/main/views/test_accept_invite.py | 20 +++++++++++++++++--- 4 files changed, 30 insertions(+), 11 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index ce425152b..4de099f5c 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -440,7 +440,7 @@ def useful_headers_after_request(response): return response -def register_errorhandlers(application): +def register_errorhandlers(application): # noqa (C901 too complex) def _error_response(error_code): application.logger.exception('Admin app errored with %s', error_code) resp = make_response(render_template("error/{0}.html".format(error_code)), error_code) diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 41cbc8c72..af3d0eede 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -4,24 +4,29 @@ from flask import ( session, flash, render_template, - abort + abort, + current_app ) from markupsafe import Markup +from notifications_utils.url_safe_token import check_token +from flask_login import current_user from app.main import main - from app import ( invite_api_client, user_api_client, service_api_client ) -from flask_login import current_user - @main.route("/invitation/") def accept_invite(token): - + check_token( + token, + current_app.config['SECRET_KEY'], + current_app.config['DANGEROUS_SALT'], + current_app.config['EMAIL_EXPIRY_SECONDS'] + ) invited_user = invite_api_client.check_token(token) if not current_user.is_anonymous and current_user.email_address != invited_user.email_address: diff --git a/tests/app/main/test_errorhandlers.py b/tests/app/main/test_errorhandlers.py index 92d712259..93bbfe3e1 100644 --- a/tests/app/main/test_errorhandlers.py +++ b/tests/app/main/test_errorhandlers.py @@ -15,8 +15,8 @@ def test_bad_url_returns_page_not_found(client): '/user-profile/email/confirm/MALFORMED_TOKEN', '/verify-email/MALFORMED_TOKEN' ]) -def test_malformed_token_returns_page_not_found(client, url): - response = client.get(url) +def test_malformed_token_returns_page_not_found(logged_in_client, url): + response = logged_in_client.get(url) assert response.status_code == 404 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index aa7b14da7..fe4058f66 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -13,14 +13,14 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( client, service_one, api_user_active, - sample_invite, - mock_get_service, mock_check_invite_token, mock_get_user_by_email, mock_get_users_by_service, mock_accept_invite, mock_add_user_to_service, + mocker, ): + mocker.patch('app.main.views.invites.check_token') expected_service = service_one['id'] expected_redirect_location = 'http://localhost/services/{}/dashboard'.format(expected_service) @@ -47,8 +47,8 @@ def test_existing_user_with_no_permissions_accept_invite( mock_get_user_by_email, mock_get_users_by_service, mock_add_user_to_service, - mock_get_service, ): + mocker.patch('app.main.views.invites.check_token') expected_service = service_one['id'] sample_invite['permissions'] = '' @@ -67,6 +67,7 @@ def test_if_existing_user_accepts_twice_they_redirect_to_sign_in( sample_invite, mock_get_service, ): + mocker.patch('app.main.views.invites.check_token') sample_invite['status'] = 'accepted' invite = InvitedUser(**sample_invite) @@ -93,6 +94,7 @@ def test_existing_user_of_service_get_redirected_to_signin( mock_get_user_by_email, mock_accept_invite, ): + mocker.patch('app.main.views.invites.check_token') sample_invite['email_address'] = api_user_active.email_address invite = InvitedUser(**sample_invite) mocker.patch('app.invite_api_client.check_token', return_value=invite) @@ -122,7 +124,9 @@ def test_existing_signed_out_user_accept_invite_redirects_to_sign_in( mock_add_user_to_service, mock_accept_invite, mock_get_service, + mocker, ): + mocker.patch('app.main.views.invites.check_token') expected_service = service_one['id'] expected_permissions = ['send_messages', 'manage_service', 'manage_api_keys'] @@ -153,7 +157,9 @@ def test_new_user_accept_invite_calls_api_and_redirects_to_registration( mock_add_user_to_service, mock_get_users_by_service, mock_get_service, + mocker, ): + mocker.patch('app.main.views.invites.check_token') expected_redirect_location = 'http://localhost/register-from-invite' @@ -174,7 +180,9 @@ def test_new_user_accept_invite_calls_api_and_views_registration_page( mock_add_user_to_service, mock_get_users_by_service, mock_get_service, + mocker, ): + mocker.patch('app.main.views.invites.check_token') response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'), follow_redirects=True) @@ -209,6 +217,7 @@ def test_cancelled_invited_user_accepts_invited_redirect_to_cancelled_invitation mock_get_user, mock_get_service, ): + mocker.patch('app.main.views.invites.check_token') cancelled_invitation = create_sample_invite(mocker, service_one, status='cancelled') mock_check_token_invite(mocker, cancelled_invitation) response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) @@ -233,7 +242,9 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify( mock_get_users_by_service, mock_add_user_to_service, mock_get_service, + mocker, ): + mocker.patch('app.main.views.invites.check_token') expected_service = service_one['id'] expected_email = sample_invite['email_address'] @@ -282,6 +293,7 @@ def test_signed_in_existing_user_cannot_use_anothers_invite( mock_accept_invite, mock_get_service, ): + mocker.patch('app.main.views.invites.check_token') invite = InvitedUser(**sample_invite) mocker.patch('app.invite_api_client.check_token', return_value=invite) mocker.patch('app.user_api_client.get_users_for_service', return_value=[api_user_active]) @@ -322,7 +334,9 @@ def test_new_invited_user_verifies_and_added_to_service( mock_get_users_by_service, mock_get_detailed_service, mock_get_usage, + mocker, ): + mocker.patch('app.main.views.invites.check_token') # visit accept token page response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) From 4acfd4101f603fecb8f992f472f8703a5b0bf0a0 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 1 Nov 2017 16:33:06 +0000 Subject: [PATCH 08/41] Add letter counts to the platform admin page. The big number counts are based on how many messages have been delivered. For letters we are using the requested count. --- app/main/views/platform_admin.py | 1 - .../views/platform-admin/_global_stats.html | 4 ++-- app/templates/views/platform-admin/services.html | 13 +++++++++++-- tests/app/main/views/test_platform_admin.py | 4 ++-- 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 24195fd07..5db76551f 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -113,7 +113,6 @@ def create_global_stats(services): 'requested': 0 } } - for service in services: for msg_type, status in itertools.product(('sms', 'email', 'letter'), ('delivered', 'failed', 'requested')): stats[msg_type][status] += service['statistics'][msg_type][status] diff --git a/app/templates/views/platform-admin/_global_stats.html b/app/templates/views/platform-admin/_global_stats.html index 0a8f8d688..5cfe1c50b 100644 --- a/app/templates/views/platform-admin/_global_stats.html +++ b/app/templates/views/platform-admin/_global_stats.html @@ -23,8 +23,8 @@
{{ big_number_with_status( - global_stats.letter.delivered + global_stats.letter.failed, - message_count_label(global_stats.letter.delivered, 'letter'), + global_stats.letter.requested, + message_count_label(global_stats.letter.requested, 'letter'), global_stats.letter.failed, global_stats.letter.failure_rate, global_stats.letter.failure_rate|float > 3, diff --git a/app/templates/views/platform-admin/services.html b/app/templates/views/platform-admin/services.html index d866658d1..237e82923 100644 --- a/app/templates/views/platform-admin/services.html +++ b/app/templates/views/platform-admin/services.html @@ -54,7 +54,7 @@ {% call row() %} {% if not service['active'] %} - {% call field(status='default') %} + {% call field(status='default', border=False) %} archived {% endcall %} {% elif service['research_mode'] %} @@ -62,7 +62,7 @@ research mode {% endcall %} {% elif not service['restricted'] %} - {% call field(status='error') %} + {% call field(status='error', border=False) %} Live {% endcall %} {% else %} @@ -72,6 +72,15 @@ {{ stats_fields('sms', service['stats']) }} {% endcall %} + {% call row() %} + + {% call field(border=False) %} + + {% endcall %} + {{ stats_fields('letter', service['stats']) }} + + {% endcall %} + {% endcall %} {% endfor %} diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index 07f89079b..62ee4d451 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -97,7 +97,7 @@ def test_should_render_platform_admin_page( response = client.get(url_for(endpoint)) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert len(page.select('tbody tr')) == expected_services_shown * 2 # one row for SMS, one for email + assert len(page.select('tbody tr')) == expected_services_shown * 3 # one row for SMS, one for email, one for letter mock_get_detailed_services.assert_called_once_with({'detailed': True, 'include_from_test_key': True, 'only_active': False}) @@ -543,7 +543,7 @@ def test_should_show_correct_sent_totals_for_platform_admin( assert email_total == 60 assert sms_total == 40 - assert letter_total == 45 + assert letter_total == 60 @pytest.mark.parametrize('endpoint, restricted, research_mode', [ From 2ea921952f505747c69058f0e44ef2e37f492a88 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 2 Nov 2017 12:07:46 +0000 Subject: [PATCH 09/41] This PR adds the ability to select a text message sender if more than one exist for the service. --- app/main/views/send.py | 48 ++++++++++++++++---- app/notify_client/notification_api_client.py | 1 + app/utils.py | 3 +- tests/app/main/views/test_send.py | 46 +++++++++++++++++-- tests/app/main/views/test_templates.py | 5 +- 5 files changed, 88 insertions(+), 15 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 5c8555970..63863f0f7 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -178,7 +178,7 @@ def set_sender(service_id, template_id): template = service_api_client.get_service_template(service_id, template_id)['data'] - if template['template_type'] != 'email': + if template['template_type'] == 'letter': return redirect_to_one_off sender_details = get_sender_details(service_id, template['template_type']) @@ -220,6 +220,11 @@ def get_sender_context(sender_details, template_type): 'title': 'Choose sender address', 'description': 'Select an address that recipients can reply to', 'field_name': 'contact_block' + }, + 'sms': { + 'title': 'Chose text message sender', + 'description': 'Select a text message sender that the recipients can reply to', + 'field_name': 'sms_sender' } }[template_type] @@ -233,7 +238,8 @@ def get_sender_context(sender_details, template_type): def get_sender_details(service_id, template_type): api_call = { 'email': service_api_client.get_reply_to_email_addresses, - 'letter': service_api_client.get_letter_contacts + 'letter': service_api_client.get_letter_contacts, + 'sms': service_api_client.get_sms_senders }[template_type] return api_call(service_id) @@ -248,7 +254,7 @@ def send_test(service_id, template_id): session['send_test_letter_page_count'] = None db_template = service_api_client.get_service_template(service_id, template_id)['data'] - if db_template['template_type'] != 'email': + if db_template['template_type'] == 'letter': session['sender_id'] = None if email_or_sms_not_enabled(db_template['template_type'], current_service['permissions']): @@ -311,7 +317,12 @@ def send_test_step(service_id, template_id, step_index): if not session.get('send_test_letter_page_count'): session['send_test_letter_page_count'] = get_page_count_for_letter(db_template) - + email_reply_to = None + sms_sender = None + if db_template['template_type'] == 'email': + email_reply_to = get_email_reply_to_address_from_session(service_id) + elif db_template['template_type'] == 'sms': + sms_sender = get_sms_sender_from_session(service_id) template = get_template( db_template, current_service, @@ -324,7 +335,8 @@ def send_test_step(service_id, template_id, step_index): filetype='png', ), page_count=session['send_test_letter_page_count'], - email_reply_to=get_email_reply_to_address_from_session(service_id), + email_reply_to=email_reply_to, + sms_sender=sms_sender ) placeholders = fields_to_fill_in( @@ -449,6 +461,12 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): remaining_messages = (current_service['message_limit'] - sum(stat['requested'] for stat in statistics.values())) contents = s3download(service_id, upload_id) + email_reply_to = None + sms_sender = None + if template_type == 'email': + email_reply_to = get_email_reply_to_address_from_session(service_id) + elif template_type == 'sms': + sms_sender = get_sms_sender_from_session(service_id) template = get_template( service_api_client.get_service_template( service_id, @@ -463,7 +481,8 @@ def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False): upload_id=upload_id, filetype='png', ) if not letters_as_pdf else None, - email_reply_to=get_email_reply_to_address_from_session(service_id), + email_reply_to=email_reply_to, + sms_sender=sms_sender ) recipients = RecipientCSV( contents, @@ -723,12 +742,18 @@ def check_notification(service_id, template_id): def _check_notification(service_id, template_id, exception=None): db_template = service_api_client.get_service_template(service_id, template_id)['data'] - + email_reply_to = None + sms_sender = None + if db_template['template_type'] == 'email': + email_reply_to = get_email_reply_to_address_from_session(service_id) + elif db_template['template_type'] == 'sms': + sms_sender = get_sms_sender_from_session(service_id) template = get_template( db_template, current_service, show_recipient=True, - email_reply_to=get_email_reply_to_address_from_session(service_id), + email_reply_to=email_reply_to, + sms_sender=sms_sender ) # go back to start of process @@ -814,3 +839,10 @@ def get_email_reply_to_address_from_session(service_id): return service_api_client.get_reply_to_email_address( service_id, session['sender_id'] )['email_address'] + + +def get_sms_sender_from_session(service_id): + if session.get('sender_id'): + return service_api_client.get_sms_sender( + service_id=service_id, sms_sender_id=session['sender_id'] + )['sms_sender'] diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index 195d089ac..d9ce5a8d8 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -61,6 +61,7 @@ class NotificationApiClient(NotifyAdminAPIClient): 'personalisation': personalisation, } if sender_id: + print(sender_id) data['sender_id'] = sender_id data = _attach_current_user(data) return self.post(url='/service/{}/send-notification'.format(service_id), data=data) diff --git a/app/utils.py b/app/utils.py index d7ad0ce15..f9ce51bd8 100644 --- a/app/utils.py +++ b/app/utils.py @@ -272,6 +272,7 @@ def get_template( page_count=1, redact_missing_personalisation=False, email_reply_to=None, + sms_sender=None ): if 'email' == template['template_type']: return EmailPreviewTemplate( @@ -287,7 +288,7 @@ def get_template( return SMSPreviewTemplate( template, prefix=service['name'], - sender=(service['sms_sender'] not in {'GOVUK', None}), + sender=sms_sender if sms_sender else (service['sms_sender'] not in {'GOVUK', None}), show_recipient=show_recipient, redact_missing_personalisation=redact_missing_personalisation, ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 82c1a0ef5..2880743a4 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -27,6 +27,8 @@ from tests.conftest import ( mock_get_live_service, multiple_reply_to_email_addresses, no_reply_to_email_addresses, + multiple_sms_senders, + no_sms_senders ) template_types = ['email', 'sms'] @@ -43,6 +45,12 @@ test_non_spreadsheet_files = glob(path.join('tests', 'non_spreadsheet_files', '* 'Choose where to send replies', 'Select an email address that recipients can reply to' ), + ( + mock_get_service_template, + multiple_sms_senders, + 'Chose text message sender', + 'Select a text message sender that the recipients can reply to' + ) ]) def test_show_correct_title_and_description_for_sender_type( client_request, @@ -67,13 +75,26 @@ def test_show_correct_title_and_description_for_sender_type( assert normalize_spaces(page.select_one('legend').text) == expected_description +@pytest.mark.parametrize('template_mock, sender_data', [ + ( + mock_get_service_email_template, + multiple_reply_to_email_addresses, + ), + ( + mock_get_service_template, + multiple_sms_senders + ) +]) def test_default_sender_is_checked_and_has_hint( client_request, service_one, fake_uuid, - mock_get_service_email_template, - multiple_reply_to_email_addresses + template_mock, + sender_data, + mocker ): + template_mock(mocker) + sender_data(mocker) page = client_request.get( '.set_sender', service_id=service_one['id'], @@ -86,13 +107,26 @@ def test_default_sender_is_checked_and_has_hint( assert not page.select('.multiple-choice input')[2].has_attr('checked') +@pytest.mark.parametrize('template_mock, sender_data', [ + ( + mock_get_service_email_template, + multiple_reply_to_email_addresses, + ), + ( + mock_get_service_template, + multiple_sms_senders + ) +]) def test_sender_session_is_present_after_selected( logged_in_client, service_one, fake_uuid, - mock_get_service_email_template, - multiple_reply_to_email_addresses + template_mock, + sender_data, + mocker ): + template_mock(mocker) + sender_data(mocker) logged_in_client.post( url_for('.set_sender', service_id=service_one['id'], template_id=fake_uuid), data={'sender': '1234'} @@ -107,6 +141,10 @@ def test_sender_session_is_present_after_selected( mock_get_service_email_template, no_reply_to_email_addresses, ), + ( + mock_get_service_template, + no_sms_senders + ) ]) def test_set_sender_redirects_if_no_sender_data( logged_in_client, diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 8a11e6283..cc3974357 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -335,13 +335,14 @@ def test_should_not_allow_creation_of_a_template_without_correct_permission( @pytest.mark.parametrize('fixture, expected_status_code', [ (mock_get_service_email_template, 200), - (mock_get_service_template, 302), + (mock_get_service_template, 200), (mock_get_service_letter_template, 302), ]) -def test_should_redirect_to_one_off_if_template_type_is_not_email( +def test_should_redirect_to_one_off_if_template_type_is_letter( logged_in_client, active_user_with_permissions, multiple_reply_to_email_addresses, + multiple_sms_senders, service_one, fake_uuid, mocker, From c6ea90a7d829ab61766b3c20bb56035f93de0dd3 Mon Sep 17 00:00:00 2001 From: chrisw Date: Wed, 1 Nov 2017 15:36:27 +0000 Subject: [PATCH 10/41] Email auth for inviting members and editing permissions --- .../stylesheets/components/tick-cross.scss | 5 + app/main/forms.py | 8 + app/main/views/manage_users.py | 38 +++- app/notify_client/invite_api_client.py | 5 +- app/notify_client/models.py | 15 +- app/notify_client/user_api_client.py | 3 +- app/templates/views/manage-users.html | 18 ++ .../views/manage-users/permissions.html | 5 + tests/__init__.py | 4 +- tests/app/main/views/test_manage_users.py | 198 +++++++++++++++++- tests/conftest.py | 49 ++++- 11 files changed, 325 insertions(+), 23 deletions(-) diff --git a/app/assets/stylesheets/components/tick-cross.scss b/app/assets/stylesheets/components/tick-cross.scss index 4f80e08b3..4cf3f1151 100644 --- a/app/assets/stylesheets/components/tick-cross.scss +++ b/app/assets/stylesheets/components/tick-cross.scss @@ -63,6 +63,11 @@ right: -135px; } + &-hint { + color: #6F777B; + padding-top: 5px; + } + } } diff --git a/app/main/forms.py b/app/main/forms.py index 75d15fb16..d100b6f2d 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -186,6 +186,14 @@ class PermissionsForm(Form): manage_templates = BooleanField("Add and edit templates") manage_service = BooleanField("Modify this service and its team") manage_api_keys = BooleanField("Create and revoke API keys") + login_authentication = RadioField( + 'Sign in using', + choices=[ + ('sms_auth', 'Text message code'), + ('email_auth', 'Email link'), + ], + validators=[DataRequired()] + ) class InviteUserForm(PermissionsForm): diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 8eae948b3..40e1dc047 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -19,7 +19,7 @@ from app.main.forms import ( InviteUserForm, PermissionsForm ) -from app import (user_api_client, service_api_client, invite_api_client) +from app import (user_api_client, current_service, service_api_client, invite_api_client) from app.utils import user_has_permissions @@ -38,6 +38,7 @@ def manage_users(service_id): users = user_api_client.get_users_for_service(service_id=service_id) invited_users = [invite for invite in invite_api_client.get_invites_for_service(service_id=service_id) if invite.status != 'accepted'] + return render_template( 'views/manage-users.html', users=users, @@ -51,7 +52,13 @@ def manage_users(service_id): @user_has_permissions('manage_users', admin_override=True) def invite_user(service_id): - form = InviteUserForm(invalid_email_address=current_user.email_address) + form = InviteUserForm( + invalid_email_address=current_user.email_address + ) + + service_has_email_auth = 'email_auth' in current_service['permissions'] + if not service_has_email_auth: + form.login_authentication.data = 'sms_auth' if form.validate_on_submit(): email_address = form.email_address.data @@ -60,7 +67,8 @@ def invite_user(service_id): current_user.id, service_id, email_address, - permissions + permissions, + form.login_authentication.data ) flash('Invite sent to {}'.format(invited_user.email_address), 'default_with_tick') @@ -68,7 +76,8 @@ def invite_user(service_id): return render_template( 'views/invite-user.html', - form=form + form=form, + service_has_email_auth=service_has_email_auth ) @@ -76,26 +85,35 @@ def invite_user(service_id): @login_required @user_has_permissions('manage_users', admin_override=True) def edit_user_permissions(service_id, user_id): + service_has_email_auth = 'email_auth' in current_service['permissions'] # TODO we should probably using the service id here in the get user # call as well. eg. /user/?&service=service_id user = user_api_client.get_user(user_id) - # Need to make the email address read only, or a disabled field? - # Do it through the template or the form class? - form = PermissionsForm(**{ - role: user.has_permissions(permissions=permissions) for role, permissions in roles.items() - }) + user_has_no_mobile_number = user.mobile_number is None + + form = PermissionsForm( + **{role: user.has_permissions(permissions=permissions) for role, permissions in roles.items()}, + login_authentication=user.auth_type + ) if form.validate_on_submit(): user_api_client.set_user_permissions( user_id, service_id, permissions=set(get_permissions_from_form(form)), ) + if service_has_email_auth: + user_api_client.update_user_attribute( + user_id, + auth_type=form.login_authentication.data + ) return redirect(url_for('.manage_users', service_id=service_id)) return render_template( 'views/edit-user-permissions.html', user=user, - form=form + form=form, + service_has_email_auth=service_has_email_auth, + user_has_no_mobile_number=user_has_no_mobile_number ) diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index eae4fc2d6..352aaef77 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -12,12 +12,13 @@ class InviteApiClient(NotifyAdminAPIClient): self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] self.api_key = app.config['ADMIN_CLIENT_SECRET'] - def create_invite(self, invite_from_id, service_id, email_address, permissions): + def create_invite(self, invite_from_id, service_id, email_address, permissions, auth_type): data = { 'service': str(service_id), 'email_address': email_address, 'from_user': invite_from_id, - 'permissions': permissions + 'permissions': permissions, + 'auth_type': auth_type } data = _attach_current_user(data) resp = self.post(url='/service/{}/invite'.format(service_id), data=data) diff --git a/app/notify_client/models.py b/app/notify_client/models.py index bedbbd051..fe9775a36 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -10,6 +10,7 @@ class User(UserMixin): self._mobile_number = fields.get('mobile_number') self._password_changed_at = fields.get('password_changed_at') self._permissions = fields.get('permissions') + self._auth_type = fields.get('auth_type') self._failed_login_count = fields.get('failed_login_count') self._state = fields.get('state') self.max_failed_login_count = max_failed_login_count @@ -108,6 +109,14 @@ class User(UserMixin): return set(self._permissions[service_id]) >= set(permissions) return False + @property + def auth_type(self): + return self._auth_type + + @auth_type.setter + def auth_type(self, auth_type): + self._auth_type = auth_type + @property def failed_login_count(self): return self._failed_login_count @@ -155,6 +164,7 @@ class InvitedUser(object): self.permissions = [] self.status = status self.created_at = created_at + self.auth_type = auth_type def has_permissions(self, permissions): return set(self.permissions) > set(permissions) @@ -164,10 +174,12 @@ class InvitedUser(object): self.service, self.from_user, self.email_address, + self.auth_type, self.status) == (other.id, other.service, other.from_user, other.email_address, + other.auth_type, other.status)) def serialize(self, permissions_as_string=False): @@ -176,7 +188,8 @@ class InvitedUser(object): 'from_user': self.from_user, 'email_address': self.email_address, 'status': self.status, - 'created_at': str(self.created_at) + 'created_at': str(self.created_at), + 'auth_type': self.auth_type } if permissions_as_string: data['permissions'] = ','.join(self.permissions) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 887339fab..3e3170689 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -6,7 +6,8 @@ from app.notify_client.models import User ALLOWED_ATTRIBUTES = { 'name', 'email_address', - 'mobile_number' + 'mobile_number', + 'auth_type', } diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 373c5b3a6..9874e045e 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -63,6 +63,15 @@ user.has_permissions(permissions=['manage_api_keys']), 'Access API keys' ) }} + {% if 'email_auth' in current_service['permissions'] %} +
+ {% if user.auth_type == 'sms_auth' %} + Signs in with a text message code + {% else %} + Signs in with an email link + {% endif %} +
+ {% endif %}
{% if current_user.has_permissions(['manage_users'], admin_override=True) %} {% if current_user.id != user.id %} @@ -104,6 +113,15 @@ user.has_permissions(permissions=['manage_api_keys']), 'Access API keys' ) }} + {% if 'email_auth' in current_service['permissions'] %} +
+ {% if user.auth_type == 'sms_auth' %} + Signs in with a text message code + {% else %} + Signs in with an email link + {% endif %} +
+ {% endif %}
  • {% if user.status == 'pending' and current_user.has_permissions(['manage_users']) %} diff --git a/app/templates/views/manage-users/permissions.html b/app/templates/views/manage-users/permissions.html index 8427a1b2e..e23eef804 100644 --- a/app/templates/views/manage-users/permissions.html +++ b/app/templates/views/manage-users/permissions.html @@ -1,4 +1,5 @@ {% from "components/checkbox.html" import checkbox %} +{% from "components/radios.html" import radios %}
    @@ -20,3 +21,7 @@
  • who the other team members are
    • + +{% if service_has_email_auth %} + {{ radios(form.login_authentication, disable=['sms_auth' if user_has_no_mobile_number]) }} +{% endif %} \ No newline at end of file diff --git a/tests/__init__.py b/tests/__init__.py index b48e2e33e..27057efbe 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -53,14 +53,14 @@ def service_json( created_at=None, letter_contact_block=None, inbound_api=None, - permissions=['email', 'sms'], + permissions=None, organisation_type='central', free_sms_fragment_limit=250000, ): if users is None: users = [] if permissions is None: - permissions = [] + permissions = ['email', 'sms'] if inbound_api is None: inbound_api = [] return { diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 2766cd032..c54da8a70 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -10,6 +10,7 @@ from tests.conftest import ( SERVICE_ONE_ID, active_user_with_permissions, active_user_view_permissions, + active_user_no_mobile, active_user_manage_template_permission, ) @@ -55,6 +56,94 @@ def test_should_show_overview_page( app.user_api_client.get_users_for_service.assert_called_once_with(service_id=SERVICE_ONE_ID) +@pytest.mark.parametrize('endpoint, extra_args, service_has_email_auth, auth_options_hidden', [ + ( + 'main.edit_user_permissions', + {'user_id': 0}, + True, + False + ), + ( + 'main.edit_user_permissions', + {'user_id': 0}, + False, + True + ), + ( + 'main.invite_user', + {}, + True, + False + ), + ( + 'main.invite_user', + {}, + False, + True + ) +]) +def test_service_with_no_email_auth_hides_auth_type_options( + client_request, + endpoint, + extra_args, + service_has_email_auth, + auth_options_hidden, + service_one +): + if service_has_email_auth: + service_one['permissions'].append('email_auth') + page = client_request.get(endpoint, service_id=service_one['id'], **extra_args) + assert (page.find('input', attrs={"name": "login_authentication"}) is None) == auth_options_hidden + + +@pytest.mark.parametrize('service_has_email_auth, displays_auth_type', [ + (True, True), + (False, False) +]) +def test_manage_users_page_shows_member_auth_type_if_service_has_email_auth_activated( + client_request, + service_has_email_auth, + service_one, + mock_get_users_by_service, + mock_get_invites_for_service, + displays_auth_type +): + if service_has_email_auth: + service_one['permissions'].append('email_auth') + page = client_request.get('main.manage_users', service_id=service_one['id']) + assert bool(page.select_one('.tick-cross-list-hint')) == displays_auth_type + + +@pytest.mark.parametrize('user, sms_option_disabled', [ + ( + active_user_no_mobile, + True, + ), + ( + active_user_with_permissions, + False, + ), +]) +def test_user_with_no_mobile_number_cant_be_set_to_sms_auth( + client_request, + user, + sms_option_disabled, + service_one, + mocker +): + service_one['permissions'].append('email_auth') + test_user = mocker.patch('app.user_api_client.get_user', return_value=user(mocker)) + + page = client_request.get( + 'main.edit_user_permissions', + service_id=service_one['id'], + user_id=test_user.id + ) + + sms_auth_radio_button = page.select_one('input[value="sms_auth"]') + assert sms_auth_radio_button.has_attr("disabled") == sms_option_disabled + + @pytest.mark.parametrize('endpoint, extra_args, expected_checkboxes', [ ( 'main.edit_user_permissions', @@ -167,6 +256,60 @@ def test_edit_some_user_permissions( ) +@pytest.mark.parametrize('auth_type', ['email_auth', 'sms_auth']) +def test_edit_user_permissions_including_authentication_with_email_auth_service( + logged_in_client, + active_user_with_permissions, + mocker, + mock_get_invites_for_service, + mock_set_user_permissions, + mock_update_user_attribute, + service_one, + auth_type +): + service_one['permissions'].append('email_auth') + + response = logged_in_client.post( + url_for( + 'main.edit_user_permissions', + service_id=service_one['id'], + user_id=active_user_with_permissions.id + ), + data={ + 'email_address': active_user_with_permissions.email_address, + 'send_messages': 'y', + 'manage_templates': 'y', + 'manage_service': 'y', + 'manage_api_keys': 'y', + 'login_authentication': auth_type + } + ) + + mock_set_user_permissions.assert_called_with( + str(active_user_with_permissions.id), + service_one['id'], + permissions={ + 'send_texts', + 'send_emails', + 'send_letters', + 'manage_users', + 'manage_templates', + 'manage_settings', + 'manage_api_keys', + 'view_activity' + } + ) + mock_update_user_attribute.assert_called_with( + str(active_user_with_permissions.id), + auth_type=auth_type + ) + + assert response.status_code == 302 + assert response.location == url_for( + 'main.manage_users', service_id=service_one['id'], _external=True + ) + + def test_should_show_page_for_inviting_user( logged_in_client, active_user_with_permissions, @@ -220,7 +363,60 @@ def test_invite_user( app.invite_api_client.create_invite.assert_called_once_with(sample_invite['from_user'], sample_invite['service'], email_address, - expected_permissions) + expected_permissions, + 'sms_auth') + + +@pytest.mark.parametrize('auth_type', [ + ('sms_auth'), + ('email_auth') +]) +@pytest.mark.parametrize('email_address, gov_user', [ + ('test@example.gov.uk', True), + ('test@nonwhitelist.com', False) +]) +def test_invite_user_with_email_auth_service( + logged_in_client, + active_user_with_permissions, + sample_invite, + email_address, + gov_user, + mocker, + service_one, + auth_type +): + service_one['permissions'].append('email_auth') + sample_invite['email_address'] = 'test@example.gov.uk' + + data = [InvitedUser(**sample_invite)] + assert is_gov_user(email_address) == gov_user + mocker.patch('app.invite_api_client.get_invites_for_service', return_value=data) + mocker.patch('app.user_api_client.get_users_for_service', return_value=[active_user_with_permissions]) + mocker.patch('app.invite_api_client.create_invite', return_value=InvitedUser(**sample_invite)) + response = logged_in_client.post( + url_for('main.invite_user', service_id=service_one['id']), + data={'email_address': email_address, + 'send_messages': 'y', + 'manage_templates': 'y', + 'manage_service': 'y', + 'manage_api_keys': 'y', + 'login_authentication': auth_type}, + follow_redirects=True + ) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Team members' + flash_banner = page.find('div', class_='banner-default-with-tick').string.strip() + assert flash_banner == 'Invite sent to test@example.gov.uk' + + expected_permissions = 'manage_api_keys,manage_settings,manage_templates,manage_users,send_emails,send_letters,send_texts,view_activity' # noqa + + app.invite_api_client.create_invite.assert_called_once_with(sample_invite['from_user'], + sample_invite['service'], + email_address, + expected_permissions, + auth_type) def test_cancel_invited_user_cancels_user_invitations( diff --git a/tests/conftest.py b/tests/conftest.py index d3a2a1a7d..d9e7f6771 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1003,7 +1003,8 @@ def platform_admin_user(fake_uuid): 'manage_settings', 'manage_api_keys', 'view_activity']}, - 'platform_admin': True + 'platform_admin': True, + 'auth_type': 'sms_auth' } user = User(user_data) return user @@ -1021,6 +1022,7 @@ def api_user_active(fake_uuid, email_address='test@user.gov.uk'): 'failed_login_count': 0, 'permissions': {}, 'platform_admin': False, + 'auth_type': 'sms_auth', 'password_changed_at': str(datetime.utcnow()) } user = User(user_data) @@ -1039,6 +1041,7 @@ def api_nongov_user_active(fake_uuid): 'failed_login_count': 0, 'permissions': {}, 'platform_admin': False, + 'auth_type': 'sms_auth', 'password_changed_at': str(datetime.utcnow()) } user = User(user_data) @@ -1065,7 +1068,35 @@ def active_user_with_permissions(fake_uuid): 'manage_settings', 'manage_api_keys', 'view_activity']}, - 'platform_admin': False + 'platform_admin': False, + 'auth_type': 'sms_auth' + } + user = User(user_data) + return user + + +@pytest.fixture(scope='function') +def active_user_no_mobile(fake_uuid): + from app.notify_client.user_api_client import User + + user_data = {'id': fake_uuid, + 'name': 'Test User', + 'password': 'somepassword', + 'password_changed_at': str(datetime.utcnow()), + 'email_address': 'test@user.gov.uk', + 'mobile_number': None, + 'state': 'active', + 'failed_login_count': 0, + 'permissions': {SERVICE_ONE_ID: ['send_texts', + 'send_emails', + 'send_letters', + 'manage_users', + 'manage_templates', + 'manage_settings', + 'manage_api_keys', + 'view_activity']}, + 'platform_admin': False, + 'auth_type': 'email_auth' } user = User(user_data) return user @@ -1084,7 +1115,8 @@ def active_user_view_permissions(fake_uuid): 'state': 'active', 'failed_login_count': 0, 'permissions': {SERVICE_ONE_ID: ['view_activity']}, - 'platform_admin': False + 'platform_admin': False, + 'auth_type': 'sms_auth' } user = User(user_data) return user @@ -1107,7 +1139,8 @@ def active_user_manage_template_permission(fake_uuid): 'manage_templates', 'view_activity', ]}, - 'platform_admin': False + 'platform_admin': False, + 'auth_type': 'sms_auth' } user = User(user_data) return user @@ -1123,7 +1156,8 @@ def api_user_locked(fake_uuid): 'mobile_number': '07700 900762', 'state': 'active', 'failed_login_count': 5, - 'permissions': {} + 'permissions': {}, + 'auth_type': 'sms_auth' } user = User(user_data) return user @@ -1140,7 +1174,8 @@ def api_user_request_password_reset(fake_uuid): 'state': 'active', 'failed_login_count': 5, 'permissions': {}, - 'password_changed_at': None + 'password_changed_at': None, + 'auth_type': 'sms_auth' } user = User(user_data) return user @@ -1157,6 +1192,7 @@ def api_user_changed_password(fake_uuid): 'state': 'active', 'failed_login_count': 5, 'permissions': {}, + 'auth_type': 'sms_auth', 'password_changed_at': str(datetime.utcnow() + timedelta(minutes=1)) } user = User(user_data) @@ -1753,6 +1789,7 @@ def mock_get_users_by_service(mocker): 'password_changed_at': None, 'name': 'Test User', 'email_address': 'notify@digital.cabinet-office.gov.uk', + 'auth_type': 'sms_auth', 'failed_login_count': 0}] return [User(data[0])] From 3f1c543735939ef67a384602ba6e6d76e0e943a9 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 2 Nov 2017 13:39:37 +0000 Subject: [PATCH 11/41] Fix some formatting on the trial mode services page --- app/templates/views/platform-admin/services.html | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/templates/views/platform-admin/services.html b/app/templates/views/platform-admin/services.html index 237e82923..a37267499 100644 --- a/app/templates/views/platform-admin/services.html +++ b/app/templates/views/platform-admin/services.html @@ -66,7 +66,8 @@ Live {% endcall %} {% else %} - {{ text_field('') }} + {% call field(border=False) %} + {% endcall %} {% endif %} {{ stats_fields('sms', service['stats']) }} From 04adb15e8567eb25e9c11f62ab4a057849df4de7 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 2 Nov 2017 13:49:24 +0000 Subject: [PATCH 12/41] make sure old invites get the proper message we were accidentally covering up the expiry message with a more generic one --- app/config.py | 1 + app/main/views/invites.py | 21 +++++++++++++++------ tests/app/main/views/test_accept_invite.py | 19 +++++++++++++++++++ 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/app/config.py b/app/config.py index dce3d4104..125b91834 100644 --- a/app/config.py +++ b/app/config.py @@ -42,6 +42,7 @@ class Config(object): 'nhs': 25000, } EMAIL_EXPIRY_SECONDS = 3600 # 1 hour + INVITATION_EXPIRY_SECONDS = 3600 * 24 * 2 # 2 days - also set on api HEADER_COLOUR = '#FFBF47' # $yellow HTTP_PROTOCOL = 'http' MAX_FAILED_LOGIN_COUNT = 10 diff --git a/app/main/views/invites.py b/app/main/views/invites.py index af3d0eede..4f1c88f7a 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -7,6 +7,7 @@ from flask import ( abort, current_app ) +from itsdangerous import SignatureExpired from markupsafe import Markup from notifications_utils.url_safe_token import check_token from flask_login import current_user @@ -21,12 +22,20 @@ from app import ( @main.route("/invitation/") def accept_invite(token): - check_token( - token, - current_app.config['SECRET_KEY'], - current_app.config['DANGEROUS_SALT'], - current_app.config['EMAIL_EXPIRY_SECONDS'] - ) + try: + check_token( + token, + current_app.config['SECRET_KEY'], + current_app.config['DANGEROUS_SALT'], + current_app.config['INVITATION_EXPIRY_SECONDS'] + ) + except SignatureExpired: + errors = [ + 'Your invitation to GOV.UK Notify has expired. ' + 'Please ask the person that invited you to send you another one' + ] + return render_template("error/400.html", message=errors), 400 + invited_user = invite_api_client.check_token(token) if not current_user.is_anonymous and current_user.email_address != invited_user.email_address: diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index fe4058f66..fd1f28924 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -1,6 +1,7 @@ from flask import url_for from bs4 import BeautifulSoup from unittest.mock import ANY +from itsdangerous import SignatureExpired import app @@ -368,3 +369,21 @@ def test_new_invited_user_verifies_and_added_to_service( raw_html = response.data.decode('utf-8') page = BeautifulSoup(raw_html, 'html.parser') assert page.find('h1').text == 'Dashboard' + + +def test_gives_message_if_token_has_expired( + app_, + client, + mock_check_invite_token, + mocker, +): + check_token = mocker.patch('app.main.views.invites.check_token', side_effect=SignatureExpired('this is too old')) + + response = client.get(url_for('main.accept_invite', token='a really old token')) + raw_html = response.data.decode('utf-8') + page = BeautifulSoup(raw_html, 'html.parser') + + check_token.assert_called_once_with(ANY, ANY, ANY, 3600 * 24 * 2) + assert response.status_code == 400 + assert 'Your invitation to GOV.UK Notify has expired' in page.find('h1').text + assert not mock_check_invite_token.called From 5152fa8e82cdd7662a19e0e61fc34a31b1207d36 Mon Sep 17 00:00:00 2001 From: Chris Patuzzo Date: Thu, 2 Nov 2017 13:51:00 +0000 Subject: [PATCH 13/41] Fix a typo: generates -> generate --- app/templates/views/integration_testing.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/integration_testing.html b/app/templates/views/integration_testing.html index b16855cff..22164abc8 100644 --- a/app/templates/views/integration_testing.html +++ b/app/templates/views/integration_testing.html @@ -50,7 +50,7 @@

    You should revoke and re-create these keys on a regular basis. You can have more than one active key at a time. To revoke a key click the revoke button on the API Key page.

    - +

    You should never send test messages to invalid numbers or addresses using a live key. This will lead to your API key being revoked. @@ -73,7 +73,7 @@ Use a test key to test the performance of your service and its integration with GOV.UK Notify.

    - Test keys don’t send real messages but do generates realistic responses. + Test keys don’t send real messages but do generate realistic responses. There’s no restriction on who you can send to.

    From 83bfc5088427d6dbb6ed68ab1ff8c84c8caab30c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 2 Nov 2017 14:58:14 +0000 Subject: [PATCH 14/41] Added a hint for Receives replies --- app/main/views/send.py | 6 +++++- app/notify_client/notification_api_client.py | 1 - tests/app/main/views/test_send.py | 19 +++++++++++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 63863f0f7..0db544410 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -192,7 +192,8 @@ def set_sender(service_id, template_id): sender_choices=sender_context['value_and_label'], sender_label=sender_context['description'] ) - option_hints = {sender_context['default_id']: 'Default'} + option_hints = {sender_context['default_id']: 'Default', + sender_context['receives_text_message']: 'Receives replies'} if form.validate_on_submit(): session['sender_id'] = form.sender.data @@ -231,6 +232,9 @@ def get_sender_context(sender_details, template_type): sender_format = context['field_name'] context['default_id'] = next(sender['id'] for sender in sender_details if sender['is_default']) + if template_type == 'sms': + context['receives_text_message'] = next( + sender['id'] for sender in sender_details if sender['inbound_number_id']) context['value_and_label'] = [(sender['id'], sender[sender_format]) for sender in sender_details] return context diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index d9ce5a8d8..195d089ac 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -61,7 +61,6 @@ class NotificationApiClient(NotifyAdminAPIClient): 'personalisation': personalisation, } if sender_id: - print(sender_id) data['sender_id'] = sender_id data = _attach_current_user(data) return self.post(url='/service/{}/send-notification'.format(service_id), data=data) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 2880743a4..55128afaa 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -107,6 +107,25 @@ def test_default_sender_is_checked_and_has_hint( assert not page.select('.multiple-choice input')[2].has_attr('checked') +def test_sms_sender_is_has_receives_replies_hint( + client_request, + service_one, + fake_uuid, + mock_get_service_template, + multiple_sms_senders +): + page = client_request.get( + '.set_sender', + service_id=service_one['id'], + template_id=fake_uuid + ) + + assert page.select('.multiple-choice input')[0].has_attr('checked') + assert normalize_spaces(page.select_one('.multiple-choice label .block-label-hint').text) == "Receives replies" + assert not page.select('.multiple-choice input')[1].has_attr('checked') + assert not page.select('.multiple-choice input')[2].has_attr('checked') + + @pytest.mark.parametrize('template_mock, sender_data', [ ( mock_get_service_email_template, From ff22c83b1d884909e0ed5c6df5495520ffabece0 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 2 Nov 2017 15:48:19 +0000 Subject: [PATCH 15/41] Added a hint to show default and receives text messages --- app/main/views/send.py | 16 +++-- app/main/views/service_settings.py | 2 +- tests/app/main/views/test_send.py | 34 +++++++++-- tests/app/main/views/test_service_settings.py | 2 +- tests/conftest.py | 60 +++++++++++++++++++ 5 files changed, 104 insertions(+), 10 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 0db544410..7835c9955 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -192,8 +192,12 @@ def set_sender(service_id, template_id): sender_choices=sender_context['value_and_label'], sender_label=sender_context['description'] ) - option_hints = {sender_context['default_id']: 'Default', - sender_context['receives_text_message']: 'Receives replies'} + option_hints = {sender_context['default_id']: '(Default)', + } + if sender_context.get('receives_text_message', None): + option_hints.update({sender_context['receives_text_message']: '(Receives replies)'}) + if sender_context.get('default_and_receives', None): + option_hints = {sender_context['default_and_receives']: '(Default and receives replies)'} if form.validate_on_submit(): session['sender_id'] = form.sender.data @@ -233,8 +237,12 @@ def get_sender_context(sender_details, template_type): context['default_id'] = next(sender['id'] for sender in sender_details if sender['is_default']) if template_type == 'sms': - context['receives_text_message'] = next( - sender['id'] for sender in sender_details if sender['inbound_number_id']) + inbound = [sender['id'] for sender in sender_details if sender['inbound_number_id']] + if inbound: + context['receives_text_message'] = next(iter(inbound)) + if context['default_id'] == context.get('receives_text_message', None): + context['default_and_receives'] = context['default_id'] + context['value_and_label'] = [(sender['id'], sender[sender_format]) for sender in sender_details] return context diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 4c2cef034..9446b69ff 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -590,7 +590,7 @@ def service_sms_senders(service_id): if sender['is_default']: hints += ["default"] if sender['inbound_number_id']: - hints += ["recieves replies"] + hints += ["receives replies"] if hints: sender['hint'] = "(" + " and ".join(hints) + ")" diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 55128afaa..0d40fdac9 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -28,7 +28,9 @@ from tests.conftest import ( multiple_reply_to_email_addresses, no_reply_to_email_addresses, multiple_sms_senders, - no_sms_senders + no_sms_senders, + multiple_sms_senders_with_diff_default, + multiple_sms_senders_no_inbound ) template_types = ['email', 'sms'] @@ -82,7 +84,11 @@ def test_show_correct_title_and_description_for_sender_type( ), ( mock_get_service_template, - multiple_sms_senders + multiple_sms_senders_with_diff_default + ), + ( + mock_get_service_template, + multiple_sms_senders_no_inbound ) ]) def test_default_sender_is_checked_and_has_hint( @@ -102,7 +108,26 @@ def test_default_sender_is_checked_and_has_hint( ) assert page.select('.multiple-choice input')[0].has_attr('checked') - assert normalize_spaces(page.select_one('.multiple-choice label .block-label-hint').text) == "Default" + assert normalize_spaces(page.select_one('.multiple-choice label .block-label-hint').text) == "(Default)" + assert not page.select('.multiple-choice input')[1].has_attr('checked') + + +def test_default_inbound_sender_is_checked_and_has_hint_with_default_and_receives_text( + client_request, + service_one, + fake_uuid, + mock_get_service_template, + multiple_sms_senders +): + page = client_request.get( + '.set_sender', + service_id=service_one['id'], + template_id=fake_uuid + ) + + assert page.select('.multiple-choice input')[0].has_attr('checked') + assert normalize_spaces( + page.select_one('.multiple-choice label .block-label-hint').text) == "(Default and receives replies)" assert not page.select('.multiple-choice input')[1].has_attr('checked') assert not page.select('.multiple-choice input')[2].has_attr('checked') @@ -121,7 +146,8 @@ def test_sms_sender_is_has_receives_replies_hint( ) assert page.select('.multiple-choice input')[0].has_attr('checked') - assert normalize_spaces(page.select_one('.multiple-choice label .block-label-hint').text) == "Receives replies" + assert normalize_spaces( + page.select_one('.multiple-choice label .block-label-hint').text) == "(Default and receives replies)" assert not page.select('.multiple-choice input')[1].has_attr('checked') assert not page.select('.multiple-choice input')[2].has_attr('checked') diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 3a7cc9884..998b32243 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -761,7 +761,7 @@ def test_api_ids_dont_show_on_option_pages_with_a_single_sender( ), ( 'main.service_sms_senders', multiple_sms_senders, - 'Example (default and recieves replies) Change 1234', + 'Example (default and receives replies) Change 1234', 'Example 2 Change 5678', 'Example 3 Change 9457' ), diff --git a/tests/conftest.py b/tests/conftest.py index d3a2a1a7d..34c0f6cec 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -304,6 +304,66 @@ def multiple_sms_senders(mocker): return mocker.patch('app.service_api_client.get_sms_senders', side_effect=_get) +@pytest.fixture(scope='function') +def multiple_sms_senders_with_diff_default(mocker): + def _get(service_id): + return [ + { + 'id': '1234', + 'service_id': service_id, + 'sms_sender': 'Example', + 'is_default': True, + 'created_at': datetime.utcnow(), + 'inbound_number_id': None, + 'updated_at': None + }, { + 'id': '5678', + 'service_id': service_id, + 'sms_sender': 'Example 2', + 'is_default': False, + 'created_at': datetime.utcnow(), + 'inbound_number_id': None, + 'updated_at': None + }, { + 'id': '9457', + 'service_id': service_id, + 'sms_sender': 'Example 3', + 'is_default': False, + 'created_at': datetime.utcnow(), + 'inbound_number_id': '12354', + 'updated_at': None + } + ] + + return mocker.patch('app.service_api_client.get_sms_senders', side_effect=_get) + + +@pytest.fixture(scope='function') +def multiple_sms_senders_no_inbound(mocker): + def _get(service_id): + return [ + { + 'id': '1234', + 'service_id': service_id, + 'sms_sender': 'Example', + 'is_default': True, + 'created_at': datetime.utcnow(), + 'inbound_number_id': None, + 'updated_at': None + }, { + 'id': '5678', + 'service_id': service_id, + 'sms_sender': 'Example 2', + 'is_default': False, + 'created_at': datetime.utcnow(), + 'inbound_number_id': None, + 'updated_at': None + } + ] + + return mocker.patch('app.service_api_client.get_sms_senders', side_effect=_get) + + @pytest.fixture(scope='function') def no_sms_senders(mocker): def _get(service_id): From 3e10cfb16528c94225d7f7e94f57119e047fd422 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 2 Nov 2017 16:30:49 +0000 Subject: [PATCH 16/41] Rename test --- tests/app/main/views/test_send.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 0d40fdac9..5d99b6d27 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -132,7 +132,7 @@ def test_default_inbound_sender_is_checked_and_has_hint_with_default_and_receive assert not page.select('.multiple-choice input')[2].has_attr('checked') -def test_sms_sender_is_has_receives_replies_hint( +def test_sms_sender_has_receives_replies_hint( client_request, service_one, fake_uuid, From 660ef8db6b4cfde3fa321f8b0a677eefa94469c9 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Thu, 2 Nov 2017 23:32:46 +0000 Subject: [PATCH 17/41] Update pyexcel-xls from 0.5.2 to 0.5.4 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 27ed7fe20..8bf508785 100644 --- a/requirements.txt +++ b/requirements.txt @@ -8,7 +8,7 @@ boto3==1.4.7 blinker==1.4 pyexcel==0.5.6 pyexcel-io==0.5.3 -pyexcel-xls==0.5.2 +pyexcel-xls==0.5.4 pyexcel-xlsx==0.5.2 pyexcel-ods3==0.5.2 pytz==2017.3 From 2c74027e0d501f5e8183b0ed23c104e428d8b37f Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Fri, 3 Nov 2017 11:56:15 +0000 Subject: [PATCH 18/41] Adding a route for 'callbacks' page Gone with callbacks as this page may be extended for delivery receipts in the future. --- app/main/views/index.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/main/views/index.py b/app/main/views/index.py index 412a8dbe4..24061b3cc 100644 --- a/app/main/views/index.py +++ b/app/main/views/index.py @@ -135,3 +135,8 @@ def using_notify(): @main.route('/information-risk-management') def information_risk_management(): return render_template('views/information-risk-management.html') + + +@main.route('/callbacks') +def callbacks(): + return render_template('views/callbacks.html') From 9eb83792cf1f84cbeb9e11b85c0b6a34ab5598f5 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Fri, 3 Nov 2017 11:57:29 +0000 Subject: [PATCH 19/41] Added test for new callbacks static page --- tests/app/main/views/test_index.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/main/views/test_index.py b/tests/app/main/views/test_index.py index 46dd31da7..2e46706e9 100644 --- a/tests/app/main/views/test_index.py +++ b/tests/app/main/views/test_index.py @@ -18,7 +18,7 @@ def test_logged_in_user_redirects_to_choose_service( @pytest.mark.parametrize('view', [ 'cookies', 'using_notify', 'pricing', 'terms', 'integration_testing', 'roadmap', - 'features', 'information_risk_management' + 'features', 'information_risk_management', 'callbacks' ]) def test_static_pages( client, From ba128d05bcfb13213ed0abdd0ff9a832068caec7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Nov 2017 13:09:57 +0000 Subject: [PATCH 20/41] Use service setting to determine prefixing of SMSs Rather than doing this nasty `if` statement, let the API work out what to do. Also means that the logic is not repeated between the two apps. --- app/utils.py | 2 +- tests/__init__.py | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/utils.py b/app/utils.py index f9ce51bd8..3a69f4486 100644 --- a/app/utils.py +++ b/app/utils.py @@ -288,7 +288,7 @@ def get_template( return SMSPreviewTemplate( template, prefix=service['name'], - sender=sms_sender if sms_sender else (service['sms_sender'] not in {'GOVUK', None}), + sender=not service['prefix_sms_with_service_name'], show_recipient=show_recipient, redact_missing_personalisation=redact_missing_personalisation, ) diff --git a/tests/__init__.py b/tests/__init__.py index b48e2e33e..820d39fe8 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -56,6 +56,7 @@ def service_json( permissions=['email', 'sms'], organisation_type='central', free_sms_fragment_limit=250000, + prefix_sms_with_service_name=None, ): if users is None: users = [] @@ -63,6 +64,8 @@ def service_json( permissions = [] if inbound_api is None: inbound_api = [] + if prefix_sms_with_service_name is None: + prefix_sms_with_service_name = (sms_sender == 'GOVUK') return { 'id': id_, 'name': name, @@ -83,6 +86,7 @@ def service_json( 'dvla_organisation': '001', 'permissions': permissions, 'inbound_api': inbound_api, + 'prefix_sms_with_service_name': prefix_sms_with_service_name, } From bc7af49b5694fa2b66b78ec86a0ff906bcce173e Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Fri, 3 Nov 2017 14:24:10 +0000 Subject: [PATCH 21/41] New page explaining the format of callback messages --- app/templates/views/callbacks.html | 63 ++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 app/templates/views/callbacks.html diff --git a/app/templates/views/callbacks.html b/app/templates/views/callbacks.html new file mode 100644 index 000000000..935667280 --- /dev/null +++ b/app/templates/views/callbacks.html @@ -0,0 +1,63 @@ +{% from "components/table.html" import mapping_table, row, text_field, edit_field, field %} +{% extends "withoutnav_template.html" %} + +{% block per_page_title %} + Callbacks +{% endblock %} + +{% block maincolumn_content %} + +

    +
    +

    Callbacks for received text messages

    + +

    + Text messages you receive can be forwarded to a URL that you specify, using our callback feature. +

    + +

    + Messages are forwarded as they are received. +

    + +

    + To protect your service, we require you to provide a bearer token. We put this token in the authorisation header of the callback requests. +

    + +

    + Once you have ‘receive text messages’ enabled, you can set up your callback on the settings page of your service. +

    + +

    + If you don’t have ‘receive text messages’ enabled for your service, get in touch and we can turn it on for you. +

    + +

    Format of the callback

    + +

    + The format of the callback message you receive is JSON. +

    + +
    + {% call mapping_table( + caption='Callback message format', + field_headings=['Key', 'Description', 'Format'], + field_headings_visible=True, + caption_visible=False + ) %} + {% for key, description, format in [ + ('id', 'Notify’s id for the received message', 'UUID'), + ('source_number', 'The phone number the message was sent from', '447700912345'), + ('destination_number', 'The number the message was sent to (your number)', '07700987654'), + ('message', 'The received message', 'Hello Notify!'), + ('date_received', 'The UTC datetime that the message was received by Notify', '2017-05-14T12:15:30.000000Z') + ] %} + {% call row() %} + {% call row_heading() %} {{ key }} {% endcall %} + {{ text_field(description) }} + {{ text_field(format) }} + {% endcall %} + {% endfor %} + {% endcall %} +
    + +{% endblock %} From 4b2ba34d685eacca66c28c50726ffd08ba080ef0 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Fri, 3 Nov 2017 14:25:31 +0000 Subject: [PATCH 22/41] Updated the label to set the callback URL on the settings page --- app/templates/views/service-settings.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 4e279f043..b31854876 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -106,7 +106,7 @@ {% if can_receive_inbound %} {% call row() %} - {{ text_field('API endpoint for received text messages') }} + {{ text_field('Callback URL for received text messages') }} {{ optional_text_field(inbound_api_url) }} {{ edit_field('Change', url_for('.service_set_inbound_api', service_id=current_service.id)) }} {% endcall %} From 431e269cf94c6cff0c23d221732bee7723052516 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Fri, 3 Nov 2017 16:02:43 +0000 Subject: [PATCH 23/41] Updated the field label for the callback URL --- app/main/forms.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 75d15fb16..1d2a65b67 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -693,10 +693,10 @@ class ServiceInboundNumberForm(Form): class ServiceInboundApiForm(Form): - url = StringField("Inbound sms url", + url = StringField("Callback URL", validators=[DataRequired(message='Can’t be empty'), Regexp(regex="^https.*", - message='Must be a valid https url')] + message='Must be a valid https URL')] ) bearer_token = PasswordFieldShowHasContent("Bearer token", validators=[DataRequired(message='Can’t be empty'), From 55093691f12f053135d85b8766dd39d77f97e220 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Fri, 3 Nov 2017 16:05:10 +0000 Subject: [PATCH 24/41] Updated test to reflect new label on callback URL field --- tests/app/main/views/test_service_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 998b32243..cea7c6a7c 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1272,7 +1272,7 @@ def test_does_not_show_research_mode_indicator( @pytest.mark.parametrize('url, bearer_token, expected_errors', [ ("", "", "Can’t be empty Can’t be empty"), - ("http://not_https.com", "1234567890", "Must be a valid https url"), + ("http://not_https.com", "1234567890", "Must be a valid https URL"), ("https://test.com", "123456789", "Must be at least 10 characters"), ]) def test_set_inbound_api_validation( From 60a39b2e493f99ccc835ddb5145516fc00ab4fc8 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Fri, 3 Nov 2017 16:10:26 +0000 Subject: [PATCH 25/41] Updated the callbacks page to add the link to new documentation. --- .../views/service-settings/set-inbound-api.html | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/templates/views/service-settings/set-inbound-api.html b/app/templates/views/service-settings/set-inbound-api.html index 0609ff126..9530b36d5 100644 --- a/app/templates/views/service-settings/set-inbound-api.html +++ b/app/templates/views/service-settings/set-inbound-api.html @@ -3,27 +3,27 @@ {% from "components/page-footer.html" import page_footer %} {% block service_page_title %} - Inbound api + Callback URL {% endblock %} {% block maincolumn_content %}
    -

    API endpoint for received text messages

    +

    Callback URL for received text messages

    - This is the https url that the inbound SMS messages will be posted to - and the bearer token used in the authorisation header of the request. + Text messages you receive can be forwarded to your systems with our callback feature. + See our documentation on the format of the callback.

    {{ textbox( form.url, width='2-3', - hint='Valid https url' + hint='Valid https URL' ) }} {{ textbox( form.bearer_token, - width='1-4', + width='2-3', hint='At least 10 characters' ) }} {{ page_footer( @@ -35,4 +35,4 @@
    -{% endblock %} \ No newline at end of file +{% endblock %} From c19855c0b0d1b78c3b2d1979c3f61615a86c62d0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Nov 2017 16:12:37 +0000 Subject: [PATCH 26/41] Fix missing import --- app/templates/views/callbacks.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/callbacks.html b/app/templates/views/callbacks.html index 935667280..38ec51669 100644 --- a/app/templates/views/callbacks.html +++ b/app/templates/views/callbacks.html @@ -1,4 +1,4 @@ -{% from "components/table.html" import mapping_table, row, text_field, edit_field, field %} +{% from "components/table.html" import mapping_table, row, text_field, edit_field, field, row_heading%} {% extends "withoutnav_template.html" %} {% block per_page_title %} @@ -12,7 +12,7 @@

    Callbacks for received text messages

    - Text messages you receive can be forwarded to a URL that you specify, using our callback feature. + Text messages you receive can be forwarded to a URL that you specify, using our callback feature.

    From 240f25eaf97bd65b9b9dab6f31bea614dd248528 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Nov 2017 16:15:39 +0000 Subject: [PATCH 27/41] Fix failing tests --- tests/app/main/views/test_service_settings.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index cea7c6a7c..27080aaee 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -121,7 +121,7 @@ def test_should_show_overview( 'Text message sender GOVUK Manage', 'International text messages On Change', 'Receive text messages On Change', - 'API endpoint for received text messages Not set Change', + 'Callback URL for received text messages Not set Change', 'Label Value Action', 'Send letters Off Change', @@ -207,7 +207,7 @@ def test_service_settings_show_elided_api_url_if_needed( non_empty_trs = [tr.find_all('td') for tr in page.find_all('tr') if tr.find_all('td')] api_url = [api_setting[1].text.strip() for api_setting in non_empty_trs - if api_setting[0].text.strip() == 'API endpoint for received text messages'][0] + if api_setting[0].text.strip() == 'Callback URL for received text messages'][0] assert api_url == elided_url assert mocked_get_fn.called is True From 1d10ad22474b5aad39dc7b5a3eb94cd0b3af5208 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Nov 2017 10:25:30 +0000 Subject: [PATCH 28/41] Stop content security policy blocking GA MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In https://github.com/alphagov/notifications-admin/pull/1583 we changed our Google Analytics settings to use newer browsers’ `sendBeacon` feature. The advantage of this is that it > [ensures] that the data has been sent during the unloading of a > document [which] is something that has traditionally been difficult > for developers – https://developer.mozilla.org/en-US/docs/Web/API/Navigator/sendBeacon To transmit this data it uses a AJAX request (`XMLHttpRequest`) underneath. AJAX requests are governed by the `connect-src` content security policy (or the `default-src` if one is not present). `connect-src`: > Applies to XMLHttpRequest (AJAX), WebSocket or EventSource. If not > allowed the browser emulates a 400 HTTP status code. – https://content-security-policy.com/ Because we didn’t have one in place, `sendBeacon` requests to GA were getting blocked in browsers that support content security policy (pretty much everything better than IE11[1]). 1. https://caniuse.com/#feat=beacon --- app/__init__.py | 1 + tests/app/main/views/test_headers.py | 1 + 2 files changed, 2 insertions(+) diff --git a/app/__init__.py b/app/__init__.py index 8cf3c7147..c45ad9a84 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -441,6 +441,7 @@ def useful_headers_after_request(response): response.headers.add('Content-Security-Policy', ( "default-src 'self' 'unsafe-inline';" "script-src 'self' *.google-analytics.com 'unsafe-inline' 'unsafe-eval' data:;" + "connect-src 'self' *.google-analytics.com;" "object-src 'self';" "font-src 'self' data:;" "img-src 'self' *.google-analytics.com *.notifications.service.gov.uk {} data:;" diff --git a/tests/app/main/views/test_headers.py b/tests/app/main/views/test_headers.py index 130beac20..7e06b3961 100644 --- a/tests/app/main/views/test_headers.py +++ b/tests/app/main/views/test_headers.py @@ -10,6 +10,7 @@ def test_owasp_useful_headers_set(client, mocker): assert response.headers['Content-Security-Policy'] == ( "default-src 'self' 'unsafe-inline';" "script-src 'self' *.google-analytics.com 'unsafe-inline' 'unsafe-eval' data:;" + "connect-src 'self' *.google-analytics.com;" "object-src 'self';" "font-src 'self' data:;" "img-src 'self' *.google-analytics.com *.notifications.service.gov.uk static-logos.test.com data:;" From 6d3855bba440fcac5d2530d280a338d2306cd419 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 3 Nov 2017 15:01:41 +0000 Subject: [PATCH 29/41] Allow updates to SMS prefixing setting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’re extracting this from being determined based on what the sender name is to its own setting. This commit will let users set it independently. Until the explicitly set it, it will still be determined based on whether their default sender name matches the default for the platform. --- app/main/forms.py | 10 ++++ app/main/views/service_settings.py | 26 +++++++++- app/notify_client/service_api_client.py | 1 + app/templates/views/service-settings.html | 6 +++ .../views/service-settings/sms-prefix.html | 24 +++++++++ tests/__init__.py | 4 +- tests/app/main/views/test_service_settings.py | 49 ++++++++++++++++++- 7 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 app/templates/views/service-settings/sms-prefix.html diff --git a/app/main/forms.py b/app/main/forms.py index 242be6879..e80fa0023 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -721,6 +721,16 @@ class InternationalSMSForm(Form): ) +class SMSPrefixForm(Form): + enabled = RadioField( + 'Start all text messages with service name', + choices=[ + ('on', 'On'), + ('off', 'Off'), + ], + ) + + def get_placeholder_form_instance( placeholder_name, dict_to_populate_from, diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 9446b69ff..ecadb06cb 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -38,6 +38,7 @@ from app.main.forms import ( OrganisationTypeForm, FreeSMSAllowance, ServiceEditInboundNumberForm, + SMSPrefixForm, ) from app import user_api_client, current_service, organisations_client, inbound_number_client from notifications_utils.formatters import formatted_list @@ -103,7 +104,8 @@ def service_settings(service_id): default_letter_contact_block=default_letter_contact_block, letter_contact_details_count=letter_contact_details_count, default_sms_sender=default_sms_sender, - sms_sender_count=sms_sender_count + sms_sender_count=sms_sender_count, + prefix_sms_with_service_name=current_service['prefix_sms_with_service_name'], ) @@ -486,6 +488,28 @@ def service_set_sms(service_id): ) +@main.route("/services//service-settings/sms-prefix", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_settings', admin_override=True) +def service_set_sms_prefix(service_id): + + form = SMSPrefixForm(enabled=( + 'on' if current_service['prefix_sms_with_service_name'] else 'off' + )) + + if form.validate_on_submit(): + service_api_client.update_service( + current_service['id'], + prefix_sms_with_service_name=(form.enabled.data == 'on') + ) + return redirect(url_for('.service_settings', service_id=service_id)) + + return render_template( + 'views/service-settings/sms-prefix.html', + form=form + ) + + @main.route("/services//service-settings/set-international-sms", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_settings', admin_override=True) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index e374487d5..3667ecba2 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -107,6 +107,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): 'permissions', 'organisation_type', 'free_sms_fragment_limit', + 'prefix_sms_with_service_name', } if disallowed_attributes: raise TypeError('Not allowed to update service attributes: {}'.format( diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index b31854876..7d417e58e 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -92,6 +92,12 @@ {{ edit_field('Manage' if sms_sender_count else 'Change', url_for('.service_sms_senders', service_id=current_service.id)) }} {% endcall %} + {% call row() %} + {{ text_field('Start all text messages with service name') }} + {{ boolean_field(prefix_sms_with_service_name) }} + {{ edit_field('Change', url_for('.service_set_sms_prefix', service_id=current_service.id)) }} + {% endcall %} + {% call row() %} {{ text_field('International text messages') }} {{ boolean_field('international_sms' in current_service.permissions) }} diff --git a/app/templates/views/service-settings/sms-prefix.html b/app/templates/views/service-settings/sms-prefix.html new file mode 100644 index 000000000..5a99b86d6 --- /dev/null +++ b/app/templates/views/service-settings/sms-prefix.html @@ -0,0 +1,24 @@ +{% extends "withnav_template.html" %} +{% from "components/radios.html" import radios %} +{% from "components/page-footer.html" import page_footer %} + +{% block service_page_title %} + Start all text messages with service name +{% endblock %} + +{% block maincolumn_content %} + +

    Start all text messages with service name

    +

    + Your service name is {{ current_service.name}}. +

    + + {{ radios(form.enabled) }} + {{ page_footer( + button_text="Save", + back_link=url_for('.service_settings', service_id=current_service.id), + back_link_text='Back to settings' + ) }} + + +{% endblock %} diff --git a/tests/__init__.py b/tests/__init__.py index b467f6daa..f59633f55 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -56,7 +56,7 @@ def service_json( permissions=None, organisation_type='central', free_sms_fragment_limit=250000, - prefix_sms_with_service_name=None, + prefix_sms_with_service_name='Treat as None', ): if users is None: users = [] @@ -64,7 +64,7 @@ def service_json( permissions = ['email', 'sms'] if inbound_api is None: inbound_api = [] - if prefix_sms_with_service_name is None: + if prefix_sms_with_service_name == 'Treat as None': prefix_sms_with_service_name = (sms_sender == 'GOVUK') return { 'id': id_, diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 27080aaee..1f09d38c4 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -44,6 +44,7 @@ from tests.conftest import ( 'Label Value Action', 'Send text messages On Change', 'Text message sender GOVUK Manage', + 'Start all text messages with service name On Change', 'International text messages Off Change', 'Receive text messages Off Change', @@ -63,6 +64,7 @@ from tests.conftest import ( 'Label Value Action', 'Send text messages On Change', 'Text message sender GOVUK Manage', + 'Start all text messages with service name On Change', 'International text messages Off Change', 'Receive text messages Off Change', @@ -119,6 +121,7 @@ def test_should_show_overview( 'Label Value Action', 'Send text messages On Change', 'Text message sender GOVUK Manage', + 'Start all text messages with service name On Change', 'International text messages On Change', 'Receive text messages On Change', 'Callback URL for received text messages Not set Change', @@ -138,6 +141,7 @@ def test_should_show_overview( 'Label Value Action', 'Send text messages On Change', 'Text message sender GOVUK Manage', + 'Start all text messages with service name On Change', 'International text messages Off Change', 'Receive text messages Off Change', @@ -713,7 +717,7 @@ def test_and_more_hint_appears_on_settings_with_more_than_just_a_single_sender( assert get_row(page, 2) == "Email reply to addresses test@example.com …and 2 more Manage" assert get_row(page, 4) == "Text message sender Example …and 2 more Manage" - assert get_row(page, 8) == "Sender addresses 1 Example Street …and 2 more Manage" + assert get_row(page, 9) == "Sender addresses 1 Example Street …and 2 more Manage" @pytest.mark.parametrize('sender_list_page, expected_output', [ @@ -2063,3 +2067,46 @@ def test_empty_letter_contact_block_returns_error( page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') error_message = page.find('span', class_='error-message').text.strip() assert error_message == 'Can’t be empty' + + +def test_show_sms_prefixing_setting_page( + client_request, + mock_update_service, +): + page = client_request.get( + 'main.service_set_sms_prefix', service_id=SERVICE_ONE_ID + ) + assert normalize_spaces(page.select_one('main p').text) == ( + 'Your service name is service one.' + ) + radios = page.select('input[type=radio]') + assert len(radios) == 2 + assert radios[0]['value'] == 'on' + assert radios[0]['checked'] == '' + assert radios[1]['value'] == 'off' + with pytest.raises(KeyError): + assert radios[1]['checked'] + + +@pytest.mark.parametrize('post_value, expected_api_argument', [ + ('on', True), + ('off', False), +]) +def test_updates_sms_prefixing( + client_request, + mock_update_service, + post_value, + expected_api_argument, +): + client_request.post( + 'main.service_set_sms_prefix', service_id=SERVICE_ONE_ID, + _data={'enabled': post_value}, + _expected_redirect=url_for( + 'main.service_settings', service_id=SERVICE_ONE_ID, + _external=True + ) + ) + mock_update_service.assert_called_once_with( + service_id=SERVICE_ONE_ID, + prefix_sms_with_service_name=expected_api_argument, + ) From 0071e37e61ff671240d131e70b266a5ad28eccc1 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Mon, 6 Nov 2017 11:47:51 +0000 Subject: [PATCH 30/41] Update notifications-python-client from 4.5.0 to 4.6.0 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 27ed7fe20..f7254af32 100644 --- a/requirements.txt +++ b/requirements.txt @@ -16,7 +16,7 @@ gunicorn==19.7.1 whitenoise==3.3.1 #manages static assets # pin to minor version 3.1.x -notifications-python-client==4.5.0 +notifications-python-client==4.6.0 # PaaS awscli>=1.11,<1.12 From fed4275403345d2bdaf6bda91e5a74bd0c1e2902 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Nov 2017 12:15:26 +0000 Subject: [PATCH 31/41] Factor out code that gets message content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nesting is getting pretty deep here. Let’s make it into its own method so it doesn’t get out of hand when we add more functionality to it. --- app/main/views/conversation.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index abdd416de..3d1c7ccc4 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -120,12 +120,7 @@ def get_sms_thread(service_id, user_number): yield { 'inbound': is_inbound, 'content': SMSPreviewTemplate( - { - 'content': ( - notification['content'] if is_inbound else - notification['template']['content'] - ) - }, + {'content': get_sms_content(notification, is_inbound)}, notification.get('personalisation'), downgrade_non_gsm_characters=(not is_inbound), redact_missing_personalisation=redact_personalisation, @@ -134,3 +129,10 @@ def get_sms_thread(service_id, user_number): 'status': notification.get('status'), 'id': notification['id'], } + + +def get_sms_content(notification, is_inbound): + return ( + notification['content'] if is_inbound else + notification['template']['content'] + ) From f6950ae987b9cd829ee6ccf552b8ab60699b8820 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Nov 2017 12:36:26 +0000 Subject: [PATCH 32/41] Stop escaping special characters in inbound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At least one of our providers gives us messages with special characters escaped, ie a newline comes through as `\n`, not a literal newline. We shouldn’t be showing these backslashes to any of our users. Python has built in codecs for dealing with encoding/decoding of strings – see https://docs.python.org/3/library/codecs.html#text-encodings for details. Using these builtins is safer than trying to do anything regex or parsing-based. --- app/main/views/conversation.py | 2 +- app/main/views/dashboard.py | 3 +++ tests/app/main/views/test_conversation.py | 23 +++++++++++++++++++++++ tests/app/main/views/test_dashboard.py | 19 +++++++++++++++++++ tests/conftest.py | 20 ++++++++++++++++++++ 5 files changed, 66 insertions(+), 1 deletion(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index 3d1c7ccc4..e975fe244 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -133,6 +133,6 @@ def get_sms_thread(service_id, user_number): def get_sms_content(notification, is_inbound): return ( - notification['content'] if is_inbound else + bytes(notification['content'], "utf-8").decode('unicode_escape') if is_inbound else notification['template']['content'] ) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index ce63fdc00..b81da435f 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -203,6 +203,9 @@ def get_inbox_partials(service_id): format_phone_number_human_readable(message['user_number']) for message in messages_to_show }: + message.update({ + 'content': bytes(message['content'], 'utf-8').decode('unicode_escape') + }) messages_to_show.append(message) if not inbound_messages: diff --git a/tests/app/main/views/test_conversation.py b/tests/app/main/views/test_conversation.py index dba58a14c..f4edb3685 100644 --- a/tests/app/main/views/test_conversation.py +++ b/tests/app/main/views/test_conversation.py @@ -161,6 +161,29 @@ def test_view_conversation( ) == expected +def test_escaped_characters_in_inbound_messages( + client_request, + mock_get_notification, + mock_get_notifications, + mock_get_inbound_sms_with_special_characters, + fake_uuid, +): + + page = client_request.get( + 'main.conversation', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + ) + + assert normalize_spaces( + str(page.select_one('.sms-message-inbound .sms-message-wrapper')) + ) == ( + "
    " + "the first line's content
    the second line's content " + "
    " + ) + + def test_view_conversation_updates( logged_in_client, mocker, diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 35a68dea4..b1787c691 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -172,6 +172,25 @@ def test_inbox_showing_inbound_messages( ) +def test_inbox_handles_escaped_characters( + client_request, + service_one, + mock_get_inbound_sms_with_special_characters, +): + + service_one['permissions'] = ['inbound_sms'] + + page = client_request.get('main.inbox', service_id=SERVICE_ONE_ID) + + assert normalize_spaces( + str(page.select_one('tbody tr .file-list-hint')) + ) == ( + "" + "the first line's content the second line's content" + "" + ) + + def test_empty_inbox( logged_in_client, service_one, diff --git a/tests/conftest.py b/tests/conftest.py index e1341994e..b61f228a2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1762,6 +1762,26 @@ def mock_get_inbound_sms(mocker): ) +@pytest.fixture(scope='function') +def mock_get_inbound_sms_with_special_characters(mocker): + def _get_inbound_sms( + service_id, + user_number=None, + ): + return [{ + 'user_number': '07900900001', + 'notify_number': '07900000002', + 'content': "the first line\\'s content\\nthe second line\\'s content", + 'created_at': datetime.utcnow().isoformat(), + 'id': sample_uuid(), + }] + + return mocker.patch( + 'app.service_api_client.get_inbound_sms', + side_effect=_get_inbound_sms, + ) + + @pytest.fixture(scope='function') def mock_get_inbound_sms_with_no_messages(mocker): def _get_inbound_sms( From f329e138cd50e18d904ef23d5829fe9d6e77248f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Nov 2017 13:22:46 +0000 Subject: [PATCH 33/41] Factor out string escaping code So that it only lives in one place. --- app/main/views/conversation.py | 4 ++-- app/main/views/dashboard.py | 3 ++- app/utils.py | 4 ++++ 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index e975fe244..53c79372a 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -10,7 +10,7 @@ from notifications_utils.recipients import format_phone_number_human_readable from notifications_utils.template import SMSPreviewTemplate from app.main import main from app.main.forms import SearchTemplatesForm -from app.utils import user_has_permissions +from app.utils import user_has_permissions, unescape_string from app import notification_api_client, service_api_client from notifications_python_client.errors import HTTPError @@ -133,6 +133,6 @@ def get_sms_thread(service_id, user_number): def get_sms_content(notification, is_inbound): return ( - bytes(notification['content'], "utf-8").decode('unicode_escape') if is_inbound else + unescape_string(notification['content']) if is_inbound else notification['template']['content'] ) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index b81da435f..5b59ff912 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -31,6 +31,7 @@ from app.utils import ( FAILURE_STATUSES, REQUESTED_STATUSES, Spreadsheet, + unescape_string, ) @@ -204,7 +205,7 @@ def get_inbox_partials(service_id): for message in messages_to_show }: message.update({ - 'content': bytes(message['content'], 'utf-8').decode('unicode_escape') + 'content': unescape_string(message['content']) }) messages_to_show.append(message) diff --git a/app/utils.py b/app/utils.py index 3a69f4486..e446f9b94 100644 --- a/app/utils.py +++ b/app/utils.py @@ -379,3 +379,7 @@ def get_cdn_domain(): domain = parsed_uri.netloc[len(subdomain + '.'):] return "static-logos.{}".format(domain) + + +def unescape_string(string): + return bytes(string, "utf-8").decode('unicode_escape') From 31497945c0cffe544a2b4b67dcfd7d8becd412a1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Nov 2017 14:12:25 +0000 Subject: [PATCH 34/41] =?UTF-8?q?Change=20wording=20based=20on=20Thom?= =?UTF-8?q?=E2=80=99s=20feedback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/main/forms.py | 2 +- app/main/views/service_settings.py | 2 ++ app/templates/views/service-settings.html | 2 +- app/templates/views/service-settings/sms-prefix.html | 7 ++----- tests/app/main/views/test_service_settings.py | 12 ++++++------ 5 files changed, 12 insertions(+), 13 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index e80fa0023..5c3c389ca 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -723,7 +723,7 @@ class InternationalSMSForm(Form): class SMSPrefixForm(Form): enabled = RadioField( - 'Start all text messages with service name', + '', choices=[ ('on', 'On'), ('off', 'Off'), diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index ecadb06cb..55f7ade46 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -497,6 +497,8 @@ def service_set_sms_prefix(service_id): 'on' if current_service['prefix_sms_with_service_name'] else 'off' )) + form.enabled.label.text = 'Start all text messages with ‘{}:’'.format(current_service['name']) + if form.validate_on_submit(): service_api_client.update_service( current_service['id'], diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 7d417e58e..1eb75ee21 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -93,7 +93,7 @@ {% endcall %} {% call row() %} - {{ text_field('Start all text messages with service name') }} + {{ text_field('Text messages start with service name') }} {{ boolean_field(prefix_sms_with_service_name) }} {{ edit_field('Change', url_for('.service_set_sms_prefix', service_id=current_service.id)) }} {% endcall %} diff --git a/app/templates/views/service-settings/sms-prefix.html b/app/templates/views/service-settings/sms-prefix.html index 5a99b86d6..e74508596 100644 --- a/app/templates/views/service-settings/sms-prefix.html +++ b/app/templates/views/service-settings/sms-prefix.html @@ -3,15 +3,12 @@ {% from "components/page-footer.html" import page_footer %} {% block service_page_title %} - Start all text messages with service name + Text messages start with service name {% endblock %} {% block maincolumn_content %} -

    Start all text messages with service name

    -

    - Your service name is {{ current_service.name}}. -

    +

    Text messages start with service name

    {{ radios(form.enabled) }} {{ page_footer( diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 1f09d38c4..ca180e3a6 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -44,7 +44,7 @@ from tests.conftest import ( 'Label Value Action', 'Send text messages On Change', 'Text message sender GOVUK Manage', - 'Start all text messages with service name On Change', + 'Text messages start with service name On Change', 'International text messages Off Change', 'Receive text messages Off Change', @@ -64,7 +64,7 @@ from tests.conftest import ( 'Label Value Action', 'Send text messages On Change', 'Text message sender GOVUK Manage', - 'Start all text messages with service name On Change', + 'Text messages start with service name On Change', 'International text messages Off Change', 'Receive text messages Off Change', @@ -121,7 +121,7 @@ def test_should_show_overview( 'Label Value Action', 'Send text messages On Change', 'Text message sender GOVUK Manage', - 'Start all text messages with service name On Change', + 'Text messages start with service name On Change', 'International text messages On Change', 'Receive text messages On Change', 'Callback URL for received text messages Not set Change', @@ -141,7 +141,7 @@ def test_should_show_overview( 'Label Value Action', 'Send text messages On Change', 'Text message sender GOVUK Manage', - 'Start all text messages with service name On Change', + 'Text messages start with service name On Change', 'International text messages Off Change', 'Receive text messages Off Change', @@ -2076,8 +2076,8 @@ def test_show_sms_prefixing_setting_page( page = client_request.get( 'main.service_set_sms_prefix', service_id=SERVICE_ONE_ID ) - assert normalize_spaces(page.select_one('main p').text) == ( - 'Your service name is service one.' + assert normalize_spaces(page.select_one('legend').text) == ( + 'Start all text messages with ‘service one:’' ) radios = page.select('input[type=radio]') assert len(radios) == 2 From 9e600b605176145e8ca7e2b6aa271259d3d58902 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Nov 2017 15:08:34 +0000 Subject: [PATCH 35/41] POST to the correct endpoint when updating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `prefix_sms_with_service_name` is a computed attribute on the service model. It’s where we get the value from, and the API does some work to get it from the database, or derive it from the default SMS sender. It can’t be updated, because it’s not itself a database column. `prefix_sms` is the name of the actual database column. This is the thing that we need to update. This will go away eventually. --- app/main/views/service_settings.py | 2 +- app/notify_client/service_api_client.py | 2 +- tests/app/main/views/test_service_settings.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 55f7ade46..79d823084 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -502,7 +502,7 @@ def service_set_sms_prefix(service_id): if form.validate_on_submit(): service_api_client.update_service( current_service['id'], - prefix_sms_with_service_name=(form.enabled.data == 'on') + prefix_sms=(form.enabled.data == 'on') ) return redirect(url_for('.service_settings', service_id=service_id)) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 3667ecba2..2d7b1975b 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -107,7 +107,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): 'permissions', 'organisation_type', 'free_sms_fragment_limit', - 'prefix_sms_with_service_name', + 'prefix_sms', } if disallowed_attributes: raise TypeError('Not allowed to update service attributes: {}'.format( diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index ca180e3a6..08d3d19fa 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -2108,5 +2108,5 @@ def test_updates_sms_prefixing( ) mock_update_service.assert_called_once_with( service_id=SERVICE_ONE_ID, - prefix_sms_with_service_name=expected_api_argument, + prefix_sms=expected_api_argument, ) From 3c9ef52ffb0d2f4530abe23e7d272a8920ebad5c Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Mon, 6 Nov 2017 15:45:20 +0000 Subject: [PATCH 36/41] Update pyexcel-xlsx from 0.5.2 to 0.5.4 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 8bf508785..ec2bc1b97 100644 --- a/requirements.txt +++ b/requirements.txt @@ -9,7 +9,7 @@ blinker==1.4 pyexcel==0.5.6 pyexcel-io==0.5.3 pyexcel-xls==0.5.4 -pyexcel-xlsx==0.5.2 +pyexcel-xlsx==0.5.4 pyexcel-ods3==0.5.2 pytz==2017.3 gunicorn==19.7.1 From 430c67e538209f060150b4bdc5e7a87c47a5d6be Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Mon, 6 Nov 2017 16:34:34 +0000 Subject: [PATCH 37/41] 102 - 103 for FormFinder Admin MOJ --- app/templates/views/signedout.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index 607352fb7..2cb8eb72f 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -116,7 +116,7 @@
    -
    102
    +
    103
    services
    From fce80b87b7fdbaed2dea50937d72231330cfdad7 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 6 Nov 2017 17:37:15 +0000 Subject: [PATCH 38/41] Move jshint configuration to .jshintrc When provided with inline configuration in a gulp task jshint will still try to load a configuration file from the current directory or the user's home directory. If user has a global .jshintrc file that sets different linting options this could lead to `npm test` output being different from the CI one. jshint only uses the first file it finds, and .jshintrc in current directory or any parent of the current directory takes precedence over the user one, so moving jshint configuration from gulpfile to .jshintrc should make `npm test` produce the same outcome regardless of the user config. --- .jshintrc | 1 + gulpfile.babel.js | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 .jshintrc diff --git a/.jshintrc b/.jshintrc new file mode 100644 index 000000000..368bcf310 --- /dev/null +++ b/.jshintrc @@ -0,0 +1 @@ +{"esversion": 6, "esnext": false} diff --git a/gulpfile.babel.js b/gulpfile.babel.js index 9ecc9c15d..872630dfb 100644 --- a/gulpfile.babel.js +++ b/gulpfile.babel.js @@ -138,7 +138,7 @@ gulp.task('lint:sass', () => gulp gulp.task('lint:js', () => gulp .src(paths.src + 'javascripts/**/*.js') - .pipe(plugins.jshint({'esversion': 6, 'esnext': false})) + .pipe(plugins.jshint()) .pipe(plugins.jshint.reporter(stylish)) .pipe(plugins.jshint.reporter('fail')) ); From 75bca28cc16f60ed2b98bb45363a3dede35dd12a Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Tue, 7 Nov 2017 16:36:05 +0000 Subject: [PATCH 39/41] 103 - 104 and 44 - 45 orgs for DBS --- app/templates/views/signedout.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index 2cb8eb72f..64776c5e6 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -116,12 +116,12 @@
    -
    103
    +
    104
    services
    -
    44
    +
    45
    organisations
    From 83d8e3d99bb9c2fadfe24de34319de4dbd33a182 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Tue, 7 Nov 2017 17:02:22 +0000 Subject: [PATCH 40/41] Updated the domain for CHECK link from cesg to ncsc --- app/templates/views/information-risk-management.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/information-risk-management.html b/app/templates/views/information-risk-management.html index a79480c10..71724b479 100644 --- a/app/templates/views/information-risk-management.html +++ b/app/templates/views/information-risk-management.html @@ -33,7 +33,7 @@
  • formal risk assessments using a methodology based on ISO 27005:2011 and supplemented by reference to NCSC standards and guidance documentation
    • -
  • CHECK-based +
  • CHECK-based IT Health Check (ITHC) testing (annual and on major change)
  • residual risk statement preparation and active management of the risk treatment plan
  • regular updates to the Privacy Impact Assessment
    • From 6325f260815bbd6abb48424c7ae51023b2947235 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 Nov 2017 17:22:57 +0000 Subject: [PATCH 41/41] Revert "Stop escaping special characters in inbound messages" --- app/main/views/conversation.py | 16 +++++++--------- app/main/views/dashboard.py | 4 ---- app/utils.py | 4 ---- tests/app/main/views/test_conversation.py | 23 ----------------------- tests/app/main/views/test_dashboard.py | 19 ------------------- tests/conftest.py | 20 -------------------- 6 files changed, 7 insertions(+), 79 deletions(-) diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index 53c79372a..abdd416de 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -10,7 +10,7 @@ from notifications_utils.recipients import format_phone_number_human_readable from notifications_utils.template import SMSPreviewTemplate from app.main import main from app.main.forms import SearchTemplatesForm -from app.utils import user_has_permissions, unescape_string +from app.utils import user_has_permissions from app import notification_api_client, service_api_client from notifications_python_client.errors import HTTPError @@ -120,7 +120,12 @@ def get_sms_thread(service_id, user_number): yield { 'inbound': is_inbound, 'content': SMSPreviewTemplate( - {'content': get_sms_content(notification, is_inbound)}, + { + 'content': ( + notification['content'] if is_inbound else + notification['template']['content'] + ) + }, notification.get('personalisation'), downgrade_non_gsm_characters=(not is_inbound), redact_missing_personalisation=redact_personalisation, @@ -129,10 +134,3 @@ def get_sms_thread(service_id, user_number): 'status': notification.get('status'), 'id': notification['id'], } - - -def get_sms_content(notification, is_inbound): - return ( - unescape_string(notification['content']) if is_inbound else - notification['template']['content'] - ) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 5b59ff912..ce63fdc00 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -31,7 +31,6 @@ from app.utils import ( FAILURE_STATUSES, REQUESTED_STATUSES, Spreadsheet, - unescape_string, ) @@ -204,9 +203,6 @@ def get_inbox_partials(service_id): format_phone_number_human_readable(message['user_number']) for message in messages_to_show }: - message.update({ - 'content': unescape_string(message['content']) - }) messages_to_show.append(message) if not inbound_messages: diff --git a/app/utils.py b/app/utils.py index e446f9b94..3a69f4486 100644 --- a/app/utils.py +++ b/app/utils.py @@ -379,7 +379,3 @@ def get_cdn_domain(): domain = parsed_uri.netloc[len(subdomain + '.'):] return "static-logos.{}".format(domain) - - -def unescape_string(string): - return bytes(string, "utf-8").decode('unicode_escape') diff --git a/tests/app/main/views/test_conversation.py b/tests/app/main/views/test_conversation.py index f4edb3685..dba58a14c 100644 --- a/tests/app/main/views/test_conversation.py +++ b/tests/app/main/views/test_conversation.py @@ -161,29 +161,6 @@ def test_view_conversation( ) == expected -def test_escaped_characters_in_inbound_messages( - client_request, - mock_get_notification, - mock_get_notifications, - mock_get_inbound_sms_with_special_characters, - fake_uuid, -): - - page = client_request.get( - 'main.conversation', - service_id=SERVICE_ONE_ID, - notification_id=fake_uuid, - ) - - assert normalize_spaces( - str(page.select_one('.sms-message-inbound .sms-message-wrapper')) - ) == ( - "
    " - "the first line's content
    the second line's content " - "
    " - ) - - def test_view_conversation_updates( logged_in_client, mocker, diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index b1787c691..35a68dea4 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -172,25 +172,6 @@ def test_inbox_showing_inbound_messages( ) -def test_inbox_handles_escaped_characters( - client_request, - service_one, - mock_get_inbound_sms_with_special_characters, -): - - service_one['permissions'] = ['inbound_sms'] - - page = client_request.get('main.inbox', service_id=SERVICE_ONE_ID) - - assert normalize_spaces( - str(page.select_one('tbody tr .file-list-hint')) - ) == ( - "" - "the first line's content the second line's content" - "" - ) - - def test_empty_inbox( logged_in_client, service_one, diff --git a/tests/conftest.py b/tests/conftest.py index b61f228a2..e1341994e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1762,26 +1762,6 @@ def mock_get_inbound_sms(mocker): ) -@pytest.fixture(scope='function') -def mock_get_inbound_sms_with_special_characters(mocker): - def _get_inbound_sms( - service_id, - user_number=None, - ): - return [{ - 'user_number': '07900900001', - 'notify_number': '07900000002', - 'content': "the first line\\'s content\\nthe second line\\'s content", - 'created_at': datetime.utcnow().isoformat(), - 'id': sample_uuid(), - }] - - return mocker.patch( - 'app.service_api_client.get_inbound_sms', - side_effect=_get_inbound_sms, - ) - - @pytest.fixture(scope='function') def mock_get_inbound_sms_with_no_messages(mocker): def _get_inbound_sms(