Merge pull request #1599 from alphagov/new-acc-email-validation

New account verification emails no longer call API

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

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'

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/<token>")
 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:

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/<path:token>', 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'))

app/main/views/verify.py

@@ -50,34 +50,26 @@ def verify():
 @main.route('/verify-email/<token>')
 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')

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."

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'))

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

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'