From 246f2585b390cc7f58f31fa643d5ca1bf4b515fd Mon Sep 17 00:00:00 2001 From: Valerie Pomerleau Date: Tue, 9 Jun 2026 12:44:54 -0700 Subject: [PATCH] fix(settings): skip redundant account-status check on set-password forward flows Because: - The post-verify set-password page blocked first paint on an accountStatus round-trip, intermittently exceeding the stage smoke test's 10s assertion and flaking passkeySetPassword.spec.ts. - The page only ever serves passwordless accounts, and the forward flows already know that, so the round-trip is redundant for them. This commit: - Trusts the account's stored password state instead of the blocking round-trip: renders for a passwordless account, redirects to /signin when one exists, and falls back to a one-time accountStatus check only when it is unknown (third-party sign-in, or a stale/direct arrival). - Records hasPassword in the flows that establish it as fact: passwordless OTP with or without TOTP (passkey already did). Third-party defers to the check. - Replaces the set-password history entry on completion so Back cannot return to it; flattens the check effect and drops the stale retry TODO and comment. - Adds container, handleNavigation, and SigninTotpCode tests. Closes #FXA-13937 --- .../PostVerify/SetPassword/container.test.tsx | 49 ++++++++++++++++- .../PostVerify/SetPassword/container.tsx | 55 +++++++------------ .../Signin/SigninPasswordlessCode/index.tsx | 1 + .../Signin/SigninTotpCode/index.test.tsx | 31 +++++++++++ .../src/pages/Signin/SigninTotpCode/index.tsx | 1 + .../src/pages/Signin/utils.test.ts | 42 ++++++++++++++ .../fxa-settings/src/pages/Signin/utils.ts | 7 ++- 7 files changed, 149 insertions(+), 37 deletions(-) diff --git a/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.test.tsx b/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.test.tsx index b8edddd4ce6..e90ac011bc3 100644 --- a/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.test.tsx +++ b/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.test.tsx @@ -5,6 +5,7 @@ import * as ModelsModule from '../../../models'; import * as CacheModule from '../../../lib/cache'; import * as SetPasswordModule from '.'; +import { StoredAccountData } from '../../../lib/storage-utils'; import AuthClient from 'fxa-auth-client/browser'; import { @@ -106,7 +107,7 @@ function mockModelsModule() { } // Call this when testing local storage function mockCurrentAccount( - storedAccount = { + storedAccount: StoredAccountData = { uid: MOCK_UID, sessionToken: MOCK_SESSION_TOKEN, email: MOCK_EMAIL, @@ -238,6 +239,52 @@ describe('SetPassword-container', () => { expect(SetPasswordModule.default).not.toHaveBeenCalled(); }); + describe('password state gating', () => { + it('skips the check and renders when stored state is passwordless', async () => { + mockCurrentAccount({ ...MOCK_STORED_ACCOUNT, hasPassword: false }); + render(); + await waitFor(() => { + expect(SetPasswordModule.default).toHaveBeenCalled(); + }); + expect(mockAuthClient.accountStatus).not.toHaveBeenCalled(); + expect(mockNavigate).not.toHaveBeenCalled(); + }); + + it('skips the check and redirects to signin when stored state has a password', async () => { + mockCurrentAccount({ ...MOCK_STORED_ACCOUNT, hasPassword: true }); + render(); + await waitFor(() => { + expect(mockNavigate).toHaveBeenCalledWith('/signin', { replace: true }); + }); + expect(mockAuthClient.accountStatus).not.toHaveBeenCalled(); + expect(SetPasswordModule.default).not.toHaveBeenCalled(); + }); + + it('falls back to the accountStatus check when stored state is unknown', async () => { + mockCurrentAccount({ ...MOCK_STORED_ACCOUNT, hasPassword: undefined }); + render(); + await waitFor(() => { + expect(SetPasswordModule.default).toHaveBeenCalled(); + }); + expect(mockAuthClient.accountStatus).toHaveBeenCalledWith( + undefined, + MOCK_SESSION_TOKEN + ); + }); + + it('treats a failed fallback check as no password and renders', async () => { + mockCurrentAccount({ ...MOCK_STORED_ACCOUNT, hasPassword: undefined }); + mockAuthClient.accountStatus = jest + .fn() + .mockRejectedValue(new Error('network failure')); + render(); + await waitFor(() => { + expect(SetPasswordModule.default).toHaveBeenCalled(); + }); + expect(mockNavigate).not.toHaveBeenCalled(); + }); + }); + describe('calling createPassword', () => { let fxaLoginSpy: jest.SpyInstance; let fxaOAuthLoginSpy: jest.SpyInstance; diff --git a/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.tsx b/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.tsx index 802d1387490..671b80c2ba2 100644 --- a/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.tsx +++ b/packages/fxa-settings/src/pages/PostVerify/SetPassword/container.tsx @@ -67,45 +67,33 @@ const SetPasswordContainer = ({ flowQueryParams as unknown as Record ); + // Trust the stored password state; fall back to a one-time accountStatus + // check only when it's unknown (e.g. third-party sign-in, stale/direct hit). + const knownHasPassword = storedLocalAccount?.hasPassword; + const passwordStateKnown = knownHasPassword !== undefined; + const [passwordStatus, setPasswordStatus] = useState<{ isLoading: boolean; hasPassword: boolean; }>({ - isLoading: true, - hasPassword: false, + isLoading: !passwordStateKnown, + hasPassword: knownHasPassword ?? false, }); - const didRunPasswordStatusCheckRef = useRef(false); + const didCheckRef = useRef(false); useEffect(() => { - if (didRunPasswordStatusCheckRef.current) return; - didRunPasswordStatusCheckRef.current = true; - const checkPasswordStatus = async () => { - if (sessionToken) { - try { - const status = await authClient.accountStatus( - undefined, - sessionToken - ); - setPasswordStatus({ - isLoading: false, - hasPassword: !!status.hasPassword, - }); - } catch (error) { - // TODO? Might need to retry. - // Request to create a PW won't go through if they already have one. - setPasswordStatus({ - isLoading: false, - hasPassword: false, - }); - } - } else { + if (passwordStateKnown || !sessionToken || didCheckRef.current) return; + didCheckRef.current = true; + authClient + .accountStatus(undefined, sessionToken) + .then((status) => setPasswordStatus({ isLoading: false, - hasPassword: false, - }); - } - }; - checkPasswordStatus(); - }, [authClient, sessionToken]); + hasPassword: !!status.hasPassword, + }) + ) + // On failure, treat as no password; the server rejects if one exists. + .catch(() => setPasswordStatus({ isLoading: false, hasPassword: false })); + }, [authClient, sessionToken, passwordStateKnown]); const { finishOAuthFlowHandler, oAuthDataError } = useFinishOAuthFlowHandler( authClient, @@ -234,10 +222,7 @@ const SetPasswordContainer = ({ return ; } - // User already has a password, redirect to signin. - // This can happen when a user signs into the browser with third party - // oauth and they try to use Sync or Send Tab later, but keys are missing. - // Firefox will send users to this page to set a password and receive keys. + // Already has a password (re-entry): sign in instead. if (passwordStatus.hasPassword) { navigateWithQuery('/signin', { replace: true }); return ; diff --git a/packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx b/packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx index 0c136806aef..9eb46bfacab 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx @@ -258,6 +258,7 @@ const SigninPasswordlessCode = ({ // Update verification status of stored current account verified: isSessionVerified, sessionVerified: isSessionVerified, + hasPassword: false, }); const navigationOptions = { diff --git a/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.test.tsx b/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.test.tsx index 1c32aa38105..d8a4ef77f2b 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.test.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.test.tsx @@ -37,6 +37,7 @@ import { navigate } from '@reach/router'; import { OAUTH_ERRORS } from '../../../lib/oauth'; import userEvent from '@testing-library/user-event'; import * as SigninUtils from '../utils'; +import * as StorageUtils from '../../../lib/storage-utils'; jest.mock('../../../lib/metrics', () => ({ usePageViewEvent: jest.fn(), @@ -414,4 +415,34 @@ describe('Sign in with TOTP code page', () => { }); }); }); + + describe('recording password state for set-password', () => { + it('records hasPassword: false on a passwordless OTP sign-in', async () => { + const user = userEvent.setup(); + jest.spyOn(SigninUtils, 'handleNavigation').mockResolvedValue({ + error: undefined, + }); + const storeAccountDataSpy = jest + .spyOn(StorageUtils, 'storeAccountData') + .mockImplementation(() => {}); + const submitTotpCode = jest.fn().mockResolvedValue({ error: undefined }); + renderWithLocalizationProvider( + + ); + await user.type(screen.getByLabelText('Enter 6-digit code'), '123456'); + await user.click(screen.getByRole('button', { name: 'Confirm' })); + + await waitFor(() => { + expect(storeAccountDataSpy).toHaveBeenCalledWith( + expect.objectContaining({ hasPassword: false }) + ); + }); + }); + }); }); diff --git a/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.tsx b/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.tsx index 7f1b6414109..007b273d667 100644 --- a/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.tsx +++ b/packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.tsx @@ -132,6 +132,7 @@ export const SigninTotpCode = ({ // Update verification status of stored current account verified: true, sessionVerified: true, + ...(signinState.isPasswordlessOtpSignin && { hasPassword: false }), }); // Passwordless Sync flow: after TOTP, redirect to set_password diff --git a/packages/fxa-settings/src/pages/Signin/utils.test.ts b/packages/fxa-settings/src/pages/Signin/utils.test.ts index aac9cb587df..2815b23bc3f 100644 --- a/packages/fxa-settings/src/pages/Signin/utils.test.ts +++ b/packages/fxa-settings/src/pages/Signin/utils.test.ts @@ -16,6 +16,7 @@ import { createMockSigninOAuthNativeSyncIntegration, createMockSigninOAuthNativeIntegration, createMockSigninOAuthIntegration, + createMockSigninWebIntegration, } from './mocks'; import { handleNavigation, @@ -137,6 +138,47 @@ describe('Signin utils', () => { expect(fxaLoginSpy).not.toHaveBeenCalled(); }); + it('replaces history on a set-password completion so Back cannot return to it', async () => { + const navigationOptions = createBaseNavigationOptions({ + integration: { + ...createMockSigninWebIntegration(), + isSync: () => true, + }, + queryParams: '?service=sync', + showSignupConfirmedSync: true, + origin: 'post-verify-set-password', + performNavigation: true, + handleFxaLogin: true, + }); + + await handleNavigation(navigationOptions); + + expect(navigateSpy).toHaveBeenCalledWith( + expect.stringContaining('/signup_confirmed_sync'), + expect.objectContaining({ replace: true }) + ); + }); + + it('does not replace history for a Sync sign-in that is not a set-password completion', async () => { + const navigationOptions = createBaseNavigationOptions({ + integration: { + ...createMockSigninWebIntegration(), + isSync: () => true, + }, + queryParams: '?service=sync', + showSignupConfirmedSync: true, + performNavigation: true, + handleFxaLogin: true, + }); + + await handleNavigation(navigationOptions); + + expect(navigateSpy).toHaveBeenCalledWith( + expect.stringContaining('/signup_confirmed_sync'), + expect.objectContaining({ replace: false }) + ); + }); + describe('unverified session navigation', () => { it('returns early for SIGN_UP verification reason', async () => { const navigationOptions = createBaseNavigationOptions({ diff --git a/packages/fxa-settings/src/pages/Signin/utils.ts b/packages/fxa-settings/src/pages/Signin/utils.ts index 7e1003e1967..8fa15706522 100644 --- a/packages/fxa-settings/src/pages/Signin/utils.ts +++ b/packages/fxa-settings/src/pages/Signin/utils.ts @@ -355,7 +355,12 @@ export async function handleNavigation(navigationOptions: NavigationOptions) { if (navigationOptions.performNavigation !== false) { const { to, locationState, shouldHardNavigate } = await getNonOAuthNavigationTarget(navigationOptions); - performNavigation({ to, locationState, shouldHardNavigate }); + performNavigation({ + to, + locationState, + shouldHardNavigate, + replace: navigationOptions.origin === 'post-verify-set-password', + }); } return { error: undefined }; }