fix(signin-unblock): Fix Fx >= 57 screen transition after signin unblock. (#5518) r=vladikoff

Esp in Fx >= 57.

Turns out there were a lot of corner cases that weren't correctly addressed.

fixes #5488

app/scripts/models/auth_brokers/base.js

@@ -69,9 +69,9 @@ define(function (require, exports, module) {
       afterCompleteSignIn: new NavigateBehavior('signin_verified'),
       afterCompleteSignUp: new NavigateBehavior('signup_verified'),
       afterDeleteAccount: new NullBehavior(),
-      afterForceAuth: new NullBehavior(),
+      afterForceAuth: new NavigateBehavior('signin_confirmed'),
       afterResetPasswordConfirmationPoll: new NullBehavior(),
-      afterSignIn: new NullBehavior(),
+      afterSignIn: new NavigateBehavior('signin_confirmed'),
       afterSignInConfirmationPoll: new NavigateBehavior('signin_confirmed'),
       afterSignUp: new NavigateBehavior('confirm'),
       afterSignUpConfirmationPoll: new NavigateBehavior('signup_confirmed'),

app/scripts/models/auth_brokers/fx-fennec-v1.js

@@ -12,15 +12,10 @@ define(function (require, exports, module) {
 
   const _ = require('underscore');
   const FxSyncWebChannelAuthenticationBroker = require('../auth_brokers/fx-sync-web-channel');
-  const NavigateBehavior = require('../../views/behaviors/navigate');
 
   var proto = FxSyncWebChannelAuthenticationBroker.prototype;
 
   var FxFennecV1AuthenticationBroker = FxSyncWebChannelAuthenticationBroker.extend({
-    defaultBehaviors: _.extend({}, proto.defaultBehaviors, {
-      afterSignIn: new NavigateBehavior('signin_confirmed')
-    }),
-
     defaultCapabilities: _.extend({}, proto.defaultCapabilities, {
       chooseWhatToSyncCheckbox: false,
       chooseWhatToSyncWebV1: true,

app/scripts/models/auth_brokers/fx-sync.js

@@ -22,7 +22,11 @@ define(function (require, exports, module) {
   module.exports = BaseAuthenticationBroker.extend({
     defaultBehaviors: _.extend({}, proto.defaultBehaviors, {
       afterCompleteSignIn: new ConnectAnotherDeviceOnSigninBehavior(proto.defaultBehaviors.afterCompleteSignIn),
-      afterCompleteSignUp: new ConnectAnotherDeviceBehavior(proto.defaultBehaviors.afterCompleteSignUp)
+      afterCompleteSignUp: new ConnectAnotherDeviceBehavior(proto.defaultBehaviors.afterCompleteSignUp),
+      // afterForceAuth is not overridden with the ConnectAnotherDeviceOnSignin behavior
+      // because force_auth is used to sign in as a particular user to view a particular
+      // page, e.g., settings.
+      afterSignIn: new ConnectAnotherDeviceOnSigninBehavior(proto.defaultBehaviors.afterSignIn),
     }),
 
     type: 'fx-sync',

app/scripts/models/auth_brokers/web.js

@@ -28,7 +28,9 @@ define(function (require, exports, module) {
       afterCompleteResetPassword: redirectToSettingsBehavior,
       afterCompleteSignIn: new SettingsIfSignedInBehavior(proto.defaultBehaviors.afterCompleteSignIn),
       afterCompleteSignUp: new SettingsIfSignedInBehavior(proto.defaultBehaviors.afterCompleteSignUp),
+      afterForceAuth: new NavigateBehavior('settings'),
       afterResetPasswordConfirmationPoll: redirectToSettingsBehavior,
+      afterSignIn: new NavigateBehavior('settings'),
       afterSignInConfirmationPoll: redirectToSettingsBehavior,
       afterSignUpConfirmationPoll: redirectToSettingsBehavior
     }),

app/scripts/views/behaviors/connect-another-device-on-signin.js

@@ -16,6 +16,8 @@
 define((require, exports, module) => {
   'use strict';
 
+  const Cocktail = require('cocktail');
+  const ConnectAnotherDeviceMixin = require('../mixins/connect-another-device-mixin');
   const p = require('lib/promise');
 
   /**
@@ -28,6 +30,8 @@ define((require, exports, module) => {
   module.exports = function (defaultBehavior) {
     const behavior = function (view, account) {
       return p().then(() => {
+        behavior.ensureConnectAnotherDeviceMixin(view);
+
         if (view.isEligibleForConnectAnotherDeviceOnSignin(account)) {
           return view.navigateToConnectAnotherDeviceOnSigninScreen(account);
         }
@@ -44,6 +48,12 @@ define((require, exports, module) => {
       });
     };
 
+    behavior.ensureConnectAnotherDeviceMixin = function (view) {
+      if (! Cocktail.isMixedIn(view, ConnectAnotherDeviceMixin)) {
+        Cocktail.mixin(view, ConnectAnotherDeviceMixin);
+      }
+    };
+
     behavior.type = 'connect-another-device-on-signin';
 
     return behavior;

app/scripts/views/mixins/connect-another-device-mixin.js

@@ -49,8 +49,7 @@ define((require, exports, module) => {
     isEligibleForConnectAnotherDeviceOnSignin (account) {
       const isEligibleForCadOnSignin = !! this.getExperimentGroup('cadOnSignin', { account });
 
-      return this.isSignIn() &&
-             this.isEligibleForConnectAnotherDevice(account) &&
+      return this.isEligibleForConnectAnotherDevice(account) &&
              isEligibleForCadOnSignin;
     },
 

app/scripts/views/mixins/signin-mixin.js

@@ -9,6 +9,7 @@ define(function (require, exports, module) {
 
   const AuthErrors = require('../../lib/auth-errors');
   const p = require('../../lib/promise');
+  const NavigateBehavior = require('../behaviors/navigate');
   const ResumeTokenMixin = require('./resume-token-mixin');
   const VerificationMethods = require('../../lib/verification-methods');
   const VerificationReasons = require('../../lib/verification-reasons');
@@ -144,11 +145,19 @@ define(function (require, exports, module) {
       // Currently, can be oauth, signin, signup, signin-unblock
       this.logViewEvent('signin.success');
 
-      var brokerMethod = this.afterSignInBrokerMethod || 'afterSignIn';
-      var navigateData = this.afterSignInNavigateData || {};
+      const brokerMethod = this.afterSignInBrokerMethod || 'afterSignIn';
+      const navigateData = this.afterSignInNavigateData || {};
 
-      return this.invokeBrokerMethod(brokerMethod, account)
-        .then(this.navigate.bind(this, this.model.get('redirectTo') || 'settings', {}, navigateData));
+      if (this.model.get('redirectTo')) {
+        // If `redirectTo` is specified, override the default behavior and
+        // redirect to the requested page.
+        const behavior = new NavigateBehavior(this.model.get('redirectTo'));
+        this.model.unset('redirectTo');
+        this.broker.setBehavior(brokerMethod, behavior, navigateData);
+      }
+
+      // Brokers handle all next steps.
+      return this.invokeBrokerMethod(brokerMethod, account);
     }
   };
 });

app/tests/spec/models/auth_brokers/base.js

@@ -276,7 +276,7 @@ define(function (require, exports, module) {
     describe('afterSignIn', function () {
       it('returns a promise', function () {
         return broker.afterSignIn(account)
-          .then(testDoesNotHalt);
+          .then(testNavigates('signin_confirmed'));
       });
     });
 
@@ -290,7 +290,7 @@ define(function (require, exports, module) {
     describe('afterForceAuth', function () {
       it('returns a promise', function () {
         return broker.afterForceAuth(account)
-          .then(testDoesNotHalt);
+          .then(testNavigates('signin_confirmed'));
       });
     });
 

app/tests/spec/models/auth_brokers/fx-desktop-v1.js

@@ -44,15 +44,9 @@ define(function (require, exports, module) {
       sinon.stub(broker, '_hasRequiredLoginFields').callsFake(() => true);
     });
 
-    it('has the `signup` capability by default', function () {
+    it('has the expected capabilities', () => {
       assert.isTrue(broker.hasCapability('signup'));
-    });
-
-    it('has the `handleSignedInNotification` capability by default', function () {
       assert.isTrue(broker.hasCapability('handleSignedInNotification'));
-    });
-
-    it('has the `emailVerificationMarketingSnippet` capability by default', function () {
       assert.isTrue(broker.hasCapability('emailVerificationMarketingSnippet'));
     });
 

app/tests/spec/models/auth_brokers/fx-desktop-v2.js

@@ -108,6 +108,7 @@ define(function (require, exports, module) {
         // the data to the browser and everybody is happy.
         account = user.initAccount({
           keyFetchToken: 'key-fetch-token',
+          uid: 'uid',
           unwrapBKey: 'unwrap-b-key'
         });
 

app/tests/spec/models/auth_brokers/fx-fennec-v1.js

@@ -47,23 +47,11 @@ define(function (require, exports, module) {
       sinon.spy(broker, 'send');
     });
 
-    it('has the `signup` capability by default', function () {
+    it('has the expected capabilities', () => {
       assert.isTrue(broker.hasCapability('signup'));
-    });
-
-    it('has the `handleSignedInNotification` capability by default', function () {
       assert.isTrue(broker.hasCapability('handleSignedInNotification'));
-    });
-
-    it('has the `chooseWhatToSyncWebV1` capability by default', function () {
       assert.isTrue(broker.hasCapability('chooseWhatToSyncWebV1'));
-    });
-
-    it('does not have the `emailVerificationMarketingSnippet` capability by default', function () {
       assert.isFalse(broker.hasCapability('emailVerificationMarketingSnippet'));
-    });
-
-    it('has all sync content types', function () {
       assert.equal(broker.defaultCapabilities.chooseWhatToSyncWebV1.engines, Constants.DEFAULT_DECLINED_ENGINES);
     });
 
@@ -88,7 +76,7 @@ define(function (require, exports, module) {
         return broker.afterSignIn(account)
           .then(function (behavior) {
             assert.isTrue(broker.send.calledWith('fxaccounts:login'));
-            assert.equal(behavior.endpoint, 'signin_confirmed');
+            assert.equal(behavior.type, 'connect-another-device-on-signin');
           });
       });
     });

app/tests/spec/models/auth_brokers/web.js

@@ -16,30 +16,28 @@ define(function (require, exports, module) {
     });
 
     function testRedirectsToSettings(brokerMethod) {
-      describe(brokerMethod, () => {
-        it('returns a NavigateBehavior to settings', () => {
-          return broker[brokerMethod]({ get: () => {} })
-            .then((behavior) => {
-              assert.equal(behavior.type, 'navigate');
-              assert.equal(behavior.endpoint, 'settings');
-            });
-        });
+      it(`${brokerMethod} returns a NavigateBehavior to settings`, () => {
+        return broker[brokerMethod]({ get: () => {} })
+          .then((behavior) => {
+            assert.equal(behavior.type, 'navigate');
+            assert.equal(behavior.endpoint, 'settings');
+          });
       });
     }
 
     function testRedirectsToSettingsIfSignedIn(brokerMethod) {
-      describe(brokerMethod, () => {
-        it('returns a NavigateBehavior to settings', () => {
-          return broker[brokerMethod]({ get: () => {} })
-            .then((behavior) => {
-              assert.equal(behavior.type, 'settings');
-            });
-        });
+      it(`${brokerMethod} returns a SettingsIfSignedInBehavior`, () => {
+        return broker[brokerMethod]({ get: () => {} })
+          .then((behavior) => {
+            assert.equal(behavior.type, 'settings');
+          });
       });
     }
 
     testRedirectsToSettings('afterCompleteResetPassword');
+    testRedirectsToSettings('afterForceAuth');
     testRedirectsToSettings('afterResetPasswordConfirmationPoll');
+    testRedirectsToSettings('afterSignIn');
     testRedirectsToSettings('afterSignInConfirmationPoll');
     testRedirectsToSettings('afterSignUpConfirmationPoll');
 

app/tests/spec/views/behaviors/connect-another-device-on-signin.js

@@ -6,6 +6,8 @@ define((require, exports, module) => {
   'use strict';
 
   const { assert } = require('chai');
+  const Cocktail = require('cocktail');
+  const ConnectAnotherDeviceMixin = require('views/mixins/connect-another-device-mixin');
   const ConnectAnotherDeviceOnSigninBehavior = require('views/behaviors/connect-another-device-on-signin');
   const NullBehavior = require('views/behaviors/null');
   const sinon = require('sinon');
@@ -21,14 +23,22 @@ define((require, exports, module) => {
       cadOnSigninBehavior = new ConnectAnotherDeviceOnSigninBehavior(defaultBehavior);
     });
 
+    it('ensureConnectAnotherDeviceMixin adds the ConnectAnotherDeviceMixin to a view', () => {
+      const view = {};
+      cadOnSigninBehavior.ensureConnectAnotherDeviceMixin(view);
+      assert.isFunction(view.isEligibleForConnectAnotherDevice);
+      assert.isFunction(view.navigateToConnectAnotherDeviceScreen);
+    });
+
     describe('eligible for CAD', () => {
       it('delegates to `view.navigateToConnectAnotherDeviceOnSigninScreen`', () => {
         const view = {
-          hasNavigated: sinon.spy(() => false),
-          isEligibleForConnectAnotherDeviceOnSignin: sinon.spy(() => true),
-          isSignIn: sinon.spy(() => true),
-          navigateToConnectAnotherDeviceOnSigninScreen: sinon.spy()
+          hasNavigated: sinon.spy(() => false)
         };
+        Cocktail.mixin(view, ConnectAnotherDeviceMixin);
+
+        sinon.stub(view, 'isEligibleForConnectAnotherDeviceOnSignin').callsFake(() => true);
+        sinon.stub(view, 'navigateToConnectAnotherDeviceOnSigninScreen').callsFake(() => {});
 
         return cadOnSigninBehavior(view, account)
           .then((behavior) => {
@@ -48,11 +58,12 @@ define((require, exports, module) => {
     describe('ineligible for CAD', () => {
       it('invokes the defaultBehavior', () => {
         const view = {
-          hasNavigated: sinon.spy(() => false),
-          isEligibleForConnectAnotherDeviceOnSignin: sinon.spy(() => false),
-          isSignIn: sinon.spy(() => true),
-          navigateToConnectAnotherDeviceOnSigninScreen: sinon.spy()
+          hasNavigated: sinon.spy(() => false)
         };
+        Cocktail.mixin(view, ConnectAnotherDeviceMixin);
+
+        sinon.stub(view, 'isEligibleForConnectAnotherDeviceOnSignin').callsFake(() => false);
+        sinon.stub(view, 'navigateToConnectAnotherDeviceOnSigninScreen').callsFake(() => {});
 
         return cadOnSigninBehavior(view, account)
           .then((behavior) => {

app/tests/spec/views/force_auth.js

@@ -398,49 +398,6 @@ define(function (require, exports, module) {
         var signInPassword = view._signIn.args[0][1];
         assert.equal(signInPassword, password);
       });
-
-      describe('onSignInSuccess', function () {
-        var account;
-
-        beforeEach(function () {
-          account = user.initAccount({
-            email: 'testuser@testuser.com',
-            verified: true
-          });
-
-          sinon.spy(broker, 'afterForceAuth');
-        });
-
-        describe('without model.redirectTo', function () {
-          beforeEach(function () {
-            return view.onSignInSuccess(account);
-          });
-
-          it('invokes `afterForceAuth` on the broker', function () {
-            assert.isTrue(broker.afterForceAuth.calledWith(account));
-          });
-
-          it('navigates to the `settings` page and clears the query parameters', function () {
-            assert.isTrue(view.navigate.calledWith('settings', {}, { clearQueryParams: true }));
-          });
-        });
-
-        describe('with model.redirectTo', function () {
-          beforeEach(function () {
-            model.set('redirectTo', 'foo');
-
-            return view.onSignInSuccess(account);
-          });
-
-          it('invokes `afterForceAuth` on the broker', function () {
-            assert.isTrue(broker.afterForceAuth.calledWith(account));
-          });
-
-          it('navigates to the `settings` page and clears the query parameters', function () {
-            assert.isTrue(view.navigate.calledWith('foo', {}, { clearQueryParams: true }));
-          });
-        });
-      });
     });
 
     describe('onSignInError', function () {

app/tests/spec/views/mixins/connect-another-device-mixin.js

@@ -69,32 +69,21 @@ define(function (require, exports, module) {
     });
 
     describe('isEligibleForConnectAnotherDeviceOnSignin', () => {
-      it('returns true if signing in, if eligible for CAD, and is part of the experiment', () => {
-        sinon.stub(view, 'isSignIn').callsFake(() => true);
+      it('returns true if eligible for CAD, and is part of the experiment', () => {
         sinon.stub(view, 'isEligibleForConnectAnotherDevice').callsFake(() => true);
         sinon.stub(view, 'getExperimentGroup').callsFake(() => 'treatment');
 
         assert.isTrue(view.isEligibleForConnectAnotherDeviceOnSignin(account));
       });
 
-      it('returns false if signing up, if eligible for CAD, and is part of the experiment', () => {
-        sinon.stub(view, 'isSignIn').callsFake(() => false);
-        sinon.stub(view, 'isEligibleForConnectAnotherDevice').callsFake(() => true);
-        sinon.stub(view, 'getExperimentGroup').callsFake(() => 'treatment');
-
-        assert.isFalse(view.isEligibleForConnectAnotherDeviceOnSignin(account));
-      });
-
-      it('returns false if signing in, not eligible for CAD, and is part of the experiment', () => {
-        sinon.stub(view, 'isSignIn').callsFake(() => true);
+      it('returns false not eligible for CAD, and is part of the experiment', () => {
         sinon.stub(view, 'isEligibleForConnectAnotherDevice').callsFake(() => false);
         sinon.stub(view, 'getExperimentGroup').callsFake(() => 'treatment');
 
         assert.isFalse(view.isEligibleForConnectAnotherDeviceOnSignin(account));
       });
 
-      it('returns false if signing in, not eligible for CAD, and is part of the experiment', () => {
-        sinon.stub(view, 'isSignIn').callsFake(() => true);
+      it('returns false not eligible for CAD, and is part of the experiment', () => {
         sinon.stub(view, 'isEligibleForConnectAnotherDevice').callsFake(() => true);
         sinon.stub(view, 'getExperimentGroup').callsFake(() => false);
 

app/tests/spec/views/mixins/signin-mixin.js

@@ -60,9 +60,7 @@ define(function (require, exports, module) {
           currentPage: 'force_auth',
           displayError: sinon.spy(),
           getStringifiedResumeToken: sinon.spy(() => RESUME_TOKEN),
-          invokeBrokerMethod: sinon.spy(function () {
-            return p();
-          }),
+          invokeBrokerMethod: sinon.spy(() => p()),
           logEvent: sinon.spy(),
           logFlowEvent: sinon.spy(),
           logViewEvent: sinon.spy(),
@@ -117,6 +115,7 @@ define(function (require, exports, module) {
         describe('with `redirectTo` specified', function () {
           beforeEach(function () {
             model.set('redirectTo', 'settings/avatar');
+            sinon.spy(broker, 'setBehavior');
 
             return view.signIn(account, 'password');
           });
@@ -137,6 +136,15 @@ define(function (require, exports, module) {
             assert.lengthOf(view.formPrefill.clear.args[0], 0);
           });
 
+          it('sets a NavigateBehavior with the expected endpoint on the broker', () => {
+            const behavior = broker.getBehavior('afterSignIn');
+            assert.equal(behavior.type, 'navigate');
+            assert.equal(behavior.endpoint, 'settings/avatar');
+
+            assert.isTrue(broker.setBehavior.calledOnce);
+            assert.isTrue(broker.setBehavior.calledWith('afterSignIn', behavior));
+          });
+
           it('calls view.invokeBrokerMethod correctly', function () {
             assert.equal(view.invokeBrokerMethod.callCount, 2);
 
@@ -150,35 +158,18 @@ define(function (require, exports, module) {
             assert.equal(args[0], 'afterSignIn');
             assert.equal(args[1], account);
           });
-
-          it('calls view.navigate correctly', function () {
-            assert.equal(view.navigate.callCount, 1);
-            var args = view.navigate.args[0];
-            assert.lengthOf(args, 4);
-            assert.equal(args[0], 'settings/avatar');
-            assert.isObject(args[1]);
-            assert.lengthOf(Object.keys(args[1]), 0);
-            assert.deepEqual(args[2], {});
-            assert.isUndefined(args[3]);
-          });
         });
 
         describe('without `redirectTo` specified', function () {
           beforeEach(function () {
             model.unset('redirectTo');
+            sinon.spy(broker, 'setBehavior');
 
             return view.signIn(account, 'password');
           });
 
-          it('calls view.navigate correctly', function () {
-            assert.equal(view.navigate.callCount, 1);
-            var args = view.navigate.args[0];
-            assert.lengthOf(args, 4);
-            assert.equal(args[0], 'settings');
-            assert.isObject(args[1]);
-            assert.lengthOf(Object.keys(args[1]), 0);
-            assert.deepEqual(args[2], {});
-            assert.isUndefined(args[3]);
+          it('does not set a NavigateBehavior on the broker', () => {
+            assert.isFalse(broker.setBehavior.called);
           });
         });
       });

app/tests/spec/views/oauth_sign_in.js

@@ -138,83 +138,70 @@ define(function (require, exports, module) {
     describe('submit', function () {
       beforeEach(() => view.render());
 
-      it('notifies the broker when a verified user signs in', function () {
-        sinon.spy(user, 'initAccount');
-        sinon.stub(user, 'signInAccount').callsFake(function (account) {
-          account.set('verified', true);
-          return p(account);
-        });
-        sinon.stub(relier, 'accountNeedsPermissions').callsFake(function () {
-          return false;
-        });
-        sinon.stub(broker, 'afterSignIn').callsFake(function () {
-          return p();
-        });
-        sinon.spy(view, 'navigate');
+      it('delegates to `signIn`', () => {
+        const account = user.initAccount({});
+        sinon.stub(user, 'initAccount').callsFake(() => account);
+
+        sinon.stub(view, 'signIn').callsFake(() => p());
 
-        const password = 'password';
         view.$('.email').val(email);
-        view.$('[type=password]').val(password);
+        view.$('[type=password]').val('password');
 
         return view.submit()
-          .then(function () {
-            const account = user.initAccount.returnValues[0];
-
-            assert.isTrue(user.signInAccount.calledWith(account));
-            assert.isTrue(TestHelpers.isEventLogged(metrics, 'signin.success'));
-            assert.isTrue(TestHelpers.isEventLogged(metrics, 'signin.success.skip-confirm'));
-            assert.isTrue(TestHelpers.isEventLogged(metrics, 'oauth.signin.signin.success'));
-            assert.isTrue(broker.afterSignIn.calledWith(account));
-            assert.isTrue(view.navigate.calledWith('settings'));
+          .then(() => {
+            assert.isTrue(view.signIn.calledOnce);
+            assert.isTrue(view.signIn.calledWith(account));
           });
       });
+    });
 
-      describe('with an unknown account', function () {
+    describe('_suggestSignUp', () => {
+      let err;
+
+      beforeEach(() => {
+        err = AuthErrors.toError('UNKNOWN_ACCOUNT');
+        sinon.spy(view, 'unsafeDisplayError');
+      });
+
+      describe('AMO migration', () => {
+        let $amoMigrationElement;
         beforeEach(() => {
-          broker.setCapability('signup', true);
-          sinon.stub(view, 'signIn').callsFake(() => p.reject(AuthErrors.toError('UNKNOWN_ACCOUNT')));
-          sinon.spy(view, 'unsafeDisplayError');
+          $amoMigrationElement = {
+            hide: sinon.spy()
+          };
+          sinon.stub(view, 'isAmoMigration').callsFake(() => true);
+          const orig$ = view.$;
+          sinon.stub(view, '$').callsFake((selector) => {
+            if (selector === '#amo-migration') {
+              return $amoMigrationElement;
+            } else {
+              return orig$.call(view, selector);
+            }
+          });
+
+          return view._suggestSignUp(err);
         });
 
-        describe('AMO migration', () => {
-          let $amoMigrationElement;
-          beforeEach(() => {
-            $amoMigrationElement = {
-              hide: sinon.spy()
-            };
-            sinon.stub(view, 'isAmoMigration').callsFake(() => true);
-            const orig$ = view.$;
-            sinon.stub(view, '$').callsFake((selector) => {
-              if (selector === '#amo-migration') {
-                return $amoMigrationElement;
-              } else {
-                return orig$.call(view, selector);
-              }
-            });
-            return view.submit();
-          });
+        it('shows addons help text with link to the signup page, hides AMO migration text', () => {
+          var err = view.unsafeDisplayError.args[0][0];
+          assert.isTrue(AuthErrors.is(err, 'UNKNOWN_ACCOUNT'));
+          assert.include(err.forceMessage, '/signup');
+          assert.include(err.forceMessage, 'Add-ons');
+          assert.isTrue($amoMigrationElement.hide.calledOnce);
+        });
+      });
 
-          it('shows addons help text with link to the signup page, hides AMO migration text', () => {
-            var err = view.unsafeDisplayError.args[0][0];
-            assert.isTrue(AuthErrors.is(err, 'UNKNOWN_ACCOUNT'));
-            assert.include(err.forceMessage, '/signup');
-            assert.include(err.forceMessage, 'Add-ons');
-            assert.isTrue($amoMigrationElement.hide.calledOnce);
-          });
+      describe('not AMO migration', () => {
+        beforeEach(() => {
+          sinon.stub(view, 'isAmoMigration').callsFake(() => false);
+          return view._suggestSignUp(err);
         });
 
-        describe('not AMO migration', () => {
-          beforeEach(() => {
-            sinon.stub(view, 'isAmoMigration').callsFake(() => false);
-            return view.submit();
-          });
-
-          it('shows a link to the signup page', () => {
-            var err = view.unsafeDisplayError.args[0][0];
-            assert.isTrue(AuthErrors.is(err, 'UNKNOWN_ACCOUNT'));
-            assert.include(err.forceMessage, '/signup');
-            assert.notInclude(err.forceMessage, 'Add-ons');
-          });
+        it('shows a link to the signup page', () => {
+          var err = view.unsafeDisplayError.args[0][0];
+          assert.isTrue(AuthErrors.is(err, 'UNKNOWN_ACCOUNT'));
+          assert.include(err.forceMessage, '/signup');
+          assert.notInclude(err.forceMessage, 'Add-ons');
         });
       });
     });

app/tests/spec/views/sign_in.js

@@ -296,83 +296,6 @@ define(function (require, exports, module) {
           assert.equal(password, 'password');
         });
       });
-
-      describe('with a reset account', () => {
-        beforeEach(() => {
-          sinon.stub(view, 'signIn').callsFake(() => {
-            return p.reject(AuthErrors.toError('ACCOUNT_RESET'));
-          });
-
-          sinon.spy(view, 'notifyOfResetAccount');
-
-          return view.submit();
-        });
-
-        it('notifies the user of the reset account', () => {
-          assert.isTrue(view.notifyOfResetAccount.called);
-          const args = view.notifyOfResetAccount.args[0];
-          const account = args[0];
-          assert.instanceOf(account, Account);
-        });
-      });
-
-      describe('with a user that cancels login', () => {
-        beforeEach(() => {
-          sinon.stub(view, 'signIn').callsFake(() => {
-            return p.reject(AuthErrors.toError('USER_CANCELED_LOGIN'));
-          });
-
-          return view.submit();
-        });
-
-        it('logs, but does not display the error', () => {
-          assert.isTrue(isEventLogged(metrics, 'signin.canceled'));
-          assert.isFalse(view.isErrorVisible());
-        });
-      });
-
-      describe('with an unknown account', () => {
-        beforeEach(() => {
-          broker.setCapability('signup', true);
-
-          sinon.stub(view, 'signIn').callsFake(() => {
-            return p.reject(AuthErrors.toError('UNKNOWN_ACCOUNT'));
-          });
-
-          sinon.spy(view, 'unsafeDisplayError');
-
-          return view.submit();
-        });
-
-        it('shows a link to the signup page', () => {
-          const err = view.unsafeDisplayError.args[0][0];
-          assert.isTrue(AuthErrors.is(err, 'UNKNOWN_ACCOUNT'));
-          assert.include(err.forceMessage, '/signup');
-        });
-      });
-
-      describe('other errors', () => {
-        let err;
-
-        beforeEach(() => {
-          sinon.stub(view, 'signIn').callsFake(() => {
-            return p.reject(AuthErrors.toError('INVALID_JSON'));
-          });
-
-          sinon.spy(view, 'displayError');
-
-          return view.validateAndSubmit()
-            .then(assert.fail, function (_err) {
-              err = _err;
-            });
-        });
-
-        it('are displayed', () => {
-          const displayedError = view.displayError.args[0][0];
-          assert.strictEqual(err, displayedError);
-          assert.isTrue(AuthErrors.is(err, 'INVALID_JSON'));
-        });
-      });
     });
 
     describe('showValidationErrors', () => {
@@ -403,6 +326,86 @@ define(function (require, exports, module) {
       });
     });
 
+    describe('onSignInError', () => {
+      let account;
+
+      beforeEach(() => {
+        account = user.initAccount({});
+      });
+
+      it('an unknown account delegates to _suggestSignUp', () => {
+        broker.setCapability('signup', true);
+        sinon.stub(view, '_suggestSignUp');
+        const err = AuthErrors.toError('UNKNOWN_ACCOUNT');
+
+        view.onSignInError(
+          account,
+          'password',
+          err
+        );
+
+        assert.isTrue(view._suggestSignUp.calledOnce);
+        assert.isTrue(view._suggestSignUp.calledWith(err));
+      });
+
+      it('when a user cancels login, the error is logged but not displayed', () => {
+        view.onSignInError(
+          account,
+          'password',
+          AuthErrors.toError('USER_CANCELED_LOGIN')
+        );
+
+        assert.isTrue(isEventLogged(metrics, 'signin.canceled'));
+        assert.isFalse(view.isErrorVisible());
+      });
+
+      it('a reset account notifies the user', () => {
+        sinon.spy(view, 'notifyOfResetAccount');
+
+        view.onSignInError(
+          account,
+          'password',
+          AuthErrors.toError('ACCOUNT_RESET')
+        );
+
+        assert.isTrue(view.notifyOfResetAccount.called);
+        assert.isTrue(view.notifyOfResetAccount.calledWith(account));
+      });
+
+      it('incorrect password shows a validation error', () => {
+        sinon.spy(view, 'showValidationError');
+        view.onSignInError(
+            account,
+            'password',
+            AuthErrors.toError('INCORRECT_PASSWORD')
+        );
+
+        assert.isTrue(view.showValidationError.calledOnce);
+      });
+
+      it('other errors are re-thrown', () => {
+        const err = AuthErrors.toError('INVALID_JSON');
+        assert.throws(() => {
+          view.onSignInError(
+            account,
+            'password',
+            err
+          );
+        }, err);
+      });
+    });
+
+    it('_suggestSignUp shows a link to the signup page', () => {
+      sinon.spy(view, 'unsafeDisplayError');
+      const err = AuthErrors.toError('UNKNOWN_ACCOUNT');
+
+      view._suggestSignUp(err);
+
+      assert.isTrue(view.unsafeDisplayError.calledOnce);
+      assert.isTrue(view.unsafeDisplayError.calledWith(err));
+      assert.include(err.forceMessage, '/signup');
+    });
+
     describe('useLoggedInAccount', () => {
       it('shows an error if session is expired', () => {
         sinon.stub(view, 'getAccount').callsFake(() => {
@@ -413,6 +416,9 @@ define(function (require, exports, module) {
           });
         });
 
+        sinon.stub(view, 'signIn').callsFake(
+          () => p.reject(AuthErrors.toError('SESSION_EXPIRED')));
+
         return view.useLoggedInAccount()
           .then(() => {
             assert.isTrue(view._isErrorVisible);
@@ -424,22 +430,18 @@ define(function (require, exports, module) {
           });
       });
 
-      it('signs in with a valid session', () => {
+      it('delegates to signIn', () => {
         const account = user.initAccount({
           email: 'a@a.com',
           sessionToken: 'abc123'
         });
         sinon.stub(view, 'getAccount').callsFake(() => account);
-        sinon.stub(user, 'signInAccount').callsFake((account) => {
-          account.set('verified', true);
-          return p(account);
-        });
+        sinon.stub(view, 'signIn').callsFake(() => p());
 
         return view.useLoggedInAccount()
           .then(() => {
-            assert.isTrue(user.signInAccount.calledWith(account));
-            assert.equal(view.$('.error').text(), '');
-            assert.notOk(view._isErrorVisible);
+            assert.isTrue(view.signIn.calledOnce);
+            assert.isTrue(view.signIn.calledWith(account));
           });
       });
     });

tests/functional/fx_firstrun_v1_sign_in.js

@@ -157,10 +157,7 @@ define([
         .then(testElementTextInclude(selectors.SIGNIN_UNBLOCK.EMAIL_FIELD, email))
         .then(fillOutSignInUnblock(email, 0))
 
-        // Only users that go through signin confirmation see
-        // `/signin_complete`, and users that go through signin unblock see
-        // the default `settings` page.
-        .then(testElementExists(selectors.SETTINGS.HEADER))
+        .then(testElementExists(selectors.SIGNIN_COMPLETE.HEADER))
         .then(testIsBrowserNotified('fxaccounts:login'));
     }
   });

tests/functional/sync_v3_sign_in.js

@@ -74,6 +74,42 @@ define([
         .then(clearBrowserState());
     },
 
+    'Fx >= 57, verified, does not need to confirm - control': function () {
+      const forceUA = uaStrings['desktop_firefox_57'];
+      const query = { forceExperiment: 'cadOnSignin', forceExperimentGroup: 'control', forceUA };
+
+      email = TestHelpers.createEmail();
+
+      return this.remote
+        .then(clearBrowserState({ force: true }))
+        .then(createUser(email, PASSWORD, { preVerified: true }))
+        .then(openPage(PAGE_URL, selectors.SIGNIN.HEADER, { query, webChannelResponses: {
+          'fxaccounts:can_link_account': { ok: true },
+          'fxaccounts:fxa_status': { capabilities: null, signedInUser: null },
+        }}))
+        .then(fillOutSignIn(email, PASSWORD))
+
+        .then(testElementExists(selectors.SIGNIN_COMPLETE.HEADER));
+    },
+
+    'Fx >= 57, verified, does not need to confirm - treatment': function () {
+      const forceUA = uaStrings['desktop_firefox_57'];
+      const query = { forceExperiment: 'cadOnSignin', forceExperimentGroup: 'treatment', forceUA };
+
+      email = TestHelpers.createEmail();
+
+      return this.remote
+        .then(clearBrowserState({ force: true }))
+        .then(createUser(email, PASSWORD, { preVerified: true }))
+        .then(openPage(PAGE_URL, selectors.SIGNIN.HEADER, { query, webChannelResponses: {
+          'fxaccounts:can_link_account': { ok: true },
+          'fxaccounts:fxa_status': { capabilities: null, signedInUser: null },
+        }}))
+        .then(fillOutSignIn(email, PASSWORD))
+
+        .then(testElementExists(selectors.CONNECT_ANOTHER_DEVICE.HEADER));
+    },
+
     'verified, verify same browser, new tab\'s P.O.V - control': function () {
       const query = { forceExperiment: 'cadOnSignin', forceExperimentGroup: 'control' };
 
@@ -199,9 +235,7 @@ define([
         .then(fillOutSignInUnblock(email, 0))
 
         // about:accounts does not take over post-verification in Fx >= 57
-        // NOTE: the user should actually be sent to the "signin complete" screen,
-        // but there is a bug that I haven't figured out yet.
-        .then(testElementExists(selectors.SETTINGS.HEADER))
+        .then(testElementExists(selectors.SIGNIN_COMPLETE.HEADER))
         .then(testIsBrowserNotified('fxaccounts:login'));
     },