Conversation
There was a problem hiding this comment.
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
| * The service is tested in isolation, so the manager is mocked to avoid hitting the database. | ||
| */ | ||
| const mockManager: jest.Mocked< | ||
| Pick< |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
What would be the advantage of switching to createMock? Easier maintenance?
There was a problem hiding this comment.
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.
31ac7b9 to
a6f7bbf
Compare
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Outdated PUT path mention; looks like we also have some PUT references to clean out from follow-up tickets too.
| PASSKEY_PRF_NOT_ENABLED: 236, | ||
| PASSKEY_VERIFICATION_PROOF_INVALID: 237, |
There was a problem hiding this comment.
Errors 236/237 are added but never used here - confirming these are needed and simply reserved for future tickets?
There was a problem hiding this comment.
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 🙂
| * The service is tested in isolation, so the manager is mocked to avoid hitting the database. | ||
| */ | ||
| const mockManager: jest.Mocked< | ||
| Pick< |
There was a problem hiding this comment.
What would be the advantage of switching to createMock? Easier maintenance?
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.
Because
passkeyWraps.This pull request
PasskeyWrapsto the kysely types with its Selectable/Insertable types, a factory, and an integration-test fixture.findPasskeyByUidAndCredentialIdto the passkey repository, so an ownership check is one primary-key lookup returning the full record.AppErrorfactories, and corrects the lib README's error-handling section.testAccountDatabaseSetupfixture SQL sequentially rather than throughPromise.all.Issue that this pull request solves
Closes: FXA-13140
Checklist
Put an
xin the boxes that applyHow to review (Optional)
passkey.wrap.repository.tsand the wrap methods inpasskey.service.ts.BINARYrather 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)
versioncolumn 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.updatedAtis deferred to its own PR. The column isNOT NULLand now always equalscreatedAt, so it should go, but removing it touches a migration plus the type, factory, fixture and tests — out of scope here.kBchange deletes wraps instead of updating them, sodeleteAllPasskeyWrapsForUseris the only invalidation route; wiring it into password reset is follow-up work.