From ab10009e4dd493377d18ed3521c3440832e57f07 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 5 Oct 2020 15:38:34 +0100 Subject: [PATCH 01/12] Redirect link included in links for password reset on sign-in page. --- app/main/views/sign_in.py | 10 ++++++---- app/templates/views/signin.html | 2 +- tests/app/main/views/test_sign_in.py | 10 ++++++++++ 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index ae15fa751..d5b9cd1bb 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -24,6 +24,7 @@ def sign_in(): return redirect(url_for('main.show_accounts_or_dashboard')) form = LoginForm() + password_reset_url = url_for('.forgot_password', next=request.args.get('next')) if form.validate_on_submit(): @@ -51,9 +52,9 @@ def sign_in(): # Vague error message for login in case of user not known, locked, inactive or password not verified flash(Markup( ( - "The email address or password you entered is incorrect." - " Forgotten your password?" - ).format(password_reset=url_for('.forgot_password')) + f"The email address or password you entered is incorrect." + f" Forgotten your password?" + ) )) other_device = current_user.logged_in_elsewhere() @@ -61,7 +62,8 @@ def sign_in(): 'views/signin.html', form=form, again=bool(request.args.get('next')), - other_device=other_device + other_device=other_device, + password_reset_url=password_reset_url ) diff --git a/app/templates/views/signin.html b/app/templates/views/signin.html index 374e59f89..c4dd5c609 100644 --- a/app/templates/views/signin.html +++ b/app/templates/views/signin.html @@ -33,7 +33,7 @@ {% call form_wrapper(autocomplete=True) %} {{ form.email_address(param_extensions={"autocomplete": "email"}) }} {{ form.password(param_extensions={"autocomplete": "current-password"}) }} - {{ page_footer("Continue", secondary_link=url_for('.forgot_password'), secondary_link_text="Forgotten your password?") }} + {{ page_footer("Continue", secondary_link=password_reset_url, secondary_link_text="Forgotten your password?") }} {% endcall %} diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index 671305998..8e0208f20 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -27,6 +27,16 @@ def test_render_sign_in_template_for_new_user( assert 'Sign in again' not in normalize_spaces(page.text) +def test_render_sign_in_template_with_next_link_for_password_reset( + client_request +): + client_request.logout() + page = client_request.get('main.sign_in', _optional_args="?next=blob", _test_page_title=False) + forgot_password_link = page.find('a', class_="govuk-link govuk-link--no-visited-state page-footer-secondary-link") + assert forgot_password_link.text == 'Forgotten your password?' + assert forgot_password_link['href'] == url_for('main.forgot_password') + "?next=blob" + + def test_sign_in_explains_session_timeout(client): response = client.get(url_for('main.sign_in', next='/foo')) assert response.status_code == 200 From d0894245013e9e130542101d4093a396421f795d Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 5 Oct 2020 15:51:44 +0100 Subject: [PATCH 02/12] Forgot password sends redirect link with reset password email. This is so when users reset their password they are still redirected to pages they were meant to visit. This change was done specifically so everyone who is meant to see broadcast tour sees it, but it will improve lives of all users who wanted to visit a page on Notify but then had to reset their password in the process. --- app/main/views/forgot_password.py | 4 ++-- app/notify_client/user_api_client.py | 4 +++- tests/app/main/views/test_forgot_password.py | 22 ++++++++++++++++++-- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/app/main/views/forgot_password.py b/app/main/views/forgot_password.py index a16c8a78a..359ad40c9 100644 --- a/app/main/views/forgot_password.py +++ b/app/main/views/forgot_password.py @@ -1,4 +1,4 @@ -from flask import render_template +from flask import render_template, request from notifications_python_client.errors import HTTPError from app import user_api_client @@ -11,7 +11,7 @@ def forgot_password(): form = ForgotPasswordForm() if form.validate_on_submit(): try: - user_api_client.send_reset_password_url(form.email_address.data) + user_api_client.send_reset_password_url(form.email_address.data, next_string=request.args.get('next')) except HTTPError as e: if e.status_code == 404: return render_template('views/password-reset-sent.html') diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index c97b241be..bafd22284 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -165,9 +165,11 @@ class UserApiClient(NotifyAdminAPIClient): endpoint = '/user/{}/service/{}/permission'.format(user_id, service_id) self.post(endpoint, data=data) - def send_reset_password_url(self, email_address): + def send_reset_password_url(self, email_address, next_string=None): endpoint = '/user/reset-password' data = {'email': email_address} + if next_string: + data['next'] = next_string self.post(endpoint, data=data) def find_users_by_full_or_partial_email(self, email_address): diff --git a/tests/app/main/views/test_forgot_password.py b/tests/app/main/views/test_forgot_password.py index c75f83210..dc7d4fd7e 100644 --- a/tests/app/main/views/test_forgot_password.py +++ b/tests/app/main/views/test_forgot_password.py @@ -31,7 +31,23 @@ def test_should_redirect_to_password_reset_sent_for_valid_email( assert response.status_code == 200 assert 'Click the link in the email to reset your password.' \ in response.get_data(as_text=True) - app.user_api_client.send_reset_password_url.assert_called_once_with(sample_user['email_address']) + app.user_api_client.send_reset_password_url.assert_called_once_with(sample_user['email_address'], next_string=None) + + +def test_forgot_password_sends_next_link_with_reset_password_email_request( + client, + fake_uuid, + mocker, +): + sample_user = user_json(email_address='test@user.gov.uk') + mocker.patch('app.user_api_client.send_reset_password_url', return_value=None) + response = client.post( + url_for('.forgot_password') + "?next=blob", + data={'email_address': sample_user['email_address']}) + assert response.status_code == 200 + app.user_api_client.send_reset_password_url.assert_called_once_with( + sample_user['email_address'], next_string="blob" + ) def test_should_redirect_to_password_reset_sent_for_missing_email( @@ -48,4 +64,6 @@ def test_should_redirect_to_password_reset_sent_for_missing_email( assert response.status_code == 200 assert 'Click the link in the email to reset your password.' \ in response.get_data(as_text=True) - app.user_api_client.send_reset_password_url.assert_called_once_with(api_user_active['email_address']) + app.user_api_client.send_reset_password_url.assert_called_once_with( + api_user_active['email_address'], next_string=None + ) From c3b7481e119b89693141dc59cd4451e17ccbc2a1 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 9 Oct 2020 11:39:01 +0100 Subject: [PATCH 03/12] Turn on redirects for check_and_resend_text_code This is part of the work to make sure user is redirected to the page they initially were meant to visit after they sign in. --- app/main/views/code_not_received.py | 7 ++++--- .../views/verification-not-received.html | 2 +- .../app/main/views/test_code_not_received.py | 20 ++++++++++++++++--- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/app/main/views/code_not_received.py b/app/main/views/code_not_received.py index ac51b3be1..5b75325b1 100644 --- a/app/main/views/code_not_received.py +++ b/app/main/views/code_not_received.py @@ -1,4 +1,4 @@ -from flask import redirect, render_template, session, url_for +from flask import redirect, render_template, request, session, url_for from app import user_api_client from app.main import main @@ -19,16 +19,17 @@ def resend_email_verification(): @redirect_to_sign_in def check_and_resend_text_code(): user = User.from_email_address(session['user_details']['email']) + redirect_url = request.args.get('next') if user.state == 'active': # this is a verified user and therefore redirect to page to request resend without edit mobile - return render_template('views/verification-not-received.html') + return render_template('views/verification-not-received.html', redirect_url=redirect_url) form = TextNotReceivedForm(mobile_number=user.mobile_number) if form.validate_on_submit(): user.send_verify_code(to=form.mobile_number.data) user.update(mobile_number=form.mobile_number.data) - return redirect(url_for('.verify')) + return redirect(url_for('.verify', next=redirect_url)) return render_template('views/text-not-received.html', form=form) diff --git a/app/templates/views/verification-not-received.html b/app/templates/views/verification-not-received.html index 5b15d35dc..085b9c210 100644 --- a/app/templates/views/verification-not-received.html +++ b/app/templates/views/verification-not-received.html @@ -16,7 +16,7 @@ {{ govukButton({ "element": "a", "text": "Resend security code", - "href": url_for('main.check_and_resend_verification_code') + "href": url_for('main.check_and_resend_verification_code', next=redirect_url) }) }}

