From 3e969b3640c73e88f3d6c71cdcf22195d64b4dd7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 7 Mar 2016 18:18:52 +0000 Subject: [PATCH] Re-implement forgot password --- app/main/dao/users_dao.py | 5 -- app/main/views/forgot_password.py | 19 +++--- app/main/views/new_password.py | 26 +++++--- app/notify_client/user_api_client.py | 7 ++- config.py | 4 +- tests/app/main/views/test_forgot_password.py | 32 ++-------- tests/app/main/views/test_new_password.py | 64 ++++++++++---------- tests/conftest.py | 44 ++++++++++---- 8 files changed, 105 insertions(+), 96 deletions(-) diff --git a/app/main/dao/users_dao.py b/app/main/dao/users_dao.py index 411a22490..932a98cd3 100644 --- a/app/main/dao/users_dao.py +++ b/app/main/dao/users_dao.py @@ -64,11 +64,6 @@ def is_email_unique(email_address): raise ex -def request_password_reset(user): - user.state = 'request_password_reset' - user_api_client.update_user(user) - - def send_verify_code(user_id, code_type, to): return user_api_client.send_verify_code(user_id, code_type, to) diff --git a/app/main/views/forgot_password.py b/app/main/views/forgot_password.py index 3eb30369b..16818bf06 100644 --- a/app/main/views/forgot_password.py +++ b/app/main/views/forgot_password.py @@ -1,25 +1,22 @@ from flask import ( render_template, - flash ) +from notifications_python_client.errors import HTTPError from app.main import main -from app.main.dao import users_dao from app.main.forms import ForgotPasswordForm -from app.notify_client.sender import send_change_password_email +from app import user_api_client @main.route('/forgot-password', methods=['GET', 'POST']) def forgot_password(): - form = ForgotPasswordForm() if form.validate_on_submit(): - if not users_dao.is_email_unique(form.email_address.data): - user = users_dao.get_user_by_email(form.email_address.data) - users_dao.request_password_reset(user) - send_change_password_email(form.email_address.data) - return render_template('views/password-reset-sent.html') - else: - return render_template('views/password-reset-sent.html') + try: + user_api_client.send_reset_password_url(form.email_address.data) + except HTTPError as e: + if e.status_code != 404: + raise e + return render_template('views/password-reset-sent.html') return render_template('views/forgot-password.html', form=form) diff --git a/app/main/views/new_password.py b/app/main/views/new_password.py index 4b28a8504..088915aaf 100644 --- a/app/main/views/new_password.py +++ b/app/main/views/new_password.py @@ -1,22 +1,33 @@ -from flask import (render_template, url_for, redirect, flash, session) +import json + +from flask import (render_template, url_for, redirect, flash, session, current_app, abort) +from itsdangerous import SignatureExpired from app.main import main from app.main.dao import users_dao from app.main.forms import NewPasswordForm -from app.notify_client.sender import check_token +from datetime import datetime @main.route('/new-password/', methods=['GET', 'POST']) def new_password(token): - email_address = check_token(token) - if not email_address: + from 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']) + 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')) + email_address = json.loads(token_data)['email'] user = users_dao.get_user_by_email(email_address=email_address) - if user and user.state != 'request_password_reset': - flash('The link in the email we sent you has already been used.') - return redirect(url_for('.index')) + # TODO: what should this be?? + if not user: + abort(404, 'user not found') + if user.password_changed_at and datetime.strptime(user.password_changed_at, '%Y-%m-%d %H:%M:%S.%f') > \ + datetime.strptime(json.loads(token_data)['created_at'], '%Y-%m-%d %H:%M:%S.%f'): + flash('The link in the email has already been used') + return redirect(url_for('main.index')) form = NewPasswordForm() @@ -26,7 +37,6 @@ def new_password(token): 'id': user.id, 'email': user.email_address, 'password': form.new_password.data} - users_dao.activate_user(user) return redirect(url_for('main.two_factor')) else: return render_template('views/new-password.html', token=token, form=form, user=user) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index ce9d83bce..542035639 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -102,4 +102,9 @@ class UserApiClient(BaseAPIClient): def set_user_permissions(self, user_id, service_id, permissions): data = [{'permission': x} for x in permissions] endpoint = '/user/{}/service/{}/permission'.format(user_id, service_id) - resp = self.post(endpoint, data=data) + self.post(endpoint, data=data) + + def send_reset_password_url(self, email_address): + endpoint = '/user/reset-password' + data = {'email': email_address} + self.post(endpoint, data=data) diff --git a/config.py b/config.py index af641a7b7..7f40c889f 100644 --- a/config.py +++ b/config.py @@ -39,9 +39,9 @@ class Config(object): ADMIN_CLIENT_SECRET = os.getenv('ADMIN_CLIENT_SECRET') WTF_CSRF_ENABLED = True - SECRET_KEY = 'secret-key' + SECRET_KEY = 'dev-notify-secret-key' HTTP_PROTOCOL = 'http' - DANGEROUS_SALT = 'itsdangeroussalt' + DANGEROUS_SALT = 'dev-notify-salt' TOKEN_MAX_AGE_SECONDS = 3600 MAX_CONTENT_LENGTH = 10 * 1024 * 1024 # 10mb diff --git a/tests/app/main/views/test_forgot_password.py b/tests/app/main/views/test_forgot_password.py index 7789b4197..8ca8e3a59 100644 --- a/tests/app/main/views/test_forgot_password.py +++ b/tests/app/main/views/test_forgot_password.py @@ -9,13 +9,10 @@ def test_should_render_forgot_password(app_): in response.get_data(as_text=True) -def test_should_redirect_to_password_reset_sent_and_state_updated( - app_, - api_user_active, - mock_get_user_by_email, - mock_update_user, - mock_send_email -): +def test_should_redirect_to_password_reset_sent_for_valid_email( + app_, + api_user_active, + mock_reset_user_password): with app_.test_request_context(): response = app_.test_client().post( url_for('.forgot_password'), @@ -24,23 +21,4 @@ def test_should_redirect_to_password_reset_sent_and_state_updated( assert ( 'You have been sent an email containing a link' ' to reset your password.') in response.get_data(as_text=True) - assert mock_send_email.call_count == 1 - - -def test_should_redirect_to_password_reset_sent_for_non_existant_email_address( - app_, - api_user_active, - mock_dont_get_user_by_email, - mock_update_user, - mock_send_email -): - with app_.test_request_context(): - response = app_.test_client().post( - url_for('.forgot_password'), - data={'email_address': 'nope@example.gov.uk'}) - assert response.status_code == 200 - assert ( - 'You have been sent an email containing a link' - ' to reset your password.') in response.get_data(as_text=True) - mock_dont_get_user_by_email.assert_called_once_with('nope@example.gov.uk') - assert not mock_send_email.called + mock_reset_user_password.assert_called_once_with(api_user_active.email_address) diff --git a/tests/app/main/views/test_new_password.py b/tests/app/main/views/test_new_password.py index a3a17061a..84106354e 100644 --- a/tests/app/main/views/test_new_password.py +++ b/tests/app/main/views/test_new_password.py @@ -1,9 +1,8 @@ +import json +from datetime import datetime + from flask import url_for - -from app.main.dao import users_dao -from app.main.encryption import check_hash -from app.notify_client.sender import generate_token - +from utils.url_safe_token import generate_token import pytest @@ -14,59 +13,62 @@ def test_should_render_new_password_template(app_, mock_get_user_by_email_request_password_reset): with app_.test_request_context(): with app_.test_client() as client: - token = generate_token(api_user_active.email_address) + data = json.dumps({'email': api_user_active.email_address, 'created_at': str(datetime.now())}) + token = generate_token(data, app_.config['SECRET_KEY'], + app_.config['DANGEROUS_SALT']) response = client.get(url_for('.new_password', token=token)) assert response.status_code == 200 assert 'You can now create a new password for your account.' in response.get_data(as_text=True) -@pytest.mark.skipif(True, reason='Password reset no implemented') -def test_should_render_new_password_template_with_message_of_bad_token(app_, - mock_get_user_by_email): +def test_should_return_404_when_email_address_does_not_exist(app_, mock_get_user_by_email_not_found): with app_.test_request_context(): with app_.test_client() as client: - token = generate_token('no_user@d.gov.uk') + data = json.dumps({'email': 'no_user@d.gov.uk', 'created_at': str(datetime.now())}) + token = generate_token(data, app_.config['SECRET_KEY'], app_.config['DANGEROUS_SALT']) response = client.get(url_for('.new_password', token=token)) - assert response.status_code == 200 - assert 'Message about email address does not exist. Some one needs to figure out the words here.' in \ - response.get_data(as_text=True) + assert response.status_code == 404 -@pytest.mark.skipif(True, reason='Password reset no implemented') def test_should_redirect_to_two_factor_when_password_reset_is_successful(app_, mock_get_user_by_email_request_password_reset, - mock_login): + mock_login, + mock_send_verify_code): with app_.test_request_context(): with app_.test_client() as client: user = mock_get_user_by_email_request_password_reset.return_value - token = generate_token(user.email_address) + data = json.dumps({'email': user.email_address, 'created_at': str(datetime.now())}) + token = generate_token(data, app_.config['SECRET_KEY'], app_.config['DANGEROUS_SALT']) response = client.post(url_for('.new_password', token=token), data={'new_password': 'a-new_password'}) assert response.status_code == 302 assert response.location == url_for('.two_factor', _external=True) + mock_get_user_by_email_request_password_reset.assert_called_once_with(user.email_address) + + +def test_should_redirect_index_if_user_has_already_changed_password(app_, + mock_get_user_by_email_user_changed_password, + mock_login, + mock_send_verify_code): + with app_.test_request_context(): + with app_.test_client() as client: + user = mock_get_user_by_email_user_changed_password.return_value + data = json.dumps({'email': user.email_address, 'created_at': str(datetime.now())}) + token = generate_token(data, app_.config['SECRET_KEY'], app_.config['DANGEROUS_SALT']) + response = client.post(url_for('.new_password', token=token), data={'new_password': 'a-new_password'}) + assert response.status_code == 302 + assert response.location == url_for('.index', _external=True) + mock_get_user_by_email_user_changed_password.assert_called_once_with(user.email_address) def test_should_redirect_to_forgot_password_with_flash_message_when_token_is_expired( - app_, mock_get_user_by_email_request_password_reset, mock_login + app_, mock_get_user_by_email_request_password_reset, mock_login ): with app_.test_request_context(): with app_.test_client() as client: app_.config['TOKEN_MAX_AGE_SECONDS'] = -1000 user = mock_get_user_by_email_request_password_reset.return_value - token = generate_token(user.email_address) + 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'}) assert response.status_code == 302 assert response.location == url_for('.forgot_password', _external=True) app_.config['TOKEN_MAX_AGE_SECONDS'] = 3600 - - -@pytest.mark.skipif(True, reason='Password reset no implemented') -def test_should_redirect_to_forgot_pass_when_user_active_should_be_request_passw_reset( - app_, mock_get_user_by_email_request_password_reset, mock_login -): - with app_.test_request_context(): - with app_.test_client() as client: - user = mock_get_user_by_email_request_password_reset.return_value - token = generate_token(user.email_address) - response = client.post(url_for('.new_password', token=token), data={'new_password': 'a-new_password'}) - assert response.status_code == 302 - assert response.location == url_for('.index', _external=True) diff --git a/tests/conftest.py b/tests/conftest.py index 0e55f6a78..206d8ae3d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,5 @@ import uuid -from datetime import date +from datetime import date, datetime, timedelta import pytest from app import create_app @@ -268,9 +268,27 @@ def api_user_request_password_reset(): 'password': 'somepassword', 'email_address': 'test@user.gov.uk', 'mobile_number': '+4412341234', - 'state': 'request_password_reset', + 'state': 'active', 'failed_login_count': 5, - 'permissions': {} + 'permissions': {}, + 'password_changed_at': None + } + user = User(user_data) + return user + + +@pytest.fixture(scope='function') +def api_user_changed_password(): + from app.notify_client.user_api_client import User + user_data = {'id': 555, + 'name': 'Test User', + 'password': 'somepassword', + 'email_address': 'test@user.gov.uk', + 'mobile_number': '+4412341234', + 'state': 'active', + 'failed_login_count': 5, + 'permissions': {}, + 'password_changed_at': str(datetime.now() + timedelta(minutes=1)) } user = User(user_data) return user @@ -345,6 +363,13 @@ def mock_get_user_by_email_request_password_reset(mocker, api_user_request_passw return_value=api_user_request_password_reset) +@pytest.fixture(scope='function') +def mock_get_user_by_email_user_changed_password(mocker, api_user_changed_password): + return mocker.patch( + 'app.user_api_client.get_user_by_email', + return_value=api_user_changed_password) + + @pytest.fixture(scope='function') def mock_get_user_by_email_locked(mocker, api_user_locked): return mocker.patch( @@ -382,14 +407,6 @@ def mock_verify_password(mocker): side_effect=_verify_password) -@pytest.fixture(scope='function') -def mock_password_reset(mocker, api_user_active): - - def _reset(email): - api_user_active.state = 'request_password_reset' - return mocker.patch('app.main.dao.users_dao.request_password_reset', side_effect=_reset) - - @pytest.fixture(scope='function') def mock_update_user(mocker): @@ -645,3 +662,8 @@ def mock_add_user_to_service(mocker, service_one, api_user_active): @pytest.fixture(scope='function') def mock_set_user_permissions(mocker): return mocker.patch('app.user_api_client.set_user_permissions', return_value=None) + + +@pytest.fixture(scope='function') +def mock_reset_user_password(mocker): + return mocker.patch('app.user_api_client.send_reset_password_url', return_value=None)