Merge pull request #20098 from mozilla/worktree-FXA-13160

bug(settings): Abandoning sync sign flow can cause down stream problems

Author: dschom

packages/functional-tests/lib/targets/base.ts

@@ -13,6 +13,7 @@ export type Credentials = Awaited<ReturnType<AuthClient['signUp']>> & {
   password: string;
   secret?: string;
   sessionToken?: string;
+  verified?: boolean;
 };
 
 export abstract class BaseTarget {

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;
   }

packages/functional-tests/lib/targets/remote.ts

@@ -33,6 +33,7 @@ export abstract class RemoteTarget extends BaseTarget {
     return {
       email,
       password,
+      verified: preVerified === 'true',
       ...creds,
     };
   }

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', () => {

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,

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
           );

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

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()}`);
 

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 };

packages/fxa-settings/src/models/Session.ts

@@ -131,9 +131,10 @@ export class Session implements SessionData {
 
   async isValid(token: string): Promise<boolean> {
     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;