Provide a _WKWebAuthenticationPanelUpdatePINInvalid update to UI clients if the returned PIN from the client is not valid.
<rdar://problem/64543894>
Created attachment 402322 [details] Patch
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?
(In reply to Brent Fulgham from comment #3) > if (pin.isNull() || pin.isEmpty()) { You never need to write this. Instead: if (pin.isEmpty()) {
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.
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.
Created attachment 402342 [details] Patch for landing
(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!
Committed r263296: <https://trac.webkit.org/changeset/263296> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402342 [details].