Bug 213404 – [WebAuthn] Provide a _WKWebAuthenticationPanelUpdatePINInvalid update to UI clients if the returned PIN from the client is not valid

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

Provide a _WKWebAuthenticationPanelUpdatePINInvalid update to UI clients if the returned PIN from the client is not valid.

[Radar WebKit Bug Importer]

<rdar://problem/64543894>

[Jiewen Tan]

Created attachment 402322 [details]
Patch

[Brent Fulgham]

Comment on attachment 402322 [details]
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?

[Darin Adler]

(In reply to Brent Fulgham from comment #3)
> if (pin.isNull() || pin.isEmpty()) {

You never need to write this. Instead:

    if (pin.isEmpty()) {

[Jiewen Tan]

Comment on attachment 402322 [details]
Patch

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

Thanks, Brent for r+ this patch.

>> 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()) {

Yes, it fails validateAndConvertToUTF8 step. Therefore, it's not necessary to check anything here. Actually, I think it is better because in the current implementation of the UI, the user can submit an empty PIN code, and I don't think we will to treat it as a cancel.

[Jiewen Tan]

Comment on attachment 402322 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:975
>> +    webAuthenticationPanelPin = "1234";
> 
> Should we have a test case where the WebKit client passed "" as the string?

It has the same effect as the "123" above. So I think it's okay to not have it.

[Jiewen Tan]

Created attachment 402342 [details]
Patch for landing

[Jiewen Tan]

(In reply to Darin Adler from comment #4)
> (In reply to Brent Fulgham from comment #3)
> > if (pin.isNull() || pin.isEmpty()) {
> 
> You never need to write this. Instead:
> 
>     if (pin.isEmpty()) {

Thanks, Darin!

[EWS]

Committed r263296: <https://trac.webkit.org/changeset/263296>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402342 [details].