From aff9d473233e6e3dbeb21a4f68092651ef0670f8 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 1 Nov 2017 14:39:14 +0000 Subject: [PATCH 1/3] 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 2/3] 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 3/3] 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'))