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.
7d08848 to
9d03a74
Compare
| * @returns WebAuthn registration options to pass to the browser | ||
| */ | ||
| async generateRegistrationChallenge( | ||
| userId: Buffer, |
There was a problem hiding this comment.
We should be consistent here. I assume this it the typcial uid param.
There was a problem hiding this comment.
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,
// ...
});
}| * 2. Transport-based fallback | ||
| * 3. Generic fallback: "Passkey" | ||
| */ | ||
| private generatePasskeyName( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Maybe we don't need to write this value to the DB?
These are just default names. Users will be able to rename their passkeys later. I as a user would expect passkey names to stay the same unless I change it. E.g., say I have Passkey 1, Passkey 2 and Passkey 3, then if I remove Passkey 1, I'd expect the name of Passkey 2 and 3 to stay the same instead of changing to 1 and 2, which would be confusing (which one did I actually delete?). So storing default names (along with enumeration) to DB makes sense. It does get complicated with l10n though, not sure how having two Passkey 1s in two different languages feels like, but this should be very edge case-y.
There was a problem hiding this comment.
Should we just be passing these up from the front end then? If the user can rename them, it sounds like we should let the user decide what to call the passkey, and of course offer a reasonable default.
There was a problem hiding this comment.
My only concern is that we'll have to make an additional call to the DB to find existing keys and enumerate the names to find if we need to add the suffix. It's an additional potential failure point while registering - let me give it a shot and see what I can come up with though! I'll push as a separate commit to check it out and can always revert it if it's not a good approach!
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
dschom
left a comment
There was a problem hiding this comment.
Looks good to me! There is one outstanding question about the passkey name, but we can always refactor this later if it turns out shifting this responsibility to the front end makes more sense.
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.