feat(passkey): create passkey authentication methods#20216
feat(passkey): create passkey authentication methods#20216MagentaManifold wants to merge 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.
Because: * we want passkey authentication methods This commit: * creates passkey authentication methods Closes #FXA-13062
MagentaManifold
force-pushed
the
FXA-13062
branch
from
8ca3e17 to
1f82bae
Compare
| const passkey = | ||
| await this.passkeyManager.findPasskeyByCredentialId(credentialId); | ||
| if (!passkey) { | ||
| throw AppError.passkeyNotFound(); |
There was a problem hiding this comment.
Let's add a metric and log here so we can monitor this condition.
| 'authentication' | ||
| ); | ||
| if (!storedChallenge) { | ||
| throw AppError.passkeyChallengeExpired(); |
There was a problem hiding this comment.
Ditto ad metric & log. Basically if we error out, let's record an passkey.authentication.fail metric. There's a few spots where this could be done.
| 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.
| throw AppError.passkeyChallengeExpired(); | ||
| } | ||
|
|
||
| if (expectedUid && !passkey.uid.equals(expectedUid)) { |
There was a problem hiding this comment.
This check can go before the call to const storedChallenge = await this.challengeManager.validateChallenge(...). Doing this before would be better, since the call to validateChallenge burns the challenge (ie uses GETDEL).
| if (!updated) { | ||
| this.log?.error('passkey.updateAfterAuth.failed', { | ||
| uid: passkey.uid.toString('hex'), | ||
| credentialId: credentialId.toString('hex'), |
There was a problem hiding this comment.
Should we log the newSignCount since that could cause the query to fail?`
| uid: passkey.uid.toString('hex'), | ||
| credentialId: credentialId.toString('hex'), | ||
| }); | ||
| throw AppError.passkeyNotFound(); |
There was a problem hiding this comment.
I think it's more likely the update fails due to a write conflict on signCount, in which case this error is misleading. That said, it might be good enough. Perhaps the calling code won't care either way.
| allowCredentials: [], | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Should we add a case for what happens if the UID doesn't match?
| newSignCount, | ||
| backupState | ||
| ); | ||
| if (!updated) { |
There was a problem hiding this comment.
I don't think we have a test covering this when updated is false.
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.