[Webkit-unassigned] [Bug 230906] Adopt platform UI for WebAuthn

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=230906

Garrett Davidson <garrett_davidson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |garrett_davidson at apple.com

--- Comment #8 from Garrett Davidson <garrett_davidson at apple.com> ---
Comment on attachment 439610
  --> https://bugs.webkit.org/attachment.cgi?id=439610
Patch

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

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:120
>> +            requestTypes = ASCCredentialRequestTypeSecurityKeyPublicKeyRegistration;
> 
> What if `attachment` is not set?  I don't see a check to ensure that `attachment` is set before comparing it.
> 
> Also, shouldn't these comparisons use`*attachment` instead of `attachment` to get the value out of the std::optional<>?

If `attachment` is unset or an unexpected value, then we intentionally leave `requestTypes` unchanged. My understanding from https://en.cppreference.com/w/cpp/utility/optional/operator_cmp is that std::optional does the expected thing with `==` if `attachment` is unset, which local testing seems to confirm (but I'm by no means an expert here ��).

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:174
>> +        requestTypes = ASCCredentialRequestTypeSecurityKeyPublicKeyAssertion;
> 
> Ditto from comment about about checking whether `attachment` is set before using it, and using `*attachment` instead of `attachment` here.

I _think_ this is currently doing what I want, but I'm happy to update it if I'm missing something or there's a style preference.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:218
>> +    [proxy performAuthorizationRequestsForContext:requestContext.get() withClearanceHandler:makeBlockPtr([this, handler = WTFMove(handler), window = WTFMove(window)](NSXPCListenerEndpoint *daemonEnpoint, NSError *error) mutable {
> 
> What happens when the `this` object is deallocated?  Is this call async, or is guaranteed to finish before the method returns?
> 
> If it's async (or `this` can be deleted from another thread), we may need to look into using a WeakPtr for `this`.  If this all happens on the main thread, you don't need WeakPtr.

This method is async. The clearanceHandler may take a while to get invoked, depending on what else is happening on the system. I'll switch to a WeakPtr.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WebAuthenticatorCoordinatorProxy.mm:269
>> +                    exceptionCode = (ExceptionCode) error.code;
> 
> It's a bit dodgy to use a C-style cast here.  Usually we create a helper function to map from one type to the other, then provide a default value if an unknown value is seen.
> 
> Can an `error.code` be returned that doesn't map to `ExceptionCode`?

Currently I don't think so, but I'm also happy to go the helper function route.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211006/7893ba9b/attachment-0001.htm>