diff --git a/tests/app/main/views/test_code_not_received.py b/tests/app/main/views/test_code_not_received.py index 23712125e..9fd5452e3 100644 --- a/tests/app/main/views/test_code_not_received.py +++ b/tests/app/main/views/test_code_not_received.py @@ -27,23 +27,32 @@ def test_should_render_email_verification_resend_show_email_address_and_resend_v mock_send_verify_email.assert_called_with(api_user_active['id'], api_user_active['email_address']) +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) def test_should_render_correct_resend_template_for_active_user( client, api_user_active, mock_get_user_by_email, mock_send_verify_code, + redirect_url ): with client.session_transaction() as session: session['user_details'] = { 'id': api_user_active['id'], 'email': api_user_active['email_address']} - response = client.get(url_for('main.check_and_resend_text_code')) + response = client.get(url_for('main.check_and_resend_text_code', next=redirect_url)) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.string == 'Resend security code' # there shouldn't be a form for updating mobile number assert page.find('form') is None + assert page.find('a', class_="govuk-button")['href'] == url_for( + 'main.check_and_resend_verification_code', + next=redirect_url + ) def test_should_render_correct_resend_template_for_pending_user( @@ -71,6 +80,10 @@ def test_should_render_correct_resend_template_for_pending_user( assert page.find('form').input['value'] == api_user_pending['mobile_number'] +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) @pytest.mark.parametrize('phone_number_to_register_with', [ '+447700900460', '+1800-555-555', @@ -82,6 +95,7 @@ def test_should_resend_verify_code_and_update_mobile_for_pending_user( mock_update_user_attribute, mock_send_verify_code, phone_number_to_register_with, + redirect_url ): mocker.patch('app.user_api_client.get_user_by_email', return_value=api_user_pending) @@ -89,10 +103,10 @@ def test_should_resend_verify_code_and_update_mobile_for_pending_user( session['user_details'] = { 'id': api_user_pending['id'], 'email': api_user_pending['email_address']} - response = client.post(url_for('main.check_and_resend_text_code'), + response = client.post(url_for('main.check_and_resend_text_code', next=redirect_url), data={'mobile_number': phone_number_to_register_with}) assert response.status_code == 302 - assert response.location == url_for('main.verify', _external=True) + assert response.location == url_for('main.verify', _external=True, next=redirect_url) mock_update_user_attribute.assert_called_once_with( api_user_pending['id'], From 1dd8b080421d777115beec2c5279c26a41c9c1c1 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 9 Oct 2020 11:39:44 +0100 Subject: [PATCH 04/12] Turn on redirects for check_and_resend_verification_code This is part of the work to make sure user is redirected to the page they initially were meant to visit after they sign in. --- app/main/views/code_not_received.py | 5 +++-- tests/app/main/views/test_code_not_received.py | 18 ++++++++++++++---- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/app/main/views/code_not_received.py b/app/main/views/code_not_received.py index 5b75325b1..a644e3f34 100644 --- a/app/main/views/code_not_received.py +++ b/app/main/views/code_not_received.py @@ -39,10 +39,11 @@ def check_and_resend_text_code(): def check_and_resend_verification_code(): user = User.from_email_address(session['user_details']['email']) user.send_verify_code() + redirect_url = request.args.get('next') if user.state == 'pending': - return redirect(url_for('main.verify')) + return redirect(url_for('main.verify', next=redirect_url)) else: - return redirect(url_for('main.two_factor')) + return redirect(url_for('main.two_factor', next=redirect_url)) @main.route('/email-not-received', methods=['GET']) diff --git a/tests/app/main/views/test_code_not_received.py b/tests/app/main/views/test_code_not_received.py index 9fd5452e3..143392987 100644 --- a/tests/app/main/views/test_code_not_received.py +++ b/tests/app/main/views/test_code_not_received.py @@ -119,27 +119,37 @@ def test_should_resend_verify_code_and_update_mobile_for_pending_user( ) +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) def test_check_and_redirect_to_two_factor_if_user_active( client, api_user_active, mock_get_user_by_email, mock_send_verify_code, + redirect_url ): with client.session_transaction() as session: session['user_details'] = { 'id': api_user_active['id'], 'email': api_user_active['email_address']} - response = client.get(url_for('main.check_and_resend_verification_code')) + response = client.get(url_for('main.check_and_resend_verification_code', next=redirect_url)) assert response.status_code == 302 - assert response.location == url_for('main.two_factor', _external=True) + assert response.location == url_for('main.two_factor', _external=True, next=redirect_url) +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) def test_check_and_redirect_to_verify_if_user_pending( client, mocker, api_user_pending, mock_get_user_pending, mock_send_verify_code, + redirect_url ): mocker.patch('app.user_api_client.get_user_by_email', return_value=api_user_pending) @@ -148,9 +158,9 @@ def test_check_and_redirect_to_verify_if_user_pending( session['user_details'] = { 'id': api_user_pending['id'], 'email': api_user_pending['email_address']} - response = client.get(url_for('main.check_and_resend_verification_code')) + response = client.get(url_for('main.check_and_resend_verification_code', next=redirect_url)) assert response.status_code == 302 - assert response.location == url_for('main.verify', _external=True) + assert response.location == url_for('main.verify', _external=True, next=redirect_url) @pytest.mark.parametrize('endpoint', [ From b0db60e417111b451db8343bb3b3e14e14d855bc Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 9 Oct 2020 11:40:04 +0100 Subject: [PATCH 05/12] Turn on redirects for email_not_received This is part of the work to make sure user is redirected to the page they initially were meant to visit after they sign in. --- app/main/views/code_not_received.py | 3 ++- app/templates/views/email-not-received.html | 2 +- .../app/main/views/test_code_not_received.py | 25 +++++++++++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/app/main/views/code_not_received.py b/app/main/views/code_not_received.py index a644e3f34..25e2f060a 100644 --- a/app/main/views/code_not_received.py +++ b/app/main/views/code_not_received.py @@ -49,7 +49,8 @@ def check_and_resend_verification_code(): @main.route('/email-not-received', methods=['GET']) @redirect_to_sign_in def email_not_received(): - return render_template('views/email-not-received.html') + redirect_url = request.args.get('next') + return render_template('views/email-not-received.html', redirect_url=redirect_url) @main.route('/send-new-email-token', methods=['GET']) diff --git a/app/templates/views/email-not-received.html b/app/templates/views/email-not-received.html index f7eb1dd96..a6ddc8881 100644 --- a/app/templates/views/email-not-received.html +++ b/app/templates/views/email-not-received.html @@ -16,7 +16,7 @@ {{ govukButton({ "element": "a", "text": "Resend email link", - "href": url_for('main.resend_email_link') + "href": url_for('main.resend_email_link', next=redirect_url) }) }}

