feat(passkey): create passkey authentication methods#20216
feat(passkey): create passkey authentication methods#20216MagentaManifold merged 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds high-level Passkey (WebAuthn) authentication/assertion flows to the passkey library, integrating challenge lifecycle management + SimpleWebAuthn verification.
Changes:
- Implemented
generateAuthenticationChallenge()to issue assertion options (optionally restricted to a user’s credentials). - Implemented
verifyAuthenticationResponse()to validate challenges, verify assertions, update passkey counters/backup state, and emit metrics/logs. - Added comprehensive unit tests for the new service methods (including signCount rollback logging).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| libs/accounts/passkey/src/lib/passkey.service.ts | Adds authentication challenge generation + assertion verification orchestration, including metrics/logging and passkey updates. |
| libs/accounts/passkey/src/lib/passkey.service.spec.ts | Adds unit test coverage for the new authentication flows and key error paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MagentaManifold
force-pushed
the
FXA-13062
branch
from
8ca3e17 to
1f82bae
Compare
| allowCredentials = passkeys.map((p) => p.credentialId); | ||
| } | ||
|
|
||
| return await generateAuthenticationOptions(this.config, { |
There was a problem hiding this comment.
It might be worth while to record a metric for the number of these challenges that we create.
There was a problem hiding this comment.
challengeManager.generateAuthenticationChallenge() already emits a metric, so there's no point in emitting another
MagentaManifold
force-pushed
the
FXA-13062
branch
from
1f82bae to
6eeda0d
Compare
MagentaManifold
force-pushed
the
FXA-13062
branch
from
6eeda0d to
73ddae6
Compare
| async consumeRegistrationChallenge( | ||
| uid: string, | ||
| challenge: string | ||
| challenge: string, |
There was a problem hiding this comment.
Any particular reason to swap this back?
There was a problem hiding this comment.
Oh, I can see why. Since the consumeAuthenticationChallenge() only takes the challenge string, now all functions have the same first parameter
There was a problem hiding this comment.
This is to ensure that all methods have consistent parameter orders. I put uid at last because it can be optional in other methods.
| signCount: passkey.signCount, | ||
| }); | ||
| } catch (err) { | ||
| // simplewebauthn throws when signCount decrements |
There was a problem hiding this comment.
This was flagged as a discrepancy with the acceptance criteria, "Rollback on error". Might be useful to add a comment saying that this is done by the simplewebauthn lib.
There was a problem hiding this comment.
You mean add a comment in code or in Jira? I've updated the ticket description in Jira about it, and in code we have this comment here. Also the ticket didn't say "rollback on error", but more like "log a warning on rollback", so I'm not quite sure what you mean 🤔
| try { | ||
| result = await verifyAuthenticationResponse(this.config, { | ||
| response, | ||
| challenge, |
There was a problem hiding this comment.
nit: Could use storedChallenge.challenge instead of raw.
Because: * we want passkey authentication methods This commit: * creates passkey authentication methods Closes #FXA-13062
MagentaManifold
force-pushed
the
FXA-13062
branch
from
73ddae6 to
4ddb34f
Compare
Because
This pull request
Issue that this pull request solves
Closes: FXA-13062
Checklist
Put an
xin the boxes that applyScreenshots (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.