feat(passkey): create server side helper functions#20101
feat(passkey): create server side helper functions#20101MagentaManifold merged 1 commit intomainfrom
Conversation
MagentaManifold
force-pushed
the
FXA-13056
branch
from
592564b to
92b38f9
Compare
|
(just rebased to resolve a conflict with main) |
nshirley
left a comment
nshirley
left a comment
There was a problem hiding this comment.
A few minor comments, but I'll leave it to you if you want to address them here - let me know what you think!
|
|
||
| const libMocks = jest.requireMock('@simplewebauthn/server') as { | ||
| generateRegistrationOptions: jest.MockedFunction< | ||
| (...args: unknown[]) => Promise<unknown> |
There was a problem hiding this comment.
Do we not know or have the types available here? If the underlying library changes function signature, we won't catch the changed with typescript, only during runtime. Probably unlikely, but also means we don't get strong typing on the expected results
There was a problem hiding this comment.
I tried introducing actual types here, but it ended up requiring using as to assert mocked values' types everywhere, which doesn't help with making types stronger. If the function changes to an incompatible signature, the main file won't compile, so I think we are fine.
There was a problem hiding this comment.
That's fair. Having to use type assertion isn't great and just kind of moves the problem elsewhere. Thanks for trying though!
| * | ||
| * @param config - PasskeyConfig instance (provides allowedOrigins, rpId) | ||
| * @param input - Per-request inputs: browser response, challenge, stored credential | ||
| * @returns verification result and extracted data (not a discriminated union, to match library output) |
There was a problem hiding this comment.
I'm curious if it makes sense to have a better interface because we're making the wrapper.
As it is, if the verification fails, a caller could inadvertently still check data and proceed. But, using a discriminated union would allow strong type support and a caller would be able to check :
const result = await verifyRegistrationResponse(config, input);
if (result.verified) {
// typescript knows the data exists here and we can do something with it
}
There was a problem hiding this comment.
There must be a reason why this function doesn't use a discriminated union, while the registration one does. Maybe it's for logging failed attempts? That said, in our case, if we only return signCount and sync status, there's no point in logging them for a failed attempt anyway. I'll make it a discriminated union
Because: * we need helper functions for passkeys This commit: * creates helpers wrapping @simplewebauthn/server library functions Closes FXA-13056
MagentaManifold
force-pushed
the
FXA-13056
branch
from
92b38f9 to
7493ca0
Compare
nshirley
left a comment
nshirley
left a comment
There was a problem hiding this comment.
Thanks for checking out my suggestions!
2 participants
Because
This pull request
Issue that this pull request solves
Closes: FXA-13056
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.