diff --git a/tests/app/main/views/test_code_not_received.py b/tests/app/main/views/test_code_not_received.py index 143392987..6e8a2b245 100644 --- a/tests/app/main/views/test_code_not_received.py +++ b/tests/app/main/views/test_code_not_received.py @@ -176,3 +176,28 @@ def test_redirect_to_sign_in_if_not_logged_in( assert response.location == url_for('main.sign_in', _external=True) assert response.status_code == 302 + + +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) +def test_should_render_correct_email_not_received_template_for_active_user( + client, + api_user_active, + mock_get_user_by_email, + mock_send_verify_code, + redirect_url +): + with client.session_transaction() as session: + session['user_details'] = { + 'id': api_user_active['id'], + 'email': api_user_active['email_address']} + response = client.get(url_for('main.email_not_received', next=redirect_url)) + assert response.status_code == 200 + + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string == 'Resend email link' + # there shouldn't be a form for updating mobile number + assert page.find('form') is None + assert page.find('a', class_="govuk-button")['href'] == url_for('main.resend_email_link', next=redirect_url) From 1fc2bee42d6159fc3d9029a8e285baae27da6186 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 9 Oct 2020 11:40:28 +0100 Subject: [PATCH 06/12] Turn on redirects for new_password This is part of the work to make sure user is redirected to the page they initially were meant to visit after they sign in. --- app/main/views/new_password.py | 3 ++- tests/app/main/views/test_new_password.py | 12 +++++++++--- tests/conftest.py | 4 ++-- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/main/views/new_password.py b/app/main/views/new_password.py index 7b3c701f9..2f35d9ddf 100644 --- a/app/main/views/new_password.py +++ b/app/main/views/new_password.py @@ -5,6 +5,7 @@ from flask import ( flash, redirect, render_template, + request, session, url_for, ) @@ -46,6 +47,6 @@ def new_password(token): else: # send user a 2fa sms code user.send_verify_code() - return redirect(url_for('main.two_factor')) + return redirect(url_for('main.two_factor', next=request.args.get('next'))) else: return render_template('views/new-password.html', token=token, form=form, user=user) diff --git a/tests/app/main/views/test_new_password.py b/tests/app/main/views/test_new_password.py index 80359bcd7..e7641438d 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 +import pytest from flask import url_for from itsdangerous import SignatureExpired from notifications_utils.url_safe_token import generate_token @@ -36,21 +37,26 @@ def test_should_return_404_when_email_address_does_not_exist( assert response.status_code == 404 +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) def test_should_redirect_to_two_factor_when_password_reset_is_successful( app_, client, mock_get_user_by_email_request_password_reset, mock_login, mock_send_verify_code, - mock_reset_failed_login_count + mock_reset_failed_login_count, + redirect_url ): user = mock_get_user_by_email_request_password_reset.return_value data = json.dumps({'email': user['email_address'], 'created_at': str(datetime.utcnow())}) token = generate_token(data, app_.config['SECRET_KEY'], app_.config['DANGEROUS_SALT']) - response = client.post(url_for_endpoint_with_token('.new_password', token=token), + response = client.post(url_for_endpoint_with_token('.new_password', token=token, next=redirect_url), data={'new_password': 'a-new_password'}) assert response.status_code == 302 - assert response.location == url_for('.two_factor', _external=True) + assert response.location == url_for('.two_factor', _external=True, next=redirect_url) mock_get_user_by_email_request_password_reset.assert_called_once_with(user['email_address']) diff --git a/tests/conftest.py b/tests/conftest.py index 9015c7cf0..a2c2c7121 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3646,9 +3646,9 @@ def mock_create_event(mocker): return mocker.patch('app.events_api_client.create_event', side_effect=_add_event) -def url_for_endpoint_with_token(endpoint, token): +def url_for_endpoint_with_token(endpoint, token, next=None): token = token.replace('%2E', '.') - return url_for(endpoint, token=token) + return url_for(endpoint, token=token, next=next) @pytest.fixture From a6543c6e57f93bf1ff9f6c880ea0881cf011294a Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 9 Oct 2020 11:41:06 +0100 Subject: [PATCH 07/12] Turn on redirects for sign_in This is part of the work to make sure user is redirected to the page they initially were meant to visit after they sign in. --- app/main/views/sign_in.py | 9 ++++---- tests/app/main/views/test_sign_in.py | 31 +++++++++++++++++++++------- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index d5b9cd1bb..7ce9225b4 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -25,6 +25,7 @@ def sign_in(): form = LoginForm() password_reset_url = url_for('.forgot_password', next=request.args.get('next')) + redirect_url = request.args.get('next') if form.validate_on_submit(): @@ -33,7 +34,7 @@ def sign_in(): ) if user and user.state == 'pending': - return redirect(url_for('main.resend_email_verification')) + return redirect(url_for('main.resend_email_verification', next=redirect_url)) if user and session.get('invited_user'): invited_user = InvitedUser.from_session() @@ -45,9 +46,9 @@ def sign_in(): invited_user.accept_invite() if user and user.sign_in(): if user.sms_auth: - return redirect(url_for('.two_factor', next=request.args.get('next'))) + return redirect(url_for('.two_factor', next=redirect_url)) if user.email_auth: - return redirect(url_for('.two_factor_email_sent')) + return redirect(url_for('.two_factor_email_sent', next=redirect_url)) # Vague error message for login in case of user not known, locked, inactive or password not verified flash(Markup( @@ -61,7 +62,7 @@ def sign_in(): return render_template( 'views/signin.html', form=form, - again=bool(request.args.get('next')), + again=bool(redirect_url), other_device=other_device, password_reset_url=password_reset_url ) diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index 8e0208f20..e22d929db 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -102,6 +102,10 @@ def test_logged_in_user_redirects_to_account( ) +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) @pytest.mark.parametrize('email_address, password', [ ('valid@example.gov.uk', 'val1dPassw0rd!'), (' valid@example.gov.uk ', ' val1dPassw0rd! '), @@ -115,34 +119,40 @@ def test_process_sms_auth_sign_in_return_2fa_template( mock_verify_password, email_address, password, + redirect_url ): response = client.post( - url_for('main.sign_in'), data={ + url_for('main.sign_in', next=redirect_url), data={ 'email_address': email_address, 'password': password}) assert response.status_code == 302 - assert response.location == url_for('.two_factor', _external=True) + assert response.location == url_for('.two_factor', next=redirect_url, _external=True) mock_verify_password.assert_called_with(api_user_active['id'], password) mock_get_user_by_email.assert_called_with('valid@example.gov.uk') +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) def test_process_email_auth_sign_in_return_2fa_template( client, api_user_active_email_auth, mock_send_verify_code, mock_verify_password, - mocker + mocker, + redirect_url ): mocker.patch('app.user_api_client.get_user', return_value=api_user_active_email_auth) mocker.patch('app.user_api_client.get_user_by_email', return_value=api_user_active_email_auth) response = client.post( - url_for('main.sign_in'), data={ + url_for('main.sign_in', next=redirect_url), data={ 'email_address': 'valid@example.gov.uk', 'password': 'val1dPassw0rd!'}) assert response.status_code == 302 - assert response.location == url_for('.two_factor_email_sent', _external=True) - mock_send_verify_code.assert_called_with(api_user_active_email_auth['id'], 'email', None, None) + assert response.location == url_for('.two_factor_email_sent', _external=True, next=redirect_url) + mock_send_verify_code.assert_called_with(api_user_active_email_auth['id'], 'email', None, redirect_url) mock_verify_password.assert_called_with(api_user_active_email_auth['id'], 'val1dPassw0rd!') @@ -185,16 +195,21 @@ def test_should_return_redirect_when_user_is_pending( assert response.status_code == 200 +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) def test_should_attempt_redirect_when_user_is_pending( client, mock_get_user_by_email_pending, mock_verify_password, + redirect_url ): response = client.post( - url_for('main.sign_in'), data={ + url_for('main.sign_in', next=redirect_url), data={ 'email_address': 'pending_user@example.gov.uk', 'password': 'val1dPassw0rd!'}) - assert response.location == url_for('main.resend_email_verification', _external=True) + assert response.location == url_for('main.resend_email_verification', _external=True, next=redirect_url) assert response.status_code == 302 From 5dd010ece821cc6720b9b2757707145c0e9589ca Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 9 Oct 2020 11:41:24 +0100 Subject: [PATCH 08/12] Turn on redirects for two_factor_email_sent This is part of the work to make sure user is redirected to the page they initially were meant to visit after they sign in. --- app/main/views/two_factor.py | 3 ++- app/templates/views/two-factor-email.html | 2 +- tests/app/main/views/test_two_factor.py | 26 +++++++++++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index e9140e8aa..1d3eee998 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -24,7 +24,8 @@ def two_factor_email_sent(): title = 'Email resent' if request.args.get('email_resent') else 'Check your email' return render_template( 'views/two-factor-email.html', - title=title + title=title, + redirect_url=request.args.get('next') ) diff --git a/app/templates/views/two-factor-email.html b/app/templates/views/two-factor-email.html index 82e2c749e..81315863e 100644 --- a/app/templates/views/two-factor-email.html +++ b/app/templates/views/two-factor-email.html @@ -13,7 +13,7 @@

