Bug 232635 - [WebAuthn] Implement add/remove_virtual_authenticator for transport=internal
Summary: [WebAuthn] Implement add/remove_virtual_authenticator for transport=internal
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: j_pascoe@apple.com
Keywords: InRadar
Depends on:
Blocks: 231242
  Show dependency treegraph
Reported: 2021-11-02 12:29 PDT by j_pascoe@apple.com
Modified: 2021-11-04 10:40 PDT (History)
5 users (show)

See Also:

Patch (54.53 KB, patch)
2021-11-02 13:14 PDT, j_pascoe@apple.com
no flags Details | Formatted Diff | Diff
Patch (51.91 KB, patch)
2021-11-02 14:28 PDT, j_pascoe@apple.com
no flags Details | Formatted Diff | Diff
Patch (51.64 KB, patch)
2021-11-03 12:03 PDT, j_pascoe@apple.com
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.60 KB, patch)
2021-11-03 13:03 PDT, j_pascoe@apple.com
no flags Details | Formatted Diff | Diff
Patch (51.77 KB, patch)
2021-11-03 17:03 PDT, j_pascoe@apple.com
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description j_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
Comment 2 j_pascoe@apple.com 2021-11-02 13:14:41 PDT
Created attachment 443126 [details]
Comment 3 j_pascoe@apple.com 2021-11-02 13:31:18 PDT
Comment 4 j_pascoe@apple.com 2021-11-02 14:28:36 PDT
Created attachment 443135 [details]
Comment 5 j_pascoe@apple.com 2021-11-03 12:03:23 PDT
Created attachment 443219 [details]
Comment 6 j_pascoe@apple.com 2021-11-03 13:03:18 PDT
Created attachment 443230 [details]
Comment 7 Brent Fulgham 2021-11-03 14:54:46 PDT
Comment on attachment 443230 [details]

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 j_pascoe@apple.com 2021-11-03 17:03:37 PDT
Created attachment 443257 [details]
Comment 9 j_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]

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].