[webkit-reviews] review granted: [Bug 238154] [WebAuthn] Support getAssertion for virtual HID authenticators : [Attachment 455276] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted j_pascoe at apple.com
<j_pascoe at apple.com>'s request for review:
Bug 238154: [WebAuthn] Support getAssertion for virtual HID authenticators
https://bugs.webkit.org/show_bug.cgi?id=238154

Attachment 455276: Patch

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




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

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

r=me, but please correct the pass-by-value mistake.

> Source/WebCore/ChangeLog:9
> +	   Virtual authenticators for WebAuthn support different transprots:
nfc, usb, internal,

*transports

> Source/WebCore/Modules/webauthn/WebAuthenticationUtils.cpp:91
> +

Nit: Extra blank line here.

>
Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualAuthenticatorManager.c
pp:62
> +    VirtualCredential cred = credential;

Weird that you have to make this copy!

>
Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualAuthenticatorManager.c
pp:66
> +Vector<VirtualCredential>
VirtualAuthenticatorManager::credentialsMatchingList(const String&
authenticatorId, const String& rpId, Vector<Vector<uint8_t>> credentialIds)

credentialIds should be passed as a const& since you only read it.

>
Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualAuthenticatorUtils.h:3
9
> +Vector<uint8_t> signatureForPrivateKey(RetainPtr<SecKeyRef> privateKey,
const Vector<uint8_t>& authData, const Vector<uint8_t> clientDataHash);

clientDataHash should be a const referenced, not just a const vector.