Bug 232635

Summary: [WebAuthn] Implement add/remove_virtual_authenticator for transport=internal
Product: WebKit Reporter: pascoe <pascoe>
Component: WebDriverAssignee: pascoe <pascoe>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, bfulgham, ews-watchlist, jiewen_tan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 231242    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch none

Description pascoe@apple.com 2021-11-02 12:29:27 PDT
This is a subtask of https://bugs.webkit.org/show_bug.cgi?id=231242

This bug covers the skeleton for the virtual authenticators commands as well as the implementation for add_virtual_authenticator/remove_virtual_authenticator for transport=internal (platform authenticator/local).
Comment 1 Radar WebKit Bug Importer 2021-11-02 12:29:40 PDT
<rdar://problem/84942173>
Comment 2 pascoe@apple.com 2021-11-02 13:14:41 PDT
Created attachment 443126 [details]
Patch
Comment 3 pascoe@apple.com 2021-11-02 13:31:18 PDT
https://www.w3.org/TR/webauthn-2/#sctn-automation
Comment 4 pascoe@apple.com 2021-11-02 14:28:36 PDT
Created attachment 443135 [details]
Patch
Comment 5 pascoe@apple.com 2021-11-03 12:03:23 PDT
Created attachment 443219 [details]
Patch
Comment 6 pascoe@apple.com 2021-11-03 13:03:18 PDT
Created attachment 443230 [details]
Patch
Comment 7 Brent Fulgham 2021-11-03 14:54:46 PDT
Comment on attachment 443230 [details]
Patch

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

I think this patch looks really good, but a couple of improvements could be made. r- to correct the pass-by-value things (or help me understand why I'm wrong), and to confirm we don't have any racy behavior with objects going out of scope.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1560
> +    WebPageProxy* page = webPageProxyForHandle(browsingContextHandle);

Could be 'auto'

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1563
> +    VirtualAuthenticatorManager& authenticatorManager = page->websiteDataStore().virtualAuthenticatorManager();

Couldn't this be "const auto&"?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1605
> +    VirtualAuthenticatorManager& authenticatorManager = page->websiteDataStore().virtualAuthenticatorManager();

Ditto the auto and "const auto&" comments above.

> Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualAuthenticatorManager.cpp:41
> +String VirtualAuthenticatorManager::createAuthenticator(VirtualAuthenticatorConfiguration config)

Seems like the config argument should be a const & or a Move operation.

> Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualLocalConnection.h:41
> +    void verifyUser(const String&, WebCore::ClientDataType, SecAccessControlRef, WebCore::UserVerificationRequirement,  UserVerificationCallback&&) final;

Extra space before UserVerificationCallback&&

> Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualLocalConnection.mm:55
> +    RunLoop::main().dispatch([configuration = m_configuration, callback = WTFMove(callback)]() mutable {

Do we copy the configuration here because we think the main object may go out of scope? If so, we should hold a weakPtr to 'this' and bail out early if it is nullptr.

If we don't think it's possible for 'this' to be deleted before this code is called, maybe pass a reference or just access the object through this?

> Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualService.h:39
> +    static UniqueRef<AuthenticatorTransportService> createVirtual(WebCore::AuthenticatorTransport, Observer&, const Vector<VirtualAuthenticatorConfiguration> configs);

Do you really want to pass a vector of VirtualAuthenticatorConfiguration by copy? Seems like you should move them, or pass a const ref.

> Source/WebKit/UIProcess/WebAuthentication/Virtual/VirtualService.mm:54
> +            observer()->authenticatorAdded(LocalAuthenticator::create(makeUniqueRef<VirtualLocalConnection>(config)));

Is it possible for 'authenticatorAdded' to call anything that could cause observer() to return nullptr?
Comment 8 pascoe@apple.com 2021-11-03 17:03:37 PDT
Created attachment 443257 [details]
Patch
Comment 9 pascoe@apple.com 2021-11-03 17:07:41 PDT
Updated patch that addresses all comments except:
> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1563
> +    VirtualAuthenticatorManager& authenticatorManager = page->websiteDataStore().virtualAuthenticatorManager();

"Couldn't this be "const auto&"?"


WebsiteDataStore::virtualAuthenticatorManager() can't be const because it sets m_authenticatorManager
Comment 10 Brent Fulgham 2021-11-04 10:30:33 PDT
Comment on attachment 443257 [details]
Patch

r=me
Comment 11 EWS 2021-11-04 10:40:47 PDT
Committed r285267 (243878@main): <https://commits.webkit.org/243878@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443257 [details].