Skip to content

Commit 74aa759

Browse files
vbudhramvbudhram
authored
Merge pull request #20195 from mozilla/fix-passwordless
fix(fxa-settings): skip passwordless OTP redirect for cached sessions
2 parents 4b56ba0 + df7d139 commit 74aa759

6 files changed

Lines changed: 214 additions & 19 deletions

File tree

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

Lines changed: 84 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

55
import { expect, test } from '../../lib/fixtures/standard';
6-
import { syncDesktopOAuthQueryParams } from '../../lib/query-params';
6+
import { relayDesktopOAuthQueryParams, syncDesktopOAuthQueryParams } from '../../lib/query-params';
77
import { getTotpCode } from '../../lib/totp';
88

99
test.describe('severity-1 #smoke', () => {
@@ -730,42 +730,88 @@ test.describe('severity-1 #smoke', () => {
730730

731731
test.describe('severity-2', () => {
732732
test.describe('Passwordless authentication - Cached login', () => {
733-
test('passwordless account navigating to content server redirects to OTP (not cached login)', async ({
733+
test('passwordless account with cached session navigating to content server sees cached signin', async ({
734734
target,
735-
pages: { page, signin, relier, signinPasswordlessCode, settings },
735+
pages: { page, signin, relier, signinPasswordlessCode },
736736
testAccountTracker,
737737
}) => {
738-
// Create passwordless account via 123done
739738
const { email } =
740739
testAccountTracker.generatePasswordlessAccountDetails();
741740

742741
await relier.goto('force_passwordless=true');
743742
await relier.clickEmailFirst();
744743
await signin.fillOutEmailFirstForm(email);
745744

746-
// Should redirect to passwordless code page
747745
await expect(page).toHaveURL(/signin_passwordless_code/);
748746

749-
// Get OTP code from email
750747
const code = await target.emailClient.getPasswordlessSignupCode(email);
751748
await signinPasswordlessCode.fillOutCodeForm(code);
752749

753-
// Should complete OAuth and redirect to RP
754750
expect(await relier.isLoggedIn()).toBe(true);
755751

756-
// Navigate to content server — passwordless accounts are always
757-
// redirected to OTP verification (container.tsx checks hasPassword=false
758-
// && passwordlessSupported=true and redirects before cached UI renders)
752+
// Navigate to / — cached session should show signin, not OTP
759753
await page.goto(target.contentServerUrl);
760754

755+
await expect(page).not.toHaveURL(/signin_passwordless_code/);
756+
await expect(signin.cachedSigninHeading).toBeVisible();
757+
});
758+
759+
test('passwordless account with cached session navigating to /signin sees cached signin page', async ({
760+
target,
761+
pages: { page, signin, relier, signinPasswordlessCode },
762+
testAccountTracker,
763+
}) => {
764+
const { email } =
765+
testAccountTracker.generatePasswordlessAccountDetails();
766+
767+
await relier.goto('force_passwordless=true');
768+
await relier.clickEmailFirst();
769+
await signin.fillOutEmailFirstForm(email);
770+
761771
await expect(page).toHaveURL(/signin_passwordless_code/);
762-
await expect(signinPasswordlessCode.heading).toBeVisible();
772+
const code = await target.emailClient.getPasswordlessSignupCode(email);
773+
await signinPasswordlessCode.fillOutCodeForm(code);
763774

764-
// Complete OTP flow to reach settings
765-
const signinCode = await target.emailClient.getPasswordlessSigninCode(email);
766-
await signinPasswordlessCode.fillOutCodeForm(signinCode);
775+
expect(await relier.isLoggedIn()).toBe(true);
776+
777+
// Navigate to / — cached session should show signin, not OTP
778+
await page.goto(target.contentServerUrl);
779+
780+
await expect(page).not.toHaveURL(/signin_passwordless_code/);
781+
await expect(signin.cachedSigninHeading).toBeVisible();
782+
783+
// Navigate to /signin directly — same behavior
784+
await page.goto(
785+
`${target.contentServerUrl}/signin?email=${email}`
786+
);
787+
await expect(page).not.toHaveURL(/signin_passwordless_code/);
788+
await expect(signin.cachedSigninHeading).toBeVisible();
789+
});
790+
791+
test('passwordless signup via Settings then navigating to / shows cached signin (not OTP)', async ({
792+
target,
793+
pages: { page, signin, signinPasswordlessCode, settings },
794+
testAccountTracker,
795+
}) => {
796+
const { email } =
797+
testAccountTracker.generatePasswordlessAccountDetails();
798+
799+
await page.goto(
800+
`${target.contentServerUrl}/?force_passwordless=true`
801+
);
802+
await signin.fillOutEmailFirstForm(email);
803+
804+
await expect(page).toHaveURL(/signin_passwordless_code/);
805+
806+
const code = await target.emailClient.getPasswordlessSignupCode(email);
807+
await signinPasswordlessCode.fillOutCodeForm(code);
767808

768809
await expect(settings.settingsHeading).toBeVisible();
810+
811+
// Navigate to / — cached session should show signin, not OTP
812+
await page.goto(target.contentServerUrl);
813+
await expect(page).not.toHaveURL(/signin_passwordless_code/);
814+
await expect(signin.cachedSigninHeading).toBeVisible();
769815
});
770816
});
771817

@@ -992,6 +1038,29 @@ test.describe('severity-2', () => {
9921038
// flow since webchannel state from first login is stale.
9931039
});
9941040
});
995-
});
9961041

1042+
test.describe('Passwordless authentication - Browser Service (Relay)', () => {
1043+
test('passwordless signin via Relay OAuth flow', async ({
1044+
target,
1045+
pages: { page, signin, signinPasswordlessCode },
1046+
testAccountTracker,
1047+
}) => {
1048+
const { email } = await testAccountTracker.signUpPasswordless();
1049+
1050+
const params = new URLSearchParams(relayDesktopOAuthQueryParams);
1051+
await signin.goto('/authorization', params);
1052+
1053+
await signin.fillOutEmailFirstForm(email);
1054+
1055+
await expect(page).toHaveURL(/signin_passwordless_code/);
1056+
await expect(signinPasswordlessCode.heading).toBeVisible();
1057+
1058+
const code = await target.emailClient.getPasswordlessSigninCode(email);
1059+
await signinPasswordlessCode.fillOutCodeForm(code);
9971060

1061+
// After OTP, the flow either redirects to set_password or
1062+
// completes the OAuth flow — verify we left the OTP page
1063+
await expect(page).not.toHaveURL(/signin_passwordless_code/);
1064+
});
1065+
});
1066+
});

‎packages/fxa-auth-server/test/mail_helper.js‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@ module.exports = (printLogs) => {
8686
const sc = mail.headers['x-signin-verify-code'];
8787
const rpc = mail.headers['x-password-forgot-otp'];
8888
const mfa = mail.headers['x-account-change-verify-code'];
89+
const plSignup = mail.headers['x-passwordless-signup-otp'];
90+
const plSignin = mail.headers['x-passwordless-signin-otp'];
8991
const template = mail.headers['x-template-name'];
9092

9193
// Workaround because the email service wraps this header in `< >`.
@@ -108,6 +110,10 @@ module.exports = (printLogs) => {
108110
console.log('\x1B[36mReset password Otp:', rpc, '\x1B[39m');
109111
} else if (mfa) {
110112
console.log('\x1B[36mMfa code:', mfa, '\x1B[39m');
113+
} else if (plSignup) {
114+
console.log('\x1B[35mPasswordless signup code:', plSignup, '\x1B[39m');
115+
} else if (plSignin) {
116+
console.log('\x1B[35mPasswordless signin code:', plSignin, '\x1B[39m');
111117
} else if (TEMPLATES_WITH_NO_CODE.has(template)) {
112118
console.log(`Notification email: ${template}`);
113119
} else {

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -868,6 +868,53 @@ describe('IndexContainer', () => {
868868
});
869869
});
870870

871+
it('should redirect cached passwordless account to signin (not OTP) when sessionToken exists', async () => {
872+
// When a logged-in passwordless user navigates to `/`, currentAccount()
873+
// returns their account with a sessionToken. The IndexContainer skips
874+
// the passwordless OTP redirect and sends to /signin for cached login.
875+
mockLocationState = {};
876+
877+
jest.spyOn(cache, 'currentAccount').mockReturnValue({
878+
uid: 'abc123',
879+
email: '[email protected]',
880+
sessionToken: 'abc123def456',
881+
lastLogin: Date.now(),
882+
});
883+
jest.spyOn(cache, 'lastStoredAccount').mockReturnValue(undefined);
884+
885+
mockUseValidatedQueryParams.mockReturnValue({
886+
queryParamModel: {},
887+
validationError: null,
888+
});
889+
890+
mockUseAuthClient.mockReturnValue({
891+
accountStatusByEmail: jest.fn().mockResolvedValue({
892+
exists: true,
893+
hasLinkedAccount: false,
894+
hasPassword: false,
895+
passwordlessSupported: true,
896+
}),
897+
});
898+
899+
renderWithLocalizationProvider(
900+
<IndexContainer
901+
{...{
902+
integration,
903+
serviceName: MozServices.Default,
904+
useFxAStatusResult: mockUseFxAStatusResult,
905+
}}
906+
/>
907+
);
908+
909+
await waitFor(() => {
910+
expect(mockNavigate).toHaveBeenCalledTimes(1);
911+
});
912+
913+
const [calledUrl, options] = mockNavigate.mock.calls[0];
914+
expect(calledUrl).toMatch(/\/signin$/);
915+
expect(options.state.email).toBe('[email protected]');
916+
});
917+
871918
describe('passwordless redirect', () => {
872919
it('redirects existing passwordless account to passwordless code without feature flag', async () => {
873920
// When passwordlessSupported=true and hasPassword=false, canUsePasswordlessExisting

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,10 @@ const IndexContainer = ({
9999
const { prefillEmail, deleteAccountSuccess, hasBounced } =
100100
location.state || {};
101101

102+
// Only used for suggestedEmail; handleSuccessNavigation reads fresh from
103+
// localStorage to avoid stale closures when checking session state.
104+
const cachedAccount = currentAccount() || lastStoredAccount();
105+
102106
const handleSuccessNavigation = useCallback(
103107
(
104108
exists: boolean,
@@ -119,8 +123,11 @@ const IndexContainer = ({
119123
const passwordlessEnabled =
120124
config.featureFlags?.passwordlessEnabled === true ||
121125
queryParamModel.forcePasswordless === true;
126+
// Skip passwordless redirect if the user has a cached session
127+
const storedAccount = currentAccount() || lastStoredAccount();
128+
const hasCachedSession = !!storedAccount?.sessionToken;
122129
const canUsePasswordlessExisting =
123-
passwordlessSupported && !hasPassword && !hasLinkedAccount;
130+
passwordlessSupported && !hasPassword && !hasLinkedAccount && !hasCachedSession;
124131
const canUsePasswordlessNew = passwordlessEnabled && passwordlessSupported && !wantsKeys;
125132

126133
if (exists) {
@@ -308,8 +315,7 @@ const IndexContainer = ({
308315
const suggestedEmail =
309316
queryParamModel.email ||
310317
queryParamModel.loginHint ||
311-
currentAccount()?.email ||
312-
lastStoredAccount()?.email;
318+
cachedAccount?.email;
313319

314320
// If we just came from another Mozilla accounts page with a prefill email in location state,
315321
// ignore suggested email. Prefill email is used for clicks on "Use different account" or "Change email".

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1324,6 +1324,72 @@ describe('signin container', () => {
13241324
);
13251325
});
13261326
});
1327+
1328+
it('passwordless user with valid sessionToken sees cached signin page, not OTP redirect', async () => {
1329+
// When a passwordless user has a valid cached session (sessionToken in localStorage),
1330+
// the useEffect should NOT redirect to /signin_passwordless_code.
1331+
// Instead, the Signin component should render with the cached session.
1332+
const storedAccount = {
1333+
...MOCK_STORED_ACCOUNT,
1334+
email: MOCK_QUERY_PARAM_EMAIL,
1335+
sessionToken: MOCK_SESSION_TOKEN,
1336+
};
1337+
mockCurrentAccount(storedAccount);
1338+
mockAuthClient.accountStatusByEmail = jest.fn().mockResolvedValue({
1339+
exists: true,
1340+
hasLinkedAccount: false,
1341+
hasPassword: false,
1342+
passwordlessSupported: true,
1343+
});
1344+
mockUseValidateModule({
1345+
queryParams: {
1346+
email: MOCK_QUERY_PARAM_EMAIL,
1347+
isV2: () => false,
1348+
},
1349+
});
1350+
render();
1351+
1352+
// The Signin component should render (not redirect to passwordless code)
1353+
await waitFor(() => {
1354+
expect(SigninModule.default).toHaveBeenCalled();
1355+
expect(currentSigninProps?.sessionToken).toBe(MOCK_SESSION_TOKEN);
1356+
});
1357+
1358+
// Should NOT have navigated to passwordless code page
1359+
expect(mockNavigate).not.toHaveBeenCalledWith(
1360+
'/signin_passwordless_code',
1361+
expect.anything()
1362+
);
1363+
});
1364+
1365+
it('passwordless user without sessionToken redirects to OTP', async () => {
1366+
const storedAccount = {
1367+
...MOCK_STORED_ACCOUNT,
1368+
email: MOCK_QUERY_PARAM_EMAIL,
1369+
sessionToken: undefined,
1370+
};
1371+
mockCurrentAccount(storedAccount);
1372+
mockAuthClient.accountStatusByEmail = jest.fn().mockResolvedValue({
1373+
exists: true,
1374+
hasLinkedAccount: false,
1375+
hasPassword: false,
1376+
passwordlessSupported: true,
1377+
});
1378+
mockUseValidateModule({
1379+
queryParams: {
1380+
email: MOCK_QUERY_PARAM_EMAIL,
1381+
isV2: () => false,
1382+
},
1383+
});
1384+
render();
1385+
1386+
await waitFor(() => {
1387+
expect(mockNavigate).toHaveBeenCalledWith(
1388+
'/signin_passwordless_code',
1389+
expect.anything()
1390+
);
1391+
});
1392+
});
13271393
});
13281394

13291395
describe('cachedSigninHandler', () => {

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,8 @@ const SigninContainer = ({
288288
passwordlessSupported &&
289289
!hasPassword &&
290290
!hasLinkedAccount &&
291-
!skipPasswordlessRedirect
291+
!skipPasswordlessRedirect &&
292+
!sessionToken
292293
) {
293294
// Existing passwordless account — server returns
294295
// passwordlessSupported=true regardless of flag/allowlist

0 commit comments

Comments
 (0)