diff --git a/app/__init__.py b/app/__init__.py index 34aa8523e..4de099f5c 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 @@ -438,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) @@ -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/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/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/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/test_errorhandlers.py b/tests/app/main/test_errorhandlers.py index f9185a1d1..93bbfe3e1 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(logged_in_client, url): + response = logged_in_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." 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')) 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'