Conversation
There was a problem hiding this comment.
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
generateRegistrationChallengeandverifyRegistrationResponsetoPasskeyService. - Refactor passkey limit enforcement into
PasskeyManager.checkPasskeyCountand 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.
| if (!storedChallenge) { | ||
| // technically, this could be because the challenge expired, was already consumed, or never existed. | ||
| // However, we can't distinguish these cases, and in all cases the appropriate response is to reject the registration attempt. |
There was a problem hiding this comment.
See comment here for why we don't validate the UID
| beforeEach(() => { | ||
| mockChallengeManager.validateChallenge.mockResolvedValue({ | ||
| challenge: MOCK_CHALLENGE, | ||
| type: 'registration', | ||
| uid: MOCK_UID.toString('hex'), | ||
| createdAt: Date.now() - 1000, | ||
| expiresAt: Date.now() + 299000, | ||
| }); |
There was a problem hiding this comment.
Leaving a comment for keeping track - on the surface this sounds good but I was thinking about the challenge manager during implementation and it doesn't feel like it should be responsible for checking the UID on the challenge. The UID exists as meta-data for consumers of the library to validate, but it also isn't guaranteed to exist. Authentication challenges for example don't take a UID so it won't be present.
That said, if Authentication should take a UID, I can always fix it! If all challenge types have a UID, then it's probably okay to validate in the manager
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
| case 'nfc': | ||
| return 'NFC Security Key'; | ||
| case 'hybrid': | ||
| return 'Passkey'; |
There was a problem hiding this comment.
These names need to be localized. Also what if two passkeys have the same name? Should we add #1 #2 etc. after them to distinguish 🤔?
Because:
This commit:
Closes: FXA-13061
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.