Skip to content

Commit f6d01d6

Browse files
vbudhramvbudhram
authored
Merge pull request #18985 from mozilla/fxa-fix-none
fix(prompt): Support prompt=none with 2FA, don't send two authorization reqs
2 parents 10481d9 + ce2b25d commit f6d01d6

6 files changed

Lines changed: 177 additions & 92 deletions

File tree

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

Lines changed: 100 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -150,82 +150,111 @@ test.describe('severity-1 #smoke', () => {
150150
//Verify error message
151151
await expect(page.getByText('Unverified user or session')).toBeVisible();
152152
});
153-
});
154153

155-
test.describe('oauth prompt none with emails', () => {
156-
test('succeeds if login_hint same as logged in user', async ({
157-
page,
158-
target,
159-
pages: { relier, settings, signin },
160-
testAccountTracker,
161-
}) => {
162-
const { email } = await signInAccount(
163-
target,
154+
test.describe('with emails', () => {
155+
test('succeeds if login_hint same as logged in user', async ({
164156
page,
165-
settings,
166-
signin,
167-
testAccountTracker
168-
);
157+
target,
158+
pages: { relier, settings, signin },
159+
testAccountTracker,
160+
}) => {
161+
const { email } = await signInAccount(
162+
target,
163+
page,
164+
settings,
165+
signin,
166+
testAccountTracker
167+
);
168+
169+
const query = new URLSearchParams({
170+
login_hint: email,
171+
return_on_error: 'false',
172+
});
173+
await page.goto(`${target.relierUrl}/?${query.toString()}`);
174+
await relier.signInPromptNone();
175+
176+
//Verify logged in to relier
177+
expect(await relier.isLoggedIn()).toBe(true);
178+
});
169179

170-
const query = new URLSearchParams({
171-
login_hint: email,
172-
return_on_error: 'false',
180+
test('succeeds if no login_hint is provided', async ({
181+
page,
182+
target,
183+
pages: { relier, settings, signin },
184+
testAccountTracker,
185+
}) => {
186+
await signInAccount(target, page, settings, signin, testAccountTracker);
187+
188+
const query = new URLSearchParams({
189+
return_on_error: 'false',
190+
});
191+
await page.goto(`${target.relierUrl}/?${query.toString()}`);
192+
await relier.signInPromptNone();
193+
194+
//Verify logged in to relier
195+
expect(await relier.isLoggedIn()).toBe(true);
173196
});
174-
await page.goto(`${target.relierUrl}/?${query.toString()}`);
175-
await relier.signInPromptNone();
176197

177-
//Verify logged in to relier
178-
expect(await relier.isLoggedIn()).toBe(true);
198+
test('fails if login_hint is different to logged in user', async ({
199+
page,
200+
target,
201+
pages: { relier, settings, signin },
202+
testAccountTracker,
203+
}) => {
204+
const loginHintAccount = testAccountTracker.generateAccountDetails();
205+
await signInAccount(target, page, settings, signin, testAccountTracker);
206+
207+
const query = new URLSearchParams({
208+
login_hint: loginHintAccount.email,
209+
return_on_error: 'false',
210+
});
211+
await page.goto(`${target.relierUrl}/?${query.toString()}`);
212+
await relier.signInPromptNone();
213+
214+
//Verify error message
215+
await expect(
216+
page.getByText('A different user is signed in')
217+
).toBeVisible();
218+
});
179219
});
180220

181-
test('succeeds if no login_hint is provided', async ({
221+
test('redirects if return_on_error=true and not signed in', async ({
182222
page,
183223
target,
184224
pages: { relier, settings, signin },
185225
testAccountTracker,
186226
}) => {
187-
await signInAccount(target, page, settings, signin, testAccountTracker);
188-
189-
const query = new URLSearchParams({
190-
return_on_error: 'false',
191-
});
192-
await page.goto(`${target.relierUrl}/?${query.toString()}`);
193-
await relier.signInPromptNone();
194-
195-
//Verify logged in to relier
196-
expect(await relier.isLoggedIn()).toBe(true);
227+
{
228+
const { email } = await signInAccount(
229+
target,
230+
page,
231+
settings,
232+
signin,
233+
testAccountTracker
234+
);
235+
236+
await settings.signOut();
237+
238+
const query = new URLSearchParams({
239+
login_hint: email,
240+
return_on_error: 'true',
241+
});
242+
await page.goto(`${target.relierUrl}/?${query.toString()}`);
243+
await relier.signInPromptNone();
244+
245+
await page.waitForResponse(/api\/oauth\?error=login_required/);
246+
// RP handled it by taking the user back to sign in
247+
await page.waitForURL(/oauth\/signin/);
248+
await expect(signin.passwordFormHeading).toBeVisible();
249+
}
197250
});
198251

199-
test('fails if login_hint is different to logged in user', async ({
252+
test('redirect to RP with prompt=none and 2FA setup', async ({
200253
page,
201254
target,
202-
pages: { relier, settings, signin },
255+
pages: { relier, settings, signin, totp, deleteAccount },
203256
testAccountTracker,
204257
}) => {
205-
const loginHintAccount = testAccountTracker.generateAccountDetails();
206-
await signInAccount(target, page, settings, signin, testAccountTracker);
207-
208-
const query = new URLSearchParams({
209-
login_hint: loginHintAccount.email,
210-
return_on_error: 'false',
211-
});
212-
await page.goto(`${target.relierUrl}/?${query.toString()}`);
213-
await relier.signInPromptNone();
214-
215-
//Verify error message
216-
await expect(
217-
page.getByText('A different user is signed in')
218-
).toBeVisible();
219-
});
220-
});
221-
222-
test('redirects if return_on_error=true and not signed in', async ({
223-
page,
224-
target,
225-
pages: { relier, settings, signin },
226-
testAccountTracker,
227-
}) => {
228-
{
229258
const { email } = await signInAccount(
230259
target,
231260
page,
@@ -234,20 +263,26 @@ test.describe('severity-1 #smoke', () => {
234263
testAccountTracker
235264
);
236265

237-
await settings.signOut();
266+
await settings.totp.addButton.click();
238267

268+
await totp.fillOutTotpForms();
269+
await expect(settings.alertBar).toHaveText(
270+
'Two-step authentication has been enabled'
271+
);
272+
273+
// Keep user signed in with a verified TOTP session
239274
const query = new URLSearchParams({
240275
login_hint: email,
241-
return_on_error: 'true',
242276
});
243277
await page.goto(`${target.relierUrl}/?${query.toString()}`);
244278
await relier.signInPromptNone();
245279

246-
await page.waitForResponse(/api\/oauth\?error=login_required/);
247-
// RP handled it by taking the user back to sign in
248-
await page.waitForURL(/oauth\/signin/);
249-
await expect(signin.passwordFormHeading).toBeVisible();
250-
}
280+
//Verify logged in to relier
281+
expect(await relier.isLoggedIn()).toBe(true);
282+
283+
await settings.goto();
284+
await settings.disconnectTotp(); // Required before teardown
285+
});
251286
});
252287
});
253288

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import {
1313
useSession,
1414
} from '../../models';
1515
import { cache } from '../../lib/cache';
16-
import { useCallback, useEffect, useState } from 'react';
16+
import { useCallback, useEffect, useState, useRef } from 'react';
1717
import { currentAccount } from '../../lib/cache';
1818
import { useFinishOAuthFlowHandler } from '../../lib/oauth/hooks';
1919
import OAuthDataError from '../../components/OAuthDataError';
@@ -61,11 +61,15 @@ const AuthorizationContainer = ({
6161
authClient,
6262
integration
6363
);
64+
const promptNoneCallCount = useRef(0);
65+
6466
if (oAuthDataError) {
6567
setOauthError(oAuthDataError);
6668
}
6769

6870
const promptNoneHandler = useCallback(async () => {
71+
promptNoneCallCount.current += 1;
72+
6973
const account = currentAccount();
7074
const relierAccount = convertToRelierAccount(account, authClient);
7175

@@ -148,6 +152,10 @@ const AuthorizationContainer = ({
148152
return;
149153
}
150154

155+
if (promptNoneCallCount.current > 0) {
156+
return;
157+
}
158+
151159
if (isOAuthWebIntegration(integration) && integration.wantsPromptNone()) {
152160
promptNoneHandler();
153161
return;

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@ const IndexContainer = ({
149149
if (!isEmail(email)) {
150150
throw AuthUiErrors.EMAIL_REQUIRED;
151151
}
152+
152153
const { exists, hasLinkedAccount, hasPassword } =
153154
await authClient.accountStatusByEmail(email, {
154155
thirdPartyAuthStatus: true,
@@ -229,6 +230,7 @@ const IndexContainer = ({
229230
suggestedEmail,
230231
shouldTrySuggestedEmail,
231232
integration.data.context,
233+
integration
232234
]);
233235

234236
useEffect(() => {

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

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ import {
6262
OAuthQueryParams,
6363
SigninQueryParams,
6464
} from '../../models/pages/signin';
65+
import { OAuthError } from '../../lib/oauth';
6566

6667
jest.mock('../../lib/channels/firefox', () => ({
6768
...jest.requireActual('../../lib/channels/firefox'),
@@ -1045,6 +1046,16 @@ describe('signin container', () => {
10451046
});
10461047

10471048
it('runs handler, calls accountProfile and recoveryEmailStatus', async () => {
1049+
mockAuthClient.accountProfile = jest.fn().mockResolvedValue({
1050+
authenticationMethods: ['pwd', 'email'],
1051+
authenticatorAssuranceLevel: 1 // email verified session
1052+
});
1053+
mockAuthClient.recoveryEmailStatus = jest.fn().mockResolvedValue({
1054+
verified: false,
1055+
sessionVerified: false,
1056+
emailVerified: true,
1057+
});
1058+
10481059
mockUseValidateModule();
10491060
render([mockGqlAvatarUseQuery()]);
10501061

@@ -1068,20 +1079,21 @@ describe('signin container', () => {
10681079
expect(handlerResult?.data?.verificationReason).toEqual(
10691080
VerificationReasons.SIGN_IN
10701081
);
1071-
expect(handlerResult?.data?.verified).toEqual(true);
1072-
expect(handlerResult?.data?.sessionVerified).toEqual(true);
1082+
expect(handlerResult?.data?.verified).toEqual(false);
1083+
expect(handlerResult?.data?.sessionVerified).toEqual(false);
10731084
expect(handlerResult?.data?.emailVerified).toEqual(true);
10741085
});
10751086
});
10761087

1077-
it('returns TOTP_2FA verification method and SIGN_UP verification reason when expected', async () => {
1088+
it('does not return a verification reason with verified 2FA session', async () => {
10781089
mockAuthClient.accountProfile = jest.fn().mockResolvedValue({
10791090
authenticationMethods: ['pwd', 'email', 'otp'],
1091+
authenticatorAssuranceLevel: 2 // 2FA verified session
10801092
});
10811093
mockAuthClient.recoveryEmailStatus = jest.fn().mockResolvedValue({
10821094
verified: true,
10831095
sessionVerified: true,
1084-
emailVerified: false,
1096+
emailVerified: true,
10851097
});
10861098

10871099
mockUseValidateModule();
@@ -1091,13 +1103,32 @@ describe('signin container', () => {
10911103
const handlerResult =
10921104
await currentSigninProps?.cachedSigninHandler(MOCK_SESSION_TOKEN);
10931105

1094-
expect(handlerResult?.data?.verificationMethod).toEqual(
1095-
VerificationMethods.TOTP_2FA
1096-
);
1097-
expect(handlerResult?.data?.verificationReason).toEqual(
1098-
VerificationReasons.SIGN_UP
1099-
);
1100-
expect(handlerResult?.data?.emailVerified).toEqual(false);
1106+
expect(handlerResult?.data?.verificationMethod).toBeUndefined()
1107+
expect(handlerResult?.data?.verificationReason).toBeUndefined()
1108+
expect(handlerResult?.data?.emailVerified).toEqual(true);
1109+
});
1110+
});
1111+
1112+
it('throws if mismatch 2FA and assurance level', async () => {
1113+
mockAuthClient.accountProfile = jest.fn().mockResolvedValue({
1114+
authenticationMethods: ['pwd', 'email', 'otp'],
1115+
authenticatorAssuranceLevel: 1 // Session verified by email
1116+
});
1117+
mockAuthClient.recoveryEmailStatus = jest.fn().mockResolvedValue({
1118+
verified: true,
1119+
sessionVerified: true,
1120+
emailVerified: true,
1121+
});
1122+
1123+
mockUseValidateModule();
1124+
render([mockGqlAvatarUseQuery()]);
1125+
1126+
await waitFor(async () => {
1127+
const handlerResult =
1128+
await currentSigninProps?.cachedSigninHandler(MOCK_SESSION_TOKEN);
1129+
expect(handlerResult).toEqual({
1130+
error: new OAuthError('PROMPT_NONE_NOT_SIGNED_IN'),
1131+
});
11011132
});
11021133
});
11031134

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,8 @@ export interface RecoveryEmailStatusResponse {
130130

131131
export interface CachedSigninHandlerResponse {
132132
data?: {
133-
verificationMethod: VerificationMethods;
134-
verificationReason: VerificationReasons;
133+
verificationMethod?: VerificationMethods;
134+
verificationReason?: VerificationReasons;
135135
uid: hexstring;
136136
} & RecoveryEmailStatusResponse;
137137
error?: AuthUiError;

0 commit comments

Comments
 (0)