[webkit-reviews] review granted: [Bug 206112] [WebAuthn] Implement SPI to tell UI clients to select assertion responses : [Attachment 387572] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 206112: [WebAuthn] Implement SPI to tell UI clients to select assertion
responses
https://bugs.webkit.org/show_bug.cgi?id=206112

Attachment 387572: Patch

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




--- Comment #10 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 387572
  --> https://bugs.webkit.org/attachment.cgi?id=387572
Patch

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

r=me with a bunch of nits.

> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.cpp:48
> +static void releaseUserHandle(unsigned char*, const void* data)

This can be inline by making it a lambda that doesn't capture anything.

> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.cpp:51
> +    static_cast<ArrayBuffer*>(const_cast<void*>(data))->deref();

Can we change the const void* to just void* in FreeDataFunction instead of
this?  A callback that gives you something to destroy should not give you a
const pointer.

> Source/WebKit/UIProcess/API/APIWebAuthenticationAssertionResponse.cpp:59
> +	   data = API::Data::createWithoutCopying((const unsigned
char*)userHandle->data(), userHandle->byteLength(), releaseUserHandle,
userHandle);

reinterpret_cast rather than c-style cast.

> Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.h:73
> +- (void)panel:(_WKWebAuthenticationPanel *)panel
selectAssertionResponses:(NSArray < _WKWebAuthenticationAssertionResponse *>
*)responses completionHandler:(void (^)(_WKWebAuthenticationAssertionResponse
*))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

selectAssertionResponse
Shouldn't be plural.  You're supposed to select one.

>
Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticationPanelClient.mm
:156
> +    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), this,
nsResponses = WTFMove(nsResponses), completionHandler =
WTFMove(completionHandler)] () mutable {

The original bug fix should've gone in the C++ caller rather than the API
layer, which shouldn't contain any behavior-modifying code.  For example, if
someone were to make another API for this behavior, like expanding the C API,
they could create an API that doesn't have this.  All APIs should have this, so
it needs to be in the implementation code, not the API code.

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapAuthenticator.cpp:158
> +		   auto returnResponse =
m_assertionResponses.take(const_cast<AuthenticatorAssertionResponse*>(&response
));

I think it would be better to pass a non-const AuthenticatorAssertionResponse&
than this.