refactor(passkey): restrict NewPasskey type to repository layer#20342
refactor(passkey): restrict NewPasskey type to repository layer#20342MagentaManifold merged 1 commit intomainfrom
Conversation
| publicKey: Buffer; | ||
| signCount: number; | ||
| transports: Json; | ||
| transports: JSONColumnType<string[]>; |
There was a problem hiding this comment.
This should provider nicer typing
There was a problem hiding this comment.
Pull request overview
This PR updates the passkey registration flow to persist prfEnabled from WebAuthn registration responses and refactors the passkey write path to pass around the Passkey shape (booleans + string[] transports) instead of mapping to NewPasskey in the service layer.
Changes:
- Add
prfEnabledextraction to the WebAuthn adapter and cover it with unit tests. - Refactor passkey creation/registration to pass a
Passkeyobject through service/manager layers and centralize MySQL storage-type conversions in the repository. - Update shared DB typings and factories to represent
transportsas a JSON array and backup/prf flags as booleans at the domain/type level.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/shared/db/mysql/account/src/lib/kysely-types.ts | Change passkeys.transports typing to a JSON column array. |
| libs/shared/db/mysql/account/src/lib/factories.ts | Update PasskeyFactory to produce Passkey shape (arrays + booleans). |
| libs/accounts/passkey/src/lib/webauthn-adapter.ts | Extract prfEnabled from authenticatorExtensionResults. |
| libs/accounts/passkey/src/lib/webauthn-adapter.spec.ts | Add test coverage for prfEnabled extraction defaults/variants. |
| libs/accounts/passkey/src/lib/passkey.service.ts | Stop mapping to NewPasskey; register using Passkey and propagate prfEnabled. |
| libs/accounts/passkey/src/lib/passkey.service.spec.ts | Update expectations to the new Passkey shape and include prfEnabled. |
| libs/accounts/passkey/src/lib/passkey.repository.ts | Convert Passkey → NewPasskey at insert time (stringify transports + boolean→0/1). |
| libs/accounts/passkey/src/lib/passkey.manager.ts | Update registerPasskey signature to accept Passkey. |
| libs/accounts/passkey/src/lib/passkey.manager.in.spec.ts | Update integration tests to use boolean flags for passkeys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export interface Passkeys { | ||
| uid: Buffer; | ||
| credentialId: Buffer; | ||
| publicKey: Buffer; | ||
| signCount: number; | ||
| transports: Json; | ||
| transports: JSONColumnType<string[]>; | ||
| aaguid: Buffer; |
There was a problem hiding this comment.
transports was changed to JSONColumnType<string[]>, which assumes the underlying MySQL column is a real JSON type and will be returned as an array at runtime. In this repo there’s still a conflicting schema used by testAccountDatabaseSetup (see libs/shared/db/mysql/account/src/test/passkeys.sql:6, where transports is varchar(255) DEFAULT NULL). If that test schema (or any deployed environment) is still in use, reads will return a string/NULL rather than string[], and code that treats transports as an array may misbehave. Consider updating the test schema to match the migrations (e.g., packages/db-migrations/databases/fxa/patches/patch-184-185.sql:15 uses transports JSON NOT NULL) or reverting the Kysely typing to reflect the actual column type used in all environments.
febe363 to
fcdc563
Compare
| @@ -95,9 +95,9 @@ describe('PasskeyManager (Integration)', () => { | |||
| // Pass lastUsedAt: null explicitly — PasskeyFactory may randomise it | |||
| const newPasskey = PasskeyFactory({ | |||
There was a problem hiding this comment.
change to const passkey?
fcdc563 to
702dfd0
Compare
Because: * NewPasskey use integers for boolean values and should be used for db operations only This commit: * make insertPasskey take in a Passkey and transform it to NewPasskey internally Closes FXA-13402
702dfd0 to
72baca7
Compare
Because
This pull request
Issue that this pull request solves
Closes: FXA-13402
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Based on #20341 because otherwise there will be a merge conflict.