-
Notifications
You must be signed in to change notification settings - Fork 222
task(libs): Add passkeys data model and db migration #20019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be nullable? Seems like we'd want to be consistent and require a name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly necessary, I can directly store the all-zero value instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe overkill, but I suppose you could write some integration tests to verify the sql.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
| DELETE FROM securityEvents WHERE uid = uidArg; | ||
| DELETE FROM sentEmails WHERE uid = uidArg; | ||
| DELETE FROM linkedAccounts WHERE uid = uidArg; | ||
| DELETE FROM passkeys WHERE uid = uidArg; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
dschom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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.