Improved validation behaviour on 2fa code verification screen

closes https://linear.app/ghost/issue/ENG-1672

- removed input on-blur validation because it can be triggered when clicking reset button giving a misleading error state
- added client-side validation for 6-digit code
- added validation when submitting the form
- added error reset when typing in the code field, including removal of button failure state, so it's clearer you're in a new submit state

ghost/admin/app/controllers/signin-verify.js

@@ -10,6 +10,8 @@ import {tracked} from '@glimmer/tracking';
 
 const {Errors} = DS;
 
+const TASK_SUCCESS = true;
+const TASK_FAILURE = false;
 const DEFAULT_RESEND_TOKEN_COUNTDOWN = 15;
 
 // eslint-disable-next-line ghost/ember/alias-model-in-controller
@@ -20,13 +22,36 @@ class VerifyData {
     errors = Errors.create();
 
     validate() {
-        this.errors.clear();
-        this.hasValidated = new TrackedArray();
+        this.resetValidations();
 
         if (!this.token?.trim()) {
-            this.errors.add('token', 'Token is required');
+            this.errors.add('token', 'Verification code is required');
             this.hasValidated.push('token');
+            return false;
         }
+
+        if (!this.token?.trim().match(/^\d{6}$/)) {
+            this.errors.add('token', 'Verification code must be 6 numbers');
+            this.hasValidated.push('token');
+            return false;
+        }
+
+        return true;
+    }
+
+    setTokenError(message) {
+        this.resetValidations();
+        this.errors.add('token', message);
+        this.hasValidated.push('token');
+    }
+
+    resetValidations() {
+        this.errors.clear();
+        this.hasValidated = new TrackedArray();
+    }
+
+    get validationMessage() {
+        return this.errors.toArray()[0]?.message;
     }
 }
 
@@ -62,30 +87,32 @@ export default class SigninVerifyController extends Controller {
         this.verifyData = new VerifyData();
     }
 
-    @action
-    validateToken() {
-        this.verifyData.validate();
-    }
-
     @action
     handleTokenInput(event) {
+        this.resetErrorState();
         this.verifyData.token = event.target.value;
     }
 
     @task
     *verifyTokenTask() {
+        this.flowErrors = '';
+
+        if (!this.verifyData.validate()) {
+            return TASK_FAILURE;
+        }
+
         try {
             yield this.session.authenticate('authenticator:cookie', {token: this.verifyData.token});
-            return true;
+            return TASK_SUCCESS;
         } catch (error) {
             if (isUnauthorizedError(error)) {
-                this.flowErrors = 'Your verification code is incorrect.';
+                this.verifyData.setTokenError('Your verification code is incorrect.');
             } else if (error && error.payload && error.payload.errors) {
                 this.flowErrors = error.payload.errors[0].message;
             } else {
                 this.flowErrors = 'There was a problem verifying the code. Please try again.';
             }
-            return false;
+            return TASK_FAILURE;
         }
     }
 
@@ -94,19 +121,22 @@ export default class SigninVerifyController extends Controller {
         const resendTokenPath = `${this.ghostPaths.apiRoot}/session/verify`;
 
         try {
-            yield this.ajax.post(resendTokenPath, {
-                contentType: 'application/json;charset=utf-8',
-                // ember-ajax will try and parse the response as JSON if not explicitly set
-                dataType: 'text'
-            });
+            yield this.ajax.post(resendTokenPath);
             this.startResendTokenCountdown();
-            return true;
+            return TASK_SUCCESS;
         } catch (error) {
             if (error && error.payload && error.payload.errors) {
                 this.flowErrors = error.payload.errors[0].message;
             } else {
                 this.flowErrors = 'There was a problem resending the verification token.';
             }
+            return TASK_FAILURE;
         }
     }
+
+    resetErrorState() {
+        this.verifyTokenTask.last = null; // resets GhTaskButton state
+        this.flowErrors = '';
+        this.verifyData.resetValidations();
+    }
 }

ghost/admin/app/templates/signin-verify.hbs

@@ -27,7 +27,6 @@
                             value={{this.token}}
                             data-test-input="token"
                             {{on "input" this.handleTokenInput}}
-                            {{on "blur" this.validateToken}}
                         />
 
                         <GhTaskButton
@@ -37,6 +36,7 @@
                             @successClass=""
                             @failureClass=""
                             @disabled={{this.resendTokenCountdownStarted}}
