Skip to content

task(passkey): Add passkeyWraps model, repository and service - #21072

Merged
nshirley merged 1 commit into
mainfrom
FXA-13140
Aug 26, 2026
Merged

task(passkey): Add passkeyWraps model, repository and service#21072
nshirley merged 1 commit into
mainfrom
FXA-13140

Conversation

@nshirley

@nshirley nshirley commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Because

  • The wrap endpoints (FXA-13142) need to read and write passkeyWraps.
  • A corrupted envelope cannot be reconstructed, so every fixed-width binary field has to round-trip untouched.

This pull request

  • Adds PasskeyWraps to the kysely types with its Selectable/Insertable types, a factory, and an integration-test fixture.
  • Adds the wrap repository — find, insert and uid-scoped delete — asserting every binary field's v1 width before writing.
  • Adds findPasskeyByUidAndCredentialId to the passkey repository, so an ownership check is one primary-key lookup returning the full record.
  • Adds service store and get methods with ownership and idempotency checks, plus StatsD counters.
  • Adds errnos 234–235 with AppError factories, and corrects the lib README's error-handling section.
  • Runs testAccountDatabaseSetup fixture SQL sequentially rather than through Promise.all.

Issue that this pull request solves

Closes: FXA-13140

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).
  • I have manually reviewed all AI generated code.

How to review (Optional)

  • Key files/areas to focus on: passkey.wrap.repository.ts and the wrap methods in passkey.service.ts.
  • Suggested review order: kysely types → repository → service → tests.
  • Risky or complex parts: the width assertions. MySQL right-pads BINARY rather than rejecting, so a wrong width is a permanently unopenable envelope.

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

  • An earlier revision added a version column and patch 196→197. Dropped after review: the only changes it could discriminate are same-width AEAD or KDF swaps, which are both unlikely and survivable by retry, while a KEM change resizes the columns anyway and is self-describing.
  • Dropping updatedAt is deferred to its own PR. The column is NOT NULL and now always equals createdAt, so it should go, but removing it touches a migration plus the type, factory, fixture and tests — out of scope here.
  • No re-seal path. A kB change deletes wraps instead of updating them, so deleteAllPasskeyWrapsForUser is the only invalidation route; wiring it into password reset is follow-up work.
  • Pairs with feat(settings): Add passkey HPKE key-wrapping crypto lib #21006 (FXA-13144), which builds the envelopes this stores.

Comment thread libs/accounts/passkey/src/lib/passkey.service.ts Outdated
Comment thread libs/accounts/passkey/src/lib/passkey.service.ts Outdated
Comment thread libs/accounts/passkey/src/lib/passkey.service.ts Outdated
Comment thread libs/accounts/passkey/src/lib/passkey.service.ts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just clean up for documentation that was out of date. It was originally written with the first iteration of errors in the library, but we later pivoted and didn't update the README

Comment thread libs/shared/db/mysql/account/src/lib/kysely-types.ts Outdated
* The service is tested in isolation, so the manager is mocked to avoid hitting the database.
*/
const mockManager: jest.Mocked<
Pick<

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have used the golevelup createMock, but that would require adding that as a dev dependency at the root and I didn't want to do that here... but might be worth it. This just let's us do near the same thing and maintain strong typing on the tests

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the advantage of switching to createMock? Easier maintenance?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, yeah. The createMock from golevelup creates a partial mock of whatever type is given, so we don't have to do this Pick<Type, 'union' | 'properties'> pattern. It would just be createMock<PasskeyManager>({/** optional property overrides */}); and that's it. Code is cleaner and easier to manage.

I'll file something under the follow up epics. It doesn't need to happen, but will make it a bit less of a headache down the road.

@nshirley
nshirley marked this pull request as ready for review August 24, 2026 16:19
@nshirley
nshirley requested a review from a team as a code owner August 24, 2026 16:19
Copilot AI balanced review requested due to automatic review settings August 24, 2026 16:19

This comment was marked as outdated.

Comment thread libs/accounts/passkey/src/lib/passkey.manager.ts Outdated
@nshirley
nshirley force-pushed the FXA-13140 branch 2 times, most recently from 31ac7b9 to a6f7bbf Compare August 25, 2026 16:13

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Snuck this in here since I wanted the integration tests for wraps, but the FK means the migrations must run in a specific order

@vpomerleau
vpomerleau requested review from vpomerleau and a balanced review from Copilot August 25, 2026 17:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

@vpomerleau vpomerleau left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks solid 💪 Just a few small comments/thoughts/questions

*
* Idempotent: an identical payload is a no-op reported as `unchanged`, so the
* route answers 200 without emitting an event. A different payload is a
* conflict — replacing goes through the PUT path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outdated PUT path mention; looks like we also have some PUT references to clean out from follow-up tickets too.

Comment thread libs/accounts/errors/src/constants.ts Outdated
Comment on lines +144 to +145
PASSKEY_PRF_NOT_ENABLED: 236,
PASSKEY_VERIFICATION_PROOF_INVALID: 237,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors 236/237 are added but never used here - confirming these are needed and simply reserved for future tickets?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are planned to be used in a future ticket, but thanks for flagging. I'll remove them to keep the scope to just the lib work and the future ticket can implement them 🙂

Comment thread libs/accounts/passkey/src/lib/passkey.manager.ts
* The service is tested in isolation, so the manager is mocked to avoid hitting the database.
*/
const mockManager: jest.Mocked<
Pick<

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the advantage of switching to createMock? Easier maintenance?

Comment thread libs/accounts/passkey/src/lib/passkey.service.ts
Comment thread libs/accounts/passkey/src/lib/passkey.wrap.repository.ts
Comment thread libs/shared/db/mysql/account/src/lib/factories.ts Outdated
Because:
* The wrap endpoints (FXA-13142) need to read and write passkeyWraps.
* A wrap's fixed-width binary fields have to round-trip untouched, since a
  corrupted envelope cannot be reconstructed.

This commit:
* Adds the PasskeyWraps kysely type, a factory and an integration-test
  fixture.
* Adds errnos 234-235 with AppError factories, for a missing wrap and a
  conflicting one.
* Adds the wrap repository and service store/get, with ownership scoped in
  SQL and idempotency by full-envelope compare.
* Adds deleteAllPasskeyWrapsForUser as the only invalidation path; a kB
  change drops wraps instead of re-sealing them.
@nshirley
nshirley merged commit 6f96e23 into main Aug 26, 2026
21 checks passed
@nshirley
nshirley deleted the FXA-13140 branch August 26, 2026 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants