feat(passkeys): add auth server passkey configs#20133
Conversation
MagentaManifold
left a comment
MagentaManifold
left a comment
There was a problem hiding this comment.
(I didn't feel the need to add a new section in README.md, with inline documentation being sufficiently detailed.)
| format: ['required', 'preferred', 'discouraged'], | ||
| }, | ||
| authenticatorAttachment: { | ||
| default: null, |
There was a problem hiding this comment.
Not sure it default should be null or undefined. Passkey config service uses undefined, but null seems more idiomatic for convict.
There was a problem hiding this comment.
Is there a reason the passkey config is using undefined instead of null?
MagentaManifold
force-pushed
the
FXA-13057
branch
from
fee1f69 to
ac54c39
Compare
| ); | ||
| Container.set(RecoveryPhoneService, recoveryPhoneService); | ||
|
|
||
| const passkeyConfig = buildPasskeyConfig(config.passkeys, log); |
There was a problem hiding this comment.
Added these just to demonstrate how it will be used in auth-server. Remove before merging
MagentaManifold
force-pushed
the
FXA-13057
branch
from
ac54c39 to
e646737
Compare
| @IsIn(['platform', 'cross-platform']) | ||
| public authenticatorAttachment?: AuthenticatorAttachment; | ||
| @IsIn(['platform', 'cross-platform', undefined]) | ||
| public authenticatorAttachment?: AuthenticatorAttachment | undefined; |
There was a problem hiding this comment.
Is the | undefined here for convict compatibility?
Sometimes side stepping the optional vs null vs undefined issue and just using an empty string or explicit value ends up being simpler. eg @IsIn(['platform', 'cross-platform', '']) or @IsIn(['platform', 'cross-platform', 'none']).
There was a problem hiding this comment.
AuthenticatorAttachment | undefined is the type that simplewebauthn/server (the webauthn lib we depend on) takes.
MagentaManifold
force-pushed
the
FXA-13057
branch
from
e646737 to
688c826
Compare
| Container.set(RecoveryPhoneService, recoveryPhoneService); | ||
|
|
||
| const passkeyConfig = buildPasskeyConfig(config.passkeys, log); | ||
| if (passkeyConfig) { |
There was a problem hiding this comment.
I said I would to remove this change before merging, but since it's now conditionally loaded, there's no runtime cost after startup, with passkeys disabled by default. Should we just keep this?
There was a problem hiding this comment.
We could leave it, but commented out for now perhaps?
| authenticatorSelection: { | ||
| residentKey: config.residentKey, | ||
| userVerification: config.userVerification, | ||
| authenticatorAttachment: config.authenticatorAttachment, |
There was a problem hiding this comment.
Piggy-backing on Dan's suggestion - if authenticatorAttachment defaults to an empty string (instead of null and manually converting in the config builder), we could use the following to handle the undefined case:
authenticatorAttachment: config.authenticatorAttachment || undefined
MagentaManifold
force-pushed
the
FXA-13057
branch
2 times, most recently
from
560dc55 to
47da024
Compare
Because: * we need to load passkey configs to auth server This commit: * defines convict passkey configs Closes FXA-13057
MagentaManifold
force-pushed
the
FXA-13057
branch
from
47da024 to
566b091
Compare
3 participants
Because
This pull request
Issue that this pull request solves
Closes: FXA-13057
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.