Skip to content

feat(passkeys): Add passkey generate and validate challenge methods#20194

Open
nshirley wants to merge 1 commit intomainfrom
FXA-13061
Open

feat(passkeys): Add passkey generate and validate challenge methods#20194
nshirley wants to merge 1 commit intomainfrom
FXA-13061

Conversation

@nshirley
Copy link
Contributor

Because:

  • We now need to expose the passkey registration generation

This commit:

  • Adds new verifyRegistrationResponse and generateRegistrationChallenge service methods
  • Adds tests for new functions
  • Updates manager to expose checkPasskeyCount for use in service and manager

Closes: FXA-13061

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).

Screenshots (Optional)

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

Other information (Optional)

Any other information that is important to this pull request.

Copy link

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 service-level APIs for passkey (WebAuthn) registration by exposing challenge generation and registration response verification, and refactors passkey-limit enforcement into a reusable manager method.

Changes:

  • Add generateRegistrationChallenge and verifyRegistrationResponse to PasskeyService.
  • Refactor passkey limit enforcement into PasskeyManager.checkPasskeyCount and reuse it from registration flows.
  • Add/extend unit + integration tests covering the new service/manager behavior and updated error expectations.

Reviewed changes

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

Show a summary per file
File Description
libs/accounts/passkey/src/lib/passkey.service.ts Adds registration challenge generation + attestation verification logic, passkey name generation, metrics/logging.
libs/accounts/passkey/src/lib/passkey.service.spec.ts Adds unit tests for new service methods and passkey naming behavior.
libs/accounts/passkey/src/lib/passkey.manager.ts Extracts passkey-limit logic into checkPasskeyCount and switches limit error to AppError.
libs/accounts/passkey/src/lib/passkey.manager.spec.ts Updates unit tests for new AppError behavior and adds checkPasskeyCount coverage.
libs/accounts/passkey/src/lib/passkey.manager.in.spec.ts Updates integration assertions for limit error and adds integration coverage for checkPasskeyCount.

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

@nshirley nshirley marked this pull request as ready for review March 17, 2026 20:42
@nshirley nshirley requested a review from a team as a code owner March 17, 2026 20:42
@nshirley nshirley force-pushed the FXA-13061 branch 2 times, most recently from 7d08848 to 9d03a74 Compare March 19, 2026 23:14
* @returns WebAuthn registration options to pass to the browser
*/
async generateRegistrationChallenge(
userId: Buffer,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be consistent here. I assume this it the typcial uid param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, and I had a reason for the name but can't remember it now. I think it was along the lines of trying to prevent leaking application state from calling code into the service. Like, internally the generateRegistrationOptions() just maps the uid parameter back to a userId parameter when calling to @simplewebauthn/server. But, I can change it to uid for a bit more consistency!

// webauthn-adapter.ts
export async function generateRegistrationOptions(
  config: PasskeyConfig,
  input: RegistrationOptionsInput
): Promise<PublicKeyCredentialCreationOptionsJSON> {
  return libGenerateRegistrationOptions({
    // ...
    userName: input.email,
    userID: input.uid, // <-- 
    challenge: input.challenge,
    // ...
  });
}

}

if (transports.length === 1) {
switch (transports[0]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we pick the first transport?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since transports are just hints at what kind of capabilities the key has, and they're a non-exhaustive list, this just becomes a "best guess" at what the passkey type is. Definitely not great, but per the specs there isn't much we can do with this and actually it's meant as a 'hint' to the client/browser to better start the authentication process, so using it for this here works, but isn't great. The comment above to use FIDO MDS will get us the best name, and this here just becomes a fallback.

I did run this past Claude as well and it suggested using a priority order to give a 'best guess' at the type of key, it would take the full transports list into consideration this way

function friendlyCredentialLabel({
  transports,
  authenticatorAttachment,
}: {
  transports?: string[];
  authenticatorAttachment?: 'platform' | 'cross-platform' | null;
}) {
  const t = new Set(transports ?? []);

  if (t.has('internal')) return 'Built-in passkey on this device';
  if (t.has('hybrid')) return 'Passkey on phone or nearby device';
  if (t.has('usb') && t.has('nfc')) return 'Security key (USB or NFC)';
  if (t.has('usb')) return 'Security key (USB)';
  if (t.has('nfc')) return 'Security key (NFC)';
  if (t.has('ble')) return 'Bluetooth security key';

  if (authenticatorAttachment === 'platform') return 'Built-in passkey';
  if (authenticatorAttachment === 'cross-platform') return 'External security key';

  return 'Passkey or security key';
}

* 2. Transport-based fallback
* 3. Generic fallback: "Passkey"
*/
private generatePasskeyName(
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking a step back, if this is just based on aaguid value and the transport state, why are we doing this here? It almost seems more like a presentation layer concern. ie Maybe we don't need to write this value to the DB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, but the current data model requires a name so we'd have to rip that out. But I get what you're saying, we should store the transports on the model (which we do) and then who/whatever displays it can decide how to handle that - which would also help with the enumeration of multiple passkeys with the same transports AND with localization since it would likely happen in settings

I think if we do want to drop this we should decide quickly before we wire up the service to auth-server and start writing records

Because:
 - We now need to expose the passkey registration generation

This commit:
 - Adds new verifyRegistrationResponse and generateRegistrationChallenge
   service methods
 - Adds tests for new functions
 - Updates manager to expose checkPasskeyCount for use in service and
   manager

Closes: FXA-13061
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