[webkit-reviews] review granted: [Bug 143491] [WebAuthN] Implement FIDO AppID extension : [Attachment 365256] 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 143491: [WebAuthN] Implement FIDO AppID extension
https://bugs.webkit.org/show_bug.cgi?id=143491

Attachment 365256: Patch

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




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

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

Nice work getting this together. r=me with the changes suggested, and assuming
tests pass when you are done.

>> Source/WebCore/Modules/webauthn/fido/U2fCommandConstructor.h:65
>> +WEBCORE_EXPORT Optional<Vector<uint8_t>> convertToU2fSignCommand(const
Vector<uint8_t>& clientDataHash, const
WebCore::PublicKeyCredentialRequestOptions&, const Vector<uint8_t>& keyHandle,
bool isAppId = false);
> 
> I should explain the change in the ChangeLog: the checkOnly flag is never
used and therefore is deleted.

Yes -- I got confused when I looked at the implementation, until I saw this
note! :-)

> Source/WebKit/UIProcess/WebAuthentication/fido/U2fHidAuthenticator.cpp:214
> +	       response->appid = true;

Could this be:

response->appid = m_isAppId;