Skip to content

task(settings): passkey registration UI flow#20385

Open
dschom wants to merge 1 commit intomainfrom
FXA-13072
Open

task(settings): passkey registration UI flow#20385
dschom wants to merge 1 commit intomainfrom
FXA-13072

Conversation

@dschom
Copy link
Copy Markdown
Contributor

@dschom dschom commented Apr 16, 2026

Because

  • We want to allow users to register passkeys

This pull request

  • Enables bass key registration on the settings page.

Issue that this pull request solves

Closes: FXA-13072

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).
  • I have manually reviewed all AI generated code.

How to review (Optional)

  • In your .env file set FEATURE_FLAGS_PASSKEYS_ENABLED=true and FEATURE_FLAGS_PASSKEY_REGISTRATION_ENABLED=true

  • Start he stack

  • Go to settings page

  • Try adding one more passkeys

  • Try deleting passkeys

  • Suggested review order: Storybooks, Manual Testing

  • Risky or complex parts: I have only really tested this on mac. I'll try to test on adroid and iphone today. I do not have a windows machine to test with, and haven't manually be be able to trip the 'unsupported' state.

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

This PR was done almost entirely by Claude by hooking up the Figma and Jira MCP servers and directing Claude to do the ticket. I've looked over the code, but will be taking a harder look now this is officially up for review. One other minor thing to note, I had to manually add support for deleting the passkey since that was in Figma, but not directly called out in the ticket.

@dschom dschom force-pushed the FXA-13072 branch 7 times, most recently from 6f7e194 to 2c1700c Compare April 16, 2026 23:18
@dschom dschom marked this pull request as ready for review April 17, 2026 15:36
@dschom dschom requested review from a team as code owners April 17, 2026 15:36
@dschom dschom requested a review from vpomerleau April 17, 2026 15:36
@vpomerleau vpomerleau requested a review from Copilot April 17, 2026 16:30
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an end-to-end passkey registration UI to the FxA Settings “Security” section, including a new “Create passkey” route guarded by MFA and UI to list existing passkeys.

Changes:

  • Introduces /settings/passkeys/add with an MFA-guarded “ceremony in progress” modal and WebAuthn registration completion.
  • Adds a Passkeys row to the Security section that fetches and renders the user’s passkeys (plus max-limit banner behavior).
  • Extends the Account model with JWT-scoped helpers for starting/finishing passkey registration.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
packages/fxa-settings/src/models/Account.ts Adds JWT-scoped wrappers for begin/complete passkey registration.
packages/fxa-settings/src/components/Settings/index.tsx Registers the MFA-guarded /passkeys/add route behind a feature flag.
packages/fxa-settings/src/components/Settings/Security/index.tsx Shows the Passkeys unit row in the Security section behind a feature flag.
packages/fxa-settings/src/components/Settings/UnitRowPasskey/index.tsx Fetches passkeys via auth-client and renders the Passkeys unit row + subrows.
packages/fxa-settings/src/components/Settings/UnitRow/index.tsx Tweaks CTA rendering logic to allow disabled CTAs to render actions container.
packages/fxa-settings/src/components/Settings/SubRow/index.tsx Updates passkey subrow typing and adds “sign in only” messaging + MFA-guarded delete modal.
packages/fxa-settings/src/components/Settings/PagePasskeyAdd/index.tsx Implements the WebAuthn registration ceremony and success/error handling.
*.test.tsx / *.stories.tsx / *.ftl Updates tests/stories and adds localization strings for the new passkey UI.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

## Error / limit messages

passkey-row-webauthn-not-supported = Your browser or device doesn't support passkeys.
passkey-row-limit-reached = You've used all 10 passkeys. Delete a passkey to create a new one.
Comment on lines +229 to +231
{config.featureFlags?.passkeyRegistrationEnabled && (
<MfaGuardPagePasskeyAdd path="/passkeys/add" />
)}
Comment on lines +89 to +94
{config.featureFlags?.passkeyRegistrationEnabled && (
<>
<hr className="unit-row-hr" />
<UnitRowPasskey />
</>
)}
Comment on lines +136 to +147
mockListPasskeys.mockResolvedValue([]);
renderUnitRowPasskey();
await waitFor(() => {
expect(screen.getByText('Not set')).toBeInTheDocument();
});
});

