Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vbudhram merged 1 commit into from
Jun 6, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 100 additions & 65 deletions packages/functional-tests/tests/react-conversion/oauthPromptNone.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
});
});
});

Expand Down
10 changes: 9 additions & 1 deletion packages/fxa-settings/src/pages/Authorization/container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -148,6 +152,10 @@ const AuthorizationContainer = ({
return;
}

if (promptNoneCallCount.current > 0) {
Copy link
Contributor

@chenba chenba Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know why it's being called twice?

Copy link
Contributor Author

@vbudhram vbudhram Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't clear to me why it was being called twice, I suspect something was changing in the dependenices but couldn't narrow it down.

return;
}

if (isOAuthWebIntegration(integration) && integration.wantsPromptNone()) {
promptNoneHandler();
return;
Expand Down
2 changes: 2 additions & 0 deletions packages/fxa-settings/src/pages/Index/container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ const IndexContainer = ({
if (!isEmail(email)) {
throw AuthUiErrors.EMAIL_REQUIRED;
}

const { exists, hasLinkedAccount, hasPassword } =
await authClient.accountStatusByEmail(email, {
thirdPartyAuthStatus: true,
Expand Down Expand Up @@ -229,6 +230,7 @@ const IndexContainer = ({
suggestedEmail,
shouldTrySuggestedEmail,
integration.data.context,
integration
]);

useEffect(() => {
Expand Down
53 changes: 42 additions & 11 deletions packages/fxa-settings/src/pages/Signin/container.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down Expand Up @@ -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()]);

Expand All @@ -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();
Expand All @@ -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'),
});
});
});

Expand Down
4 changes: 2 additions & 2 deletions packages/fxa-settings/src/pages/Signin/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,8 @@ export interface RecoveryEmailStatusResponse {

export interface CachedSigninHandlerResponse {
data?: {
verificationMethod: VerificationMethods;
verificationReason: VerificationReasons;
verificationMethod?: VerificationMethods;
verificationReason?: VerificationReasons;
uid: hexstring;
} & RecoveryEmailStatusResponse;
error?: AuthUiError;
Expand Down
Loading