From 62150e5596d169520f788e729687585212350136 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 28 Jan 2016 16:36:36 +0000 Subject: [PATCH] Added fixes for forms to hide potential email philshing scams. --- app/main/forms.py | 17 ---------- app/main/views/forgot_password.py | 15 ++++----- app/main/views/register.py | 48 +++++++++++++++------------- app/main/views/sign_in.py | 23 ++++++------- tests/app/main/test_validators.py | 2 +- tests/app/main/views/test_sign_in.py | 12 ++++--- 6 files changed, 53 insertions(+), 64 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index e7136b43c..c51eec285 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -105,9 +105,6 @@ class LoginForm(Form): class RegisterUserForm(Form): - def __init__(self, unique_email_func, *args, **kwargs): - self.unique_email_func = unique_email_func - super(RegisterUserForm, self).__init__(*args, **kwargs) name = StringField('Full name', validators=[DataRequired(message='Name can not be empty')]) @@ -115,11 +112,6 @@ class RegisterUserForm(Form): mobile_number = mobile_number() password = password() - def validate_email_address(self, field): - # Validate email address is unique. - if not self.unique_email_func(field.data): - raise ValidationError('Email address already exists') - class TwoFactorForm(Form): def __init__(self, validate_code_func, *args, **kwargs): @@ -221,17 +213,8 @@ class TemplateForm(Form): class ForgotPasswordForm(Form): - - def __init__(self, user_email_exists_func, *args, **kwargs): - self._user_email_exists_func = user_email_exists_func - super(ForgotPasswordForm, self).__init__(*args, **kwargs) - email_address = email_address() - def validate_email_address(self, field): - if not self._user_email_exists_func(field.data): - raise ValidationError('The email is not registered on our system') - class NewPasswordForm(Form): new_password = password() diff --git a/app/main/views/forgot_password.py b/app/main/views/forgot_password.py index 64629c28b..ba4c3d67d 100644 --- a/app/main/views/forgot_password.py +++ b/app/main/views/forgot_password.py @@ -8,14 +8,13 @@ from app.notify_client.sender import send_change_password_email @main.route('/forgot-password', methods=['GET', 'POST']) def forgot_password(): - def _email_exists(email): - return not users_dao.is_email_unique(email) - - form = ForgotPasswordForm(_email_exists) + form = ForgotPasswordForm() if form.validate_on_submit(): - 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') + 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') + flash('There was an error processing your request') return render_template('views/forgot-password.html', form=form) diff --git a/app/main/views/register.py b/app/main/views/register.py index 66d0cca75..ebaf9ec92 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -5,7 +5,8 @@ from flask import ( redirect, session, abort, - url_for + url_for, + flash ) from flask.ext.login import current_user @@ -24,29 +25,32 @@ def register(): if current_user and current_user.is_authenticated(): return redirect(url_for('main.choose_service')) - form = RegisterUserForm(users_dao.is_email_unique) + form = RegisterUserForm() if form.validate_on_submit(): - try: - user = user_api_client.register_user(form.name.data, - form.email_address.data, - form.mobile_number.data, - form.password.data) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + if users_dao.is_email_unique(form.email_address.data): + try: + user = user_api_client.register_user(form.name.data, + form.email_address.data, + form.mobile_number.data, + form.password.data) + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e - # TODO possibly there should be some exception handling - # for sending sms and email codes. - # How do we report to the user there is a problem with - # sending codes apart from service unavailable? - # at the moment i believe http 500 is fine. - users_dao.send_verify_code(user.id, 'sms') - users_dao.send_verify_code(user.id, 'email') - session['expiry_date'] = str(datetime.now() + timedelta(hours=1)) - session['user_details'] = {"email": user.email_address, "id": user.id} - return redirect(url_for('main.verify')) + # TODO possibly there should be some exception handling + # for sending sms and email codes. + # How do we report to the user there is a problem with + # sending codes apart from service unavailable? + # at the moment i believe http 500 is fine. + users_dao.send_verify_code(user.id, 'sms') + users_dao.send_verify_code(user.id, 'email') + session['expiry_date'] = str(datetime.now() + timedelta(hours=1)) + session['user_details'] = {"email": user.email_address, "id": user.id} + return redirect(url_for('main.verify')) + else: + flash('There was an error registering your account') return render_template('views/register.html', form=form) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index 3aed7a5ae..376451cd9 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -3,7 +3,8 @@ from flask import ( redirect, url_for, session, - abort + abort, + flash ) from flask.ext.login import current_user @@ -19,26 +20,26 @@ def sign_in(): return redirect(url_for('main.choose_service')) form = LoginForm() if form.validate_on_submit(): - user = _get_and_verify_user(form.email_address.data, form.password.data) + user = users_dao.get_user_by_email(form.email_address.data) + user = _get_and_verify_user(user, form.password.data) if user: - users_dao.send_verify_code(user.id, 'sms') session['user_details'] = {"email": user.email_address, "id": user.id} - return redirect(url_for('.two_factor')) - else: - # Vague error message for login in case of user not known, locked, inactive or password not verified - form.password.errors.append('Username or password is incorrect') + if user.state == 'pending': + return redirect(url_for('.verify')) + elif user.is_active(): + users_dao.send_verify_code(user.id, 'sms') + return redirect(url_for('.two_factor')) + # Vague error message for login in case of user not known, locked, inactive or password not verified + flash('Username or password is incorrect') return render_template('views/signin.html', form=form) -def _get_and_verify_user(email_address, password): - user = users_dao.get_user_by_email(email_address) +def _get_and_verify_user(user, password): if not user: return None elif user.is_locked(): return None - elif not user.is_active(): - return None elif not users_dao.verify_password(user.id, password): return None else: diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index 551ef4f00..219a04844 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -3,7 +3,7 @@ from app.main.forms import RegisterUserForm def test_should_raise_validation_error_for_password(app_, mock_get_user_by_email): - form = RegisterUserForm(users_dao.get_user_by_email) + form = RegisterUserForm() 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_sign_in.py b/tests/app/main/views/test_sign_in.py index 35420f7dc..1d72b6a00 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -42,8 +42,8 @@ def test_process_sign_in_return_2fa_template(app_, url_for('main.sign_in'), data={ 'email_address': 'valid@example.gov.uk', 'password': 'val1dPassw0rd!'}) - assert response.status_code == 302 - assert response.location == 'http://localhost/two-factor' + assert response.status_code == 302 + assert response.location == url_for('.two_factor', _external=True) mock_verify_password.assert_called_with(api_user_active.id, 'val1dPassw0rd!') @@ -80,11 +80,13 @@ def test_should_return_200_when_user_does_not_exist(app_, mock_get_user_by_email assert 'Username or password is incorrect' in response.get_data(as_text=True) -def test_should_return_200_when_user_is_pending(app_, mock_get_user_by_email_pending): +def test_should_return_redirect_when_user_is_pending(app_, + mock_get_user_by_email_pending, + mock_verify_password): with app_.test_request_context(): response = app_.test_client().post( url_for('main.sign_in'), data={ 'email_address': 'pending_user@example.gov.uk', 'password': 'val1dPassw0rd!'}) - assert response.status_code == 200 - assert 'Username or password is incorrect' in response.get_data(as_text=True) + assert response.status_code == 302 + assert response.location == url_for('main.verify', _external=True)