From c858869a523349c6eeecc8a157948b3badea2deb Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 8 Jan 2016 15:12:14 +0000 Subject: [PATCH] Removed exceptions, found a better way to handle them. Refactored the forms so that fields like email_address can be used in multiple forms. Refactored form validation so that a query function is passed into the form to be run, this way the form is not exposed to the dao layer and the query is more efficient. This PR still requires some frontend attention. Will work with Chris to update the templates. --- app/main/dao/users_dao.py | 16 +++----- app/main/exceptions.py | 8 ---- app/main/forms.py | 20 ++++----- app/main/views/__init__.py | 43 +++++++------------- app/main/views/forgot_password.py | 10 ++--- app/main/views/new_password.py | 30 ++++++-------- app/main/views/register.py | 17 ++------ app/templates/views/forgot-password.html | 2 +- app/templates/views/new-password.html | 4 ++ tests/app/main/dao/test_users_dao.py | 2 +- tests/app/main/test_validators.py | 5 +-- tests/app/main/views/test_forgot_password.py | 18 ++++++++ tests/app/main/views/test_new_password.py | 42 +++++++++---------- 13 files changed, 96 insertions(+), 121 deletions(-) delete mode 100644 app/main/exceptions.py diff --git a/app/main/dao/users_dao.py b/app/main/dao/users_dao.py index 94510704c..3e198395b 100644 --- a/app/main/dao/users_dao.py +++ b/app/main/dao/users_dao.py @@ -3,7 +3,6 @@ from datetime import datetime from sqlalchemy.orm import load_only from app import db, login_manager -from app.main.exceptions import NoDataFoundException from app.models import User from app.main.encryption import hashpw @@ -60,16 +59,11 @@ def update_mobile_number(id, mobile_number): db.session.commit() -def update_password(email, password): - user = get_user_by_email(email) - if user: - user.password = hashpw(password) - user.password_changed_at = datetime.now() - db.session.add(user) - db.session.commit() - return user - else: - raise NoDataFoundException('The user does not exist') +def update_password(user, password): + user.password = hashpw(password) + user.password_changed_at = datetime.now() + db.session.add(user) + db.session.commit() def find_all_email_address(): diff --git a/app/main/exceptions.py b/app/main/exceptions.py deleted file mode 100644 index 7b09af0eb..000000000 --- a/app/main/exceptions.py +++ /dev/null @@ -1,8 +0,0 @@ -class AdminApiClientException(Exception): - def __init__(self, message): - self.value = message - - -class NoDataFoundException(Exception): - def __init__(self, message): - self.value = message diff --git a/app/main/forms.py b/app/main/forms.py index 96980aeb5..038dbacea 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -57,9 +57,8 @@ class LoginForm(Form): class RegisterUserForm(Form): - def __init__(self, existing_email_addresses, existing_mobile_numbers, *args, **kwargs): + def __init__(self, existing_email_addresses, *args, **kwargs): self.existing_emails = existing_email_addresses - self.existing_mobiles = existing_mobile_numbers super(RegisterUserForm, self).__init__(*args, **kwargs) name = StringField('Full name', @@ -70,16 +69,9 @@ class RegisterUserForm(Form): def validate_email_address(self, field): # Validate email address is unique. - if field.data in self.existing_emails: + if self.existing_emails(field.data): raise ValidationError('Email address already exists') - def validate_mobile_number(self, field): - # Validate mobile number is unique - # Code to re-added later - # if field.data in self.existing_mobiles: - # raise ValidationError('Mobile number already exists') - pass - class TwoFactorForm(Form): def __init__(self, user_codes, *args, **kwargs): @@ -132,6 +124,14 @@ class AddServiceForm(Form): class ForgotPasswordForm(Form): email_address = email_address() + def __init__(self, q, *args, **kwargs): + self.query_function = q + super(ForgotPasswordForm, self).__init__(*args, *kwargs) + + def validate_email_address(self, a): + if not self.query_function(a.data): + raise ValidationError('The email address is not recognized. Enter the password you registered with.') + class NewPasswordForm(Form): new_password = password() diff --git a/app/main/views/__init__.py b/app/main/views/__init__.py index d85bbfbc9..a76b4feb4 100644 --- a/app/main/views/__init__.py +++ b/app/main/views/__init__.py @@ -1,11 +1,9 @@ -import traceback from random import randint 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 def create_verify_code(): @@ -14,40 +12,30 @@ def create_verify_code(): def send_sms_code(user_id, mobile_number): sms_code = create_verify_code() - try: - verify_codes_dao.add_code(user_id=user_id, code=sms_code, code_type='sms') - admin_api_client.send_sms(mobile_number=mobile_number, message=sms_code, token=admin_api_client.auth_token) - except: - raise AdminApiClientException('Exception when sending sms.') + verify_codes_dao.add_code(user_id=user_id, code=sms_code, code_type='sms') + admin_api_client.send_sms(mobile_number=mobile_number, message=sms_code, token=admin_api_client.auth_token) + return sms_code def send_email_code(user_id, email): email_code = create_verify_code() - try: - verify_codes_dao.add_code(user_id=user_id, code=email_code, code_type='email') - admin_api_client.send_email(email_address=email, - from_str='notify@digital.cabinet-office.gov.uk', - message=email_code, - subject='Verification code', - token=admin_api_client.auth_token) - except: - raise AdminApiClientException('Exception when sending email.') - + verify_codes_dao.add_code(user_id=user_id, code=email_code, code_type='email') + admin_api_client.send_email(email_address=email, + from_str='notify@digital.cabinet-office.gov.uk', + message=email_code, + subject='Verification code', + token=admin_api_client.auth_token) return email_code def send_change_password_email(email): - try: - 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, - subject='Reset password for GOV.UK Notify', - token=admin_api_client.auth_token) - except: - traceback.print_exc() - raise AdminApiClientException('Exception when sending email.') + 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, + subject='Reset password for GOV.UK Notify', + token=admin_api_client.auth_token) def generate_token(email): @@ -64,4 +52,3 @@ def check_token(token): 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 0488afac3..eae320df1 100644 --- a/app/main/views/forgot_password.py +++ b/app/main/views/forgot_password.py @@ -7,13 +7,9 @@ from app.main.views import send_change_password_email @main.route('/forgot-password', methods=['GET', 'POST']) def forgot_password(): - form = ForgotPasswordForm() + form = ForgotPasswordForm(users_dao.get_user_by_email) if form.validate_on_submit(): - 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. Enter the password you registered with.') - return render_template('views/forgot-password.html', form=form) + send_change_password_email(form.email_address.data) + return render_template('views/password-reset-sent.html') 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 4be020e74..9e0af8355 100644 --- a/app/main/views/new_password.py +++ b/app/main/views/new_password.py @@ -1,6 +1,4 @@ -from datetime import datetime - -from flask import (render_template, url_for, redirect, flash, current_app) +from flask import (render_template, url_for, redirect, flash) from app.main import main from app.main.dao import users_dao @@ -10,20 +8,18 @@ from app.main.views import send_sms_code, check_token @main.route('/new-password/', methods=['GET', 'POST']) def new_password(token): + email_address = check_token(token) + if not email_address: + flash('The token we sent you has expired. Enter your email address to try again.') + return redirect(url_for('.forgot_password')) + + user = users_dao.get_user_by_email(email_address=email_address.decode('utf-8')) + form = NewPasswordForm() + if form.validate_on_submit(): - 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')) + users_dao.update_password(user, form.new_password.data) + send_sms_code(user.id, user.mobile_number) + return redirect(url_for('main.two_factor')) else: - return render_template('views/new-password.html', toke=token, form=form) - - -def valid_token(token): - return token and datetime.now() <= token.expiry_date + return render_template('views/new-password.html', token=token, form=form, user=user) diff --git a/app/main/views/register.py b/app/main/views/register.py index 2e1c7f309..9eef37a4a 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -1,12 +1,9 @@ from datetime import datetime, timedelta -from flask import render_template, redirect, jsonify, session -from sqlalchemy.exc import SQLAlchemyError +from flask import render_template, redirect, session from app.main import main from app.main.dao import users_dao -from app.main.encryption import hashpw -from app.main.exceptions import AdminApiClientException from app.main.forms import RegisterUserForm from app.main.views import send_sms_code, send_email_code from app.models import User @@ -15,16 +12,8 @@ from app.models import User # TODO how do we handle duplicate unverifed email addresses? # malicious or otherwise. @main.route('/register', methods=['GET', 'POST']) -def process_register(): - try: - existing_emails, existing_mobiles = zip( - *[(x.email_address, x.mobile_number) for x in - users_dao.get_all_users()]) - except ValueError: - # Value error is raised if the db is empty. - existing_emails, existing_mobiles = [], [] - - form = RegisterUserForm(existing_emails, existing_mobiles) +def register(): + form = RegisterUserForm(users_dao.get_user_by_email) if form.validate_on_submit(): user = User(name=form.name.data, diff --git a/app/templates/views/forgot-password.html b/app/templates/views/forgot-password.html index d7b9fc1c0..4d36f2a86 100644 --- a/app/templates/views/forgot-password.html +++ b/app/templates/views/forgot-password.html @@ -16,7 +16,7 @@ GOV.UK Notify {{ form.hidden_tag() }} {{ render_field(form.email_address, class='form-control-2-3') }}

- +

diff --git a/app/templates/views/new-password.html b/app/templates/views/new-password.html index d3f3c827b..580325410 100644 --- a/app/templates/views/new-password.html +++ b/app/templates/views/new-password.html @@ -8,6 +8,7 @@ GOV.UK Notify
+ {% if user %}

Create a new password

You can now create a new password for your account.

@@ -24,6 +25,9 @@ GOV.UK Notify

+ {% else %} + Message about email address does not exist. Some one needs to figure out the words here. + {% endif %}
diff --git a/tests/app/main/dao/test_users_dao.py b/tests/app/main/dao/test_users_dao.py index 60d5dcbc8..387653b26 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.email_address, 'newpassword') + users_dao.update_password(saved, '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_validators.py b/tests/app/main/test_validators.py index d85159f51..512f0350a 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -1,10 +1,9 @@ -from pytest import fail - +from app.main.dao import users_dao from app.main.forms import RegisterUserForm def test_should_raise_validation_error_for_password(notifications_admin): - form = RegisterUserForm([], []) + form = RegisterUserForm(users_dao.get_user_by_email) form.name.data = 'test' form.email_address.data = 'teset@example.gov.uk' form.mobile_number.data = '+441231231231' diff --git a/tests/app/main/views/test_forgot_password.py b/tests/app/main/views/test_forgot_password.py index aea89d869..688960791 100644 --- a/tests/app/main/views/test_forgot_password.py +++ b/tests/app/main/views/test_forgot_password.py @@ -1,3 +1,6 @@ +from flask import url_for + +from app.main.views import generate_token from tests.app.main import create_test_user @@ -21,5 +24,20 @@ def test_should_redirect_to_password_reset_sent(notifications_admin, assert 'You have been sent an email containing a url to reset your password.' in response.get_data(as_text=True) +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') + 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 == url_for('.forgot_password', _external=True) + notifications_admin.config['TOKEN_MAX_AGE_SECONDS'] = 86400 + + def _set_up_mocker(mocker): mocker.patch("app.admin_api_client.send_email") diff --git a/tests/app/main/views/test_new_password.py b/tests/app/main/views/test_new_password.py index d6721b1c5..74a560c4a 100644 --- a/tests/app/main/views/test_new_password.py +++ b/tests/app/main/views/test_new_password.py @@ -1,16 +1,30 @@ -from pytest import fail +from flask import url_for from app.main.dao import users_dao from app.main.encryption import check_hash -from app.main.exceptions import NoDataFoundException 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): - 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) + with notifications_admin.test_request_context(): + with notifications_admin.test_client() as client: + user = create_test_user('active') + token = generate_token(user.email_address) + response = client.get('/new-password/{}'.format(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_render_new_password_template_with_message_of_bad_token(notifications_admin, notifications_admin_db, + notify_db_session): + with notifications_admin.test_request_context(): + with notifications_admin.test_client() as client: + token = generate_token('no_user@d.gov.uk') + response = client.get('/new-password/{}'.format(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) def test_should_redirect_to_two_factor_when_password_reset_is_successful(notifications_admin, @@ -25,7 +39,7 @@ def test_should_redirect_to_two_factor_when_password_reset_is_successful(notific 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' + assert response.location == url_for('.two_factor', _external=True) saved_user = users_dao.get_user_by_id(user.id) assert check_hash('a-new_password', saved_user.password) @@ -41,23 +55,9 @@ def test_should_redirect_to_forgot_password_with_flash_message_when_token_is_exp response = client.post('/new-password/{}'.format(token), data={'new_password': 'a-new_password'}) assert response.status_code == 302 - assert response.location == 'http://localhost/forgot-password' + assert response.location == url_for('.forgot_password', _external=True) notifications_admin.config['TOKEN_MAX_AGE_SECONDS'] = 86400 -def test_should_return_raise_no_data_found_exception_when_email_address_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') - try: - client.post('/new-password/{}'.format(token), - data={'new_password': 'a-new_password'}) - fail('Expected NoDataFoundException') - except NoDataFoundException: - pass - - def _set_up_mocker(mocker): mocker.patch("app.admin_api_client.send_sms")