Summary: | [WebAuthn] Implement add/remove_virtual_authenticator for transport=internal | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | pascoe <pascoe> | ||||||||||||
Component: | WebDriver | Assignee: | 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
pascoe@apple.com
2021-11-02 12:29:27 PDT
Created attachment 443126 [details]
Patch
Created attachment 443135 [details]
Patch
Created attachment 443219 [details]
Patch
Created attachment 443230 [details]
Patch
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? Created attachment 443257 [details]
Patch
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 on attachment 443257 [details]
Patch
r=me
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]. |