Skip to content

Commit e8640d8

Browse files
committed
fix(settings): pass isPasswordlessFlow through handleNavigation to TOTP page
Because: - Passwordless users with TOTP signing in through non-Sync OAuth flows (e.g. Relay) hit a "Bad Request" error on the TOTP code page - The isPasswordlessFlow flag was not propagated through createSigninLocationState - Relay browser-service tests used Chromium instead of Firefox, causing flaky WebChannel failures under parallel execution This commit: - Propagates isPasswordlessFlow through createSigninLocationState in utils.ts - Guards set_password redirect in SigninTotpCode with integration.isSync() so non-Sync OAuth flows (Relay) complete normally via handleNavigation - Adds isResend tag to passwordless.sendCode.success statsd metric - Adds statsd test for resend_code with isResend: 'true' - Adds functional test for Relay OAuth + TOTP passwordless signin flow - Strengthens test assertions with concrete URL checks and explicit error throws - Switches Relay browser-service tests to syncOAuthBrowserPages (Firefox) Closes: FXA-13388
1 parent 6d88ad3 commit e8640d8

7 files changed

Lines changed: 142 additions & 10 deletions

File tree

packages/functional-tests/tests/passwordless/signinPasswordless.spec.ts

Lines changed: 85 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1085,7 +1085,7 @@ test.describe('severity-2', () => {
10851085
test.describe('Passwordless authentication - Browser Service (Relay)', () => {
10861086
test('passwordless signin via Relay OAuth flow', async ({
10871087
target,
1088-
pages: { page, signin, signinPasswordlessCode },
1088+
syncOAuthBrowserPages: { page, signin, signinPasswordlessCode },
10891089
testAccountTracker,
10901090
}) => {
10911091
const { email } = await testAccountTracker.signUpPasswordless();
@@ -1109,7 +1109,7 @@ test.describe('severity-2', () => {
11091109

11101110
test('passwordless signup via Relay OAuth flow - service allowed', async ({
11111111
target,
1112-
pages: { page, signin, signinPasswordlessCode },
1112+
syncOAuthBrowserPages: { page, signin, signinPasswordlessCode },
11131113
testAccountTracker,
11141114
}, { project }) => {
11151115
test.skip(
@@ -1137,6 +1137,89 @@ test.describe('severity-2', () => {
11371137
await expect(page).not.toHaveURL(/signin_passwordless_code/);
11381138
});
11391139

1140+
test('passwordless signin via Relay OAuth flow - account with 2FA proceeds to TOTP verification', async ({
1141+
target,
1142+
syncOAuthBrowserPages: { page, signin, signinPasswordlessCode, signinTotpCode },
1143+
testAccountTracker,
1144+
}) => {
1145+
// Create passwordless account and set up TOTP via API
1146+
const { email, sessionToken } =
1147+
await testAccountTracker.signUpPasswordless();
1148+
const account: any = testAccountTracker.accounts.find(
1149+
(a) => a.email === email
1150+
);
1151+
if (!account) {
1152+
throw new Error(
1153+
`Account for email ${email} not found in testAccountTracker.accounts`
1154+
);
1155+
}
1156+
const password = account.password;
1157+
1158+
const { secret } = await target.authClient.createTotpToken(
1159+
sessionToken,
1160+
{}
1161+
);
1162+
const totpCode = await getTotpCode(secret);
1163+
await target.authClient.verifyTotpSetupCode(sessionToken, totpCode);
1164+
await target.authClient.completeTotpSetup(sessionToken);
1165+
1166+
if (account) {
1167+
account.secret = secret;
1168+
account.sessionToken = sessionToken;
1169+
}
1170+
1171+
await signin.clearCache();
1172+
1173+
// Sign in via Relay OAuth flow
1174+
const params = new URLSearchParams(relayDesktopOAuthQueryParams);
1175+
params.set('force_passwordless', 'true');
1176+
await signin.goto('/authorization', params);
1177+
1178+
await signin.fillOutEmailFirstForm(email);
1179+
1180+
// Should redirect to passwordless code page
1181+
await page.waitForURL(/signin_passwordless_code/);
1182+
1183+
const passwordlessCode =
1184+
await target.emailClient.getPasswordlessSigninCode(email);
1185+
await signinPasswordlessCode.fillOutCodeForm(passwordlessCode);
1186+
1187+
// Should redirect to TOTP code entry page
1188+
await page.waitForURL(/signin_totp_code/);
1189+
1190+
const newTotpCode = await getTotpCode(secret);
1191+
await signinTotpCode.fillOutCodeForm(newTotpCode);
1192+
1193+
// Should complete OAuth flow and land on settings
1194+
await page.waitForURL(/\/settings/);
1195+
1196+
// Cleanup: set password so testAccountTracker can destroy the account
1197+
await target.authClient.passwordlessSendCode(email, {
1198+
clientId: 'dcdb5ae7add825d2',
1199+
});
1200+
const cleanupCode =
1201+
await target.emailClient.getPasswordlessSigninCode(email);
1202+
const cleanupResult = await target.authClient.passwordlessConfirmCode(
1203+
email,
1204+
cleanupCode,
1205+
{ clientId: 'dcdb5ae7add825d2' }
1206+
);
1207+
const cleanupTotpCode = await getTotpCode(secret);
1208+
await target.authClient.verifyTotpCode(
1209+
cleanupResult.sessionToken,
1210+
cleanupTotpCode
1211+
);
1212+
await target.authClient.createPassword(
1213+
cleanupResult.sessionToken,
1214+
email,
1215+
password
1216+
);
1217+
1218+
if (account) {
1219+
account.isPasswordless = false;
1220+
}
1221+
});
1222+
11401223
test('passwordless signup blocked for service not in allowedClientServices', async ({
11411224
target,
11421225
pages: { page, signin },

packages/fxa-auth-server/lib/routes/passwordless.spec.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1200,6 +1200,48 @@ describe('passwordless statsd metrics', () => {
12001200
expect(mockStatsd.increment.args[0][0]).toBe(
12011201
'passwordless.sendCode.success'
12021202
);
1203+
expect(mockStatsd.increment.args[0][1]).toMatchObject({
1204+
isResend: 'false',
1205+
});
1206+
});
1207+
});
1208+
1209+
it('should increment statsd counter when OTP is resent', () => {
1210+
mockDB.accountRecord = sinon.spy(() =>
1211+
Promise.resolve({
1212+
uid,
1213+
email: TEST_EMAIL,
1214+
verifierSetAt: 0,
1215+
emails: [{ email: TEST_EMAIL, isPrimary: true }],
1216+
})
1217+
);
1218+
1219+
routes = makeRoutes({
1220+
log: mockLog,
1221+
db: mockDB,
1222+
customs: mockCustoms,
1223+
statsd: mockStatsd,
1224+
config: {
1225+
passwordlessOtp: {
1226+
enabled: true,
1227+
ttl: 300,
1228+
digits: 6,
1229+
allowedClientServices: {
1230+
'test-client-id': { allowedServices: ['*'] },
1231+
},
1232+
},
1233+
},
1234+
});
1235+
route = getRoute(routes, '/account/passwordless/resend_code', 'POST');
1236+
1237+
return runTest(route, mockRequest, () => {
1238+
expect(mockStatsd.increment.callCount).toBe(1);
1239+
expect(mockStatsd.increment.args[0][0]).toBe(
1240+
'passwordless.sendCode.success'
1241+
);
1242+
expect(mockStatsd.increment.args[0][1]).toMatchObject({
1243+
isResend: 'true',
1244+
});
12031245
});
12041246
});
12051247

packages/fxa-auth-server/lib/routes/passwordless.ts

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ class PasswordlessHandler {
181181
request
182182
);
183183

184-
return this.generateAndSendOtp(request, email, account, isNewAccount);
184+
return this.generateAndSendOtp(request, email, account, isNewAccount, false);
185185
}
186186

187187
async confirmCode(request: AuthRequest) {
@@ -349,7 +349,7 @@ class PasswordlessHandler {
349349
const otpKey = account ? account.uid : email;
350350
await this.otpManager.delete(otpKey);
351351

352-
return this.generateAndSendOtp(request, email, account, isNewAccount);
352+
return this.generateAndSendOtp(request, email, account, isNewAccount, true);
353353
}
354354

355355
/**
@@ -361,7 +361,8 @@ class PasswordlessHandler {
361361
request: AuthRequest,
362362
email: string,
363363
account: any,
364-
isNewAccount: boolean
364+
isNewAccount: boolean,
365+
isResend: boolean
365366
) {
366367
const otpKey = account ? account.uid : email;
367368
const code = await this.otpManager.create(otpKey);
@@ -443,10 +444,10 @@ class PasswordlessHandler {
443444
});
444445
}
445446

446-
this.statsd.increment(
447-
'passwordless.sendCode.success',
448-
getClientServiceTags(request)
449-
);
447+
this.statsd.increment('passwordless.sendCode.success', {
448+
...getClientServiceTags(request),
449+
isResend: String(isResend),
450+
});
450451

451452
// Record security event
452453
await recordSecurityEvent('account.passwordless_login_otp_sent', {

packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,7 @@ const SigninPasswordlessCode = ({
236236
performNavigation: !(
237237
integration.isFirefoxMobileClient() && isSessionVerified
238238
),
239+
isPasswordlessFlow: true,
239240
};
240241

241242
// For existing users signing into Sync (signin flow), show merge warning

packages/fxa-settings/src/pages/Signin/SigninTotpCode/index.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,9 @@ export const SigninTotpCode = ({
137137
// Passwordless Sync flow: after TOTP, redirect to set_password
138138
// for key derivation. The session is now verified so
139139
// /password/create (which requires verifiedSessionToken) will work.
140-
if (signinState.isPasswordlessFlow) {
140+
// Only redirect to set_password for Sync integrations, non-Sync
141+
// OAuth flows (e.g. Relay) should continue through handleNavigation.
142+
if (signinState.isPasswordlessFlow && integration.isSync()) {
141143
navigateWithQuery('/post_verify/third_party_auth/set_password', {
142144
replace: true,
143145
state: {

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,7 @@ export interface NavigationOptions {
246246
// If false, skip actually navigating. Still sends web channel messages etc.
247247
performNavigation?: boolean;
248248
isServiceWithEmailVerification?: boolean;
249+
isPasswordlessFlow?: boolean;
249250
}
250251

251252
export interface OAuthSigninResult {

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,7 @@ const createSigninLocationState = (
367367
},
368368
showInlineRecoveryKeySetup,
369369
isSignInWithThirdPartyAuth,
370+
isPasswordlessFlow,
370371
origin,
371372
} = navigationOptions;
372373
return {
@@ -379,6 +380,7 @@ const createSigninLocationState = (
379380
verificationReason,
380381
showInlineRecoveryKeySetup,
381382
isSignInWithThirdPartyAuth,
383+
isPasswordlessFlow,
382384
origin,
383385
};
384386
};

0 commit comments

Comments
 (0)