Conversation
There was a problem hiding this comment.
lol, that's not supposed to be here!
bd97bd6 to
5d51bd8
Compare
| .env.sentry-build-plugin | ||
|
|
||
| # Anthropic's Claude AI — ignore everything except checked-in skills | ||
| # Anthropic's Claude AI — ignore personal data and project files. |
There was a problem hiding this comment.
Add these because I had a projects/ file that snuck into a commit from separate Claude session. These look to be the expected folders/files that we don't want to commit anyway according to the linked page
There was a problem hiding this comment.
Pull request overview
Adds end-to-end support for gating the “Sign in with passkey” CTA on whether the target account actually has any registered passkeys, to avoid showing an unusable CTA when arriving on non-email-first sign-in surfaces with a prefilled email.
Changes:
- Auth server: extends unauthenticated
POST /account/status(thirdPartyAuthStatus branch) to optionally returnhasPasskey. - Passkeys lib: adds
PasskeyService.hasPasskey(uid)backed by a passkey count query. - Settings UI: threads
hasPasskeythrough sign-in routing/location state and applies a sharedshouldShowPasskeySigninhelper + tests to gate the CTA across non-email-first surfaces.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/mocks.tsx | Adds hasPasskey to test subject wiring. |
| packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/interfaces.ts | Threads hasPasskey through props and location state typing. |
| packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.tsx | Switches CTA gating to shared shouldShowPasskeySignin helper. |
| packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/index.test.tsx | Adds wiring-level tests for CTA visibility based on hasPasskey. |
| packages/fxa-settings/src/pages/Signin/SigninPasswordlessCode/container.tsx | Reads hasPasskey from location state and passes through to page. |
| packages/fxa-settings/src/pages/Signin/interfaces.ts | Adds hasPasskey to sign-in location state and shared props. |
| packages/fxa-settings/src/pages/Signin/index.tsx | Gates passkey CTA via shared helper using hasPasskey. |
| packages/fxa-settings/src/pages/Signin/index.test.tsx | Adds wiring-level tests for CTA gating on hasPasskey. |
| packages/fxa-settings/src/pages/Signin/container.tsx | Stores/threads hasPasskey from account status + location state into SigninDecider/pages. |
| packages/fxa-settings/src/pages/Signin/components/SigninDecider/index.tsx | Propagates hasPasskey into passwordless route state and downstream surfaces. |
| packages/fxa-settings/src/pages/Signin/components/SigninAlternativeAuthOptions/index.tsx | Gates passkey CTA via shared helper using hasPasskey. |
| packages/fxa-settings/src/pages/Signin/components/SigninAlternativeAuthOptions/index.test.tsx | Adds wiring-level tests for CTA gating on hasPasskey. |
| packages/fxa-settings/src/pages/Index/container.tsx | Pulls hasPasskey from accountStatusByEmail and passes into navigation state. |
| packages/fxa-settings/src/lib/passkeys/should-show-passkey-signin.ts | New pure helper encapsulating passkey CTA gating logic (flags + hasPasskey === true). |
| packages/fxa-settings/src/lib/passkeys/should-show-passkey-signin.test.ts | Unit tests for the shared gating helper. |
| packages/fxa-auth-server/test/remote/account_status.in.spec.ts | Adds remote integration coverage for hasPasskey behavior on account status responses. |
| packages/fxa-auth-server/test/client/api.js | Extends test client accountStatusByEmail to accept options (e.g., thirdPartyAuthStatus). |
| packages/fxa-auth-server/lib/routes/account.ts | Adds optional hasPasskey computation to account status handler + response schema. |
| libs/accounts/passkey/src/lib/passkey.service.ts | Adds PasskeyService.hasPasskey method. |
| libs/accounts/passkey/src/lib/passkey.service.spec.ts | Unit coverage for PasskeyService.hasPasskey. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Computed here since this branch already holds the account record. | ||
| // Fail closed: on lookup error leave hasPasskey undefined (client hides | ||
| // the CTA) rather than failing the whole status check. | ||
| if (this.config.passkeys?.enabled && Container.has(PasskeyService)) { |
There was a problem hiding this comment.
This sounds like a good idea 👍
3f7e684 to
4cb57d6
Compare
vpomerleau
left a comment
There was a problem hiding this comment.
Tested locally and this is working well! Comments are mainly nits/enhancements
| // Computed here since this branch already holds the account record. | ||
| // Fail closed: on lookup error leave hasPasskey undefined (client hides | ||
| // the CTA) rather than failing the whole status check. | ||
| if (this.config.passkeys?.enabled && Container.has(PasskeyService)) { |
There was a problem hiding this comment.
This sounds like a good idea 👍
| ); | ||
| }); | ||
|
|
||
| it('returns false when hasPasskey is true but isSignup is true', () => { |
There was a problem hiding this comment.
When would both hasPasskey and isSignup be true?
There was a problem hiding this comment.
I guess in practice that's not something that should ever happen. Feels worthwhile to still have a test to make sure it fails closed, but maybe I can take a look at the wording to make it a bit better. OR, see if we have similar coverage in another test and can just remove this one.
|
|
||
| it('renders the passkey CTA when hasPasskey is true', () => { | ||
| renderWithPasskeyFlags({ hasPasskey: true }); | ||
| expect(passkeyButton()).toBeInTheDocument(); |
There was a problem hiding this comment.
Would this be a good spot to check if the view metric is emitted?
There was a problem hiding this comment.
probably doesn't hurt, though I am partial to having separate tests for this kind of thing. That way, if for some reason the metric stops being emitted, the test name/failure is loud and clear as to what changed. I'll get one added!
| }; | ||
|
|
||
| try { | ||
| if (thirdPartyAuthStatus) { |
There was a problem hiding this comment.
Should we rename this check? Seems like this is closer to something like checkAvailableAuthOptions
There was a problem hiding this comment.
ha, so I got really confused by this even! It's definitely not a great name. But, I'll put an issue in to keep the scope for this pull request focused
| expect(passkeyButton()).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('hides the passkey CTA when hasPasskey is undefined (fails closed)', () => { |
There was a problem hiding this comment.
I wasn't sure about this, but probably fine since we unconditionally show the option on the email-first page and passkey is never the user's only sign-in option.
There was a problem hiding this comment.
yeah, I think while we're still rolling out support it makes sense to go this direction with fail-closed. But, once adoption is up and we're more confident in the stability we can update the code to fail-open
| !isSignup && | ||
| config.featureFlags?.passkeysEnabled && | ||
| config.featureFlags?.passkeyAuthenticationEnabled | ||
| ); |
There was a problem hiding this comment.
Should we also gate on isWebAuthnSupported? (= hide the button if we know webauthn isn't supported in the current browser)
(This would require a rebase on latest main, this branch doesn't include the bug fix that added this helper)
There was a problem hiding this comment.
Yep, good idea! I'll get other comments addressed then rebased and add this!
Because: - Per product, the "Sign in with Passkey" CTA should only appear for accounts that actually have a passkey, on every page except email-first. This commit: - Exposes hasPasskey on the unauthenticated POST /account/status, computed via PasskeyService.hasPasskey when the feature is enabled; fails closed on error. - Threads hasPasskey through the signin flow and gates the CTA on the signin, alternative-auth, and passwordless-OTP pages via a shared helper. Closes FXA-13823
Because
This pull request
hasPasskeyboolean on the unauthenticatedPOST /account/status. It is computed in the existingthirdPartyAuthStatusbranch (which already holds the account record) viaPasskeyService.hasPasskey, only whenconfig.passkeys.enabled, and fails closed (undefined) on any lookup error.PasskeyService.hasPasskeyinlibs/accounts/passkey; the feature-gate and fail-closed handling stay inline in the account-status handler, matching the sibling authenticated-profile lookup.hasPasskeyfromaccountStatusByEmailthroughIndex/container.tsx,Signin/container.tsx, andSigninDeciderinto the consuming pages and the passwordless location state.packages/fxa-settings/src/lib/passkeys/should-show-passkey-signin.ts(hasPasskey === trueplus the feature flags;undefinedfails closed). Email-first stays unconditional.PasskeyService.hasPasskey, keeps the per-component gating tests to wiring checks, and adds remote integration cases onaccount_status.in.spec.ts.Issue that this pull request solves
Closes: FXA-13823
Checklist
Put an
xin the boxes that applyHow to review (Optional)
packages/fxa-auth-server/lib/routes/account.ts(server field + inline fail-closed gate),libs/accounts/passkey/src/lib/passkey.service.ts(hasPasskey),packages/fxa-settings/src/lib/passkeys/should-show-passkey-signin.ts(the shared gate).account.ts→PasskeyService.hasPasskey→ shared helper → the three consuming pages.Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)