diff --git a/packages/fxa-auth-client/lib/client.ts b/packages/fxa-auth-client/lib/client.ts index 409f8b4814..680eae0949 100644 --- a/packages/fxa-auth-client/lib/client.ts +++ b/packages/fxa-auth-client/lib/client.ts @@ -507,7 +507,7 @@ export default class AuthClient { code: string, passwordForgotToken: hexstring, options: { - accountResetWithoutRecoveryKey?: boolean; + accountResetWithRecoveryKey?: boolean; } = {}, headers: Headers = new Headers() ) { diff --git a/packages/fxa-settings/src/models/Account.ts b/packages/fxa-settings/src/models/Account.ts index 93236a44a4..34ff1b3723 100644 --- a/packages/fxa-settings/src/models/Account.ts +++ b/packages/fxa-settings/src/models/Account.ts @@ -623,10 +623,41 @@ export class Account implements AccountData { return result; } + /** + * TODO: Remove this once we update the GQL endpoint to accept the + * `accountResetWithRecoveryKey` option and fix graphql-api not reporting + * the correct IP address. + * + * @param token passwordForgotToken + * @param code code + * @param accountResetWithRecoveryKey is account being reset with recovery key? + * */ + async passwordForgotVerifyCode( + token: string, + code: string, + accountResetWithRecoveryKey = false + ): Promise { + // TODO: There is a bug in Backbone and React reset PW around `accountResetWithRecoveryKey`. + // We attempt to validate the `code` and `token` provided here, but because the + // `accountResetToken` can only be fetched once but uses the `accountResetWithRecoveryKey` + // option, auth-server fails to send an email if the user has a recovery key, attempts + // to use it unsuccessfully, and then goes through a normal reset via the link back + // to a normal reset if a user can't use their key. + const { accountResetToken } = + await this.authClient.passwordForgotVerifyCode(code, token, { + accountResetWithRecoveryKey, + }); + return accountResetToken; + } + /** * Verify a passwordForgotToken, which returns an accountResetToken that can * be used to perform the actual password reset. * + * NOTE! and TODO: this is currently unused. We need to update the GQL + * endpoint to accept the `accountResetWithRecoveryKey` option and + * fix graphql-api not reporting the correct IP address. + * * @param token passwordForgotToken * @param code code */ @@ -669,7 +700,8 @@ export class Account implements AccountData { token: string, code: string, email: string, - newPassword: string + newPassword: string, + resetToken?: string ): Promise { try { // TODO: Temporary workaround (use auth-client directly) for GraphQL not @@ -678,10 +710,8 @@ export class Account implements AccountData { // token, // code // ); - const { accountResetToken } = - await this.authClient.passwordForgotVerifyCode(code, token, { - accountResetWithoutRecoveryKey: true, - }); + const accountResetToken = + resetToken || (await this.passwordForgotVerifyCode(token, code)); const { data: { accountReset }, } = await this.apolloClient.mutate({ 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 5a0f648568..633dc1594d 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.test.tsx +++ b/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.test.tsx @@ -70,9 +70,7 @@ const accountWithValidResetToken = { recoveryData: 'mockRecoveryData', recoveryKeyId: MOCK_RECOVERY_KEY_ID, }), - verifyPasswordForgotToken: jest - .fn() - .mockResolvedValue({ accountResetToken: MOCK_RESET_TOKEN }), + passwordForgotVerifyCode: jest.fn().mockResolvedValue(MOCK_RESET_TOKEN), } as unknown as Account; const renderSubject = ({ @@ -127,7 +125,7 @@ describe('PageAccountRecoveryConfirmKey', () => { it('renders the component as expected when provided with an expired link', async () => { const accountWithTokenError = { resetPasswordStatus: jest.fn().mockResolvedValue(false), - verifyPasswordForgotToken: jest.fn().mockImplementation(() => { + passwordForgotVerifyCode: jest.fn().mockImplementation(() => { throw AuthUiErrors.INVALID_TOKEN; }), } as unknown as Account; @@ -287,9 +285,7 @@ describe('PageAccountRecoveryConfirmKey', () => { it('submits successfully after invalid recovery key submission', async () => { const accountWithKeyInvalidOnce = { resetPasswordStatus: jest.fn().mockResolvedValue(true), - verifyPasswordForgotToken: jest - .fn() - .mockResolvedValue({ accountResetToken: MOCK_RESET_TOKEN }), + passwordForgotVerifyCode: jest.fn().mockResolvedValue(MOCK_RESET_TOKEN), getRecoveryKeyBundle: jest .fn() .mockImplementationOnce(() => { @@ -316,17 +312,18 @@ describe('PageAccountRecoveryConfirmKey', () => { screen.getByRole('button', { name: 'Confirm account recovery key' }) ); - // only ever calls `verifyPasswordForgotToken` once despite number of submissions + // only ever calls `passwordForgotVerifyCode` once despite number of submissions await waitFor(() => expect( - accountWithKeyInvalidOnce.verifyPasswordForgotToken + accountWithKeyInvalidOnce.passwordForgotVerifyCode ).toHaveBeenCalledTimes(1) ); expect( - accountWithKeyInvalidOnce.verifyPasswordForgotToken + accountWithKeyInvalidOnce.passwordForgotVerifyCode ).toHaveBeenCalledWith( mockCompleteResetPasswordParams.token, - mockCompleteResetPasswordParams.code + mockCompleteResetPasswordParams.code, + true ); expect( accountWithKeyInvalidOnce.getRecoveryKeyBundle diff --git a/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.tsx b/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.tsx index 08876a1b65..4d254e43a4 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.tsx +++ b/packages/fxa-settings/src/pages/ResetPassword/AccountRecoveryConfirmKey/index.tsx @@ -146,9 +146,10 @@ const AccountRecoveryConfirmKey = ({ try { let resetToken = fetchedResetToken; if (!resetToken) { - const { accountResetToken } = await account.verifyPasswordForgotToken( + const accountResetToken = await account.passwordForgotVerifyCode( token, - code + code, + true ); setFetchedResetToken(accountResetToken); resetToken = accountResetToken; @@ -308,7 +309,10 @@ const AccountRecoveryConfirmKey = ({ to={`/complete_reset_password${location.search}`} className="link-blue text-sm" id="lost-recovery-key" - state={{ lostRecoveryKey: true }} + state={{ + lostRecoveryKey: true, + accountResetToken: fetchedResetToken, + }} onClick={() => { logViewEvent( 'flow', diff --git a/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.test.tsx b/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.test.tsx index d29e22bec4..ea5e87862a 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.test.tsx +++ b/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.test.tsx @@ -24,6 +24,7 @@ import { mockAppContext, renderWithRouter, } from '../../../models/mocks'; +import { MOCK_RESET_TOKEN } from '../../mocks'; // import { getFtlBundle, testAllL10n } from 'fxa-react/lib/test-utils'; // import { FluentBundle } from '@fluent/bundle'; @@ -43,6 +44,7 @@ jest.mock('../../../lib/metrics', () => ({ let account: Account; let lostRecoveryKey: boolean; +let accountResetToken: string | undefined; const mockNavigate = jest.fn(); const mockSearchParams = { @@ -61,6 +63,7 @@ const mockLocation = () => { search, state: { lostRecoveryKey, + accountResetToken, }, }; }; @@ -355,7 +358,8 @@ describe('CompleteResetPassword page', () => { token, code, emailToHashWith, - PASSWORD + PASSWORD, + undefined ); }); it('submits with email if emailToHashWith is missing', async () => { @@ -367,7 +371,24 @@ describe('CompleteResetPassword page', () => { token, code, email, - PASSWORD + PASSWORD, + undefined + ); + }); + + it('submits with accountResetToken if available', async () => { + lostRecoveryKey = true; + accountResetToken = MOCK_RESET_TOKEN; + render(, account); + const { token, emailToHashWith, code } = mockCompleteResetPasswordParams; + + await enterPasswordAndSubmit(); + expect(account.completeResetPassword).toHaveBeenCalledWith( + token, + code, + emailToHashWith, + PASSWORD, + MOCK_RESET_TOKEN ); }); diff --git a/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.tsx b/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.tsx index 143b01edd8..f96d035cd7 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.tsx +++ b/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/index.tsx @@ -86,7 +86,13 @@ const CompleteResetPassword = ({ * If users clicked the link leading back to this page from `account_recovery_confirm_key`, * we assume the user has lost the key and pass along a `lostRecoveryKey` flag * so we don't perform the check and redirect again. + * * If the link is -not- valid, we render link expired or link damaged. + * + * Additionally, the user can submit an invalid account recovery key and receive back + * an `accountResetToken`, then follow the link back to this page. In this case, we + * should _not_ check if the 'token' parameter is valid, since it will be invalid after + * this token is provided to us. */ useEffect(() => { const checkPasswordForgotToken = async (token: string) => { @@ -147,12 +153,19 @@ const CompleteResetPassword = ({ setShowLoadingSpinner(false); logPageViewEvent(viewName, REACT_ENTRYPOINT); }; - checkPasswordForgotToken(linkModel.token); + // If a user comes from AccountRecoveryConfirmKey and attempted a recovery key + // submission, 'token' was already verified and used to fetch the reset token + if (!location.state?.accountResetToken) { + checkPasswordForgotToken(linkModel.token); + } else { + renderCompleteResetPassword(); + } }, [ account, navigate, location.search, location.state?.lostRecoveryKey, + location.state?.accountResetToken, linkModel.email, linkModel.token, setLinkStatus, @@ -187,7 +200,8 @@ const CompleteResetPassword = ({ token, code, emailToUse, - newPassword + newPassword, + location.state?.accountResetToken ); /* NOTE: Session check/totp check must come after completeResetPassword since those @@ -255,6 +269,7 @@ const CompleteResetPassword = ({ [ account, integration, + location.state?.accountResetToken, alertSuccessAndNavigate, ftlMsgResolver, setLinkStatus, diff --git a/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/interfaces.ts b/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/interfaces.ts index d91ac0ffbf..2b0ac6f23b 100644 --- a/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/interfaces.ts +++ b/packages/fxa-settings/src/pages/ResetPassword/CompleteResetPassword/interfaces.ts @@ -24,6 +24,7 @@ export type CompleteResetPasswordSubmitData = { export interface CompleteResetPasswordLocationState { lostRecoveryKey: boolean; + accountResetToken: string; } export interface CompleteResetPasswordParams { diff --git a/packages/fxa-settings/src/pages/mocks.ts b/packages/fxa-settings/src/pages/mocks.ts index c8e2ce5402..b63d8025e5 100644 --- a/packages/fxa-settings/src/pages/mocks.ts +++ b/packages/fxa-settings/src/pages/mocks.ts @@ -12,5 +12,6 @@ export const MOCK_SERVICE = MozServices.FirefoxMonitor; export const MOCK_SESSION_TOKEN = 'sessionToken'; export const MOCK_UNWRAP_BKEY = 'unwrapBKey'; export const MOCK_KEY_FETCH_TOKEN = 'keyFetchToken'; +export const MOCK_RESET_TOKEN = 'mockResetToken'; export const MOCK_AUTH_AT = 12345; export const MOCK_PASSWORD = 'notYourAveragePassW0Rd';