Summary: | [WebAuthn] Provide a _WKWebAuthenticationPanelUpdatePINInvalid update to UI clients if the returned PIN from the client is not valid | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||
Component: | WebKit Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bfulgham, darin, jiewen_tan, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 181943 | ||||||||
Attachments: |
|
Description
Jiewen Tan
2020-06-19 13:54:06 PDT
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]. |