From a0e7d569e99660e7b0798f5c7317bd890930e277 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 13 Oct 2016 17:05:37 +0100 Subject: [PATCH 1/2] Send an email to the user when they change email address This PR changes the flow to change an email address. Once the user enter their password, they are told "Check your email". An email has been sent to them containing a link to notify which contains an encrypted token. The encrypted token contains the user id and new email address. Once the link is clicked the user's email address is updated to the new email address. They are redirected to the /user-profile page. Also in this commit is an update from flask.ext.login to flask_login. --- app/main/views/index.py | 8 +--- app/main/views/register.py | 5 +-- app/main/views/sign_in.py | 2 +- app/main/views/sign_out.py | 2 +- app/main/views/user_profile.py | 39 +++++++++++++---- app/notify_client/__init__.py | 2 +- app/notify_client/models.py | 2 +- app/notify_client/user_api_client.py | 5 +++ .../views/change-email-continue.html | 13 ++++++ tests/app/main/views/test_user_profile.py | 42 ++++++++++++------- tests/conftest.py | 5 +++ 11 files changed, 87 insertions(+), 38 deletions(-) create mode 100644 app/templates/views/change-email-continue.html diff --git a/app/main/views/index.py b/app/main/views/index.py index 3192f20cd..2cc653e6c 100644 --- a/app/main/views/index.py +++ b/app/main/views/index.py @@ -1,12 +1,8 @@ -import markdown -import os -from flask import (render_template, url_for, redirect, Markup, request, abort) +from flask import (render_template, url_for, redirect, request, abort) from app.main import main from app import convert_to_boolean -from flask_login import login_required +from flask_login import (login_required, current_user) -from flask.ext.login import current_user -from mdx_gfm import GithubFlavoredMarkdownExtension from notifications_utils.renderers import HTMLEmail diff --git a/app/main/views/register.py b/app/main/views/register.py index 722cc6682..51edcbfe7 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -8,11 +8,10 @@ from flask import ( redirect, session, abort, - url_for, - flash + url_for ) -from flask.ext.login import current_user +from flask_login import current_user from app.main import main diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index 524a6339e..c93dfd847 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -9,7 +9,7 @@ from flask import ( Markup ) -from flask.ext.login import ( +from flask_login import ( current_user, login_fresh, confirm_login diff --git a/app/main/views/sign_out.py b/app/main/views/sign_out.py index 42ddefdd7..3fe6b389b 100644 --- a/app/main/views/sign_out.py +++ b/app/main/views/sign_out.py @@ -4,7 +4,7 @@ from flask import ( session ) -from flask.ext.login import logout_user +from flask_login import logout_user from app.main import main diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index 796e9cb65..bade84481 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -1,19 +1,21 @@ +import json + from flask import ( render_template, redirect, url_for, - session -) + session, + current_app) + +from flask_login import login_required, current_user +from notifications_utils.url_safe_token import check_token -from flask.ext.login import current_user -from flask_login import login_required from app.main import main from app.main.forms import ( ChangePasswordForm, ChangeNameForm, ChangeEmailForm, - ConfirmEmailForm, ChangeMobileNumberForm, ConfirmMobileNumberForm, ConfirmPasswordForm @@ -82,10 +84,9 @@ def user_profile_email_authenticate(): return redirect('main.user_profile_email') if form.validate_on_submit(): - current_user.email_address = session[NEW_EMAIL] - del session[NEW_EMAIL] - user_api_client.update_user(current_user) - return redirect(url_for('.user_profile')) + user_api_client.send_change_email_verification(current_user.id, session[NEW_EMAIL]) + return render_template('views/change-email-continue.html', + new_email=session[NEW_EMAIL]) return render_template( 'views/user-profile/authenticate.html', @@ -95,6 +96,26 @@ def user_profile_email_authenticate(): ) +@main.route("/user-profile/email/confirm/", methods=['GET']) +@login_required +def user_profile_email_confirm(token): + + 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) + user_id = token_data['user_id'] + new_email = token_data['email'] + user = user_api_client.get_user(user_id) + user.email_address = new_email + user_api_client.update_user(user) + if session.get(NEW_EMAIL, None): + del session[NEW_EMAIL] + + return redirect(url_for('.user_profile')) + + @main.route("/user-profile/mobile-number", methods=['GET', 'POST']) @login_required def user_profile_mobile_number(): diff --git a/app/notify_client/__init__.py b/app/notify_client/__init__.py index 69b374978..ca4812040 100644 --- a/app/notify_client/__init__.py +++ b/app/notify_client/__init__.py @@ -1,4 +1,4 @@ -from flask.ext.login import current_user +from flask_login import current_user def _attach_current_user(data): diff --git a/app/notify_client/models.py b/app/notify_client/models.py index e1a93b470..a7acb9558 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -1,4 +1,4 @@ -from flask.ext.login import (UserMixin, login_fresh) +from flask_login import (UserMixin, login_fresh) class User(UserMixin): diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 2d4a0155a..d3c077333 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -127,3 +127,8 @@ class UserApiClient(BaseAPIClient): return self.update_user(user) else: return user + + def send_change_email_verification(self, user_id, new_email): + endpoint = '/user/{}/change-email-verification'.format(user_id) + data = {'email': new_email} + self.post(endpoint, data) diff --git a/app/templates/views/change-email-continue.html b/app/templates/views/change-email-continue.html new file mode 100644 index 000000000..ac82905b0 --- /dev/null +++ b/app/templates/views/change-email-continue.html @@ -0,0 +1,13 @@ +{% extends "withoutnav_template.html" %} + +{% block page_title %} + Check your email – GOV.UK Notify +{% endblock %} + +{% block maincolumn_content %} + +

