fix: add MFA for WebAuthn bindings #960
Conversation
| credential_request_options: { publicKey: PublicKeyCredentialRequestOptionsJSON } | ||
| } | ||
| error: null | ||
| } | ||
| | { data: null; error: AuthError } | ||
|
|
||
| export type AuthMFAListFactorsResponse = |
There was a problem hiding this comment.
Decide whether to include aaguid to image / authenticator type mapping for user: https://github.com/passkeydeveloper/passkey-authenticator-aaguids
or to include it in documentation
|
|
||
| /** Timestamp in UNIX seconds when this challenge will no longer be usable. */ | ||
| expires_at: number | ||
| credential_creation_options: { publicKey: PublicKeyCredentialCreationOptionsJSON } |
There was a problem hiding this comment.
Try to see if it's possible to merge this into a single type again
| * | ||
| * Helper method to compliment `bufferToBase64URLString` | ||
| */ | ||
| export function base64URLStringToBuffer(base64URLString: string): ArrayBuffer { |
There was a problem hiding this comment.
Can switch to base64URL-js as well. This is taken from simplewebauthn, I think they should be similar functionally
There was a problem hiding this comment.
Yes please let's use our library as that will come in handy for asymmetric JWTs too.
| cause: Error | ||
| name?: string | ||
| }) { | ||
| // @ts-ignore: help Rollup understand that `cause` is okay to set |
There was a problem hiding this comment.
The build fails because of this ts-ignore comment in this helper function, which is ported over from simple-webauthn. Removing this and the related function for identifying the type of navigator error will allow the build to compile.
Still figuring out how to resolve this so we can keep the function.
| * The two types of credentials as defined by bit 3 ("Backup Eligibility") in authenticator data: | ||
| * - `"singleDevice"` credentials will never be backed up | ||
| * - `"multiDevice"` credentials can be backed up | ||
| */ |
There was a problem hiding this comment.
Types are ported from Simple-WebAuthn will add a comment to acknowledge.
| if (error) return { data: null, error } | ||
|
|
||
| await this._saveSession({ | ||
| expires_at: Math.round(Date.now() / 1000) + data.expires_in, |
There was a problem hiding this comment.
Let's move this in _saveSession.
| function warnOnBrokenImplementation(methodName: string, cause: Error): void { | ||
| console.warn( | ||
| `The browser extension that intercepted this WebAuthn API call incorrectly implemented ${methodName}. You should report this error to them.\n`, | ||
| cause | ||
| ) | ||
| } |
There was a problem hiding this comment.
How does this apply to MFA WebAuthn -- we're not using passkeys as we're asking for the cross-platform authenticators.
| /** | ||
| * Attempt to intuit _why_ an error was raised after calling `navigator.credentials.create()` | ||
| */ | ||
| export function identifyRegistrationError({ |
There was a problem hiding this comment.
Are these lifted directly from simplewebauthn? I'd prefer if we didn't do any additional error handling other than what the browser returns. Most of these are not relevant for the MFA use case anyway.
| }) | ||
| } | ||
| } else if (error.name === 'ConstraintError') { | ||
| if (publicKey.authenticatorSelection?.requireResidentKey === true) { |
| cause: error, | ||
| }) | ||
| } else if (publicKey.authenticatorSelection?.userVerification === 'required') { | ||
| // https://www.w3.org/TR/webauthn-2/#sctn-op-make-cred (Step 5) |
There was a problem hiding this comment.
This error can be simply included in the enroll step.
| } else if (error.name === 'InvalidStateError') { | ||
| // https://www.w3.org/TR/webauthn-2/#sctn-createCredential (Step 20) | ||
| // https://www.w3.org/TR/webauthn-2/#sctn-op-make-cred (Step 3) | ||
| return new WebAuthnError({ |
There was a problem hiding this comment.
I also don't think this applies for MFA.
| cause: error, | ||
| }) | ||
| } else if (error.name === 'NotSupportedError') { | ||
| const validPubKeyCredParams = publicKey.pubKeyCredParams.filter( |
There was a problem hiding this comment.
This is unlikely to be ever thrown as all authenticators support P-256 and/or RSA.
| }) | ||
| } else if (error.name === 'SecurityError') { | ||
| const effectiveDomain = window.location.hostname | ||
| if (!isValidDomain(effectiveDomain)) { |
There was a problem hiding this comment.
Not possible to be thrown as the library chooses the origin for the developer.
| cause: error, | ||
| }) | ||
| } else if (publicKey.rp.id !== effectiveDomain) { | ||
| // https://www.w3.org/TR/webauthn-2/#sctn-createCredential (Step 8) |
| }) | ||
| } | ||
| } else if (error.name === 'TypeError') { | ||
| if (publicKey.user.id.byteLength < 1 || publicKey.user.id.byteLength > 64) { |
There was a problem hiding this comment.
Also impossible to be thrown given the server should always correctly set up a user ID.
What kind of change does this PR introduce?
We introduce bindings for MFA (WebAuthn) which consists of the following:
enroll()a. Single Step -
enroll({factorType: 'webauthn'})b. Multi-Step -
enroll(factorType: 'webauthn, webAuthn{...})challenge()-verify()a. Single Step -
verify({factorType: 'webauthn'})b. Multi-Step -
verify({factorType: 'webauthn', webAuthn{..}})We also expose a few helper methods:
identifyAuthenticationErrorbrowserSupportsWebAuthn