From 4d678aec9379c4cff801fc3d175775f568ef8c21 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 May 2018 21:24:23 +0100 Subject: [PATCH 1/5] Give better error messages for incorrect code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we know the code won’t pass the validation on the API side, we might as well tell the user before even passing it to the API. So this commit: - adds some more validators to the field - rewrites the validation function on the form to actually call the field-level validators before hitting the API 🤦‍♂️ - refactors the tests to be parametrize, which means they can be shorter, easier to read, and more comprehensive --- app/main/forms.py | 25 +++-- tests/app/main/test_two_factor_form.py | 122 ++++++++++++------------- 2 files changed, 74 insertions(+), 73 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 342a2a6b1..b0d7f5afe 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -161,11 +161,12 @@ def password(label='Password'): def sms_code(): - verify_code = '^\d{5}$' - return StringField('Text message code', - validators=[DataRequired(message='Can’t be empty'), - Regexp(regex=verify_code, - message='Code not found')]) + return StringField('Text message code', validators=[ + DataRequired(message='Can’t be empty'), + Regexp(regex='^\d+$', message='Numbers only'), + Length(min=5, message='Not enough numbers'), + Length(max=5, message='Too many numbers'), + ]) def organisation_type(): @@ -317,10 +318,18 @@ class TwoFactorForm(StripWhitespaceForm): sms_code = sms_code() - def validate_sms_code(self, field): - is_valid, reason = self.validate_code_func(field.data) + def validate(self): + + if not self.sms_code.validate(self): + return False + + is_valid, reason = self.validate_code_func(self.sms_code.data) + if not is_valid: - raise ValidationError(reason) + self.sms_code.errors.append(reason) + return False + + return True class EmailNotReceivedForm(StripWhitespaceForm): diff --git a/tests/app/main/test_two_factor_form.py b/tests/app/main/test_two_factor_form.py index 0c87befae..5d8ae0907 100644 --- a/tests/app/main/test_two_factor_form.py +++ b/tests/app/main/test_two_factor_form.py @@ -1,82 +1,74 @@ +import pytest + from app import user_api_client from app.main.forms import TwoFactorForm +from tests.conftest import ( + mock_check_verify_code, + mock_check_verify_code_code_expired, + mock_check_verify_code_code_not_found, +) +def _check_code(code): + return user_api_client.check_verify_code('1', code, "sms") + + +@pytest.mark.parametrize('post_data', [ + {'sms_code': '12345'}, + {'sms_code': ' 12345 '}, +]) def test_form_is_valid_returns_no_errors( app_, mock_check_verify_code, + post_data, ): - with app_.test_request_context( - method='POST', - data={'sms_code': '12345'} - ): - def _check_code(code): - return user_api_client.check_verify_code('1', code, "sms") + with app_.test_request_context(method='POST', data=post_data): form = TwoFactorForm(_check_code) assert form.validate() is True - assert len(form.errors) == 0 + assert form.errors == {} +@pytest.mark.parametrize('mock, post_data, expected_error', ( + ( + mock_check_verify_code, + {'sms_code': '1234'}, + 'Not enough numbers', + ), + ( + mock_check_verify_code, + {'sms_code': '123456'}, + 'Too many numbers', + ), + ( + mock_check_verify_code, + {}, + 'Can’t be empty', + ), + ( + mock_check_verify_code, + {'sms_code': '12E45'}, + 'Numbers only', + ), + ( + mock_check_verify_code_code_expired, + {'sms_code': '99999'}, + 'Code has expired', + ), + ( + mock_check_verify_code_code_not_found, + {'sms_code': '99999'}, + 'Code not found', + ), +)) def test_returns_errors_when_code_is_too_short( app_, - mock_check_verify_code, + mocker, + mock, + post_data, + expected_error, ): - with app_.test_request_context( - method='POST', - data={'sms_code': '145'} - ): - def _check_code(code): - return user_api_client.check_verify_code('1', code, "sms") + mock(mocker) + with app_.test_request_context(method='POST', data=post_data): form = TwoFactorForm(_check_code) assert form.validate() is False - assert len(form.errors) == 1 - assert set(form.errors) == set({'sms_code': ['Code not found', 'Code does not match']}) - - -def test_returns_errors_when_code_is_missing( - app_, - mock_check_verify_code, -): - with app_.test_request_context( - method='POST', - data={} - ): - def _check_code(code): - return user_api_client.check_verify_code('1', code, "sms") - form = TwoFactorForm(_check_code) - assert form.validate() is False - assert len(form.errors) == 1 - assert set(form.errors) == set({'sms_code': ['Code must not be empty']}) - - -def test_returns_errors_when_code_contains_letters( - app_, - mock_check_verify_code, -): - with app_.test_request_context( - method='POST', - data={'sms_code': 'asdfg'} - ): - def _check_code(code): - return user_api_client.check_verify_code('1', code, "sms") - form = TwoFactorForm(_check_code) - assert form.validate() is False - assert len(form.errors) == 1 - assert set(form.errors) == set({'sms_code': ['Code not found', 'Code does not match']}) - - -def test_should_return_errors_when_code_is_expired( - app_, - mock_check_verify_code_code_expired, -): - with app_.test_request_context( - method='POST', - data={'sms_code': '23456'} - ): - def _check_code(code): - return user_api_client.check_verify_code('1', code, "sms") - form = TwoFactorForm(_check_code) - assert form.validate() is False - errors = form.errors - assert len(errors) == 1 - assert errors == {'sms_code': ['Code has expired']} + assert form.errors == {'sms_code': [expected_error]} From 063f9cc0815b1a5832f396b84885596d51b23b80 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 May 2018 22:26:24 +0100 Subject: [PATCH 2/5] Enable numeric keypad for text message code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you’re signing in on a phone, it’s easier to type the two factor code with a numeric keypad. The most reliable way to get the numeric keypad to show up on multiple devices is: - `type='tel'` (not `type='number'` because that’s only meant for numbers, not string of digits, ie `01234` is not a number) - `pattern='[0-9]*'`, without which it doesn’t work on iOS Based on the guidance here: - https://github.com/alphagov/govuk-design-system-backlog/issues/74 - https://docs.google.com/document/d/1wozIhOdt6wvlgqVReauUnlsJI-3fqUlNuQFwUI7tqAA/edit --- app/main/forms.py | 9 ++++++++- app/templates/views/two-factor.html | 2 +- tests/app/main/views/test_two_factor.py | 7 ++++++- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index b0d7f5afe..784b8f2e8 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -160,8 +160,15 @@ def password(label='Password'): Blacklist(message='Choose a password that’s harder to guess')]) +class SMSCode(StringField): + def __call__(self, **kwargs): + return super().__call__( + type='tel', pattern='[0-9]*', **kwargs + ) + + def sms_code(): - return StringField('Text message code', validators=[ + return SMSCode('Text message code', validators=[ DataRequired(message='Can’t be empty'), Regexp(regex='^\d+$', message='Numbers only'), Length(min=5, message='Not enough numbers'), diff --git a/app/templates/views/two-factor.html b/app/templates/views/two-factor.html index a58df2f32..d7177c4c6 100644 --- a/app/templates/views/two-factor.html +++ b/app/templates/views/two-factor.html @@ -15,7 +15,7 @@