it('renders all passkey sub-rows', () => {
it('renders all passkey sub-rows', async () => {
renderUnitRowPasskey();
expect(screen.getByText('MacBook Pro')).toBeInTheDocument();
expect(screen.getByText('iPhone 15')).toBeInTheDocument();
await waitFor(() => {
expect(
screen.getByTestId('passkey-sub-row-passkey-1')
const [passkeys, setPasskeys] = useState<Passkey[]>([]);
const [loading, setLoading] = useState(true);

const fetchPasskeys = useCallback(async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passkey display & delete are basically the whole point of my ticket 😭 The way you did it isn't the best though (see my PR). It's nice to have them for testing registration stuff out, but I recommend not including them when you merge it. Or... you can copy everything from my PR and merge all these in one go 🤔 (in hindsight these two tickets really should be done at the same time by one person, bc they are kinda mutually blocking)


const learnMoreLink = (
<FtlMsg id="passkey-row-info-link-2">
<LinkExternal
Comment on lines +10 to +11
page-passkey-add-error-timed-out = Passkey setup timed out or was cancelled
page-passkey-add-error-not-supported = Your browser or device doesn't support passkeys.
Comment on lines +42 to +49
const abortControllerRef = useRef<AbortController | null>(null);

const navigateToSettings = useCallback(() => {
navigateWithQuery(SETTINGS_PATH + '#security', { replace: true });
}, [navigateWithQuery]);

const handleCancel = useCallback(() => {
abortControllerRef.current?.abort();
Comment on lines +89 to +92
} finally {
setLoading(false);
}
};
Comment thread packages/fxa-settings/src/components/Settings/SubRow/index.tsx
Comment on lines +442 to +449
{...(!passkey.backupEligible && {
statusIcon: 'alert',
message: (
<FtlMsg id="passkey-sub-row-sign-in-only">
<p>Sign in only. Can’t be used to sync.</p>
</FtlMsg>
),
})}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block should be removed for a few reasons:

  • we are not showing an alert/warning in phase 1, as it is not actionable
  • backupEligible is not related to Firefox Sync or (future) eligibility for passwordless sign-in to sync. backupEligible is about the passkey itself and whether it is device-bound (e.g., security key) or capable of backup between devices (e.g., Apple keychain) - we're not using that information for any UI state at the moment
Suggested change
{...(!passkey.backupEligible && {
statusIcon: 'alert',
message: (
<FtlMsg id="passkey-sub-row-sign-in-only">
<p>Sign in only. Can’t be used to sync.</p>
</FtlMsg>
),
})}

>
<div className="absolute inset-0 bg-grey-100/50 dark:bg-grey-900/50" />
<div className="relative bg-white dark:bg-grey-700 rounded-xl shadow-lg p-8 w-[480px] max-w-[calc(100vw-2rem)] text-center">
<LoadingSpinner className="flex justify-center mb-6" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image I dunno why, but the loading spinner looks weird in storybook (it looks fine in other places)

className="fixed inset-0 flex items-center justify-center z-50"
data-testid="page-passkey-add"
>
<div className="absolute inset-0 bg-grey-100/50 dark:bg-grey-900/50" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a Modal component that handles darkening the background. Should we just use that instead?

(🤔 Modal gets dismissed when you click outside, so maybe that could be a reason to not use it here?)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's a separate page then we don't need this backdrop at all since there's nothing behind it

data-testid="page-passkey-add"
>
<div className="absolute inset-0 bg-grey-100/50 dark:bg-grey-900/50" />
<div className="relative bg-white dark:bg-grey-700 rounded-xl shadow-lg p-8 w-[480px] max-w-[calc(100vw-2rem)] text-center">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max-w-[calc(100vw-2rem)] feels over complicated. mx-2 would work. (Mixing rem and px also isn't great) w-[480px] can be replaced with w-120. And again, Modal does all these for you, unless there's a good reason to not use it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually wait, is this supposed to be a separate page or a pop up modal? The story makes it look like a modal. If it's a separate page, we have FlowContainer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOOOO the design updated after I made the components! Passkey delete is also a separate page now 😭 I gotta redo it

Comment thread packages/fxa-settings/src/components/Settings/SubRow/index.tsx
const [passkeys, setPasskeys] = useState<Passkey[]>([]);
const [loading, setLoading] = useState(true);

const fetchPasskeys = useCallback(async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passkey display & delete are basically the whole point of my ticket 😭 The way you did it isn't the best though (see my PR). It's nice to have them for testing registration stuff out, but I recommend not including them when you merge it. Or... you can copy everything from my PR and merge all these in one go 🤔 (in hindsight these two tickets really should be done at the same time by one person, bc they are kinda mutually blocking)

Copy link
Copy Markdown
Contributor

@vpomerleau vpomerleau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked out the branch and successfully tested the registration flow in local environment. 🙌

Some of the issues I spotted are flagged in my comments or the copilot review.

In addition, I noticed that registration with 1 password fails. Might not be a big deal since production doesn't support extensions but the fix looks to be simple (sent via Slack).

});
}

async beginPasskeyRegistrationWithJwt(): Promise<PublicKeyCredentialCreationOptionsJSON> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these are unused and safe to remove.

alertBar.error(
ftlMsgResolver.getMsg(
'passkey-row-webauthn-not-supported',
"Your browser or device doesn't support passkeys."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All strings in fallbacks and ftl should use curly quote instead of straight quote '

Suggested change
"Your browser or device doesn't support passkeys."
"Your browser or device doesnt support passkeys."

const handleDeletePasskey = async (credentialId: string) => {
try {
// Delete the passkeys
await account.deletePasskeyWithJwt(credentialId);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's overlap between this PR and #20380 for passkey listing/deletion, but either way we should be consistent about directly using authClient or adding to the account model (we have been moving towards directly using authClient)

@vpomerleau vpomerleau mentioned this pull request Apr 17, 2026
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants