Merge pull request #2068 from alphagov/2fa-input

Improve 2fa code input

app/main/forms.py

@@ -160,12 +160,16 @@ def password(label='Password'):
                                      Blacklist(message='Choose a password that’s harder to guess')])
 
 
-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')])
+class SMSCode(StringField):
+    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():
@@ -315,12 +319,20 @@ 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):
+
+        if not self.sms_code.validate(self):
+            return False
+
+        is_valid, reason = self.validate_code_func(self.sms_code.data)
 
-    def validate_sms_code(self, field):
-        is_valid, reason = self.validate_code_func(field.data)
         if not is_valid:
-            raise ValidationError(reason)
+            self.sms_code.errors.append(reason)
+            return False
+
+        return True
 
 
 class EmailNotReceivedForm(StripWhitespaceForm):
@@ -477,19 +489,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):

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)

app/templates/views/two-factor.html

@@ -15,7 +15,7 @@
 
     <p>We’ve sent you a text message with a security code.</p>
 
-    <form autocomplete="off" method="post" class="extra-tracking">
+    <form autocomplete="off" method="post" class="extra-tracking" novalidate>
       {{ textbox(
         form.sms_code,
         width='5em',

app/templates/views/user-profile/confirm.html

@@ -15,8 +15,12 @@
       <p>
         We’ve sent a security code to your new {{ thing }}.
       </p>
-      <form method="post">
-        {{ textbox(form_field) }}
+      <form method="post" autocomplete="off" novalidate>
+        {{ textbox(
+          form_field,
+          width='5em',
+          autofocus=True
+        ) }}
         {{ page_footer(
           'Confirm',
           destructive=destructive,

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]}

tests/app/main/views/test_two_factor.py

@@ -17,7 +17,15 @@ 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('label').text.strip(
+        'Text message 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(