[webkit-reviews] review granted: [Bug 213404] [WebAuthn] Provide a _WKWebAuthenticationPanelUpdatePINInvalid update to UI clients if the returned PIN from the client is not valid : [Attachment 402322] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 213404: [WebAuthn] Provide a _WKWebAuthenticationPanelUpdatePINInvalid
update to UI clients if the returned PIN from the client is not valid
https://bugs.webkit.org/show_bug.cgi?id=213404

Attachment 402322: Patch

https://bugs.webkit.org/attachment.cgi?id=402322&action=review




--- Comment #3 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 402322
  --> https://bugs.webkit.org/attachment.cgi?id=402322
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402322&action=review

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp:277
> +    if (!pin) {

I know this works, but it's confusing. I'd prefer to see:

if (pin.isNull()) {

Also: What if the pin is an empty string? Shouldn't we hit this code too?

operator ! only checks null, which means we would pass the empty string to the
late parts of this method. Maybe that's okay because it fails the
'validateAndConvertToUTF8' step?

if (pin.isNull() || pin.isEmpty()) {

> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:975
> +    webAuthenticationPanelPin = "1234";

Should we have a test case where the WebKit client passed "" as the string?