We’ve emailed you a link to sign in to Notify.

Clicking the link will open Notify in a new browser window, so you can close this one.

{{ page_footer( - secondary_link=url_for('main.email_not_received'), + secondary_link=url_for('main.email_not_received', next=redirect_url), secondary_link_text='Not received an email?' ) }} diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index 2c2c9dd2e..49a41a037 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -11,6 +11,32 @@ from tests.conftest import ( ) +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) +@pytest.mark.parametrize('email_resent, page_title', [ + (None, 'Check your email'), + (True, 'Email resent') +]) +def test_two_factor_email_sent_page( + client, + email_resent, + page_title, + redirect_url +): + response = client.get(url_for('main.two_factor_email_sent', next=redirect_url, email_resent=email_resent)) + assert response.status_code == 200 + + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string == page_title + # there shouldn't be a form for updating mobile number + assert page.find('form') is None + resend_email_link = page.find('a', class_="govuk-link govuk-link--no-visited-state page-footer-secondary-link") + assert resend_email_link.text == 'Not received an email?' + assert resend_email_link['href'] == url_for('main.email_not_received', next=redirect_url) + + def test_should_render_two_factor_page( client, api_user_active, From a531c888bacad654b9f7978652b08d0347645ec6 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 9 Oct 2020 11:41:47 +0100 Subject: [PATCH 09/12] Turn on redirects two_factor_email This is part of the work to make sure user is redirected to the page they initially were meant to visit after they sign in. --- app/main/views/two_factor.py | 5 +++-- app/templates/views/email-link-invalid.html | 6 +++++- tests/app/main/views/test_two_factor.py | 23 +++++++++++++++++---- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 1d3eee998..0be1239fc 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -36,6 +36,7 @@ def two_factor_email_interstitial(token): @main.route('/email-auth/', methods=['POST']) def two_factor_email(token): + redirect_url = request.args.get('next') if current_user.is_authenticated: return redirect_when_logged_in(platform_admin=current_user.platform_admin) @@ -48,14 +49,14 @@ def two_factor_email(token): current_app.config['EMAIL_2FA_EXPIRY_SECONDS'] )) except SignatureExpired: - return render_template('views/email-link-invalid.html') + return render_template('views/email-link-invalid.html', redirect_url=redirect_url) user_id = token_data['user_id'] # checks if code was already used logged_in, msg = user_api_client.check_verify_code(user_id, token_data['secret_code'], "email") if not logged_in: - return render_template('views/email-link-invalid.html') + return render_template('views/email-link-invalid.html', redirect_url=redirect_url) return log_in_user(user_id) diff --git a/app/templates/views/email-link-invalid.html b/app/templates/views/email-link-invalid.html index 613173d8a..8d45c46a0 100644 --- a/app/templates/views/email-link-invalid.html +++ b/app/templates/views/email-link-invalid.html @@ -11,7 +11,11 @@

