MasterKale / SimpleWebAuthn

WebAuthn, Simplified. A collection of TypeScript-first libraries for simpler WebAuthn integration. Supports modern browsers, Node, Deno, and more.
https://simplewebauthn.dev
MIT License
1.62k stars 137 forks source link

ReferenceError: PublicKeyCredential is not defined #154

Closed MaKleSoft closed 3 years ago

MaKleSoft commented 3 years ago

Looks like platformAuthenticatorIsAvailable is missing a check for whether PublicKeyCredential is defined at all: https://github.com/MasterKale/SimpleWebAuthn/blob/master/packages/browser/src/helpers/platformAuthenticatorIsAvailable.ts#L8. Maybe just add (await browserSupportsWebAuthn()) && ..?

Also, minor nitpick: isPlatformAuthenticatorAvailable would seem like a more sensible name to me since it's actually phrased like a question and not a statements plus it's a little closer to the original method name. Food for thought 😊

MasterKale commented 3 years ago

Hmm, so the new method never has a chance to execute because the bundle itself is broken? And in what browser did you get that ReferenceError?

MaKleSoft commented 3 years ago

No, the bundle is fine. It's just a runtime error that occurs in browsers/webviews that don't support Webauthn at all. I encountered this in the Android webview.

MasterKale commented 3 years ago

@MaKleSoft Thank you for clarifying, I went ahead and prepped PR #156 to fix this.

Also, minor nitpick: isPlatformAuthenticatorAvailable would seem like a more sensible name to me since it's actually phrased like a question and not a statements plus it's a little closer to the original method name. Food for thought 😊

My intention with naming the feature check methods what they are right now was to make the if statements they get used in sound almost like normal sentences:

if (browserSupportsWebAuthn()) {
  // ...
}

if (await platformAuthenticatorIsAvailable()) {
  // ...
}

I may have painted myself into a corner by not thinking it through all the way, but it was a stylistic choice I still stick by for now.

...I blame parental sleep deprivation 😪

MaKleSoft commented 3 years ago

My intention with naming the feature check methods what they are right now was to make the if statements they get used in sound almost like normal sentences:

Yeah that came to me as well after writing my comment. I guess it doesn't really matter as long as the naming is somewhat consistent - and your version is in fact more consistent with browserSupportsWebAuthn. Well, you know what they say about naming in programming 😄 . Anyway, enough with the bikeshedding 🙊

MasterKale commented 3 years ago

Hmm, I don't think I like PR merges closing issues, I might try to turn that off. In any case I've merged #156 and have it earmarked for v4.1.0 release. I'll follow up when that's been published.

MasterKale commented 3 years ago

This issue has been fixed in the new @simplewebauthn/browser@4.1.0.