From 03e801726ed55e9c29b32c32ff4e51348302d1e5 Mon Sep 17 00:00:00 2001 From: Dan Schomburg Date: Wed, 10 Jun 2026 12:03:32 -0700 Subject: [PATCH] bug(settings): Fix extranious email being during cached signin flows. --- .../src/lib/passkeys/signin-flow.test.tsx | 4 + .../src/lib/passkeys/signin-flow.ts | 3 +- packages/fxa-settings/src/models/mocks.tsx | 2 + .../src/pages/Authorization/container.tsx | 7 +- .../PostVerify/SetPassword/container.tsx | 1 + .../ThirdPartyAuthCallback/index.test.tsx | 36 ++++++--- .../ThirdPartyAuthCallback/index.tsx | 2 + .../SigninPasskeyFallback/container.tsx | 1 + .../Signin/SigninPasswordlessCode/index.tsx | 1 + .../Signin/SigninRecoveryCode/index.test.tsx | 8 ++ .../pages/Signin/SigninRecoveryCode/index.tsx | 5 +- .../Signin/SigninRecoveryPhone/container.tsx | 1 + .../pages/Signin/SigninTokenCode/index.tsx | 4 + .../src/pages/Signin/SigninTotpCode/index.tsx | 4 +- .../src/pages/Signin/SigninTotpCode/mocks.tsx | 26 ++++--- .../pages/Signin/SigninUnblock/index.test.tsx | 8 ++ .../src/pages/Signin/SigninUnblock/index.tsx | 3 + .../Signin/components/SigninCached/index.tsx | 5 +- .../src/pages/Signin/container.test.tsx | 10 ++- .../src/pages/Signin/container.tsx | 4 +- .../fxa-settings/src/pages/Signin/index.tsx | 2 + .../src/pages/Signin/interfaces.ts | 2 + .../src/pages/Signin/utils.test.ts | 77 +++++++++++++++++++ .../fxa-settings/src/pages/Signin/utils.ts | 27 ++++--- 24 files changed, 196 insertions(+), 47 deletions(-) diff --git a/packages/fxa-settings/src/lib/passkeys/signin-flow.test.tsx b/packages/fxa-settings/src/lib/passkeys/signin-flow.test.tsx index 4c821d136a8..3ac9bedded3 100644 --- a/packages/fxa-settings/src/lib/passkeys/signin-flow.test.tsx +++ b/packages/fxa-settings/src/lib/passkeys/signin-flow.test.tsx @@ -128,11 +128,13 @@ const buildArgs = ( emails: [{ email: EMAIL, isPrimary: true, verified: true }], totp: { exists: false, verified: false }, }); + const sessionResendVerifyCode = jest.fn(); const authClient = { beginPasskeyAuthentication, completePasskeyAuthentication, account, + sessionResendVerifyCode } as jest.Mocked; const integration = { isSync: () => false, @@ -231,6 +233,7 @@ describe('usePasskeySignIn', () => { performNavigation: true, isPasskeySession: true, accountHasTotp: false, + authClient: args.authClient, }); expect(storeAccountData).toHaveBeenCalledWith({ email: EMAIL, @@ -281,6 +284,7 @@ describe('usePasskeySignIn', () => { performNavigation: true, isPasskeySession: true, accountHasTotp: false, + authClient: args.authClient, }); }); diff --git a/packages/fxa-settings/src/lib/passkeys/signin-flow.ts b/packages/fxa-settings/src/lib/passkeys/signin-flow.ts index 0cb055f864f..f6333008264 100644 --- a/packages/fxa-settings/src/lib/passkeys/signin-flow.ts +++ b/packages/fxa-settings/src/lib/passkeys/signin-flow.ts @@ -154,7 +154,7 @@ export type PasskeySignInIntegration = NavigationOptions['integration']; /** Pick<> so tests can pass minimal mocks without `as any`. */ export type PasskeySignInAuthClient = Pick< AuthClient, - 'beginPasskeyAuthentication' | 'completePasskeyAuthentication' | 'account' + 'beginPasskeyAuthentication' | 'completePasskeyAuthentication' | 'account' | 'sessionResendVerifyCode' >; /** @@ -373,6 +373,7 @@ export function usePasskeySignIn({ performNavigation: true, isPasskeySession: true, accountHasTotp, + authClient }); if (navError) { diff --git a/packages/fxa-settings/src/models/mocks.tsx b/packages/fxa-settings/src/models/mocks.tsx index be5df9aff72..cf00eb2d8ff 100644 --- a/packages/fxa-settings/src/models/mocks.tsx +++ b/packages/fxa-settings/src/models/mocks.tsx @@ -147,10 +147,12 @@ export function mockAuthClient() { if (typeof jest !== 'undefined') { return { sessionStatus: jest.fn().mockResolvedValue(mockSessionStatus), + sessionResendVerifyCode: jest.fn().mockResolvedValue(undefined), } as unknown as AuthClient; } else { return { sessionStatus: () => Promise.resolve(mockSessionStatus), + sessionResendVerifyCode: () => Promise.resolve(undefined), } as unknown as AuthClient; } } diff --git a/packages/fxa-settings/src/pages/Authorization/container.tsx b/packages/fxa-settings/src/pages/Authorization/container.tsx index a01e02a44b8..ea8cd461301 100644 --- a/packages/fxa-settings/src/pages/Authorization/container.tsx +++ b/packages/fxa-settings/src/pages/Authorization/container.tsx @@ -11,7 +11,6 @@ import { RelierAccount, useAuthClient, useConfig, - useSession, } from '../../models'; import { useCallback, useEffect, useState, useRef } from 'react'; @@ -58,7 +57,6 @@ const AuthorizationContainer = ({ const config = useConfig(); const location = useLocation(); const navigateWithQuery = useNavigateWithQuery(); - const session = useSession(); const { finishOAuthFlowHandler, oAuthDataError } = useFinishOAuthFlowHandler( authClient, integration @@ -76,12 +74,9 @@ const AuthorizationContainer = ({ relierAccount ); - const isOauthPromptNone = true; const { data, error } = await cachedSignIn( account?.sessionToken!, authClient, - session, - isOauthPromptNone ); if (error === AuthUiErrors.SESSION_EXPIRED) { @@ -122,6 +117,7 @@ const AuthorizationContainer = ({ redirectTo: integration.data.redirectTo, finishOAuthFlowHandler, queryParams: location.search, + authClient }; const { error: navError } = await handleNavigation(navigationOptions); @@ -156,7 +152,6 @@ const AuthorizationContainer = ({ integration, location.search, navigateWithQuery, - session, ]); useEffect(() => { diff --git a/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.tsx b/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.tsx index 802d1387490..28d53f8e63b 100644 --- a/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.tsx +++ b/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.tsx @@ -193,6 +193,7 @@ const SetPasswordContainer = ({ // users will see a "flash" of whatever page we navigate them to // before the client closes the view. See FXA-11944 performNavigation: !integration.isFirefoxMobileClient(), + authClient }; const { error } = await handleNavigation(navigationOptions); diff --git a/packages/fxa-settings/src/pages/PostVerify/ThirdPartyAuthCallback/index.test.tsx b/packages/fxa-settings/src/pages/PostVerify/ThirdPartyAuthCallback/index.test.tsx index 19f0303313b..612f90b2d29 100644 --- a/packages/fxa-settings/src/pages/PostVerify/ThirdPartyAuthCallback/index.test.tsx +++ b/packages/fxa-settings/src/pages/PostVerify/ThirdPartyAuthCallback/index.test.tsx @@ -12,7 +12,7 @@ import { renderWithLocalizationProvider } from 'fxa-react/lib/test-utils/localiz import ThirdPartyAuthCallback from '.'; import { AppContext } from '../../../models'; import { createAppContext, mockAppContext } from '../../../models/mocks'; -import { useAccount } from '../../../models'; +import { useAccount, useAuthClient } from '../../../models'; import { useFinishOAuthFlowHandler } from '../../../lib/oauth/hooks'; import { handleNavigation, @@ -23,17 +23,19 @@ import { MOCK_EMAIL, MOCK_SESSION_TOKEN } from '../../mocks'; import { LocationProvider } from '@reach/router'; import { GenericData } from '../../../lib/model-data'; -jest.mock('../../../models', () => ({ - ...jest.requireActual('../../../models'), - useClientInfoState: jest.fn(), - useProductInfoState: jest.fn(), - useAccount: jest.fn(), - useAuthClient: () => { - return { - checkTotpTokenExists: jest.fn().mockResolvedValue({ verified: true }), - }; - }, -})); +jest.mock('../../../models', () => { + const mockAuthClient = { + checkTotpTokenExists: jest.fn().mockResolvedValue({ verified: true }), + sessionResendVerifyCode: jest.fn().mockResolvedValue(undefined), + }; + return { + ...jest.requireActual('../../../models'), + useClientInfoState: jest.fn(), + useProductInfoState: jest.fn(), + useAccount: jest.fn(), + useAuthClient: () => mockAuthClient, + }; +}); jest.mock('@reach/router', () => ({ ...jest.requireActual('@reach/router'), @@ -165,6 +167,15 @@ describe('ThirdPartyAuthCallback component', () => { mockHandleNavigation = jest.fn().mockResolvedValue({ error: null }); (handleNavigation as jest.Mock).mockReturnValue(mockHandleNavigation); (ensureCanLinkAcountOrRedirect as jest.Mock).mockResolvedValue(true); + // useAuthClient returns a stable mock; restore its implementations here + // because afterEach's resetAllMocks() wipes them between tests. + const authClient = useAuthClient(); + (authClient.checkTotpTokenExists as jest.Mock).mockResolvedValue({ + verified: true, + }); + (authClient.sessionResendVerifyCode as jest.Mock).mockResolvedValue( + undefined + ); mockNavigateWithQuery.mockClear(); mockCurrentAccount(); }); @@ -262,6 +273,7 @@ describe('ThirdPartyAuthCallback component', () => { isSignInWithThirdPartyAuth: true, queryParams: '?', redirectTo: redirectTo, + authClient: useAuthClient(), signinData: { sessionToken: MOCK_SESSION_TOKEN, uid: '123', diff --git a/packages/fxa-settings/src/pages/PostVerify/ThirdPartyAuthCallback/index.tsx b/packages/fxa-settings/src/pages/PostVerify/ThirdPartyAuthCallback/index.tsx index d29abf3647f..ca52042b0d0 100644 --- a/packages/fxa-settings/src/pages/PostVerify/ThirdPartyAuthCallback/index.tsx +++ b/packages/fxa-settings/src/pages/PostVerify/ThirdPartyAuthCallback/index.tsx @@ -128,6 +128,7 @@ const ThirdPartyAuthCallback = ({ // user sets a password and keys are available (see SetPassword/container.tsx). handleFxaLogin: isFirefoxNonSync, handleFxaOAuthLogin: isFirefoxNonSync, + authClient }; const { error: navError } = await handleNavigation(navigationOptions); @@ -144,6 +145,7 @@ const ThirdPartyAuthCallback = ({ navigateWithQuery, webRedirectCheck, ftlMsgResolver, + authClient ] ); diff --git a/packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/container.tsx b/packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/container.tsx index 5eb2e67d6aa..2fda1837cfb 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/container.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/container.tsx @@ -149,6 +149,7 @@ const SigninPasskeyFallbackContainer = ({ queryParams: location.search, handleFxaLogin: true, handleFxaOAuthLogin: true, + authClient }); if (navError) { GleanMetrics.passkeyEnterPassword.submitFrontendError({ diff --git a/packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx b/packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx index 116c4d40622..13b8c7d4acc 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx @@ -292,6 +292,7 @@ const SigninPasswordlessCode = ({ integration.isFirefoxMobileClient() && isSessionVerified ), isPasswordlessOtpSignin: true, + authClient }; // For existing users signing into Sync (signin flow), show merge warning diff --git a/packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/index.test.tsx b/packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/index.test.tsx index 52cc54490e1..82f6021204f 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/index.test.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/index.test.tsx @@ -29,6 +29,14 @@ jest.mock('../../../lib/hooks/useNavigateWithQuery', () => ({ useNavigateWithQuery: () => mockNavigateWithQuery, })); +const mockSessionResendVerifyCode = jest.fn().mockResolvedValue(undefined); +jest.mock('../../../models', () => ({ + ...jest.requireActual('../../../models'), + useAuthClient: () => ({ + sessionResendVerifyCode: mockSessionResendVerifyCode, + }), +})); + jest.mock('../../../lib/glean', () => ({ __esModule: true, default: { diff --git a/packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/index.tsx b/packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/index.tsx index 89bed4a0da2..2c74def22f4 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/index.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninRecoveryCode/index.tsx @@ -5,7 +5,7 @@ import { useCallback, useEffect, useState } from 'react'; import { RouteComponentProps, useLocation } from '@reach/router'; import { FtlMsg } from 'fxa-react/lib/utils'; -import { isWebIntegration, useFtlMsgResolver } from '../../../models'; +import { isWebIntegration, useAuthClient, useFtlMsgResolver } from '../../../models'; import { BackupCodesImage } from '../../../components/images'; import LinkExternal from 'fxa-react/components/LinkExternal'; import FormVerifyCode, { @@ -55,6 +55,7 @@ const SigninRecoveryCode = ({ 'Backup authentication code required' ); const location = useLocation(); + const authClient = useAuthClient(); const webRedirectCheck = useWebRedirect(integration.data.redirectTo); @@ -109,6 +110,7 @@ const SigninRecoveryCode = ({ handleFxaLogin: true, handleFxaOAuthLogin: true, performNavigation: !integration.isFirefoxMobileClient(), + authClient }; const { error } = await handleNavigation(navigationOptions); @@ -129,6 +131,7 @@ const SigninRecoveryCode = ({ uid, unwrapBKey, ftlMsgResolver, + authClient ]); const localizedInvalidCodeError = getLocalizedErrorMessage( diff --git a/packages/fxa-settings/src/pages/Signin/SigninRecoveryPhone/container.tsx b/packages/fxa-settings/src/pages/Signin/SigninRecoveryPhone/container.tsx index 527ff91c97e..e592e12f771 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninRecoveryPhone/container.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninRecoveryPhone/container.tsx @@ -130,6 +130,7 @@ const SigninRecoveryPhoneContainer = ({ handleFxaLogin: true, handleFxaOAuthLogin: true, performNavigation: !integration.isFirefoxMobileClient(), + authClient }; await handleNavigation(navigationOptions); diff --git a/packages/fxa-settings/src/pages/Signin/SigninTokenCode/index.tsx b/packages/fxa-settings/src/pages/Signin/SigninTokenCode/index.tsx index 23f60d6e2e4..c6b62a048a0 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninTokenCode/index.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninTokenCode/index.tsx @@ -7,6 +7,7 @@ import { RouteComponentProps, useLocation } from '@reach/router'; import { FtlMsg } from 'fxa-react/lib/utils'; import { isWebIntegration, + useAuthClient, useFtlMsgResolver, useSession, } from '../../../models'; @@ -62,6 +63,7 @@ const SigninTokenCode = ({ const [resendCountdown, setResendCountdown] = useState(0); const { isThrottled, startThrottle } = useThrottle(); + const authClient = useAuthClient(); const webRedirectCheck = useWebRedirect(integration.data.redirectTo); const redirectTo = isWebIntegration(integration) && webRedirectCheck?.isValid @@ -186,6 +188,7 @@ const SigninTokenCode = ({ handleFxaLogin: false, handleFxaOAuthLogin: true, performNavigation: !integration.isFirefoxMobileClient(), + authClient }; await GleanMetrics.isDone(); @@ -228,6 +231,7 @@ const SigninTokenCode = ({ showInlineRecoveryKeySetup, onSessionVerified, startThrottle, + authClient ] ); diff --git a/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.tsx b/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.tsx index 7f1b6414109..1f0e1d07704 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.tsx @@ -5,7 +5,7 @@ import React, { useEffect, useState } from 'react'; import { Link, RouteComponentProps, useLocation } from '@reach/router'; import { FtlMsg } from 'fxa-react/lib/utils'; -import { useAlertBar, useFtlMsgResolver, useSession } from '../../../models'; +import { useAlertBar, useAuthClient, useFtlMsgResolver, useSession } from '../../../models'; import { logViewEvent } from '../../../lib/metrics'; import { MozServices } from '../../../lib/types'; import firefox from '../../../lib/channels/firefox'; @@ -58,6 +58,7 @@ export const SigninTotpCode = ({ const session = useSession(); const isSessionAALUpgrade = signinState.isSessionAALUpgrade; const alertBar = useAlertBar(); + const authClient = useAuthClient(); const [bannerError, setBannerError] = useState(''); @@ -176,6 +177,7 @@ export const SigninTotpCode = ({ handleFxaLogin: true, handleFxaOAuthLogin: true, performNavigation: !integration.isFirefoxMobileClient(), + authClient }; const { error: navError } = await handleNavigation(navigationOptions); diff --git a/packages/fxa-settings/src/pages/Signin/SigninTotpCode/mocks.tsx b/packages/fxa-settings/src/pages/Signin/SigninTotpCode/mocks.tsx index 88dd4835050..1f9752f278c 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninTotpCode/mocks.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninTotpCode/mocks.tsx @@ -5,10 +5,12 @@ import { LocationProvider } from '@reach/router'; import { OAuthNativeServices } from '@fxa/accounts/oauth'; import { + AppContext, IntegrationData, IntegrationType, RelierCmsInfo, } from '../../../models'; +import { mockAppContext } from '../../../models/mocks'; import { SigninTotpCode, SigninTotpCodeProps } from '.'; import { MOCK_EMAIL, @@ -100,16 +102,18 @@ export const Subject = ({ submitTotpCode = mockSubmitTotpCode, }: Partial) => { return ( - - - + + + + + ); }; diff --git a/packages/fxa-settings/src/pages/Signin/SigninUnblock/index.test.tsx b/packages/fxa-settings/src/pages/Signin/SigninUnblock/index.test.tsx index ec88f685825..61d50a58e94 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninUnblock/index.test.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninUnblock/index.test.tsx @@ -52,6 +52,14 @@ jest.mock('@reach/router', () => ({ navigate: jest.fn(), })); +const mockSessionResendVerifyCode = jest.fn().mockResolvedValue(undefined); +jest.mock('../../../models', () => ({ + ...jest.requireActual('../../../models'), + useAuthClient: () => ({ + sessionResendVerifyCode: mockSessionResendVerifyCode, + }), +})); + const email = MOCK_EMAIL; const hasLinkedAccount = false; const hasPassword = true; diff --git a/packages/fxa-settings/src/pages/Signin/SigninUnblock/index.tsx b/packages/fxa-settings/src/pages/Signin/SigninUnblock/index.tsx index 5e30509a80d..7d2ad7d878a 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninUnblock/index.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninUnblock/index.tsx @@ -17,6 +17,7 @@ import FormVerifyCode, { import { isWebIntegration, useAlertBar, + useAuthClient, useFtlMsgResolver, } from '../../../models'; import LinkExternal from 'fxa-react/components/LinkExternal'; @@ -59,6 +60,7 @@ export const SigninUnblock = ({ const ftlMsgResolver = useFtlMsgResolver(); const location = useLocation(); const navigateWithQuery = useNavigateWithQuery(); + const authClient = useAuthClient(); const webRedirectCheck = useWebRedirect(integration.data.redirectTo); const redirectTo = @@ -152,6 +154,7 @@ export const SigninUnblock = ({ performNavigation: !( integration.isFirefoxMobileClient() && isFullyVerified ), + authClient }; // If the web redirect is invalid, this shows an "Invalid redirect" message in alertBar diff --git a/packages/fxa-settings/src/pages/Signin/components/SigninCached/index.tsx b/packages/fxa-settings/src/pages/Signin/components/SigninCached/index.tsx index 2a2a26f1bd8..175d8c7be12 100644 --- a/packages/fxa-settings/src/pages/Signin/components/SigninCached/index.tsx +++ b/packages/fxa-settings/src/pages/Signin/components/SigninCached/index.tsx @@ -12,7 +12,7 @@ import CardHeader from '../../../../components/CardHeader'; import TermsPrivacyAgreement from '../../../../components/TermsPrivacyAgreement'; import { AuthUiErrors } from '../../../../lib/auth-errors/auth-errors'; import GleanMetrics from '../../../../lib/glean'; -import { useFtlMsgResolver, isWebIntegration } from '../../../../models'; +import { useFtlMsgResolver, isWebIntegration, useAuthClient } from '../../../../models'; import { SigninCachedProps } from '../../interfaces'; import { handleNavigation, ensureCanLinkAcountOrRedirect } from '../../utils'; import { useWebRedirect } from '../../../../lib/hooks/useWebRedirect'; @@ -43,6 +43,7 @@ const SigninCached = ({ setCurrentSplitLayout, onSessionExpired, }: SigninCachedProps & RouteComponentProps) => { + const authClient = useAuthClient(); const config = useConfig(); const location = useLocation(); const navigateWithQuery = useNavigateWithQuery(); @@ -153,6 +154,7 @@ const SigninCached = ({ handleFxaOAuthLogin: !isSync, // Redirect passwordless Sync users to set_password after session verification. isSignInWithThirdPartyAuth: isSync, + authClient }; const { error: navError } = await handleNavigation(navigationOptions); if (navError) { @@ -191,6 +193,7 @@ const SigninCached = ({ setLocalizedBannerError, hasPassword, onSessionExpired, + authClient ]); return ( diff --git a/packages/fxa-settings/src/pages/Signin/container.test.tsx b/packages/fxa-settings/src/pages/Signin/container.test.tsx index 53750390f85..4d4d314943f 100644 --- a/packages/fxa-settings/src/pages/Signin/container.test.tsx +++ b/packages/fxa-settings/src/pages/Signin/container.test.tsx @@ -1417,7 +1417,10 @@ describe('signin container', () => { expect(mockAuthClient.sessionStatus).toHaveBeenCalledWith( MOCK_SESSION_TOKEN ); - expect(mockSession.sendVerificationCode).toHaveBeenCalled(); + // cachedSignIn no longer sends the verification code; the code is now + // sent later in handleNavigation, just before navigating to the + // verification page. See FXA-12972. + expect(mockSession.sendVerificationCode).not.toHaveBeenCalled(); expect(handlerResult?.data?.verificationMethod).toEqual( VerificationMethods.EMAIL_OTP ); @@ -1452,7 +1455,10 @@ describe('signin container', () => { const handlerResult = await currentSigninProps?.cachedSigninHandler(MOCK_SESSION_TOKEN); - expect(mockSession.sendVerificationCode).toHaveBeenCalled(); + // cachedSignIn no longer sends the verification code; the code is now + // sent later in handleNavigation, just before navigating to the + // verification page. See FXA-12972. + expect(mockSession.sendVerificationCode).not.toHaveBeenCalled(); expect(handlerResult?.data?.verificationMethod).toEqual( VerificationMethods.EMAIL_OTP ); diff --git a/packages/fxa-settings/src/pages/Signin/container.tsx b/packages/fxa-settings/src/pages/Signin/container.tsx index 1fd36d105a2..d3a4a707887 100644 --- a/packages/fxa-settings/src/pages/Signin/container.tsx +++ b/packages/fxa-settings/src/pages/Signin/container.tsx @@ -623,8 +623,8 @@ const SigninContainer = ({ const cachedSigninHandler: CachedSigninHandler = useCallback( async (sessionToken: hexstring) => - cachedSignIn(sessionToken, authClient, session), - [authClient, session] + cachedSignIn(sessionToken, authClient), + [authClient] ); const sendUnblockEmailHandler = useCallback( diff --git a/packages/fxa-settings/src/pages/Signin/index.tsx b/packages/fxa-settings/src/pages/Signin/index.tsx index 08d7cee971b..5dfee4c9b11 100644 --- a/packages/fxa-settings/src/pages/Signin/index.tsx +++ b/packages/fxa-settings/src/pages/Signin/index.tsx @@ -170,6 +170,7 @@ const Signin = ({ integration.isFirefoxMobileClient() && isFullyVerified ), isServiceWithEmailVerification, + authClient }; const { error: navError } = await handleNavigation(navigationOptions); @@ -274,6 +275,7 @@ const Signin = ({ webRedirectCheck, sensitiveDataClient, isServiceWithEmailVerification, + authClient ] ); diff --git a/packages/fxa-settings/src/pages/Signin/interfaces.ts b/packages/fxa-settings/src/pages/Signin/interfaces.ts index 279fa2b8451..383078faaca 100644 --- a/packages/fxa-settings/src/pages/Signin/interfaces.ts +++ b/packages/fxa-settings/src/pages/Signin/interfaces.ts @@ -13,6 +13,7 @@ import { MozServices } from '../../lib/types'; import { Integration } from '../../models'; import { QueryParams } from '../..'; import { UseFxAStatusResult } from '../../lib/hooks/useFxAStatus'; +import AuthClient from 'fxa-auth-client/browser'; export interface AvatarResponse { account: { @@ -277,6 +278,7 @@ export interface NavigationOptions { // with accountHasTotp to drive the AAL2-RP TOTP redirect in utils.ts. isPasskeySession?: boolean; accountHasTotp?: boolean; + authClient: Pick } export interface OAuthSigninResult { diff --git a/packages/fxa-settings/src/pages/Signin/utils.test.ts b/packages/fxa-settings/src/pages/Signin/utils.test.ts index aac9cb587df..15778bedb50 100644 --- a/packages/fxa-settings/src/pages/Signin/utils.test.ts +++ b/packages/fxa-settings/src/pages/Signin/utils.test.ts @@ -310,6 +310,83 @@ describe('Signin utils', () => { }); }); + describe('email OTP resend before /signin_token_code', () => { + it('resends the email OTP code when navigating to /signin_token_code with an EMAIL_OTP verification method', async () => { + const sessionResendVerifyCode = jest.fn().mockResolvedValue({}); + const navigationOptions = createBaseNavigationOptions({ + signinData: { + ...createBaseNavigationOptions().signinData, + emailVerified: true, + sessionVerified: false, + verificationMethod: VerificationMethods.EMAIL_OTP, + verificationReason: VerificationReasons.SIGN_IN, + }, + isServiceWithEmailVerification: true, + integration: createMockSigninOAuthIntegration(), + authClient: { sessionResendVerifyCode }, + }); + + const result = await handleNavigation(navigationOptions); + + expect(result.error).toBeUndefined(); + expect(sessionResendVerifyCode).toHaveBeenCalledWith(MOCK_SESSION_TOKEN); + expect(navigateSpy).toHaveBeenCalledWith( + '/signin_token_code', + expect.any(Object) + ); + }); + + it('does not resend the email OTP code when the verification method is not EMAIL_OTP', async () => { + const sessionResendVerifyCode = jest.fn().mockResolvedValue({}); + const navigationOptions = createBaseNavigationOptions({ + signinData: { + ...createBaseNavigationOptions().signinData, + emailVerified: true, + sessionVerified: false, + verificationMethod: VerificationMethods.EMAIL, + verificationReason: VerificationReasons.SIGN_IN, + }, + isServiceWithEmailVerification: true, + integration: createMockSigninOAuthIntegration(), + authClient: { sessionResendVerifyCode }, + }); + + const result = await handleNavigation(navigationOptions); + + expect(result.error).toBeUndefined(); + expect(sessionResendVerifyCode).not.toHaveBeenCalled(); + expect(navigateSpy).toHaveBeenCalledWith( + '/signin_token_code', + expect.any(Object) + ); + }); + + it('does not resend the email OTP code when navigating to a page other than /signin_token_code', async () => { + const sessionResendVerifyCode = jest.fn().mockResolvedValue({}); + const navigationOptions = createBaseNavigationOptions({ + signinData: { + ...createBaseNavigationOptions().signinData, + emailVerified: false, + sessionVerified: false, + verificationMethod: VerificationMethods.EMAIL_OTP, + verificationReason: VerificationReasons.SIGN_IN, + }, + isServiceWithEmailVerification: true, + integration: createMockSigninOAuthIntegration(), + authClient: { sessionResendVerifyCode }, + }); + + const result = await handleNavigation(navigationOptions); + + expect(result.error).toBeUndefined(); + expect(sessionResendVerifyCode).not.toHaveBeenCalled(); + expect(navigateSpy).toHaveBeenCalledWith( + '/confirm_signup_code', + expect.any(Object) + ); + }); + }); + describe('third party auth with non-Sync services', () => { it('sends fxaOAuthLogin and navigates to settings', async () => { const fxaOAuthLoginSpy = jest.spyOn(firefox, 'fxaOAuthLogin'); diff --git a/packages/fxa-settings/src/pages/Signin/utils.ts b/packages/fxa-settings/src/pages/Signin/utils.ts index 7e1003e1967..4afc90a4884 100644 --- a/packages/fxa-settings/src/pages/Signin/utils.ts +++ b/packages/fxa-settings/src/pages/Signin/utils.ts @@ -8,7 +8,6 @@ import { NavigationOptions, SigninLocationState } from './interfaces'; import type { SetPasswordLocationState } from '../PostVerify/SetPassword/interfaces'; import { AuthUiError, AuthUiErrors } from '../../lib/auth-errors/auth-errors'; import { - useSession, isOAuthIntegration, isOAuthNativeIntegration, useAuthClient, @@ -138,18 +137,12 @@ export function getSyncNavigate( export const cachedSignIn = async ( sessionToken: string, authClient: ReturnType, - session: ReturnType, - isOauthPromptNone = false ) => { try { // retrieves the account's authentication methods only // authenticatorAssuranceLevel reflects account AAL // and does not reflect the token's AAL (==> not useful here) - const { - authenticationMethods, - }: { - authenticationMethods: AuthenticationMethods[]; - } = await authClient.accountProfile(sessionToken); + const { authenticationMethods } = await authClient.accountProfile(sessionToken); const totpIsActive = authenticationMethods.includes( AuthenticationMethods.OTP @@ -166,7 +159,8 @@ export const cachedSignIn = async ( if (!emailVerified) { verificationReason = VerificationReasons.SIGN_UP; verificationMethod = VerificationMethods.EMAIL_OTP; - !isOauthPromptNone && (await session.sendVerificationCode()); + // We should send the code prior to nav. That couples with an actual navigation action, where it belongs. + // !isOauthPromptNone && (await session.sendVerificationCode()); } else if (!sessionVerified) { // we are inferring verification method and verification reason here // ideally we would check the server directly to allow for more verification reasons @@ -174,8 +168,11 @@ export const cachedSignIn = async ( if (totpIsActive) { verificationMethod = VerificationMethods.TOTP_2FA; } else { + // Note when using this method, the expectation is that we will send the user + // a verification code, if and only if the navigation handler directs them to + // a verification code page. There are some RPs that require email_otp verification, + // so for these RPs, we can just pass the user straight through. verificationMethod = VerificationMethods.EMAIL_OTP; - !isOauthPromptNone && (await session.sendVerificationCode()); } } @@ -295,6 +292,16 @@ export async function handleNavigation(navigationOptions: NavigationOptions) { sendFxaLogin(navigationOptions); } + // If we are about to direct a user to /signin_totp_code, where they will be prompoted for a + // totp code and we know their session isn't fully verified then send them an otp code. + if ( + to?.includes('signin_token_code') && + navigationOptions.signinData.sessionToken && + navigationOptions.signinData.verificationMethod === VerificationMethods.EMAIL_OTP + ) { + await navigationOptions.authClient.sessionResendVerifyCode(navigationOptions.signinData.sessionToken); + } + if ( navigationOptions.signinData.verificationReason === VerificationReasons.SIGN_UP ||