[webkit-reviews] review granted: [Bug 191535] [WebAuthN] Support U2F HID Authenticators on macOS : [Attachment 358633] 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 191535: [WebAuthN] Support U2F HID Authenticators on macOS
https://bugs.webkit.org/show_bug.cgi?id=191535

Attachment 358633: Patch

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




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

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

Looks good, but you may need to land it manually (iOS project file seems
unhappy).

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm:120
>      // FIXME(191535): Support U2F authenticators.

Do you still need this comment? If you do, I don't think the Bugzilla # is
right! :-)

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:138
> +	   if (m_requestMessage->cmd() == FidoHidDeviceCommand::kCbor) {

Are there other request messages we are supporting (besides kCbor)? If not,
consider early return here.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:219
> +		   message = FidoHidMessage::create(m_currentChannel,
FidoHidDeviceCommand::kError, {
static_cast<uint8_t>(CtapDeviceResponseCode::kCtap1ErrInvalidCommand) });

Does this cause a console message a developer can see? We might need good
logging here to explain that we don't support older device protocols.

> Source/WebKit/UIProcess/WebAuthentication/fido/U2fHidAuthenticator.cpp:52
> +    // FIXME(191520): We need a way to convert std::unique_ptr to UniqueRef.

You should talk to David Kilzer about that one...