The link has expired

-

Sign in again to get a new link.

+

+ + Sign in again + to get a new link. +

diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index 49a41a037..5fc08934f 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -339,17 +339,22 @@ def test_valid_two_factor_email_link_logs_in_user( assert response.location == url_for('main.show_accounts_or_dashboard', _external=True) +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) def test_two_factor_email_link_has_expired( app_, valid_token, client, mock_send_verify_code, - fake_uuid + fake_uuid, + redirect_url ): with set_config(app_, 'EMAIL_2FA_EXPIRY_SECONDS', -1): response = client.post( - url_for_endpoint_with_token('main.two_factor_email', token=valid_token), + url_for_endpoint_with_token('main.two_factor_email', token=valid_token, next=redirect_url), follow_redirects=True, ) @@ -357,6 +362,8 @@ def test_two_factor_email_link_has_expired( page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.text.strip() == 'The link has expired' + assert page.select_one('a:contains("Sign in again")')['href'] == url_for('main.sign_in', next=redirect_url) + assert mock_send_verify_code.called is False @@ -372,20 +379,26 @@ def test_two_factor_email_link_is_invalid( assert normalize_spaces( page.select_one('.banner-dangerous').text ) == "There’s something wrong with the link you’ve used." + assert response.status_code == 404 +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) def test_two_factor_email_link_is_already_used( client, valid_token, mocker, - mock_send_verify_code + mock_send_verify_code, + redirect_url ): mocker.patch('app.user_api_client.check_verify_code', return_value=(False, 'Code has expired')) response = client.post( - url_for_endpoint_with_token('main.two_factor_email', token=valid_token), + url_for_endpoint_with_token('main.two_factor_email', token=valid_token, next=redirect_url), follow_redirects=True ) @@ -393,6 +406,8 @@ def test_two_factor_email_link_is_already_used( assert response.status_code == 200 assert page.h1.text.strip() == 'The link has expired' + assert page.select_one('a:contains("Sign in again")')['href'] == url_for('main.sign_in', next=redirect_url) + assert mock_send_verify_code.called is False From 44ddee23ac5f9ec90bdc8f85bc70e3d7e573d651 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 9 Oct 2020 11:42:21 +0100 Subject: [PATCH 10/12] Turn on redirects two_factor This is part of the work to make sure user is redirected to the page they initially were meant to visit after they sign in. --- app/main/views/two_factor.py | 7 ++++--- app/templates/views/two-factor.html | 2 +- tests/app/main/views/test_two_factor.py | 21 +++++++++++++++++---- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 0be1239fc..a6af6c094 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -70,15 +70,16 @@ def two_factor(): return user_api_client.check_verify_code(user_id, code, "sms") form = TwoFactorForm(_check_code) + redirect_url = request.args.get('next') if form.validate_on_submit(): if is_less_than_90_days_ago(user.email_access_validated_at): return log_in_user(user_id) else: - user_api_client.send_verify_code(user.id, 'email', None, request.args.get('next')) - return redirect(url_for('.revalidate_email_sent')) + user_api_client.send_verify_code(user.id, 'email', None, redirect_url) + return redirect(url_for('.revalidate_email_sent', next=redirect_url)) - return render_template('views/two-factor.html', form=form) + return render_template('views/two-factor.html', form=form, redirect_url=redirect_url) @main.route('/re-validate-email', methods=['GET']) diff --git a/app/templates/views/two-factor.html b/app/templates/views/two-factor.html index 8a7cfa2f7..d0a252f14 100644 --- a/app/templates/views/two-factor.html +++ b/app/templates/views/two-factor.html @@ -22,7 +22,7 @@ }) }} {{ page_footer( "Continue", - secondary_link=url_for('main.check_and_resend_text_code'), + secondary_link=url_for('main.check_and_resend_text_code', next=redirect_url), secondary_link_text='Not received a text message?' ) }} {% endcall %} diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index 5fc08934f..15f6c5568 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -37,11 +37,16 @@ def test_two_factor_email_sent_page( assert resend_email_link['href'] == url_for('main.email_not_received', next=redirect_url) +@pytest.mark.parametrize('redirect_url', [ + None, + 'blob', +]) def test_should_render_two_factor_page( client, api_user_active, mock_get_user_by_email, - mocker + mocker, + redirect_url ): # TODO this lives here until we work out how to # reassign the session after it is lost mid register process @@ -50,7 +55,7 @@ def test_should_render_two_factor_page( 'id': api_user_active['id'], 'email': api_user_active['email_address']} mocker.patch('app.user_api_client.get_user', return_value=api_user_active) - response = client.get(url_for('main.two_factor')) + response = client.get(url_for('main.two_factor', next=redirect_url)) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.select_one('main p').text.strip() == ( @@ -62,6 +67,10 @@ def test_should_render_two_factor_page( assert page.select_one('input')['type'] == 'tel' assert page.select_one('input')['pattern'] == '[0-9]*' + assert page.select_one( + 'a:contains("Not received a text message?")' + )['href'] == url_for('main.check_and_resend_text_code', next=redirect_url) + @freeze_time('2020-01-27T12:00:00') def test_should_login_user_and_should_redirect_to_next_url( @@ -104,11 +113,15 @@ def test_should_send_email_and_redirect_to_info_page_if_user_needs_to_revalidate session['user_details'] = { 'id': api_user_active['id'], 'email': api_user_active['email_address']} - response = client.post(url_for('main.two_factor', next='/services/{}'.format(SERVICE_ONE_ID)), + response = client.post(url_for('main.two_factor', next=f'/services/{SERVICE_ONE_ID}'), data={'sms_code': '12345'}) assert response.status_code == 302 - assert response.location == url_for('main.revalidate_email_sent', _external=True) + assert response.location == url_for( + 'main.revalidate_email_sent', + _external=True, + next=f'/services/{SERVICE_ONE_ID}' + ) mock_send_verify_code.assert_called_with(api_user_active['id'], 'email', None, mocker.ANY) From 2203fae195361ef7608692f9ed2a47aa14207dc6 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 9 Oct 2020 11:42:46 +0100 Subject: [PATCH 11/12] Turn on redirects revalidate_email_sent This is part of the work to make sure user is redirected to the page they initially were meant to visit after they sign in. --- app/main/views/two_factor.py | 3 ++- app/templates/views/re-validate-email-sent.html | 2 +- tests/app/main/views/test_two_factor.py | 11 +++++------ 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index a6af6c094..8affdaa61 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -85,7 +85,8 @@ def two_factor(): @main.route('/re-validate-email', methods=['GET']) def revalidate_email_sent(): title = 'Email resent' if request.args.get('email_resent') else 'Check your email' - return render_template('views/re-validate-email-sent.html', title=title) + redirect_url = request.args.get('next') + return render_template('views/re-validate-email-sent.html', title=title, redirect_url=redirect_url) # see http://flask.pocoo.org/snippets/62/ diff --git a/app/templates/views/re-validate-email-sent.html b/app/templates/views/re-validate-email-sent.html index af74486d0..d14a4c621 100644 --- a/app/templates/views/re-validate-email-sent.html +++ b/app/templates/views/re-validate-email-sent.html @@ -14,7 +14,7 @@

We’ve sent you a link to sign in to Notify. The link will open in a new browser window, so you can close this one.

{{ page_footer( - secondary_link=url_for('main.email_not_received'), + secondary_link=url_for('main.email_not_received', next=redirect_url), secondary_link_text='Not received an email?' ) }} diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index 15f6c5568..a6f6b381a 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -11,10 +11,8 @@ from tests.conftest import ( ) -@pytest.mark.parametrize('redirect_url', [ - None, - 'blob', -]) +@pytest.mark.parametrize('request_url', ['two_factor_email_sent', 'revalidate_email_sent']) +@pytest.mark.parametrize('redirect_url', [None, 'blob']) @pytest.mark.parametrize('email_resent, page_title', [ (None, 'Check your email'), (True, 'Email resent') @@ -23,9 +21,10 @@ def test_two_factor_email_sent_page( client, email_resent, page_title, - redirect_url + redirect_url, + request_url ): - response = client.get(url_for('main.two_factor_email_sent', next=redirect_url, email_resent=email_resent)) + response = client.get(url_for(f'main.{request_url}', next=redirect_url, email_resent=email_resent)) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') From 65787191039e72a60a01a7a606525880d8e1c381 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 12 Oct 2020 12:01:39 +0100 Subject: [PATCH 12/12] Test next redirects with realistic URL This change has been made following PR review, to ensure that special signs are transformed correctly when passing through the next URL. --- tests/app/main/views/test_code_not_received.py | 12 +++++++----- tests/app/main/views/test_forgot_password.py | 5 +++-- tests/app/main/views/test_new_password.py | 4 ++-- tests/app/main/views/test_sign_in.py | 16 ++++++++++------ tests/app/main/views/test_two_factor.py | 8 ++++---- 5 files changed, 26 insertions(+), 19 deletions(-) diff --git a/tests/app/main/views/test_code_not_received.py b/tests/app/main/views/test_code_not_received.py index 6e8a2b245..11405790f 100644 --- a/tests/app/main/views/test_code_not_received.py +++ b/tests/app/main/views/test_code_not_received.py @@ -2,6 +2,8 @@ import pytest from bs4 import BeautifulSoup from flask import url_for +from tests.conftest import SERVICE_ONE_ID + def test_should_render_email_verification_resend_show_email_address_and_resend_verify_email( client, @@ -29,7 +31,7 @@ def test_should_render_email_verification_resend_show_email_address_and_resend_v @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) def test_should_render_correct_resend_template_for_active_user( client, @@ -82,7 +84,7 @@ def test_should_render_correct_resend_template_for_pending_user( @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) @pytest.mark.parametrize('phone_number_to_register_with', [ '+447700900460', @@ -121,7 +123,7 @@ def test_should_resend_verify_code_and_update_mobile_for_pending_user( @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) def test_check_and_redirect_to_two_factor_if_user_active( client, @@ -141,7 +143,7 @@ def test_check_and_redirect_to_two_factor_if_user_active( @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) def test_check_and_redirect_to_verify_if_user_pending( client, @@ -180,7 +182,7 @@ def test_redirect_to_sign_in_if_not_logged_in( @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) def test_should_render_correct_email_not_received_template_for_active_user( client, diff --git a/tests/app/main/views/test_forgot_password.py b/tests/app/main/views/test_forgot_password.py index dc7d4fd7e..f3a106da6 100644 --- a/tests/app/main/views/test_forgot_password.py +++ b/tests/app/main/views/test_forgot_password.py @@ -4,6 +4,7 @@ from notifications_python_client.errors import HTTPError import app from tests import user_json +from tests.conftest import SERVICE_ONE_ID def test_should_render_forgot_password(client): @@ -42,11 +43,11 @@ def test_forgot_password_sends_next_link_with_reset_password_email_request( sample_user = user_json(email_address='test@user.gov.uk') mocker.patch('app.user_api_client.send_reset_password_url', return_value=None) response = client.post( - url_for('.forgot_password') + "?next=blob", + url_for('.forgot_password') + f"?next=/services/{SERVICE_ONE_ID}/templates", data={'email_address': sample_user['email_address']}) assert response.status_code == 200 app.user_api_client.send_reset_password_url.assert_called_once_with( - sample_user['email_address'], next_string="blob" + sample_user['email_address'], next_string=f'/services/{SERVICE_ONE_ID}/templates' ) diff --git a/tests/app/main/views/test_new_password.py b/tests/app/main/views/test_new_password.py index e7641438d..867c25a6b 100644 --- a/tests/app/main/views/test_new_password.py +++ b/tests/app/main/views/test_new_password.py @@ -6,7 +6,7 @@ from flask import url_for from itsdangerous import SignatureExpired from notifications_utils.url_safe_token import generate_token -from tests.conftest import url_for_endpoint_with_token +from tests.conftest import SERVICE_ONE_ID, url_for_endpoint_with_token def test_should_render_new_password_template( @@ -39,7 +39,7 @@ def test_should_return_404_when_email_address_does_not_exist( @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) def test_should_redirect_to_two_factor_when_password_reset_is_successful( app_, diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index e22d929db..29a7d4d05 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -5,7 +5,7 @@ from bs4 import BeautifulSoup from flask import url_for from app.models.user import User -from tests.conftest import normalize_spaces +from tests.conftest import SERVICE_ONE_ID, normalize_spaces def test_render_sign_in_template_for_new_user( @@ -31,10 +31,14 @@ def test_render_sign_in_template_with_next_link_for_password_reset( client_request ): client_request.logout() - page = client_request.get('main.sign_in', _optional_args="?next=blob", _test_page_title=False) + page = client_request.get( + 'main.sign_in', + _optional_args=f"?next=/services/{SERVICE_ONE_ID}/templates", + _test_page_title=False + ) forgot_password_link = page.find('a', class_="govuk-link govuk-link--no-visited-state page-footer-secondary-link") assert forgot_password_link.text == 'Forgotten your password?' - assert forgot_password_link['href'] == url_for('main.forgot_password') + "?next=blob" + assert forgot_password_link['href'] == url_for('main.forgot_password', next=f'/services/{SERVICE_ONE_ID}/templates') def test_sign_in_explains_session_timeout(client): @@ -104,7 +108,7 @@ def test_logged_in_user_redirects_to_account( @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) @pytest.mark.parametrize('email_address, password', [ ('valid@example.gov.uk', 'val1dPassw0rd!'), @@ -133,7 +137,7 @@ def test_process_sms_auth_sign_in_return_2fa_template( @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) def test_process_email_auth_sign_in_return_2fa_template( client, @@ -197,7 +201,7 @@ def test_should_return_redirect_when_user_is_pending( @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) def test_should_attempt_redirect_when_user_is_pending( client, diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index a6f6b381a..64204a870 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -12,7 +12,7 @@ from tests.conftest import ( @pytest.mark.parametrize('request_url', ['two_factor_email_sent', 'revalidate_email_sent']) -@pytest.mark.parametrize('redirect_url', [None, 'blob']) +@pytest.mark.parametrize('redirect_url', [None, f'/services/{SERVICE_ONE_ID}/templates']) @pytest.mark.parametrize('email_resent, page_title', [ (None, 'Check your email'), (True, 'Email resent') @@ -38,7 +38,7 @@ def test_two_factor_email_sent_page( @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) def test_should_render_two_factor_page( client, @@ -353,7 +353,7 @@ def test_valid_two_factor_email_link_logs_in_user( @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) def test_two_factor_email_link_has_expired( app_, @@ -397,7 +397,7 @@ def test_two_factor_email_link_is_invalid( @pytest.mark.parametrize('redirect_url', [ None, - 'blob', + f'/services/{SERVICE_ONE_ID}/templates', ]) def test_two_factor_email_link_is_already_used( client,