From a860f713d236d2f55ffa5ed5b5a8175adb615413 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 7 Jan 2016 17:13:49 +0000 Subject: [PATCH] Implementation of the new_password endpoint. Found a way to create the token that does not need to persist it to the database. This requires proper error messages, written by people who speak menglis good. --- app/assets/stylesheets/main.scss | 1 + app/main/dao/password_reset_token_dao.py | 19 ------- app/main/dao/users_dao.py | 5 +- app/main/forms.py | 9 ---- app/main/views/__init__.py | 28 ++++++++--- app/main/views/forgot_password.py | 14 +++--- app/main/views/new_password.py | 20 ++++---- app/models.py | 7 --- app/templates/admin_template.html | 16 +++++- app/templates/components/form-field.html | 2 +- config.py | 1 + .../versions/80_password_reset_token.py | 35 ------------- .../app/main/dao/test_password_reset_token.py | 12 ----- tests/app/main/dao/test_users_dao.py | 2 +- tests/app/main/test_forgot_password_form.py | 13 ----- tests/app/main/views/test_forgot_password.py | 10 ---- tests/app/main/views/test_new_password.py | 49 ++++++++++--------- 17 files changed, 87 insertions(+), 156 deletions(-) delete mode 100644 app/main/dao/password_reset_token_dao.py delete mode 100644 migrations/versions/80_password_reset_token.py delete mode 100644 tests/app/main/dao/test_password_reset_token.py delete mode 100644 tests/app/main/test_forgot_password_form.py diff --git a/app/assets/stylesheets/main.scss b/app/assets/stylesheets/main.scss index 9beb5dd10..7a0a95feb 100644 --- a/app/assets/stylesheets/main.scss +++ b/app/assets/stylesheets/main.scss @@ -20,6 +20,7 @@ @import '../govuk_elements/public/sass/elements/details'; @import '../govuk_elements/public/sass/elements/elements-typography'; @import '../govuk_elements/public/sass/elements/forms'; +@import '../govuk_elements/public/sass/elements/forms/form-validation'; @import '../govuk_elements/public/sass/elements/forms/form-block-labels'; @import '../govuk_elements/public/sass/elements/icons'; @import '../govuk_elements/public/sass/elements/layout'; diff --git a/app/main/dao/password_reset_token_dao.py b/app/main/dao/password_reset_token_dao.py deleted file mode 100644 index c43cc92bd..000000000 --- a/app/main/dao/password_reset_token_dao.py +++ /dev/null @@ -1,19 +0,0 @@ -from datetime import datetime, timedelta -from app import db -from app.models import PasswordResetToken - - -def insert(token, user_id): - password_reset_token = PasswordResetToken(token=token, - user_id=user_id, - expiry_date=datetime.now() + timedelta(hours=1)) - insert_token(password_reset_token) - - -def insert_token(password_reset_token): - db.session.add(password_reset_token) - db.session.commit() - - -def get_token(token): - return PasswordResetToken.query.filter_by(token=token).first() diff --git a/app/main/dao/users_dao.py b/app/main/dao/users_dao.py index 8329d057d..64e306b79 100644 --- a/app/main/dao/users_dao.py +++ b/app/main/dao/users_dao.py @@ -59,12 +59,13 @@ def update_mobile_number(id, mobile_number): db.session.commit() -def update_password(id, password): - user = get_user_by_id(id) +def update_password(email, password): + user = get_user_by_email(email) user.password = hashpw(password) user.password_changed_at = datetime.now() db.session.add(user) db.session.commit() + return user def find_all_email_address(): diff --git a/app/main/forms.py b/app/main/forms.py index 3652f5708..46321303d 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -24,7 +24,6 @@ verify_code = '^\d{5}$' class RegisterUserForm(Form): - def __init__(self, existing_email_addresses, existing_mobile_numbers, *args, **kwargs): self.existing_emails = existing_email_addresses self.existing_mobiles = existing_mobile_numbers @@ -125,10 +124,6 @@ class AddServiceForm(Form): class ForgotPasswordForm(Form): - def __init__(self, email_addresses, *args, **kargs): - self.email_addresses = email_addresses - super(ForgotPasswordForm, self).__init__(*args, **kargs) - email_address = StringField('Email address', validators=[Length(min=5, max=255), DataRequired(message='Email cannot be empty'), @@ -136,10 +131,6 @@ class ForgotPasswordForm(Form): Regexp(regex=gov_uk_email, message='Please enter a gov.uk email address') ]) - def validate_email_address(self, a): - if self.email_address.data not in self.email_addresses: - raise ValidationError('Please enter the email address that you registered with') - class NewPasswordForm(Form): new_password = StringField('Create a password', diff --git a/app/main/views/__init__.py b/app/main/views/__init__.py index 88a894ee8..d85bbfbc9 100644 --- a/app/main/views/__init__.py +++ b/app/main/views/__init__.py @@ -1,12 +1,11 @@ import traceback -import uuid from random import randint -from flask import url_for +from flask import url_for, current_app from app import admin_api_client +from app.main.dao import verify_codes_dao from app.main.exceptions import AdminApiClientException -from app.main.dao import verify_codes_dao, password_reset_token_dao def create_verify_code(): @@ -38,11 +37,9 @@ def send_email_code(user_id, email): return email_code -def send_change_password_email(email, user_id): +def send_change_password_email(email): try: - reset_password_token = str(uuid.uuid4()).replace('-', '') - link_to_change_password = url_for('.new_password', token=reset_password_token, _external=True) - password_reset_token_dao.insert(reset_password_token, user_id) + link_to_change_password = url_for('.new_password', token=generate_token(email), _external=True) admin_api_client.send_email(email_address=email, from_str='notify@digital.cabinet-office.gov.uk', message=link_to_change_password, @@ -51,3 +48,20 @@ def send_change_password_email(email, user_id): except: traceback.print_exc() raise AdminApiClientException('Exception when sending email.') + + +def generate_token(email): + from itsdangerous import TimestampSigner + signer = TimestampSigner(current_app.config['SECRET_KEY']) + return signer.sign(email).decode('utf8') + + +def check_token(token): + from itsdangerous import TimestampSigner, SignatureExpired + signer = TimestampSigner(current_app.config['SECRET_KEY']) + try: + email = signer.unsign(token, max_age=current_app.config['TOKEN_MAX_AGE_SECONDS']) + return email + except SignatureExpired as e: + current_app.logger.info('token expired %s' % e) + return None diff --git a/app/main/views/forgot_password.py b/app/main/views/forgot_password.py index 2d21eff09..8413d7270 100644 --- a/app/main/views/forgot_password.py +++ b/app/main/views/forgot_password.py @@ -1,5 +1,4 @@ -from flask import render_template - +from flask import render_template, flash from app.main import main from app.main.dao import users_dao from app.main.forms import ForgotPasswordForm @@ -8,10 +7,13 @@ from app.main.views import send_change_password_email @main.route('/forgot-password', methods=['GET', 'POST']) def forgot_password(): - form = ForgotPasswordForm(users_dao.find_all_email_address()) + form = ForgotPasswordForm() if form.validate_on_submit(): - user = users_dao.get_user_by_email(form.email_address.data) - send_change_password_email(form.email_address.data, user.id) - return render_template('views/password-reset-sent.html') + if users_dao.get_user_by_email(form.email_address.data): + send_change_password_email(form.email_address.data) + return render_template('views/password-reset-sent.html') + else: + flash('The email address is not recognized. Try again.') + return render_template('views/forgot-password.html', form=form) else: 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 0c479d59f..4be020e74 100644 --- a/app/main/views/new_password.py +++ b/app/main/views/new_password.py @@ -1,26 +1,26 @@ from datetime import datetime -from flask import (Markup, render_template, url_for, redirect) +from flask import (render_template, url_for, redirect, flash, current_app) from app.main import main -from app.main.dao import (password_reset_token_dao, users_dao) +from app.main.dao import users_dao from app.main.forms import NewPasswordForm -from app.main.views import send_sms_code +from app.main.views import send_sms_code, check_token @main.route('/new-password/', methods=['GET', 'POST']) def new_password(token): form = NewPasswordForm() if form.validate_on_submit(): - password_reset_token = password_reset_token_dao.get_token(str(Markup.escape(token))) - if not valid_token(password_reset_token): - form.new_password.errors.append('token is invalid') # Is there a better way - return render_template('views/new-password.html', form=form) - else: - users_dao.update_password(password_reset_token.user_id, form.new_password.data) - user = users_dao.get_user_by_id(password_reset_token.user_id) + email_address = check_token(token) + if email_address: + user = users_dao.update_password(email_address.decode('utf-8'), form.new_password.data) send_sms_code(user.id, user.mobile_number) return redirect(url_for('main.two_factor')) + else: + flash('expired token request again') + current_app.logger.info('we got here') + return redirect(url_for('.forgot_password')) else: return render_template('views/new-password.html', toke=token, form=form) diff --git a/app/models.py b/app/models.py index cff7d18a6..3608f040f 100644 --- a/app/models.py +++ b/app/models.py @@ -111,13 +111,6 @@ class Service(db.Model): return filter_null_value_fields(serialized) -class PasswordResetToken(db.Model): - id = db.Column(db.Integer, primary_key=True) - token = db.Column(db.String, unique=True, index=True, nullable=False) - user_id = db.Column(db.Integer, db.ForeignKey('users.id'), unique=False, nullable=False) - expiry_date = db.Column(db.DateTime, nullable=False) - - def filter_null_value_fields(obj): return dict( filter(lambda x: x[1] is not None, obj.items()) diff --git a/app/templates/admin_template.html b/app/templates/admin_template.html index 4613ca442..a03e47612 100644 --- a/app/templates/admin_template.html +++ b/app/templates/admin_template.html @@ -43,6 +43,7 @@ {% endblock %} + {% set global_header_text = "GOV.UK Notify" %} @@ -53,9 +54,20 @@ {% endif %} {% block content %} -
+
+ {% with messages = get_flashed_messages() %} + {% if messages %} +
+
    + {% for message in messages %} +
  • {{ message }}
    • + {% endfor %} +
+
+ {% endif %} + {% endwith %} {% block fullwidth_content %}{% endblock %} -
+
{% endblock %} {% block body_end %} diff --git a/app/templates/components/form-field.html b/app/templates/components/form-field.html index 643099498..691b4c579 100644 --- a/app/templates/components/form-field.html +++ b/app/templates/components/form-field.html @@ -2,7 +2,7 @@
{{ field.label }}
{{ field(**kwargs)|safe }} {% if field.errors %} -
    +
      {% for error in field.errors %}
    • {{ error }}
      • {% endfor %} diff --git a/config.py b/config.py index d53cf17f9..40f4a58c2 100644 --- a/config.py +++ b/config.py @@ -29,6 +29,7 @@ class Config(object): SECRET_KEY = 'secret-key' HTTP_PROTOCOL = 'http' DANGEROUS_SALT = 'itsdangeroussalt' + TOKEN_MAX_AGE_SECONDS = 86400 class Development(Config): diff --git a/migrations/versions/80_password_reset_token.py b/migrations/versions/80_password_reset_token.py deleted file mode 100644 index 5d2da6ad0..000000000 --- a/migrations/versions/80_password_reset_token.py +++ /dev/null @@ -1,35 +0,0 @@ -"""empty message - -Revision ID: 80_password_reset_token -Revises: 70_unique_email -Create Date: 2016-01-05 17:47:46.395959 - -""" - -# revision identifiers, used by Alembic. -revision = '80_password_reset_token' -down_revision = '70_unique_email' - -from alembic import op -import sqlalchemy as sa - - -def upgrade(): - ### commands auto generated by Alembic - please adjust! ### - op.create_table('password_reset_token', - sa.Column('id', sa.Integer(), nullable=False), - sa.Column('token', sa.String(), nullable=False), - sa.Column('user_id', sa.Integer(), nullable=False), - sa.Column('expiry_date', sa.DateTime(), nullable=False), - sa.ForeignKeyConstraint(['user_id'], ['users.id'], ), - sa.PrimaryKeyConstraint('id') - ) - op.create_index(op.f('ix_password_reset_token_token'), 'password_reset_token', ['token'], unique=True) - ### end Alembic commands ### - - -def downgrade(): - ### commands auto generated by Alembic - please adjust! ### - op.drop_index(op.f('ix_password_reset_token_token'), table_name='password_reset_token') - op.drop_table('password_reset_token') - ### end Alembic commands ### diff --git a/tests/app/main/dao/test_password_reset_token.py b/tests/app/main/dao/test_password_reset_token.py deleted file mode 100644 index ebc52fd39..000000000 --- a/tests/app/main/dao/test_password_reset_token.py +++ /dev/null @@ -1,12 +0,0 @@ -import uuid - -from app.main.dao import password_reset_token_dao -from tests.app.main import create_test_user - - -def test_should_insert_and_return_token(notifications_admin, notifications_admin_db, notify_db_session): - user = create_test_user('active') - token_id = str(uuid.uuid4()) - password_reset_token_dao.insert(token=token_id, user_id=user.id) - saved_token = password_reset_token_dao.get_token(token_id) - assert saved_token.token == token_id diff --git a/tests/app/main/dao/test_users_dao.py b/tests/app/main/dao/test_users_dao.py index e608cb9d3..60d5dcbc8 100644 --- a/tests/app/main/dao/test_users_dao.py +++ b/tests/app/main/dao/test_users_dao.py @@ -176,7 +176,7 @@ def test_should_update_password(notifications_admin, notifications_admin_db, not saved = users_dao.get_user_by_id(user.id) assert check_hash('somepassword', saved.password) assert saved.password_changed_at is None - users_dao.update_password(saved.id, 'newpassword') + users_dao.update_password(saved.email_address, 'newpassword') updated = users_dao.get_user_by_id(user.id) assert check_hash('newpassword', updated.password) assert updated.password_changed_at < datetime.now() diff --git a/tests/app/main/test_forgot_password_form.py b/tests/app/main/test_forgot_password_form.py deleted file mode 100644 index e4ae6aa19..000000000 --- a/tests/app/main/test_forgot_password_form.py +++ /dev/null @@ -1,13 +0,0 @@ -from werkzeug.datastructures import MultiDict - -from app.main.forms import ForgotPasswordForm - - -def test_should_return_validation_error_if_email_address_does_not_exist(notifications_admin, - notifications_admin_db, - notify_db_session): - with notifications_admin.test_request_context(): - form = ForgotPasswordForm(['first@it.gov.uk', 'second@it.gov.uk'], - formdata=MultiDict([('email_address', 'not_found@it.gov.uk')])) - form.validate() - assert {'email_address': ['Please enter the email address that you registered with']} == form.errors diff --git a/tests/app/main/views/test_forgot_password.py b/tests/app/main/views/test_forgot_password.py index 7e3f46e20..aea89d869 100644 --- a/tests/app/main/views/test_forgot_password.py +++ b/tests/app/main/views/test_forgot_password.py @@ -8,16 +8,6 @@ def test_should_render_forgot_password(notifications_admin, notifications_admin_ in response.get_data(as_text=True) -def test_should_have_validate_error_when_email_does_not_exist(notifications_admin, - notifications_admin_db, - notify_db_session): - create_test_user('active') - response = notifications_admin.test_client().post('/forgot-password', - data={'email_address': 'email_does_not@exist.gov.uk'}) - assert response.status_code == 200 - assert 'Please enter the email address that you registered with' in response.get_data(as_text=True) - - def test_should_redirect_to_password_reset_sent(notifications_admin, notifications_admin_db, mocker, diff --git a/tests/app/main/views/test_new_password.py b/tests/app/main/views/test_new_password.py index eeb9510b8..aa01d411e 100644 --- a/tests/app/main/views/test_new_password.py +++ b/tests/app/main/views/test_new_password.py @@ -1,19 +1,13 @@ -from datetime import datetime, timedelta - -from app.main.dao import password_reset_token_dao, users_dao -from app.models import PasswordResetToken -from tests.app.main import create_test_user +from app.main.dao import users_dao from app.main.encryption import check_hash +from app.main.views import generate_token +from tests.app.main import create_test_user def test_should_render_new_password_template(notifications_admin, notifications_admin_db, notify_db_session): - with notifications_admin.test_request_context(): - with notifications_admin.test_client() as client: - user = create_test_user('active') - password_reset_token_dao.insert('some_token', user.id) - response = client.get('/new-password/some_token') - assert response.status_code == 200 - assert ' You can now create a new password for your account.' in response.get_data(as_text=True) + response = notifications_admin.test_client().get('/new-password/some_token') + assert response.status_code == 200 + assert ' You can now create a new password for your account.' in response.get_data(as_text=True) def test_should_redirect_to_two_factor_when_password_reset_is_successful(notifications_admin, notifications_admin_db, @@ -21,8 +15,8 @@ def test_should_redirect_to_two_factor_when_password_reset_is_successful(notific with notifications_admin.test_request_context(): with notifications_admin.test_client() as client: user = create_test_user('active') - password_reset_token_dao.insert('some_token', user.id) - response = client.post('/new-password/some_token', + token = generate_token(user.email_address) + response = client.post('/new-password/{}'.format(token), data={'new_password': 'a-new_password'}) assert response.status_code == 302 assert response.location == 'http://localhost/two-factor' @@ -30,15 +24,26 @@ def test_should_redirect_to_two_factor_when_password_reset_is_successful(notific assert check_hash('a-new_password', saved_user.password) -def test_should_return_validation_error_that_token_is_expired(notifications_admin, notifications_admin_db, - notify_db_session): +def test_should_redirect_to_forgot_password_with_flash_message_when_token_is_expired(notifications_admin, + notifications_admin_db, + notify_db_session): with notifications_admin.test_request_context(): with notifications_admin.test_client() as client: + notifications_admin.config['TOKEN_MAX_AGE_SECONDS'] = -1000 user = create_test_user('active') - expired_token = PasswordResetToken(id=1, token='some_token', user_id=user.id, - expiry_date=datetime.now() + timedelta(hours=-2)) - password_reset_token_dao.insert_token(expired_token) - response = client.post('/new-password/some_token', + token = generate_token(user.email_address) + response = client.post('/new-password/{}'.format(token), data={'new_password': 'a-new_password'}) - assert response.status_code == 200 - assert 'token is invalid' in response.get_data(as_text=True) + assert response.status_code == 302 + assert response.location == 'http://localhost/forgot-password' + notifications_admin.config['TOKEN_MAX_AGE_SECONDS'] = 86400 + + +def test_should_return_500_error_page_when_email_addres_does_not_exist(notifications_admin, notifications_admin_db, + notify_db_session): + with notifications_admin.test_request_context(): + with notifications_admin.test_client() as client: + token = generate_token('doesnotexist@it.gov.uk') + response = client.post('/new-password/{}'.format(token), + data={'new_password': 'a-new_password'}) + assert response.status_code == 500