+                            data-test-button="resend-token"
                             as |task|
                         >
                             {{#if this.resendTokenCountdownStarted}}
@@ -58,7 +58,9 @@
                     data-test-button="verify" />
             </form>
 
-            <p class="{{if this.flowErrors "main-error" "main-notification"}}" data-test-flow-notification>{{if this.flowErrors this.flowErrors this.flowNotification}}&nbsp;</p>
+            {{#let (or this.flowErrors this.verifyData.validationMessage) as |error|}}
+                <p class="{{if error "main-error" "main-notification"}}" data-test-flow-notification>{{error}}&nbsp;</p>
+            {{/let}}
         </section>
     </div>
 </div>

ghost/admin/mirage/config/authentication.js

@@ -4,11 +4,21 @@ import {dasherize} from '@ember/string';
 import {isBlank} from '@ember/utils';
 
 export default function mockAuthentication(server) {
+    // Password sign-in
     server.post('/session', function () {
-        // Password sign-in
         return new Response(201);
     });
 
+    // 2fa code verification
+    server.put('/session/verify', function () {
+        return new Response(201);
+    });
+
+    // 2fa code re-send
+    server.post('/session/verify', function () {
+        return new Response(200);
+    });
+
     server.post('/authentication/password_reset', function (schema, request) {
         let {password_reset} = JSON.parse(request.requestBody);
         let email = password_reset[0].email;

ghost/admin/tests/acceptance/authentication-test.js

@@ -9,6 +9,28 @@ import {run} from '@ember/runloop';
 import {setupApplicationTest} from 'ember-mocha';
 import {setupMirage} from 'ember-cli-mirage/test-support';
 
+function setupVerificationRequired(server, responseCode = 403) {
+    server.post('/session', function () {
+        return new Response(responseCode, {}, {
+            errors: [{
+                code: '2FA_TOKEN_REQUIRED'
+            }]
+        });
+    });
+}
+
+function setupVerificationSuccess(server) {
+    server.put('/session/verify', function () {
+        return new Response(201);
+    });
+}
+
+function setupVerificationFailed(server) {
+    server.put('/session/verify', function () {
+        return new Response(401, {}, null);
+    });
+}
+
 describe('Acceptance: Authentication', function () {
     let originalReplaceLocation;
 
@@ -36,28 +58,6 @@ describe('Acceptance: Authentication', function () {
     describe('general page', function () {
         let newLocation;
 
-        function setupVerificationRequired(server) {
-            server.post('/session', function () {
-                return new Response(403, {}, {
-                    errors: [{
-                        code: '2FA_TOKEN_REQUIRED'
-                    }]
-                });
-            });
-        }
-
-        function setupVerificationSuccess(server) {
-            server.put('/session/verify', function () {
-                return new Response(201);
-            });
-        }
-
-        function setupVerificationFailed(server) {
-            server.put('/session/verify', function () {
-                return new Response(401, {}, null);
-            });
-        }
-
         async function completeSignIn() {
             await invalidateSession();
             await visit('/signin');
@@ -75,6 +75,22 @@ describe('Acceptance: Authentication', function () {
             expect(find('[data-test-flow-notification]')).to.have.trimmed.text(expectedMessage);
         }
 
+        function testTokenInputHasErrorState(expectHasError = true) {
+            if (expectHasError) {
+                expect(find('[data-test-input="token"]').closest('.form-group')).to.have.class('error');
+            } else {
+                expect(find('[data-test-input="token"]').closest('.form-group')).not.to.have.class('error');
+            }
+        }
+
+        function testButtonHasErrorState(expectHasError = true) {
+            if (expectHasError) {
+                expect(find('[data-test-button="verify"]')).to.have.class('gh-btn-red');
+            } else {
+                expect(find('[data-test-button="verify"]')).not.to.have.class('gh-btn-red');
+            }
+        }
+
         beforeEach(function () {
             originalReplaceLocation = windowProxy.replaceLocation;
             windowProxy.replaceLocation = function (url) {
@@ -205,12 +221,7 @@ describe('Acceptance: Authentication', function () {
         });
 
         it('handles 2fa-required on a 2xx response from signin', async function () {
-            this.server.post('/session', function () {
-                return new Response(200, {}, {
-                    errors: [{code: '2FA_TOKEN_REQUIRED'}]
-                });
-            });
-
+            setupVerificationRequired(this.server, 200);
             await completeSignIn();
 
             expect(currentURL(), 'url after email+password submit').to.equal('/signin/verify');
@@ -228,6 +239,38 @@ describe('Acceptance: Authentication', function () {
             expect(currentURL(), 'url after email+password submit').to.equal('/signin');
             testMainErrorMessage('Insufficient permissions');
         });
+
+        it('has client-side validation of verification code', async function () {
+            setupVerificationRequired(this.server);
+            await completeSignIn();
+
+            // no error state when starting flow
+            testTokenInputHasErrorState(false);
+            testButtonHasErrorState(false);
+            testMainErrorMessage('');
+
+            // client-side validation of token presence
+            await click('[data-test-button="verify"]');
+            testTokenInputHasErrorState();
+            testMainErrorMessage('Verification code is required');
+
+            // resets validation state when typing
+            await fillIn('[data-test-input="token"]', '1234');
+            testTokenInputHasErrorState(false);
+            testButtonHasErrorState(false);
+            testMainErrorMessage('');
+
+            // client-side validation of token format
+            await click('[data-test-button="verify"]');
+            testTokenInputHasErrorState();
+            testButtonHasErrorState();
+            testMainErrorMessage('Verification code must be 6 numbers');
+
+            // can correctly submit after failed attempts
+            await fillIn('[data-test-input="token"]', '123456');
+            await click('[data-test-button="verify"]');
+            expect(currentURL()).to.equal('/dashboard');
+        });
     });
 
     describe('editor', function () {