task(libs): Add passkeys data model and db migration#20019
Conversation
fe27ff0 to
5907c00
Compare
5907c00 to
4f71766
Compare
|
This should now be resolved. |
c003c2b to
fdcb65f
Compare
|
|
||
| ### name | ||
|
|
||
| - **Type**: string | null |
There was a problem hiding this comment.
Should this be nullable? Seems like we'd want to be consistent and require a name?
There was a problem hiding this comment.
Good call, we do want this to be defined even if it's just with a default value like "Passkey"
There was a problem hiding this comment.
Seems reasonable to me. I don't have a good feeling for what names would typically look like, but that seems fine.
| - Software authenticators (browser built-in passkey managers) | ||
| - Privacy-focused hardware keys | ||
| - Platform authenticators (depending on policy) | ||
| - **Normalization**: The PasskeyService normalizes all-zeros AAGUID to `NULL` before storage |
There was a problem hiding this comment.
Just curious we we need to do and maintain this null conversion on 00000000-0000-0000-0000-000000000000?
There was a problem hiding this comment.
Not strictly necessary, I can directly store the all-zero value instead.
There was a problem hiding this comment.
I'm good with either approach, but if there's not a good argument for storing a null value, I'd lean towards the stance that the fewer nullables we have the better.
dschom
left a comment
There was a problem hiding this comment.
Looks great. R+WC. The only thing I'd question is if cascade on delete is really a good idea... It's definitely convenient, but want to make sure we know it can also cause problems in some scenarios.
| .where('credentialId', '=', credentialId) | ||
| .execute(); | ||
|
|
||
| return result.length; |
There was a problem hiding this comment.
Does this always return 1? Thought there was a numUpdatedRows property
| * @param uid - User ID (16-byte Buffer) | ||
| * @returns Array of passkeys for the user (unordered) | ||
| */ | ||
| export async function findPasskeysByUid( |
There was a problem hiding this comment.
Maybe overkill, but I suppose you could write some integration tests to verify the sql.
There was a problem hiding this comment.
I've added a few simple tests!
fdcb65f to
8097077
Compare
Because: * We are adding WebAuthn/FIDO2 passkey support This commit: * Adds database migration creating passkeys table with composite primary key and proper indexing * Generates Kysely types (Passkey, NewPasskey, PasskeyUpdate) with type guards for validation * Implements 8 pure repository functions for CRUD operations following FxA patterns * Creates PasskeyFactory for generating test data and updates integration test infrastructure * Provides comprehensive field documentation in PASSKEY_FIELDS.md covering WebAuthn spec and usage Closes #FXA-12901
8097077 to
512c441
Compare
| import { AccountManager } from '@fxa/shared/account/account'; | ||
| import * as PasskeyRepository from './passkey.repository'; | ||
|
|
||
| describe('PasskeyRepository (Integration)', () => { |
| DELETE FROM securityEvents WHERE uid = uidArg; | ||
| DELETE FROM sentEmails WHERE uid = uidArg; | ||
| DELETE FROM linkedAccounts WHERE uid = uidArg; | ||
| DELETE FROM passkeys WHERE uid = uidArg; |
Because
This pull request
Issue that this pull request solves
Closes: FXA-12901
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.