fix(prompt): Support prompt=none with 2FA, don't send two authorization reqs

Author: vbudhram

packages/functional-tests/tests/react-conversion/oauthPromptNone.spec.ts

@@ -150,82 +150,111 @@ test.describe('severity-1 #smoke', () => {
       //Verify error message
       await expect(page.getByText('Unverified user or session')).toBeVisible();
     });
-  });
 
-  test.describe('oauth prompt none with emails', () => {
-    test('succeeds if login_hint same as logged in user', async ({
-      page,
-      target,
-      pages: { relier, settings, signin },
-      testAccountTracker,
-    }) => {
-      const { email } = await signInAccount(
-        target,
+    test.describe('with emails', () => {
+      test('succeeds if login_hint same as logged in user', async ({
         page,
-        settings,
-        signin,
-        testAccountTracker
-      );
+        target,
+        pages: { relier, settings, signin },
+        testAccountTracker,
+      }) => {
+        const { email } = await signInAccount(
+          target,
+          page,
+          settings,
+          signin,
+          testAccountTracker
+        );
+
+        const query = new URLSearchParams({
+          login_hint: email,
+          return_on_error: 'false',
+        });
+        await page.goto(`${target.relierUrl}/?${query.toString()}`);
+        await relier.signInPromptNone();
+
+        //Verify logged in to relier
+        expect(await relier.isLoggedIn()).toBe(true);
+      });
 
-      const query = new URLSearchParams({
-        login_hint: email,
-        return_on_error: 'false',
+      test('succeeds if no login_hint is provided', async ({
+        page,
+        target,
+        pages: { relier, settings, signin },
+        testAccountTracker,
+      }) => {
+        await signInAccount(target, page, settings, signin, testAccountTracker);
+
+        const query = new URLSearchParams({
+          return_on_error: 'false',
+        });
+        await page.goto(`${target.relierUrl}/?${query.toString()}`);
+        await relier.signInPromptNone();
+
+        //Verify logged in to relier
+        expect(await relier.isLoggedIn()).toBe(true);
       });
-      await page.goto(`${target.relierUrl}/?${query.toString()}`);
-      await relier.signInPromptNone();
 
-      //Verify logged in to relier
-      expect(await relier.isLoggedIn()).toBe(true);
+      test('fails if login_hint is different to logged in user', async ({
+        page,
+        target,
+        pages: { relier, settings, signin },
+        testAccountTracker,
+      }) => {
+        const loginHintAccount = testAccountTracker.generateAccountDetails();
+        await signInAccount(target, page, settings, signin, testAccountTracker);
+
+        const query = new URLSearchParams({
+          login_hint: loginHintAccount.email,
+          return_on_error: 'false',
+        });
+        await page.goto(`${target.relierUrl}/?${query.toString()}`);
+        await relier.signInPromptNone();
+
+        //Verify error message
+        await expect(
+          page.getByText('A different user is signed in')
+        ).toBeVisible();
+      });
     });
 
-    test('succeeds if no login_hint is provided', async ({
+    test('redirects if return_on_error=true and not signed in', async ({
       page,
       target,
       pages: { relier, settings, signin },
       testAccountTracker,
     }) => {
-      await signInAccount(target, page, settings, signin, testAccountTracker);
-
-      const query = new URLSearchParams({
-        return_on_error: 'false',
-      });
-      await page.goto(`${target.relierUrl}/?${query.toString()}`);
-      await relier.signInPromptNone();
-
-      //Verify logged in to relier
-      expect(await relier.isLoggedIn()).toBe(true);
+      {
+        const { email } = await signInAccount(
+          target,
+          page,
+          settings,
+          signin,
+          testAccountTracker
+        );
+
+        await settings.signOut();
+
+        const query = new URLSearchParams({
+          login_hint: email,
+          return_on_error: 'true',
+        });
+        await page.goto(`${target.relierUrl}/?${query.toString()}`);
+        await relier.signInPromptNone();
+
+        await page.waitForResponse(/api\/oauth\?error=login_required/);
+        // RP handled it by taking the user back to sign in
+        await page.waitForURL(/oauth\/signin/);
+        await expect(signin.passwordFormHeading).toBeVisible();
+      }
     });
 
-    test('fails if login_hint is different to logged in user', async ({
+    test('redirect to RP with prompt=none and 2FA setup', async ({
       page,
       target,
-      pages: { relier, settings, signin },
+      pages: { relier, settings, signin, totp, deleteAccount },
       testAccountTracker,
     }) => {
-      const loginHintAccount = testAccountTracker.generateAccountDetails();
-      await signInAccount(target, page, settings, signin, testAccountTracker);
-
-      const query = new URLSearchParams({
-        login_hint: loginHintAccount.email,
-        return_on_error: 'false',
-      });
-      await page.goto(`${target.relierUrl}/?${query.toString()}`);
-      await relier.signInPromptNone();
-
-      //Verify error message
-      await expect(
-        page.getByText('A different user is signed in')
-      ).toBeVisible();
-    });
-  });
-
-  test('redirects if return_on_error=true and not signed in', async ({
-    page,
-    target,
-    pages: { relier, settings, signin },
-    testAccountTracker,
-  }) => {
-    {
       const { email } = await signInAccount(
         target,
         page,
@@ -234,20 +263,26 @@ test.describe('severity-1 #smoke', () => {
         testAccountTracker
       );
 
-      await settings.signOut();
+      await settings.totp.addButton.click();
 
+      await totp.fillOutTotpForms();
+      await expect(settings.alertBar).toHaveText(
+        'Two-step authentication has been enabled'
+      );
+
+      // Keep user signed in with a verified TOTP session
       const query = new URLSearchParams({
         login_hint: email,
-        return_on_error: 'true',
       });
       await page.goto(`${target.relierUrl}/?${query.toString()}`);
       await relier.signInPromptNone();
 
-      await page.waitForResponse(/api\/oauth\?error=login_required/);
-      // RP handled it by taking the user back to sign in
-      await page.waitForURL(/oauth\/signin/);
-      await expect(signin.passwordFormHeading).toBeVisible();
-    }
+      //Verify logged in to relier
+      expect(await relier.isLoggedIn()).toBe(true);
+
+      await settings.goto();
+      await settings.disconnectTotp(); // Required before teardown
+    });
   });
 });
 

packages/fxa-settings/src/pages/Authorization/container.tsx

@@ -13,7 +13,7 @@ import {
   useSession,
 } from '../../models';
 import { cache } from '../../lib/cache';
-import { useCallback, useEffect, useState } from 'react';
+import { useCallback, useEffect, useState, useRef } from 'react';
 import { currentAccount } from '../../lib/cache';
 import { useFinishOAuthFlowHandler } from '../../lib/oauth/hooks';
 import OAuthDataError from '../../components/OAuthDataError';
@@ -61,11 +61,15 @@ const AuthorizationContainer = ({
     authClient,
     integration
   );
+  const promptNoneCallCount = useRef(0);
+
   if (oAuthDataError) {
     setOauthError(oAuthDataError);
   }
 
   const promptNoneHandler = useCallback(async () => {
+    promptNoneCallCount.current += 1;
+
     const account = currentAccount();
     const relierAccount = convertToRelierAccount(account, authClient);
 
@@ -148,6 +152,10 @@ const AuthorizationContainer = ({
       return;
     }
 
+    if (promptNoneCallCount.current > 0) {
+      return;
+    }
+
     if (isOAuthWebIntegration(integration) && integration.wantsPromptNone()) {
       promptNoneHandler();
       return;

packages/fxa-settings/src/pages/Index/container.tsx

@@ -149,6 +149,7 @@ const IndexContainer = ({
         if (!isEmail(email)) {
           throw AuthUiErrors.EMAIL_REQUIRED;
         }
+
         const { exists, hasLinkedAccount, hasPassword } =
           await authClient.accountStatusByEmail(email, {
             thirdPartyAuthStatus: true,
@@ -229,6 +230,7 @@ const IndexContainer = ({
     suggestedEmail,
     shouldTrySuggestedEmail,
     integration.data.context,
+    integration
   ]);
 
   useEffect(() => {

packages/fxa-settings/src/pages/Signin/container.test.tsx

@@ -62,6 +62,7 @@ import {
   OAuthQueryParams,
   SigninQueryParams,
 } from '../../models/pages/signin';
+import { OAuthError } from '../../lib/oauth';
 
 jest.mock('../../lib/channels/firefox', () => ({
   ...jest.requireActual('../../lib/channels/firefox'),
@@ -1045,6 +1046,16 @@ describe('signin container', () => {
     });
 
     it('runs handler, calls accountProfile and recoveryEmailStatus', async () => {
+      mockAuthClient.accountProfile = jest.fn().mockResolvedValue({
+        authenticationMethods: ['pwd', 'email'],
+        authenticatorAssuranceLevel: 1 // email verified session
+      });
+      mockAuthClient.recoveryEmailStatus = jest.fn().mockResolvedValue({
+        verified: false,
+        sessionVerified: false,
+        emailVerified: true,
+      });
+
       mockUseValidateModule();
       render([mockGqlAvatarUseQuery()]);
 
@@ -1068,20 +1079,21 @@ describe('signin container', () => {
         expect(handlerResult?.data?.verificationReason).toEqual(
           VerificationReasons.SIGN_IN
         );
-        expect(handlerResult?.data?.verified).toEqual(true);
-        expect(handlerResult?.data?.sessionVerified).toEqual(true);
+        expect(handlerResult?.data?.verified).toEqual(false);
+        expect(handlerResult?.data?.sessionVerified).toEqual(false);
         expect(handlerResult?.data?.emailVerified).toEqual(true);
       });
     });
 
-    it('returns TOTP_2FA verification method and SIGN_UP verification reason when expected', async () => {
+    it('does not return a verification reason with verified 2FA session', async () => {
       mockAuthClient.accountProfile = jest.fn().mockResolvedValue({
         authenticationMethods: ['pwd', 'email', 'otp'],
+        authenticatorAssuranceLevel: 2 // 2FA verified session
       });
       mockAuthClient.recoveryEmailStatus = jest.fn().mockResolvedValue({
         verified: true,
         sessionVerified: true,
-        emailVerified: false,
+        emailVerified: true,
       });
 
       mockUseValidateModule();
@@ -1091,13 +1103,32 @@ describe('signin container', () => {
         const handlerResult =
           await currentSigninProps?.cachedSigninHandler(MOCK_SESSION_TOKEN);
 
-        expect(handlerResult?.data?.verificationMethod).toEqual(
-          VerificationMethods.TOTP_2FA
-        );
-        expect(handlerResult?.data?.verificationReason).toEqual(
-          VerificationReasons.SIGN_UP
-        );
-        expect(handlerResult?.data?.emailVerified).toEqual(false);
+        expect(handlerResult?.data?.verificationMethod).toBeUndefined()
+        expect(handlerResult?.data?.verificationReason).toBeUndefined()
+        expect(handlerResult?.data?.emailVerified).toEqual(true);
+      });
+    });
+
+    it('throws if mismatch 2FA and assurance level', async () => {
+      mockAuthClient.accountProfile = jest.fn().mockResolvedValue({
+        authenticationMethods: ['pwd', 'email', 'otp'],
+        authenticatorAssuranceLevel: 1 // Session verified by email
+      });
+      mockAuthClient.recoveryEmailStatus = jest.fn().mockResolvedValue({
+        verified: true,
+        sessionVerified: true,
+        emailVerified: true,
+      });
+
+      mockUseValidateModule();
+      render([mockGqlAvatarUseQuery()]);
+
+      await waitFor(async () => {
+        const handlerResult =
+          await currentSigninProps?.cachedSigninHandler(MOCK_SESSION_TOKEN);
+        expect(handlerResult).toEqual({
+          error: new OAuthError('PROMPT_NONE_NOT_SIGNED_IN'),
+        });
       });
     });
 

packages/fxa-settings/src/pages/Signin/interfaces.ts

@@ -130,8 +130,8 @@ export interface RecoveryEmailStatusResponse {
 
 export interface CachedSigninHandlerResponse {
   data?: {
-    verificationMethod: VerificationMethods;
-    verificationReason: VerificationReasons;
+    verificationMethod?: VerificationMethods;
+    verificationReason?: VerificationReasons;
     uid: hexstring;
   } & RecoveryEmailStatusResponse;
   error?: AuthUiError;