Merge pull request #16939 from mozilla/allow-signin-with-keystretch-upgrade-failure

task(settings): Fallback to key stretch v1 in the event of upgrade failure

packages/functional-tests/tests/misc/keyStretchingV2.spec.ts

@@ -12,6 +12,8 @@ import { SettingsPage } from '../../pages/settings';
 import { ChangePasswordPage } from '../../pages/settings/changePassword';
 import { RecoveryKeyPage } from '../../pages/settings/recoveryKey';
 import { SignupReactPage } from '../../pages/signupReact';
+import { TotpPage } from '../../pages/settings/totp';
+import { getCode } from 'fxa-settings/src/lib/totp';
 
 // Disable this check for these tests. We are holding assertion in shared functions due
 // to how test permutations work, and these setup falsely trips this rule.
@@ -42,6 +44,7 @@ test.describe('key-stretching-v2', () => {
       resetPasswordReact: ResetPasswordReactPage;
       changePassword: ChangePasswordPage;
       recoveryKey: RecoveryKeyPage;
+      totp: TotpPage;
     };
 
     // Helpers
@@ -119,7 +122,8 @@ test.describe('key-stretching-v2', () => {
       >,
       signOut: boolean,
       email: string,
-      password: string
+      password: string,
+      totpCredentials?: { secret: string; recoveryCodes: string[] }
     ) {
       const { page, target, signupReact, login, settings } = opts;
       const stretch = version === 2 ? 'stretch=2' : '';
@@ -130,16 +134,29 @@ test.describe('key-stretching-v2', () => {
         await signupReact.fillOutEmailForm(email);
         await page.fill('[name="password"]', password);
         await page.click('[type="submit"]');
-        await page.waitForURL(/settings/);
+        if (totpCredentials) {
+          await page.waitForURL(/signin_totp_code/);
+          const code = await getCode(totpCredentials.secret);
+          await page.fill('[name="code"]', code);
+          await page.click('[type="submit"]');
+        }
       } else {
         await page.goto(`${target.contentServerUrl}?${stretch}`);
         await login.setEmail(email);
         await login.clickSubmit();
         await login.setPassword(password);
         await login.clickSubmit();
-        expect(await login.isUserLoggedIn()).toBe(true);
+        if (totpCredentials) {
+          await page.waitForURL(/signin_totp_code/);
+          const code = await getCode(totpCredentials.secret);
+          await page.fill('[type="number"]', code);
+          await page.click('[type="submit"]');
+        }
       }
 
+      await page.waitForURL(/settings/);
+      expect(await login.isUserLoggedIn()).toBe(true);
+
       if (signOut) {
         await settings.signOut();
       }
@@ -158,6 +175,20 @@ test.describe('key-stretching-v2', () => {
       return await recoveryKey.createRecoveryKey(password, hint);
     }
 
+    async function _enabledTotp(opts: Pick<Opts, 'settings' | 'totp'>) {
+      const { settings, totp } = opts;
+
+      await expect(settings.settingsHeading).toBeVisible();
+      await settings.totp.addButton.click();
+      const totpCredentials = await totp.fillOutTotpForms();
+      await expect(settings.settingsHeading).toBeVisible();
+      await expect(settings.totp.status).toHaveText('Enabled');
+
+      await settings.signOut();
+
+      return totpCredentials;
+    }
+
     async function _changePassword(
       version: 1 | 2,
       opts: Pick<Opts, 'settings' | 'changePassword'>,
@@ -328,6 +359,25 @@ test.describe('key-stretching-v2', () => {
       );
     });
 
+    async function testTotpLogin(
+      p1: 1 | 2,
+      p2: 1 | 2,
+      opts: Pick<
+        Opts,
+        'page' | 'target' | 'signupReact' | 'settings' | 'login' | 'totp'
+      >,
+      email: string,
+      password: string
+    ) {
+      await _signUp(p1, opts, false, email, password);
+      const totpCredentials = await _enabledTotp(opts);
+      await _login(p2, opts, false, email, password, totpCredentials);
+
+      // Remove 2fa to allow test cleanup
+      await opts.settings.totp.disableButton.click();
+      await opts.settings.clickModalConfirm();
+    }
+
     /**
      * Checks password reset from 'forgot password' link on login
      */
@@ -878,6 +928,7 @@ test.describe('key-stretching-v2', () => {
         newPassword
       );
     });
+
     test(`signs up as v2 changes password from settings as v2 for ${mode}`, async ({
       page,
       target,
@@ -902,5 +953,51 @@ test.describe('key-stretching-v2', () => {
         newPassword
       );
     });
+
+    test(`signs up as v1, enable totp, login as v2 for ${mode}`, async ({
+      page,
+      target,
+      pages: { settings, signupReact, login, totp },
+      testAccountTracker,
+    }) => {
+      const { email, password } = testAccountTracker.generateAccountDetails();
+      await testTotpLogin(
+        1,
+        2,
+        {
+          page,
+          target,
+          login,
+          signupReact,
+          settings,
+          totp,
+        },
+        email,
+        password
+      );
+    });
+
+    test(`signs up as v2, enable totp, login as v1 for ${mode}`, async ({
+      page,
+      target,
+      pages: { settings, signupReact, login, totp },
+      testAccountTracker,
+    }) => {
+      const { email, password } = testAccountTracker.generateAccountDetails();
+      await testTotpLogin(
+        2,
+        1,
+        {
+          page,
+          target,
+          login,
+          signupReact,
+          settings,
+          totp,
+        },
+        email,
+        password
+      );
+    });
   });
 });

packages/fxa-auth-client/lib/client.ts

@@ -4,9 +4,9 @@
 
 import * as crypto from './crypto';
 import { Credentials } from './crypto';
-import { PasswordV2UpgradeError } from './error';
 import * as hawk from './hawk';
 import { SaltVersion, createSaltV2 } from './salt';
+import * as Sentry from '@sentry/browser';
 
 enum ERRORS {
   INVALID_TIMESTAMP = 111,
@@ -448,14 +448,24 @@ export default class AuthClient {
       if (this.keyStretchVersion === 2) {
         if (credentials.upgradeNeeded) {
           // This condition indicates an upgrade already was attempted but did not take hold.
-          // If this state occurs, it means something went wrong, and an error should occur.
-          // If an error is not raised, we might end up in an infinite recursive loop!
+          // If this state occurs, it means something went wrong, and we want to fallback to
+          // v1 credentials and set the client's target key stretch version back to 1.
           if (options.postPasswordUpgrade === true) {
-            throw new PasswordV2UpgradeError();
+            this.keyStretchVersion = 1;
+            return await this.signIn(email, password, {
+              ...options,
+              postPasswordUpgrade: true,
+            });
           }
 
           // Try to upgrade the password, and sign in.
-          await this.passwordChange(email, password, password, options);
+          try {
+            await this.passwordChange(email, password, password, options);
+          } catch (err) {
+            Sentry.captureMessage(
+              'Failure to complete v2 key stretch upgrade.'
+            );
+          }
           return await this.signIn(email, password, {
             ...options,
             postPasswordUpgrade: true,

packages/fxa-auth-client/lib/error.ts

@@ -1,9 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-export class PasswordV2UpgradeError extends Error {
-  constructor() {
-    super('V2 Password Upgrade Failed')
-  }
-}

packages/fxa-settings/src/pages/Signin/container.tsx

@@ -228,10 +228,6 @@ const SigninContainer = ({
           passwordChangeFinish
         );
 
-      if (error) {
-        return { error };
-      }
-
       const options = {
         verificationMethod: VerificationMethods.EMAIL_OTP,
         keys: wantsKeys,
@@ -243,7 +239,7 @@ const SigninContainer = ({
       return await trySignIn(
         email,
         v1Credentials,
-        v2Credentials,
+        error ? undefined : v2Credentials,
         unverifiedAccount,
         beginSignin,
         options