Conversation
There was a problem hiding this comment.
Pull request overview
Adds a reachable “passkey sign-in → password verification” fallback flow so Sync OAuth can obtain wrapped Sync keys after a passwordless (passkey) sign-in, and fixes passkey session token material returned from the auth-server.
Changes:
- fxa-settings: Route and wire
SigninPasskeyFallbackto/session/reauth?keys=true, including refresh-safe hydration from cached session state. - fxa-auth-server: Suppress duplicate sign-in notifications on
/session/reauthfor passkey-verified sessions; fix/passkey/authentication/finishto return the session token seed (data) instead of HKDF-derivedid. - fxa-auth-client + tests: Add passkey authentication ceremony helpers and add functional/integration coverage for the passkey→password fallback Sync flow.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/index.tsx | Adds UI wiring for submitting password, showing email, and displaying localized backend errors. |
| packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/index.test.tsx | Replaces snapshot test with behavioral tests for callbacks and error banner rendering. |
| packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/index.stories.tsx | Updates stories to exercise handlers and error banner state. |
| packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/container.tsx | New container integrating OAuth navigation + /session/reauth?keys=true for Sync key retrieval. |
| packages/fxa-settings/src/pages/Signin/SigninPasskeyFallback/snapshots/index.test.tsx.snap | Removes obsolete snapshot after switching to behavioral tests. |
| packages/fxa-settings/src/components/App/index.tsx | Routes /signin_passkey_fallback to the new container. |
| packages/fxa-auth-server/test/remote/passkeys.in.spec.ts | Adds remote integration coverage for passkey session + /session/reauth?keys=true keyFetchToken issuance. |
| packages/fxa-auth-server/lib/routes/session.spec.ts | Adds unit coverage that /session/reauth skips sign-in notifications for passkey sessions. |
| packages/fxa-auth-server/lib/routes/session.js | Implements notification suppression for passkey-verified sessions during reauth. |
| packages/fxa-auth-server/lib/routes/passkeys.ts | Fixes /passkey/authentication/finish to return sessionToken.data (seed) instead of sessionToken.id. |
| packages/fxa-auth-server/lib/routes/passkeys.spec.ts | Updates expectations/mocks for the corrected session token return value. |
| packages/fxa-auth-client/lib/client.ts | Adds beginPasskeyAuthentication and completePasskeyAuthentication methods to the public client API. |
| packages/functional-tests/tests/passkeys/passkeyPasswordFallback.spec.ts | Adds an end-to-end Playwright test covering passkey sign-in + fallback password submit leading to Sync OAuthLogin. |
| packages/functional-tests/lib/passkeyPolyfill.ts | Exposes raw minted credential objects for “new device” ceremony simulation in tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (oAuthDataError) { | ||
| return <OAuthDataError error={oAuthDataError} />; | ||
| } | ||
|
|
||
| if (!sessionToken || !email) { | ||
| navigateWithQuery('/'); | ||
| return <AppLayout loading />; | ||
| } |
There was a problem hiding this comment.
This seems like a valid comment
| /** | ||
| * Starts a passkey authentication ceremony, returning the WebAuthn | ||
| * `PublicKeyCredentialRequestOptions` for the browser. | ||
| */ | ||
| async beginPasskeyAuthentication( | ||
| headers?: Headers | ||
| ): Promise<PublicKeyCredentialRequestOptionsJSON> { | ||
| return this.request('POST', '/passkey/authentication/start', {}, headers); | ||
| } | ||
|
|
||
| /** | ||
| * Completes a passkey authentication ceremony by submitting the assertion | ||
| * and the original challenge. Returns a passkey-verified session token. | ||
| * | ||
| * @param response Assertion produced by `navigator.credentials.get`. | ||
| * @param challenge The challenge string returned by `beginPasskeyAuthentication`. | ||
| * @param service Optional client identifier (`'sync'` triggers | ||
| * `requiresPasswordForSync` in the response). | ||
| */ | ||
| async completePasskeyAuthentication( | ||
| response: AuthenticationResponseJSON, | ||
| challenge: string, | ||
| service?: string, | ||
| headers?: Headers | ||
| ): Promise<{ | ||
| uid: hexstring; | ||
| sessionToken: hexstring; | ||
| verified: boolean; | ||
| requiresPasswordForSync: boolean; | ||
| hasPassword: boolean; | ||
| }> { | ||
| return this.request( | ||
| 'POST', | ||
| '/passkey/authentication/finish', | ||
| service ? { response, challenge, service } : { response, challenge }, | ||
| headers | ||
| ); | ||
| } |
There was a problem hiding this comment.
Outdated, no longer in PR. This is also mentioned in the PR description - update for consistency?
43bd6bb to
b392d63
Compare
| // Simulate a fresh device: drop all cached signin state from the initial | ||
| // Sync OAuth flow so the fallback page hydrates purely from the | ||
| // passkey-verified session we seed below. | ||
| await page.context().clearCookies(); |
There was a problem hiding this comment.
This will need follow-up, but figured a net positive to have something exercising the flow end to end. The test simulates a verified passkey login, seeds the resulting session into localStorage, then signin_passkey_fallback from the browser.
Because: - Sync needs the wrapped sync keys (kB) to complete OAuth, which require a fresh authPW after a passwordless passkey signin. - The fallback page existed but had no backend integration or entry route. This commit: - Wires SigninPasskeyFallback to /session/reauth?keys=true via authClient.sessionReauth, routes it at /signin_passkey_fallback, and hydrates from cached session on refresh. - Suppresses redundant signin notifications when /session/reauth runs on a passkey-verified session (the passkey signin already fired them). - Adds auth-client methods for the passkey authentication ceremony. - Fixes a pre-existing bug in /passkey/authentication/finish where sessionToken.id (HKDF-derived) was returned in place of sessionToken.data. - Adds unit and integration tests and a browser-driven Playwright spec covering the full Sync OAuth, passkey, fallback flow.
vpomerleau
left a comment
There was a problem hiding this comment.
This is looking good and is probably ok to land independently - but optionally you could rebase/wait on #20575 to test out the E2E flow.
Comments are nice to have but not blocking.
| rpId, | ||
| }); | ||
| return target.authClient.completePasskeyAuthentication( | ||
| assertion as any, |
There was a problem hiding this comment.
nit: Could drop as any or use the actual type PublicKeyCredentialJSON
| <h1 className="card-header mb-4">Enter your password to sync</h1> | ||
| </FtlMsg> | ||
|
|
||
| {email && ( |
There was a problem hiding this comment.
The design may have changed a tad since this page was first created - it looks like the avatar is also included now. This would be a good time to update to match: https://www.figma.com/design/4ToDEuz5HU1HyBrFCIZe4v/Passkeys?node-id=3036-65903&t=DSWA1DCh8GoxOaXz-4
|
|
||
| return ( | ||
| <AppLayout> | ||
| {localizedErrorMessage && ( |
There was a problem hiding this comment.
Was going to say that our banners are usually below the heading, but I see this matches Figma. I'll ask Laurel if we can standardize (non-blocker here)
Because
kB), which require a freshauthPWafter a passwordless passkey signinSigninPasskeyFallbackpage existed but had no backend integration or route, so the flow was unreachableThis pull request
SigninPasskeyFallbackto/session/reauth?keys=trueviaauthClient.sessionReauth, routes it at/signin_passkey_fallback, and hydrates from cached session on refresh (container.tsx,App/index.tsx)/session/reauthruns on a passkey-verified session, since the passkey signin already fired them (session.js)beginPasskeyAuthenticationandcompletePasskeyAuthenticationtofxa-auth-clientfor the new-device passkey ceremony (client.ts)/passkey/authentication/finishwheresessionToken.id(HKDF-derived) was returned in place ofsessionToken.data(seed), breaking client-side Hawk re-derivation (passkeys.ts)Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13096
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13100
Checklist
Other information
How to test:
authClient.beginPasskeyAuthentication/completePasskeyAuthentication(the entry-point button is tracked separately)/signin_passkey_fallbackwith Sync OAuth params, enter the account password, click ContinueOAuthLoginwebchannel message carries thehttps://identity.mozilla.com/apps/oldsyncscope and the user lands in Sync