Check your email​

+

An email has been sent to {{ new_email }}.

+

Click the link in the email to confirm the change to your email address.

+ +{% endblock %} \ No newline at end of file diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index d077b124d..cf0047108 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -1,5 +1,6 @@ import json from flask import url_for +from notifications_utils.url_safe_token import generate_token def test_should_show_overview_page(app_, @@ -65,8 +66,6 @@ def test_should_show_email_page(app_, def test_should_redirect_after_email_change(app_, api_user_active, mock_login, - mock_get_user, - mock_get_user_by_email_not_found, mock_is_email_unique): with app_.test_request_context(): with app_.test_client() as client: @@ -83,9 +82,7 @@ def test_should_redirect_after_email_change(app_, def test_should_show_authenticate_after_email_change(app_, api_user_active, - mock_login, - mock_get_user, - mock_verify_password): + mock_login): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) @@ -93,31 +90,44 @@ def test_should_show_authenticate_after_email_change(app_, session['new-email'] = 'new_notify@notify.gov.uk' response = client.get(url_for('main.user_profile_email_authenticate')) + assert response.status_code == 200 assert 'Change your email address' in response.get_data(as_text=True) assert 'Confirm' in response.get_data(as_text=True) - assert response.status_code == 200 -def test_should_redirect_to_profile_after_email_change_confirm(app_, - api_user_active, - mock_login, - mock_get_user, - mock_verify_password, - mock_send_verify_code, - mock_is_email_unique): +def test_should_render_change_email_continue_after_authenticate_email(app_, + api_user_active, + mock_login, + mock_verify_password, + mock_send_change_email_verification): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active) - data = {'email-code': '12345'} + data = {'password': '12345'} with client.session_transaction() as session: session['new-email'] = 'new_notify@notify.gov.uk' response = client.post( url_for('main.user_profile_email_authenticate'), data=data) + assert response.status_code == 200 + assert 'Click the link in the email to confirm the change to your email address.' \ + in response.get_data(as_text=True) + + +def test_should_redirect_to_user_profile_when_user_confirms_email_link(app_, + api_user_active, + mock_login + ): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + + token = generate_token(payload=json.dumps({'user_id': api_user_active.id, 'email': 'new_email@gov.uk'}), + secret=app_.config['SECRET_KEY'], salt=app_.config['DANGEROUS_SALT']) + response = client.get(url_for('main.user_profile_email_confirm', token=token)) assert response.status_code == 302 - assert response.location == url_for( - 'main.user_profile', _external=True) + assert response.location == url_for('main.user_profile', _external=True) def test_should_show_mobile_number_page(app_, diff --git a/tests/conftest.py b/tests/conftest.py index d43e7d2b2..f777c2e37 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -580,6 +580,11 @@ def api_user_changed_password(fake_uuid): return user +@pytest.fixture(scope='function') +def mock_send_change_email_verification(mocker): + return mocker.patch('app.user_api_client.send_change_email_verification') + + @pytest.fixture(scope='function') def mock_register_user(mocker, api_user_pending): def _register(name, email_address, mobile_number, password): From 789ba58c2ea8a89e1be7c435a21c39d0993deb72 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 14 Oct 2016 14:46:31 +0100 Subject: [PATCH 2/2] Removed unused form. And used session.pop to remove NEW_EMAIL from the session. Also removed variable not being used in user_profile. --- app/main/forms.py | 14 -------------- app/main/views/user_profile.py | 4 +--- 2 files changed, 1 insertion(+), 17 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 02984da69..c85d58b6e 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -22,7 +22,6 @@ from wtforms.fields.html5 import EmailField, TelField from wtforms.validators import (DataRequired, Email, Length, Regexp, Optional) from app.main.validators import (Blacklist, CsvFileValidator, ValidEmailDomainRegex, NoCommasInPlaceHolders) -from app.notify_client.api_key_api_client import KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM def get_time_value_and_label(future_time): @@ -285,19 +284,6 @@ class ChangeEmailForm(Form): raise ValidationError("The email address is already in use") -class ConfirmEmailForm(Form): - def __init__(self, validate_code_func, *args, **kwargs): - self.validate_code_func = validate_code_func - super(ConfirmEmailForm, self).__init__(*args, **kwargs) - - email_code = email_code() - - def validate_email_code(self, field): - is_valid, msg = self.validate_code_func(field.data) - if not is_valid: - raise ValidationError(msg) - - class ChangeMobileNumberForm(Form): mobile_number = mobile_number() diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index bade84481..84c13bd81 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -25,7 +25,6 @@ from app import user_api_client NEW_EMAIL = 'new-email' NEW_MOBILE = 'new-mob' -NEW_EMAIL_PASSWORD_CONFIRMED = 'new-email-password-confirmed' NEW_MOBILE_PASSWORD_CONFIRMED = 'new-mob-password-confirmed' @@ -110,8 +109,7 @@ def user_profile_email_confirm(token): user = user_api_client.get_user(user_id) user.email_address = new_email user_api_client.update_user(user) - if session.get(NEW_EMAIL, None): - del session[NEW_EMAIL] + session.pop(NEW_EMAIL, None) return redirect(url_for('.user_profile'))