From 62a7ce23a831fb95236e92946469be559fefc20b Mon Sep 17 00:00:00 2001 From: dschom Date: Tue, 24 Feb 2026 18:24:43 -0800 Subject: [PATCH] task(settings): Fix issues with login after previously abonded browser login Because: - If a user only partially loged in from a FF sign in (sync, smartwindow, etc..), then subsquent logins would break. This Commit: - Configures nightly with smart window on - Recreates issue with abadoning sync as functional test - Fixes bogus cached account state - Discard old session token in oauth recovery flow - Handle unverified session scenarios during oauth flow - Make sure functional test testing credentials include verified flag --- packages/functional-tests/lib/targets/base.ts | 1 + .../functional-tests/lib/targets/local.ts | 1 + .../functional-tests/lib/targets/remote.ts | 1 + .../tests/oauth/syncSignIn.spec.ts | 37 +++++++++++++++++++ packages/fxa-dev-launcher/profile.mjs | 8 ++++ .../fxa-settings/src/components/App/index.tsx | 8 +++- packages/fxa-settings/src/lib/cache.ts | 8 ++++ .../lib/hooks/useOAuthFlowRecovery/index.tsx | 10 ++++- packages/fxa-settings/src/lib/oauth/hooks.tsx | 4 ++ packages/fxa-settings/src/models/Session.ts | 7 ++-- .../fxa-settings/src/pages/Signin/utils.ts | 28 +++++++++++--- 11 files changed, 102 insertions(+), 11 deletions(-) diff --git a/packages/functional-tests/lib/targets/base.ts b/packages/functional-tests/lib/targets/base.ts index 901c8db3078..e04710a5dc4 100644 --- a/packages/functional-tests/lib/targets/base.ts +++ b/packages/functional-tests/lib/targets/base.ts @@ -13,6 +13,7 @@ export type Credentials = Awaited> & { password: string; secret?: string; sessionToken?: string; + verified?: boolean; }; export abstract class BaseTarget { diff --git a/packages/functional-tests/lib/targets/local.ts b/packages/functional-tests/lib/targets/local.ts index fb1d10f447e..d83e74fae8b 100644 --- a/packages/functional-tests/lib/targets/local.ts +++ b/packages/functional-tests/lib/targets/local.ts @@ -42,6 +42,7 @@ export class LocalTarget extends BaseTarget { return { email, password, + verified: options.preVerified === 'true', ...result, } as Credentials; } diff --git a/packages/functional-tests/lib/targets/remote.ts b/packages/functional-tests/lib/targets/remote.ts index 0dc8e717b6f..e56afdd1604 100644 --- a/packages/functional-tests/lib/targets/remote.ts +++ b/packages/functional-tests/lib/targets/remote.ts @@ -33,6 +33,7 @@ export abstract class RemoteTarget extends BaseTarget { return { email, password, + verified: preVerified === 'true', ...creds, }; } diff --git a/packages/functional-tests/tests/oauth/syncSignIn.spec.ts b/packages/functional-tests/tests/oauth/syncSignIn.spec.ts index 06be9623eae..eab9d39866f 100644 --- a/packages/functional-tests/tests/oauth/syncSignIn.spec.ts +++ b/packages/functional-tests/tests/oauth/syncSignIn.spec.ts @@ -59,6 +59,43 @@ test.describe('severity-1 #smoke', () => { await expect(signin.cachedSigninHeading).toBeVisible(); await expect(page.getByText(email)).toBeVisible(); }); + + test('can sign in to OAuth after abandoning sync confirmation code', async ({ + target, + syncBrowserPages: { page, signin, signinTokenCode, relier }, + testAccountTracker, + }) => { + const syncCredentials = await testAccountTracker.signUpSync(); + + // Start sign-into-sync flow + await page.goto( + `${target.contentServerUrl}?context=fx_desktop_v3&service=sync&action=email&` + ); + await signin.fillOutEmailFirstForm(syncCredentials.email); + await signin.fillOutPasswordForm(syncCredentials.password); + + // Confirm we reached the token code page, but intentionally skip entering the code + await expect(page).toHaveURL(/signin_token_code/); + + // Navigate to 123done without completing sync verification + await relier.goto(); + await relier.clickEmailFirst(); + + // FxA sees the existing session and shows cached account + await expect(signin.cachedSigninHeading).toBeVisible(); + await signin.signInButton.click(); + + // We get a signin code, because we are using a restmail address, and forces + // verification. ie Must verify will always be set on this client. + await expect(page).toHaveURL(/signin_token_code/); + const signinCode = await target.emailClient.getVerifyLoginCode( + syncCredentials.email + ); + await signinTokenCode.fillOutCodeForm(signinCode); + + // OAuth sign-in should succeed even though the sync session was not verified + expect(await relier.isLoggedIn()).toBe(true); + }); }); test.describe('signin to Sync after OAuth', () => { diff --git a/packages/fxa-dev-launcher/profile.mjs b/packages/fxa-dev-launcher/profile.mjs index 6ce110b04e5..0e1624bb0c3 100644 --- a/packages/fxa-dev-launcher/profile.mjs +++ b/packages/fxa-dev-launcher/profile.mjs @@ -46,6 +46,7 @@ const CONFIGS = { }, }; +const isNightly = /Nightly/gi.test(process.env.FIREFOX_BIN || ''); const env = process.env.FXA_ENV || 'local'; const FXA_DESKTOP_CONTEXT = process.env.FXA_DESKTOP_CONTEXT || 'oauth_webchannel_v1'; @@ -65,7 +66,14 @@ if (!fxaEnv) { }; } +const nightlyProfile = isNightly + ? { + 'browser.smartwindow.enabled': true, + } + : {}; + const fxaProfile = { + ...nightlyProfile, // enable debugger and toolbox 'devtools.chrome.enabled': true, 'devtools.debugger.remote-enabled': true, diff --git a/packages/fxa-settings/src/components/App/index.tsx b/packages/fxa-settings/src/components/App/index.tsx index d1ae72c4a38..861300ac0cd 100644 --- a/packages/fxa-settings/src/components/App/index.tsx +++ b/packages/fxa-settings/src/components/App/index.tsx @@ -221,7 +221,13 @@ export const App = ({ integration.data?.service || '' ); - if (userFromBrowser?.sessionToken) { + // Don't intialize session state from partially succesful firefox logins (ie sync, relay, smartwindow). + // This reprensents an abandonded login. Basically the user hasn't actually confirmed the session yet, so + // don't assume it's valid. + if ( + userFromBrowser?.sessionToken && + userFromBrowser.verified === true + ) { const isValidSession = await session.isValid( userFromBrowser.sessionToken ); diff --git a/packages/fxa-settings/src/lib/cache.ts b/packages/fxa-settings/src/lib/cache.ts index 572c2b466e1..677baecc2df 100644 --- a/packages/fxa-settings/src/lib/cache.ts +++ b/packages/fxa-settings/src/lib/cache.ts @@ -134,7 +134,15 @@ export function discardSessionToken() { const account = currentAccount(); if (account) { account.sessionToken = undefined; + account.verified = undefined; + account.sessionVerified = undefined; + account.hasPassword = undefined; + currentAccount(account); + + // If there was a state change, dispatch an event + dispatchStorageEvent('accounts'); + dispatchStorageEvent('currentAccountUid'); } } catch (e) { // noop diff --git a/packages/fxa-settings/src/lib/hooks/useOAuthFlowRecovery/index.tsx b/packages/fxa-settings/src/lib/hooks/useOAuthFlowRecovery/index.tsx index db449585196..a5509333723 100644 --- a/packages/fxa-settings/src/lib/hooks/useOAuthFlowRecovery/index.tsx +++ b/packages/fxa-settings/src/lib/hooks/useOAuthFlowRecovery/index.tsx @@ -10,6 +10,7 @@ import { } from '../../../models'; import firefox from '../../channels/firefox'; import { hardNavigate } from 'fxa-react/lib/utils'; +import { discardSessionToken } from '../../cache'; export type OAuthFlowRecoveryResult = { isRecovering: boolean; @@ -67,9 +68,16 @@ export function useOAuthFlowRecovery( currentParams.set('code_challenge', params.code_challenge); } if (params.code_challenge_method) { - currentParams.set('code_challenge_method', params.code_challenge_method); + currentParams.set( + 'code_challenge_method', + params.code_challenge_method + ); } + // Discard the current session token. We need to do this, otherwise the cached session will be used + // and the UI will show an option to enter a password. This can lead to an infinite sign in loop. + discardSessionToken(); + // Redirect to /signin - user will re-enter password hardNavigate(`/signin?${currentParams.toString()}`); diff --git a/packages/fxa-settings/src/lib/oauth/hooks.tsx b/packages/fxa-settings/src/lib/oauth/hooks.tsx index b8ad2d014ea..4e8ceb534e7 100644 --- a/packages/fxa-settings/src/lib/oauth/hooks.tsx +++ b/packages/fxa-settings/src/lib/oauth/hooks.tsx @@ -231,7 +231,11 @@ export function useFinishOAuthFlowHandler( } catch (error) { // We only care about these errors, else just tell the user to try again. if ( + // Signals that session requires verification, ie a mustVerify state + error.errno === AuthUiErrors.UNVERIFIED_SESSION.errno || + // Signals that session requires totp verification error.errno === AuthUiErrors.TOTP_REQUIRED.errno || + // Signals that we are dealing with an RP that requires a second factor error.errno === AuthUiErrors.INSUFFICIENT_ACR_VALUES.errno ) { return { error }; diff --git a/packages/fxa-settings/src/models/Session.ts b/packages/fxa-settings/src/models/Session.ts index e7f7499972c..6f687808a46 100644 --- a/packages/fxa-settings/src/models/Session.ts +++ b/packages/fxa-settings/src/models/Session.ts @@ -131,9 +131,10 @@ export class Session implements SessionData { async isValid(token: string): Promise { try { - await this.authClient.sessionStatus(token); - setSessionVerified(true); - return true; + const resp = await this.authClient.sessionStatus(token); + const isVerified = resp.state === 'verified'; + setSessionVerified(isVerified); + return isVerified; } catch (e) { setSessionVerified(false); return false; diff --git a/packages/fxa-settings/src/pages/Signin/utils.ts b/packages/fxa-settings/src/pages/Signin/utils.ts index 72ca100a993..4e5f7211693 100644 --- a/packages/fxa-settings/src/pages/Signin/utils.ts +++ b/packages/fxa-settings/src/pages/Signin/utils.ts @@ -513,17 +513,33 @@ const getOAuthNavigationTarget = async ( ); if (error) { if ( + error.errno === AuthUiErrors.UNVERIFIED_SESSION.errno || error.errno === AuthUiErrors.TOTP_REQUIRED.errno || error.errno === AuthUiErrors.INSUFFICIENT_ACR_VALUES.errno ) { GleanMetrics.login.error({ event: { reason: error.message } }); - // If user already has TOTP enabled, send them to enter their code instead of setup - const hasTotp = - locationState.verificationMethod === VerificationMethods.TOTP_2FA; + + const to = (() => { + // If the user doesn't have totp, and encountered an unverified_session error, send them + // to signin_token_code, this is a 'mustVerify' case + if ( + locationState.verificationMethod !== VerificationMethods.TOTP_2FA && + error.errno === AuthUiErrors.UNVERIFIED_SESSION.errno + ) { + return `/signin_token_code${navigationOptions.queryParams || ''}`; + } + + // If user already has TOTP enabled, send them to enter their code instead of setup + if (locationState.verificationMethod === VerificationMethods.TOTP_2FA) { + return `/signin_totp_code${navigationOptions.queryParams || ''}`; + } + + // Otherwise, we are dealing with an RP that requires totp setup. Send them there. + return `/inline_totp_setup${navigationOptions.queryParams || ''}`; + })(); + return { - to: hasTotp - ? `/signin_totp_code${navigationOptions.queryParams || ''}` - : `/inline_totp_setup${navigationOptions.queryParams || ''}`, + to, locationState, }; }