fix(react): Password reset unexpected error fixes, with and without r…

…ecovery key

Because:
* There's an error when attempting to reset a password with an account recovery key, and other issues were uncovered during the fix

This commit:
* Fixes logic around accountResetToken, ensuring we only fetch getRecoveryBundle once with the same token
* Requests, receives, and sets sessionToken from account recovery reset and normal PW reset
* Uses this session token to perform other requests that must use authClient for now due to 'sessionToken.mustVerify' checks

Closes FXA-7189

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

@@ -89,7 +89,6 @@ export async function generateRecoveryKey(
  */
 export async function decryptRecoveryKeyData(
   recoveryKey: Uint8Array,
-  recoveryKeyId: string,
   recoveryData: string,
   uid: hexstring
 ): Promise<DecryptedRecoveryKeyData> {

packages/fxa-graphql-api/src/gql/account.resolver.ts

@@ -502,14 +502,18 @@ export class AccountResolver {
     @GqlXHeaders() headers: Headers,
     @Args('input', { type: () => AccountResetInput })
     input: AccountResetInput
-  ) {
-    return this.authAPI.accountReset(
+  ): Promise<AccountResetPayload> {
+    const result = await this.authAPI.accountReset(
       input.email,
       input.newPassword,
       input.accountResetToken,
       input.options,
       headers
     );
+    return {
+      clientMutationId: input.clientMutationId,
+      ...result,
+    };
   }
 
   @Mutation((returns) => SignedUpAccountPayload, {

packages/fxa-settings/src/lib/cache.ts

@@ -14,6 +14,22 @@ export interface OldSettingsData {
   metricsEnabled?: boolean;
 }
 
+export function getOldSettingsData({
+  uid,
+  sessionToken,
+  alertText,
+  displayName,
+  metricsEnabled,
+}: Record<string, any>): OldSettingsData {
+  return {
+    uid,
+    sessionToken,
+    alertText,
+    displayName,
+    metricsEnabled,
+  };
+}
+
 type LocalAccount = OldSettingsData | undefined;
 type LocalAccounts = Record<hexstring, LocalAccount> | undefined;
 

packages/fxa-settings/src/models/Account.ts

@@ -11,7 +11,7 @@ import AuthClient, {
   generateRecoveryKey,
   getRecoveryKeyIdByUid,
 } from 'fxa-auth-client/browser';
-import { currentAccount, sessionToken } from '../lib/cache';
+import { currentAccount, getOldSettingsData, sessionToken } from '../lib/cache';
 import firefox from '../lib/channels/firefox';
 import Storage from '../lib/storage';
 import random from '../lib/random';
@@ -712,16 +712,29 @@ export class Account implements AccountData {
         token,
         code
       );
-      await this.apolloClient.mutate({
+      const {
+        data: { accountReset },
+      } = await this.apolloClient.mutate({
         mutation: gql`
           mutation accountReset($input: AccountResetInput!) {
             accountReset(input: $input) {
               clientMutationId
+              sessionToken
+              uid
             }
           }
         `,
-        variables: { input: { accountResetToken, email, newPassword } },
+        variables: {
+          input: {
+            accountResetToken,
+            email,
+            newPassword,
+            options: { sessionToken: true },
+          },
+        },
       });
+      currentAccount(getOldSettingsData(accountReset));
+      sessionToken(accountReset.sessionToken);
     } catch (err) {
       const errno = (err as ApolloError).graphQLErrors[0].extensions?.errno;
       if (errno && AuthUiErrorNos[errno]) {
@@ -897,6 +910,26 @@ export class Account implements AccountData {
     );
   }
 
+  /* TODO: Remove this and use GQL instead. We can't check for verified sessions
+   * unless you've already got one (oof) in at least the PW reset flow due to
+   * sessionToken.mustVerify which was added here: https://github.com/mozilla/fxa/pull/7512
+   * The unverified session token returned by a password reset contains `mustVerify`
+   * which causes the 'Must verify' error to be thrown and a redirect to occur */
+  async isSessionVerifiedAuthClient() {
+    const { state } = await this.withLoadingStatus(
+      this.authClient.sessionStatus(sessionToken()!)
+    );
+    return state === 'verified';
+  }
+
+  // TODO: Same as isSessionVerifiedAuthClient
+  async hasTotpAuthClient() {
+    const { verified } = await this.withLoadingStatus(
+      this.authClient.checkTotpTokenExists(sessionToken()!)
+    );
+    return verified;
+  }
+
   async generateRecoveryCodes(count: number, length: number) {
     const recoveryCodes: string[] = [];
     const gen = random.base32(length);
@@ -1234,9 +1267,11 @@ export class Account implements AccountData {
       opts.emailToHashWith,
       opts.password,
       opts.recoveryKeyId,
-      { kB: opts.kB }
+      { kB: opts.kB },
+      { sessionToken: true }
     );
-    currentAccount(data);
+    currentAccount(currentAccount(getOldSettingsData(data)));
+    sessionToken(data.sessionToken);
     const cache = this.apolloClient.cache;
     cache.modify({
       id: cache.identify({ __typename: 'Account' }),

packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.test.tsx

@@ -4,7 +4,7 @@
 
 import React from 'react';
 import '@testing-library/jest-dom/extend-expect';
-import { fireEvent, render, screen } from '@testing-library/react';
+import { fireEvent, render, screen, waitFor } from '@testing-library/react';
 // import { getFtlBundle, testAllL10n } from 'fxa-react/lib/test-utils';
 // import { FluentBundle } from '@fluent/bundle';
 import { usePageViewEvent, logViewEvent } from '../../../lib/metrics';
@@ -257,6 +257,47 @@ describe('PageAccountRecoveryConfirmKey', () => {
         }
       );
     });
+
+    it('submits successfully after invalid recovery key submission', async () => {
+      account = {
+        ...account,
+        getRecoveryKeyBundle: jest
+          .fn()
+          .mockImplementationOnce(() => {
+            return Promise.reject(new Error('Oh noes'));
+          })
+          .mockResolvedValue({
+            recoveryData: 'mockRecoveryData',
+            recoveryKeyId: MOCK_RECOVERY_KEY_ID,
+          }),
+      } as unknown as Account;
+
+      render(<Subject {...{ account }} />);
+      await typeByLabelText('Enter account recovery key')(MOCK_RECOVERY_KEY);
+      fireEvent.click(
+        screen.getByRole('button', { name: 'Confirm account recovery key' })
+      );
+      await screen.findByText('Invalid account recovery key');
+
+      fireEvent.click(
+        screen.getByRole('button', { name: 'Confirm account recovery key' })
+      );
+
+      // only ever calls `verifyPasswordForgotToken` once despite number of submissions
+      await waitFor(() =>
+        expect(account.verifyPasswordForgotToken).toHaveBeenCalledTimes(1)
+      );
+      expect(account.verifyPasswordForgotToken).toHaveBeenCalledWith(
+        mockToken,
+        mockCode
+      );
+      expect(account.getRecoveryKeyBundle).toHaveBeenCalledTimes(2);
+      expect(account.getRecoveryKeyBundle).toHaveBeenCalledWith(
+        MOCK_RESET_TOKEN,
+        MOCK_RECOVERY_KEY,
+        mockUid
+      );
+    });
   });
 
   describe('emits metrics events', () => {

packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.tsx

@@ -102,9 +102,8 @@ const AccountRecoveryConfirmKey = (_: RouteComponentProps) => {
       const decodedRecoveryKey = base32Decode(recoveryKey, 'Crockford');
       const uint8RecoveryKey = new Uint8Array(decodedRecoveryKey);
 
-      const decryptedData = await decryptRecoveryKeyData(
+      const { kB } = await decryptRecoveryKeyData(
         uint8RecoveryKey,
-        recoveryKeyId,
         recoveryData,
         uid
       );
@@ -113,7 +112,7 @@ const AccountRecoveryConfirmKey = (_: RouteComponentProps) => {
         state: {
           accountResetToken,
           recoveryKeyId,
-          kB: decryptedData.kB,
+          kB,
         },
       });
     },
@@ -123,21 +122,17 @@ const AccountRecoveryConfirmKey = (_: RouteComponentProps) => {
   const checkRecoveryKey = useCallback(
     async ({ recoveryKey, token, code, email, uid }: SubmitData) => {
       try {
-        if (!fetchedResetToken) {
+        let resetToken = fetchedResetToken;
+        if (!resetToken) {
           const { accountResetToken } = await account.verifyPasswordForgotToken(
             token,
             code
           );
           setFetchedResetToken(accountResetToken);
-          await getRecoveryBundleAndNavigate({
-            accountResetToken,
-            recoveryKey,
-            uid,
-            email,
-          });
+          resetToken = accountResetToken;
         }
         await getRecoveryBundleAndNavigate({
-          accountResetToken: fetchedResetToken,
+          accountResetToken: resetToken,
           recoveryKey,
           uid,
           email,

packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryResetPassword/index.test.tsx

@@ -38,6 +38,13 @@ let mockOauthClient = mocks.mockOauthClient();
 let mockNavigate = mocks.mockNavigate();
 let mockNotifier = mocks.mockNotifier();
 
+const mockUseNavigateWithoutRerender = jest.fn();
+
+jest.mock('../../../lib/hooks/useNavigateWithoutRerender', () => ({
+  __esModule: true,
+  default: () => mockUseNavigateWithoutRerender,
+}));
+
 jest.mock('../../../models/hooks', () => {
   return {
     __esModule: true,
@@ -242,6 +249,8 @@ describe('AccountRecoveryResetPassword page', () => {
       beforeEach(async () => {
         mockAccount.setLastLogin = jest.fn();
         mockAccount.resetPasswordWithRecoveryKey = jest.fn();
+        mockAccount.isSessionVerifiedAuthClient = jest.fn();
+        mockAccount.hasTotpAuthClient = jest.fn().mockResolvedValue(false);
 
         await act(async () => {
           enterPassword('foo12356789!');
@@ -256,8 +265,14 @@ describe('AccountRecoveryResetPassword page', () => {
         );
       });
 
-      it('calls resetPasswordWithRecoveryKey', () => {
-        expect(mockAccount.resetPasswordWithRecoveryKey).toHaveBeenCalled();
+      it('calls account API methods', () => {
+        // Check that resetPasswordWithRecoveryKey was the first function called
+        // because it retrieves the session token required by other calls
+        expect(
+          (mockAccount.resetPasswordWithRecoveryKey as jest.Mock).mock.calls[0]
+        ).toBeTruthy();
+        expect(mockAccount.isSessionVerifiedAuthClient).toHaveBeenCalled();
+        expect(mockAccount.hasTotpAuthClient).toHaveBeenCalled();
       });
 
       it('sets relier state', () => {
@@ -290,6 +305,43 @@ describe('AccountRecoveryResetPassword page', () => {
           expect(notifyFirefoxOfLogin).toHaveBeenCalled();
         });
       });
+
+      it('navigates as expected without totp', async () => {
+        await waitFor(() => {
+          expect(mockNavigate).toHaveBeenCalledWith(
+            '/reset_password_with_recovery_key_verified'
+          );
+        });
+      });
+    });
+  });
+
+  describe('successful reset with totp', () => {
+    // Window mocks not needed once this page doesn't use `hardNavigateToContentServer`
+    const originalWindow = window.location;
+    beforeAll(() => {
+      // @ts-ignore
+      delete window.location;
+      window.location = { ...originalWindow, href: '' };
+    });
+    beforeEach(async () => {
+      window.location.href = originalWindow.href;
+      mockAccount.setLastLogin = jest.fn();
+      mockAccount.resetPasswordWithRecoveryKey = jest.fn();
+      mockAccount.isSessionVerifiedAuthClient = jest.fn();
+      mockAccount.hasTotpAuthClient = jest.fn().mockResolvedValue(true);
+      await renderPage();
+      await act(async () => {
+        enterPassword('foo12356789!');
+        clickResetPassword();
+      });
+    });
+    afterAll(() => {
+      window.location = originalWindow;
+    });
+
+    it('navigates as expected', async () => {
+      expect(window.location.href).toContain('/signin_totp_code');
     });
   });
 

packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryResetPassword/index.tsx

@@ -9,7 +9,7 @@ import {
   useLocation,
   useNavigate,
 } from '@reach/router';
-import { FtlMsg } from 'fxa-react/lib/utils';
+import { FtlMsg, hardNavigateToContentServer } from 'fxa-react/lib/utils';
 import { useForm } from 'react-hook-form';
 
 import AppLayout from '../../../components/AppLayout';
@@ -252,27 +252,27 @@ const AccountRecoveryResetPassword = ({
           verificationInfo.emailToHashWith || verificationInfo.email,
       };
 
-      const [sessionisVerified] = await Promise.all([
-        account.isSessionVerified(),
-        account.resetPasswordWithRecoveryKey(options),
-      ]);
+      await account.resetPasswordWithRecoveryKey(options);
+      // must come after completeResetPassword since that receives the sessionToken
+      // required for this check
+      const sessionIsVerified = await account.isSessionVerifiedAuthClient();
 
-      // FOLLOW-UP: Functionality not yet available.
+      // FOLLOW-UP: Functionality not yet available. FXA-7045
       await account.setLastLogin(Date.now());
 
-      // FOLLOW-UP: Functionality not yet available.
+      // FOLLOW-UP: Functionality not yet available. FXA-7045
       notifier.onAccountSignIn(account);
 
       relier.resetPasswordConfirm = true;
       logViewEvent(viewName, 'verification.success');
 
       switch (integration.type) {
         case IntegrationType.SyncDesktop:
-          notifyFirefoxOfLogin(account, sessionisVerified);
+          notifyFirefoxOfLogin(account, sessionIsVerified);
           break;
         case IntegrationType.OAuth:
           if (
-            sessionisVerified &&
+            sessionIsVerified &&
             // a user can only redirect back to the relier from the original tab
             // to avoid two tabs redirecting.
             isOriginalTab()
@@ -314,18 +314,20 @@ const AccountRecoveryResetPassword = ({
     setBannerState(BannerState.PasswordResetSuccess);
   }
 
-  function navigateAway() {
-    setUserPreference('account-recovery', account.recoveryKey);
+  async function navigateAway() {
+    // TODO / refactor: the initial account result with useAccount() does not
+    // contain account data due to no session token. Users receive the session
+    // token on PW reset, so we can query here for it.
+    const hasTotp = await account.hasTotpAuthClient();
+
+    setUserPreference('account-recovery', false);
     logViewEvent(viewName, 'recovery-key-consume.success');
 
-    // When users reset a PW with their recovery key we always want to navigate to
-    // the page encouraging them to generate another. If they don't have a verified
-    // session, clicking on any link taking them into Settings should take them to
-    // `signin_token_code` to verify their session first, then take them to Settings.
-    // We don't prompt them for a TOTP code if enabled, unlike a regular password reset,
-    // because posession of the primary email to use the reset link while using a
-    // recovery key is sufficient proof of ownership.
-    navigate(`/reset_password_with_recovery_key_verified?${location.search}`);
+    if (hasTotp) {
+      hardNavigateToContentServer(`/signin_totp_code${location.search}`);
+    } else {
+      navigate(`/reset_password_with_recovery_key_verified${location.search}`);
+    }
   }
 
   function alertValidationError(err: ModelValidationErrors) {