We’ve sent you a text message with a security code.

-
+ {{ textbox( form.sms_code, width='5em', diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index f710e05fd..e6498beff 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -17,7 +17,12 @@ def test_should_render_two_factor_page( 'email': api_user_active.email_address} response = client.get(url_for('main.two_factor')) assert response.status_code == 200 - assert '''We’ve sent you a text message with a security code.''' in response.get_data(as_text=True) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.select_one('main p').text.strip() == ( + 'We’ve sent you a text message with a security code.' + ) + assert page.select_one('input')['type'] == 'tel' + assert page.select_one('input')['pattern'] == '[0-9]*' def test_should_login_user_and_should_redirect_to_next_url( From 60c56be048d96770164041a21921bc0b2f2ea908 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 May 2018 22:53:36 +0100 Subject: [PATCH 3/5] Remove `ConfirmMobileNumberForm` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s exactly the same code as `TwoFactorForm` was. --- app/main/forms.py | 13 ------------- app/main/views/user_profile.py | 4 ++-- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 784b8f2e8..05114c362 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -493,19 +493,6 @@ class ChangeMobileNumberForm(StripWhitespaceForm): mobile_number = international_phone_number() -class ConfirmMobileNumberForm(StripWhitespaceForm): - def __init__(self, validate_code_func, *args, **kwargs): - self.validate_code_func = validate_code_func - super(ConfirmMobileNumberForm, self).__init__(*args, **kwargs) - - sms_code = sms_code() - - def validate_sms_code(self, field): - is_valid, msg = self.validate_code_func(field.data) - if not is_valid: - raise ValidationError(msg) - - class ChooseTimeForm(StripWhitespaceForm): def __init__(self, *args, **kwargs): diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index 47abc7e0c..335d7526d 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -18,8 +18,8 @@ from app.main.forms import ( ChangeMobileNumberForm, ChangeNameForm, ChangePasswordForm, - ConfirmMobileNumberForm, ConfirmPasswordForm, + TwoFactorForm, ) from app.utils import is_gov_user @@ -169,7 +169,7 @@ def user_profile_mobile_number_confirm(): if NEW_MOBILE_PASSWORD_CONFIRMED not in session: return redirect(url_for('.user_profile_mobile_number')) - form = ConfirmMobileNumberForm(_check_code) + form = TwoFactorForm(_check_code) if form.validate_on_submit(): user = user_api_client.get_user(current_user.id) From 02907afce1467f1708efef8066c3e200c63710cd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 May 2018 22:57:18 +0100 Subject: [PATCH 4/5] Refactor `sms_code` functionality into the class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So it’s all in one place, not two. --- app/main/forms.py | 16 ++++++---------- tests/app/main/views/test_two_factor.py | 3 +++ 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 05114c362..883c84b8e 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -161,19 +161,15 @@ def password(label='Password'): class SMSCode(StringField): - def __call__(self, **kwargs): - return super().__call__( - type='tel', pattern='[0-9]*', **kwargs - ) - - -def sms_code(): - return SMSCode('Text message code', validators=[ + validators = [ DataRequired(message='Can’t be empty'), Regexp(regex='^\d+$', message='Numbers only'), Length(min=5, message='Not enough numbers'), Length(max=5, message='Too many numbers'), - ]) + ] + + def __call__(self, **kwargs): + return super().__call__(type='tel', pattern='[0-9]*', **kwargs) def organisation_type(): @@ -323,7 +319,7 @@ class TwoFactorForm(StripWhitespaceForm): self.validate_code_func = validate_code_func super(TwoFactorForm, self).__init__(*args, **kwargs) - sms_code = sms_code() + sms_code = SMSCode('Text message code') def validate(self): diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index e6498beff..b6788de47 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -21,6 +21,9 @@ def test_should_render_two_factor_page( assert page.select_one('main p').text.strip() == ( 'We’ve sent you a text message with a security code.' ) + assert page.select_one('label').text.strip( + 'Text message code' + ) assert page.select_one('input')['type'] == 'tel' assert page.select_one('input')['pattern'] == '[0-9]*' From 7aec58728ea89160d371cbdf83ed3645a9456e18 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 8 May 2018 14:46:07 +0100 Subject: [PATCH 5/5] Make confirm page consistent with 2fa page --- app/templates/views/user-profile/confirm.html | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/templates/views/user-profile/confirm.html b/app/templates/views/user-profile/confirm.html index 8acf8bcc9..0ea5c72ab 100644 --- a/app/templates/views/user-profile/confirm.html +++ b/app/templates/views/user-profile/confirm.html @@ -15,8 +15,12 @@

We’ve sent a security code to your new {{ thing }}.

- - {{ textbox(form_field) }} + + {{ textbox( + form_field, + width='5em', + autofocus=True + ) }} {{ page_footer( 'Confirm', destructive=destructive,