diff --git a/packages/fxa-auth-client/lib/recoveryKey.ts b/packages/fxa-auth-client/lib/recoveryKey.ts index c7646ec8744..2288efb0ab8 100644 --- a/packages/fxa-auth-client/lib/recoveryKey.ts +++ b/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 { diff --git a/packages/fxa-content-server/app/scripts/models/account.js b/packages/fxa-content-server/app/scripts/models/account.js index a1cabeda705..bb1950ffb59 100644 --- a/packages/fxa-content-server/app/scripts/models/account.js +++ b/packages/fxa-content-server/app/scripts/models/account.js @@ -74,6 +74,9 @@ const DEFAULTS = _.extend( verificationReason: undefined, totpVerified: undefined, providerUid: undefined, + + // TODO before merge: probably don't need this anymore with the getOldSettingsData fn + authAt: undefined, }, PERSISTENT ); diff --git a/packages/fxa-graphql-api/src/gql/account.resolver.ts b/packages/fxa-graphql-api/src/gql/account.resolver.ts index f83c3c5854d..f32e1dc18df 100644 --- a/packages/fxa-graphql-api/src/gql/account.resolver.ts +++ b/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 { + const result = await this.authAPI.accountReset( input.email, input.newPassword, input.accountResetToken, input.options, headers ); + return { + clientMutationId: input.clientMutationId, + ...result, + }; } @Mutation((returns) => SignedUpAccountPayload, { diff --git a/packages/fxa-settings/src/lib/cache.ts b/packages/fxa-settings/src/lib/cache.ts index 7ce16ce3a84..f6f1245bf35 100644 --- a/packages/fxa-settings/src/lib/cache.ts +++ b/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): OldSettingsData { + return { + uid, + sessionToken, + alertText, + displayName, + metricsEnabled, + }; +} + type LocalAccount = OldSettingsData | undefined; type LocalAccounts = Record | undefined; diff --git a/packages/fxa-settings/src/models/Account.ts b/packages/fxa-settings/src/models/Account.ts index e09a927a03d..3a27a341098 100644 --- a/packages/fxa-settings/src/models/Account.ts +++ b/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' }), diff --git a/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.test.tsx b/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.test.tsx index f0df943ec7b..01fb61511e1 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.test.tsx +++ b/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(); + 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', () => { diff --git a/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.tsx b/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.tsx index 4ea6334bb95..162356437d1 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.tsx +++ b/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, diff --git a/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryResetPassword/index.test.tsx b/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryResetPassword/index.test.tsx index 7053dff5fb1..887a65bfb30 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryResetPassword/index.test.tsx +++ b/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, @@ -290,6 +297,39 @@ describe('AccountRecoveryResetPassword page', () => { expect(notifyFirefoxOfLogin).toHaveBeenCalled(); }); }); + + describe('navigateAway', () => { + // Not needed once this page doesn't use `hardNavigateToContentServer` + const originalWindow = window.location; + beforeAll(() => { + // @ts-ignore + delete window.location; + window.location = { ...originalWindow, href: '' }; + }); + beforeEach(() => { + window.location.href = originalWindow.href; + }); + afterAll(() => { + window.location = originalWindow; + }); + + it('account has TOTP', async () => { + // todo + + expect(window.location.href).toBe('/signin_totp_code'); + }); + + it('account does not have TOTP', async () => { + // todo + + expect(mockUseNavigateWithoutRerender).toHaveBeenCalledWith( + '/reset_password_verified', + { + replace: true, + } + ); + }); + }); }); }); diff --git a/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryResetPassword/index.tsx b/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryResetPassword/index.tsx index 0dd47b9b956..18af22c4b9d 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryResetPassword/index.tsx +++ b/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,10 +252,10 @@ 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. await account.setLastLogin(Date.now()); @@ -268,11 +268,11 @@ const AccountRecoveryResetPassword = ({ 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) { diff --git a/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.tsx b/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.tsx index 935c9375a66..a97895bc8d2 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.tsx +++ b/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.tsx @@ -166,21 +166,35 @@ const CompleteResetPassword = ({ // account's original email because this will maintain backwards compatibility with // how account password hashing works previously. const emailToUse = emailToHashWith || email; - const [sessionisVerified] = await Promise.all([ - account.isSessionVerified(), - account.completeResetPassword(token, code, emailToUse, newPassword), + + await account.completeResetPassword( + token, + code, + emailToUse, + newPassword + ); + + /* NOTE: Session check/totp check must come after completeResetPassword since those + * require session tokens that we retrieve in PW reset. We will want to refactor this + * later but there's a `mustVerify` check getting in the way (see Account.ts comment). + * + * We may also want to consider putting a different error message in place for when + * PW reset succeeds, but one of these fails. */ + const [sessionIsVerified, hasTotp] = await Promise.all([ + account.isSessionVerifiedAuthClient(), + account.hasTotpAuthClient(), ]); switch (integration.type) { case IntegrationType.SyncDesktop: - notifyFirefoxOfLogin(account, sessionisVerified); + notifyFirefoxOfLogin(account, sessionIsVerified); break; case IntegrationType.OAuth: if ( - sessionisVerified && + sessionIsVerified && // only allow this redirect if 2FA is not enabled, otherwise users must enter // their TOTP code first - !account.totp.verified && + !hasTotp && // a user can only redirect back to the relier from the original tab // to avoid two tabs redirecting. isOriginalTab() @@ -191,17 +205,21 @@ const CompleteResetPassword = ({ } else if (!isOriginalTab()) { // allows a navigation to a "complete" screen or TOTP screen if it is setup // TODO: check if relier has state - if (account.totp.verified) { + if (hasTotp) { // finishing OAuth flow occurs on this page after entering TOTP // TODO: probably need to pass some params - hardNavigateToContentServer('/signin_totp_code'); + hardNavigateToContentServer( + `/signin_totp_code${location.search}` + ); } } break; case IntegrationType.Web: - if (account.totp.verified) { + if (hasTotp) { // take users to Settings after entering TOTP - hardNavigateToContentServer('/signin_totp_code'); + hardNavigateToContentServer( + `/signin_totp_code${location.search}` + ); } // TODO: if no TOTP, navigate users to /settings with the alert bar message // for now, just navigate to reset_password_verified