This task aims to make an AuthenticatorManager abstraction such that: 1) It lives in UI Process. 2) It manages different authenticators including their lifetime and task distribution. 3) It talks to underlying transport protocol, i.e., HID, BLE, NFC and internal. That means, it will then handle device discovery/removal and provides connections between authenticator abstractions and corresponding physical devices. This functionality could be implemented through some "Service" abstraction, like HIDService for instance. 4) It will also talk to the WebProcess via AuthenticatorCoordinatorProxy. 5) Finally, it will probably handle UI interactions as well, which is currently undefined.
<rdar://problem/44116792>
Created attachment 350561 [details] Patch
Attachment 350561 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:174: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:449: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 77 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 350588 [details] Patch
Attachment 350588 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:174: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:449: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 78 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 350588 [details] Patch Attachment 350588 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9323522 New failing tests: http/wpt/webauthn/public-key-credential-create-failure-local.https.html http/wpt/webauthn/public-key-credential-create-success-local.https.html http/wpt/webauthn/public-key-credential-get-success-local.https.html
Created attachment 350590 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 350593 [details] Patch
Attachment 350593 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:174: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:449: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 80 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 350595 [details] Patch
Attachment 350595 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:174: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:449: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 80 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 350595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350595&action=review Sorry. Only had time to review half the patch. I will get back to it later today. > Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:111 > + if (!authenticatorSelection) { You do not need this encoding / decoding logic I believe. AFAIK, we have coders for std::optional that take care of this for you. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:577 > + auto localRef = static_cast<WKDictionaryRef>(WKDictionaryGetItemForKey(configurationRef, WKStringCreateWithUTF8CString("Local"))); You are leaking the string returned by WKStringCreateWithUTF8CString(). You need to adopt it and store it in a local RetainPtr. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:580 > + configuration.local.acceptAuthentication = WKBooleanGetValue(static_cast<WKBooleanRef>(WKDictionaryGetItemForKey(localRef, WKStringCreateWithUTF8CString("AcceptAuthentication")))); Ditto. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:581 > + configuration.local.acceptAttestation = WKBooleanGetValue(static_cast<WKBooleanRef>(WKDictionaryGetItemForKey(localRef, WKStringCreateWithUTF8CString("AcceptAttestation")))); Ditto. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:582 > + configuration.local.privateKeyBase64 = WebKit::toImpl(static_cast<WKStringRef>(WKDictionaryGetItemForKey(localRef, WKStringCreateWithUTF8CString("PrivateKeyBase64"))))->string(); Ditto. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:584 > + WebKit::toImpl(dataStoreRef)->websiteDataStore().setMockWebAuthenticationConfiguration(configuration); WTFMove(configuration) ? > Source/WebKit/UIProcess/WebAuthentication/Authenticator.cpp:39 > + RunLoop::main().dispatch([weakThis = makeWeakPtr(this)] { makeWeakPtr(*this) would avoid an unnecessary null check. > Source/WebKit/UIProcess/WebAuthentication/Authenticator.cpp:51 > + ASSERT(isMainThread()); ASSERT(RunLoop::isMain()); because an app may be using both WebKit2 and WebKit1 in the UIProcess. > Source/WebKit/UIProcess/WebAuthentication/Authenticator.h:43 > +using Respond = Variant<WebCore::PublicKeyCredentialData, WebCore::ExceptionData>; I think this is too generic a name to be in the global scope. We should probably move it to the Authenticator class scope and use a less generic name. > Source/WebKit/UIProcess/WebAuthentication/Authenticator.h:53 > + Authenticator() = default; This should be protected, not public. > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:43 > +static TransportSet collectTransports(const std::optional<PublicKeyCredentialCreationOptions::AuthenticatorSelectionCriteria> authenticatorSelection) const std::optional<PublicKeyCredentialCreationOptions::AuthenticatorSelectionCriteria> -> const std::optional<PublicKeyCredentialCreationOptions::AuthenticatorSelectionCriteria>& > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:173 > + for (auto transport : transports) { auto& ? > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:43 > +using Callback = CompletionHandler<void(Respond&&)>; Too generic a name to be at global scope. Please move inside class or rename. > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:44 > +using Respond = Variant<WebCore::PublicKeyCredentialData, WebCore::ExceptionData>; Ditto. > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:45 > +using TransportSet = HashSet<WebCore::AuthenticatorTransport, WTF::IntHash<WebCore::AuthenticatorTransport>, WTF::StrongEnumHashTraits<WebCore::AuthenticatorTransport>>; ditto. > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:68 > + // Overrided by MockAuthenticatorManager. Overriden ? > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:70 > + // Overrided to return every exception for tests to confirm. Overriden ? > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorTransportService.cpp:57 > + RunLoop::main().dispatch([weakThis = makeWeakPtr(this)] { makeWeakPtr(*this) > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorTransportService.h:54 > + AuthenticatorTransportService(Observer&); explicit. Also should be make protected since you have create() factories. > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h:53 > + return adoptRef(* new LocalAuthenticator(WTFMove(connection))); No space between * and new. > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h:56 > +protected: Should be private, there is no subclass and class is marked as final. > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h:57 > + LocalAuthenticator(UniqueRef<LocalConnection>&&); explicit. > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:66 > +static Vector<uint8_t> buildAuthData(const String& rpId, const uint8_t flags, const uint32_t counter, const Vector<uint8_t>& optionalAttestedCredentialData) const uint32_t counter: No need for the const. > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:69 > + authData.reserveCapacity(authDataPrefixFixedSize + optionalAttestedCredentialData.size()); reserveInitialCapacity is more efficient here since this is a newly allocated Vector. > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:99 > + return transports.isEmpty() ? true : transports.contains(target); Little bit odd. So if there is no transports we lie and say it contains it? > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:102 > +inline HashSet<String> produceHashSet(const Vector<PublicKeyCredentialDescriptor>& credentialDescriptors) I suggest a less generic function name. Also, it should likely be static. > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:114 > +inline Vector<uint8_t> toVector(NSData *data) static > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:186 > + auto callback = [weakThis = makeWeakPtr(this)](bool success) { makeWeakPtr(*this) > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:187 > + ASSERT(isMainThread()); ASSERT(RunLoop::isMain()); > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:226 > + auto callback = [weakThis = makeWeakPtr(this)](SecKeyRef _Nullable privateKey, NSArray * _Nullable certificates, NSError * _Nullable error) { makeWeakPtr(*this) > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:227 > + ASSERT(isMainThread()); RunLoop::isMain() > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:269 > + label.append("@" + requestData().creationOptions.rp.id + "-rk"); // -rk is added by DeviceIdentity.Framework. This line and the previous one could probably be a single makeString(). > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:385 > + CFDataRef dataRef = SecCertificateCopyData((__bridge SecCertificateRef)certificates[i]); In this file, you keep using raw Ref types and then later on adoptCF() them. It is odd and error prone. Why not use RetainPtr directly? E.g. auto data = adoptCF(SecCertificateCopyData((__bridge SecCertificateRef)certificates[I]));
Comment on attachment 350595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350595&action=review > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h:34 > +using AttestationCallback = CompletionHandler<void(SecKeyRef, NSArray *, NSError *)>; Do these really need to be global? Can they be moved inside LocalConnection class? > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h:35 > +using UserConsentCallback = CompletionHandler<void(bool)>; We should really avoid boolean parameters, it makes call sites hard to read. Should be an enum class such as UserContented { No, Yes }; > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h:36 > +using UserConsentContextCallback = CompletionHandler<void(bool, LAContext *)>; Ditto. > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:46 > + ASSERT(!isMainThread()); !RunLoop::isMain() > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:64 > + ASSERT(!isMainThread()); !RunLoop::isMain() > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:96 > + label.append("@" + rpId); String label = makeString(username, "@", rpId); ? > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalService.h:38 > + LocalService(Observer&); explicit > Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:77 > + callback(false); This looks really obscure. Using an enum class instead of a boolean would look much nicer. > Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:123 > + label.append("@" + rpId + "-rk"); // Mock what DeviceIdentity would do. makeString() > Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp:58 > + ASSERT(isMainThread()); RunLoop::isMain() > Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp:74 > + ASSERT(isMainThread()); RunLoop::isMain() > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1695 > + if (!isMockAuthetnicatorManager) { Typo: isMockAuthetnicatorManager > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:261 > + bool isMockAuthetnicatorManager { false }; Seems like this could be a virtual function on AuthenticatorManager rather than a separate boolean? Unless this is perf sensitive somehow? > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2391 > + localValues.append(toWK(JSValueToStringCopy(context, privateKeyBase64Value, 0))); Looks like you're leaking the value returned by JSValueToStringCopy(). > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2405 > + configurationValues.append(WKDictionaryCreate(rawLocalKeys.data(), rawLocalValues.data(), rawLocalKeys.size())); You need to adopt the value returned by WKDictionaryCreate() > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2460 > +bool TestRunner::isKeyExisted(JSStringRef attrLabel, JSStringRef applicationTagBase64) isKeyExisted(): I am French but this does not sound right. Also the naming is too generic for TestRunner. Could be any kind of key. > Tools/WebKitTestRunner/TestController.h:258 > + void addTestKeyToKeychain(String privateKeyBase64, String attrLabel, String applicationTagBase64); const String& > Tools/WebKitTestRunner/TestController.h:259 > + void cleanUpKeychain(String attrLabel); ditto > Tools/WebKitTestRunner/TestController.h:260 > + bool isKeyExisted(String attrLabel, String applicationTagBase64); ditto
Comment on attachment 350595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350595&action=review The patch is large. Thanks Chris for reviewing the patch. >> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:111 >> + if (!authenticatorSelection) { > > You do not need this encoding / decoding logic I believe. AFAIK, we have coders for std::optional that take care of this for you. Switch to that, but it is weird that I couldn't use: if (!decoder.decode(result.authenticatorSelection)) return std::nullopt; >> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:577 >> + auto localRef = static_cast<WKDictionaryRef>(WKDictionaryGetItemForKey(configurationRef, WKStringCreateWithUTF8CString("Local"))); > > You are leaking the string returned by WKStringCreateWithUTF8CString(). You need to adopt it and store it in a local RetainPtr. Fixed. >> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:580 >> + configuration.local.acceptAuthentication = WKBooleanGetValue(static_cast<WKBooleanRef>(WKDictionaryGetItemForKey(localRef, WKStringCreateWithUTF8CString("AcceptAuthentication")))); > > Ditto. Fixed. >> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:582 >> + configuration.local.privateKeyBase64 = WebKit::toImpl(static_cast<WKStringRef>(WKDictionaryGetItemForKey(localRef, WKStringCreateWithUTF8CString("PrivateKeyBase64"))))->string(); > > Ditto. Fixed. >> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:584 >> + WebKit::toImpl(dataStoreRef)->websiteDataStore().setMockWebAuthenticationConfiguration(configuration); > > WTFMove(configuration) ? Yes, for sure. >> Source/WebKit/UIProcess/WebAuthentication/Authenticator.cpp:39 >> + RunLoop::main().dispatch([weakThis = makeWeakPtr(this)] { > > makeWeakPtr(*this) would avoid an unnecessary null check. Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Authenticator.cpp:51 >> + ASSERT(isMainThread()); > > ASSERT(RunLoop::isMain()); because an app may be using both WebKit2 and WebKit1 in the UIProcess. Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Authenticator.h:43 >> +using Respond = Variant<WebCore::PublicKeyCredentialData, WebCore::ExceptionData>; > > I think this is too generic a name to be in the global scope. We should probably move it to the Authenticator class scope and use a less generic name. Move into the class. >> Source/WebKit/UIProcess/WebAuthentication/Authenticator.h:53 >> + Authenticator() = default; > > This should be protected, not public. Fixed. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:43 >> +static TransportSet collectTransports(const std::optional<PublicKeyCredentialCreationOptions::AuthenticatorSelectionCriteria> authenticatorSelection) > > const std::optional<PublicKeyCredentialCreationOptions::AuthenticatorSelectionCriteria> > -> > const std::optional<PublicKeyCredentialCreationOptions::AuthenticatorSelectionCriteria>& Fixed. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:173 >> + for (auto transport : transports) { > > auto& ? Transport is an enum so that's why I think it might not be needed to do a reference. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:43 >> +using Callback = CompletionHandler<void(Respond&&)>; > > Too generic a name to be at global scope. Please move inside class or rename. Move into the class. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:44 >> +using Respond = Variant<WebCore::PublicKeyCredentialData, WebCore::ExceptionData>; > > Ditto. Same. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:45 >> +using TransportSet = HashSet<WebCore::AuthenticatorTransport, WTF::IntHash<WebCore::AuthenticatorTransport>, WTF::StrongEnumHashTraits<WebCore::AuthenticatorTransport>>; > > ditto. Same. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:68 >> + // Overrided by MockAuthenticatorManager. > > Overriden ? Fixed. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:70 >> + // Overrided to return every exception for tests to confirm. > > Overriden ? Fixed. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorTransportService.cpp:57 >> + RunLoop::main().dispatch([weakThis = makeWeakPtr(this)] { > > makeWeakPtr(*this) Fixed. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorTransportService.h:54 >> + AuthenticatorTransportService(Observer&); > > explicit. > Also should be make protected since you have create() factories. Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h:53 >> + return adoptRef(* new LocalAuthenticator(WTFMove(connection))); > > No space between * and new. Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h:56 >> +protected: > > Should be private, there is no subclass and class is marked as final. Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h:57 >> + LocalAuthenticator(UniqueRef<LocalConnection>&&); > > explicit. Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:66 >> +static Vector<uint8_t> buildAuthData(const String& rpId, const uint8_t flags, const uint32_t counter, const Vector<uint8_t>& optionalAttestedCredentialData) > > const uint32_t counter: No need for the const. Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:69 >> + authData.reserveCapacity(authDataPrefixFixedSize + optionalAttestedCredentialData.size()); > > reserveInitialCapacity is more efficient here since this is a newly allocated Vector. Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:99 >> + return transports.isEmpty() ? true : transports.contains(target); > > Little bit odd. So if there is no transports we lie and say it contains it? emptyTransportsOrContain? >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:102 >> +inline HashSet<String> produceHashSet(const Vector<PublicKeyCredentialDescriptor>& credentialDescriptors) > > I suggest a less generic function name. Also, it should likely be static. The function is under namespace LocalAuthenticatorInternal. So the name should be okay. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:114 >> +inline Vector<uint8_t> toVector(NSData *data) > > static Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:186 >> + auto callback = [weakThis = makeWeakPtr(this)](bool success) { > > makeWeakPtr(*this) Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:187 >> + ASSERT(isMainThread()); > > ASSERT(RunLoop::isMain()); Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:226 >> + auto callback = [weakThis = makeWeakPtr(this)](SecKeyRef _Nullable privateKey, NSArray * _Nullable certificates, NSError * _Nullable error) { > > makeWeakPtr(*this) Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:227 >> + ASSERT(isMainThread()); > > RunLoop::isMain() Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:269 >> + label.append("@" + requestData().creationOptions.rp.id + "-rk"); // -rk is added by DeviceIdentity.Framework. > > This line and the previous one could probably be a single makeString(). Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:385 >> + CFDataRef dataRef = SecCertificateCopyData((__bridge SecCertificateRef)certificates[i]); > > In this file, you keep using raw Ref types and then later on adoptCF() them. It is odd and error prone. Why not use RetainPtr directly? E.g. > auto data = adoptCF(SecCertificateCopyData((__bridge SecCertificateRef)certificates[I])); You are right. Some situations are the the raw Ref types are returned via & from API calls. So, I need to declare them first and then adopt them. For this one, it is wrong though. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h:34 >> +using AttestationCallback = CompletionHandler<void(SecKeyRef, NSArray *, NSError *)>; > > Do these really need to be global? Can they be moved inside LocalConnection class? Move into the class. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h:35 >> +using UserConsentCallback = CompletionHandler<void(bool)>; > > We should really avoid boolean parameters, it makes call sites hard to read. Should be an enum class such as UserContented { No, Yes }; Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h:36 >> +using UserConsentContextCallback = CompletionHandler<void(bool, LAContext *)>; > > Ditto. Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:46 >> + ASSERT(!isMainThread()); > > !RunLoop::isMain() Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:64 >> + ASSERT(!isMainThread()); > > !RunLoop::isMain() Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:96 >> + label.append("@" + rpId); > > String label = makeString(username, "@", rpId); ? Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalService.h:38 >> + LocalService(Observer&); > > explicit Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:77 >> + callback(false); > > This looks really obscure. Using an enum class instead of a boolean would look much nicer. Fixed. >> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:123 >> + label.append("@" + rpId + "-rk"); // Mock what DeviceIdentity would do. > > makeString() Fixed. >> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp:58 >> + ASSERT(isMainThread()); > > RunLoop::isMain() Fixed. >> Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp:74 >> + ASSERT(isMainThread()); > > RunLoop::isMain() Fixed. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1695 >> + if (!isMockAuthetnicatorManager) { > > Typo: isMockAuthetnicatorManager Fixed. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:261 >> + bool isMockAuthetnicatorManager { false }; > > Seems like this could be a virtual function on AuthenticatorManager rather than a separate boolean? Unless this is perf sensitive somehow? Yup. Sounds like a much better idea. >> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2391 >> + localValues.append(toWK(JSValueToStringCopy(context, privateKeyBase64Value, 0))); > > Looks like you're leaking the value returned by JSValueToStringCopy(). Fixed. >> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2405 >> + configurationValues.append(WKDictionaryCreate(rawLocalKeys.data(), rawLocalValues.data(), rawLocalKeys.size())); > > You need to adopt the value returned by WKDictionaryCreate() Fixed. >> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2460 >> +bool TestRunner::isKeyExisted(JSStringRef attrLabel, JSStringRef applicationTagBase64) > > isKeyExisted(): I am French but this does not sound right. Also the naming is too generic for TestRunner. Could be any kind of key. Changed to keyExistedInKeychain. >> Tools/WebKitTestRunner/TestController.h:258 >> + void addTestKeyToKeychain(String privateKeyBase64, String attrLabel, String applicationTagBase64); > > const String& Fixed. >> Tools/WebKitTestRunner/TestController.h:259 >> + void cleanUpKeychain(String attrLabel); > > ditto Fixed. >> Tools/WebKitTestRunner/TestController.h:260 >> + bool isKeyExisted(String attrLabel, String applicationTagBase64); > > ditto Fixed.
Created attachment 350724 [details] Patch
Created attachment 350725 [details] Patch
Attachment 350725 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:173: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:445: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 80 files If any of these errors are false positives, please file a bug against check-webkit-style.
Build fails on GTK / WPE.
Created attachment 350759 [details] Patch
(In reply to Chris Dumez from comment #18) > Build fails on GTK / WPE. Fixed. Forgot to update the signature of GTK ports...
Attachment 350759 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:173: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:445: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 80 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 350759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350759&action=review > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:578 > + auto localRef = static_cast<WKDictionaryRef>(WKDictionaryGetItemForKey(configurationRef, WKStringCreateWithUTF8CString("Local"))); The value returned by WKStringCreateWithUTF8CString() is leaked.
(In reply to Chris Dumez from comment #22) > Comment on attachment 350759 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350759&action=review > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:578 > > + auto localRef = static_cast<WKDictionaryRef>(WKDictionaryGetItemForKey(configurationRef, WKStringCreateWithUTF8CString("Local"))); > > The value returned by WKStringCreateWithUTF8CString() is leaked. Fixed.
Comment on attachment 350759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350759&action=review r=me > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:57 > + // FIXME: Maybe we should use safe casting. Safe casting macros would still require this virtual function as it is so I am not sure we need a FIXME comment here. > Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:83 > + HashMap<Authenticator*, Ref<Authenticator>> m_authenticators; Why isn't this a HashSet<Ref<Authenticator>> ? > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:96 > +static inline bool emptyTransportsOrContain(const Vector<AuthenticatorTransport>& transports, AuthenticatorTransport target) emptyTransportsOrContains with an 's' would be better I think > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2387 > + localValues.append(WKBooleanCreate(acceptAuthentication)); You need to adopt the value returned by WKBooleanCreate, otherwise you ref one time too many. > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2389 > + localValues.append(WKBooleanCreate(acceptAttestation)); ditto. > Tools/WebKitTestRunner/TestController.cpp:3179 > +bool TestController::keyExistedInKeychain(const String&, const String&) Why keyExisted and not keyExists ? Isn't it checking if the key is present in the keychain *now*?
Comment on attachment 350759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350759&action=review Thanks Chris for r+ the patch. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:57 >> + // FIXME: Maybe we should use safe casting. > > Safe casting macros would still require this virtual function as it is so I am not sure we need a FIXME comment here. Removed. >> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h:83 >> + HashMap<Authenticator*, Ref<Authenticator>> m_authenticators; > > Why isn't this a HashSet<Ref<Authenticator>> ? Authenticators can be removed but not for now. Therefore, I need something to identify the authenticator. >> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:96 >> +static inline bool emptyTransportsOrContain(const Vector<AuthenticatorTransport>& transports, AuthenticatorTransport target) > > emptyTransportsOrContains with an 's' would be better I think I don't think so. Transport_s_. >> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2387 >> + localValues.append(WKBooleanCreate(acceptAuthentication)); > > You need to adopt the value returned by WKBooleanCreate, otherwise you ref one time too many. Fixed. >> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2389 >> + localValues.append(WKBooleanCreate(acceptAttestation)); > > ditto. Fixed. >> Tools/WebKitTestRunner/TestController.cpp:3179 >> +bool TestController::keyExistedInKeychain(const String&, const String&) > > Why keyExisted and not keyExists ? Isn't it checking if the key is present in the keychain *now*? Sure.
Created attachment 350792 [details] Patch for landing
Comment on attachment 350792 [details] Patch for landing Rejecting attachment 350792 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 350792, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 5000 characters of output: file Source/WebKit/UIProcess/WebAuthentication/Authenticator.h patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorTransportService.cpp patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorTransportService.h patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticationSoftLink.h patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalService.h patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalService.mm patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.h patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.h patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalService.cpp patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalService.h patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h patching file Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationRequestData.h patching file Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp patching file Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.h patching file Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp patching file Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h patching file Source/WebKit/WebKit.xcodeproj/project.pbxproj patching file Tools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj patching file Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm rm 'Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm' patching file Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.h patching file Tools/WebKitTestRunner/TestController.cpp patching file Tools/WebKitTestRunner/TestController.h patching file Tools/WebKitTestRunner/TestInvocation.cpp patching file Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj patching file Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/TestExpectations patching file LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local.https-expected.txt patching file LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local.https.html patching file LayoutTests/http/wpt/webauthn/public-key-credential-create-success-local.https-expected.txt patching file LayoutTests/http/wpt/webauthn/public-key-credential-create-success-local.https.html patching file LayoutTests/http/wpt/webauthn/public-key-credential-create-success.https.html patching file LayoutTests/http/wpt/webauthn/public-key-credential-get-failure-local.https-expected.txt patching file LayoutTests/http/wpt/webauthn/public-key-credential-get-failure-local.https.html patching file LayoutTests/http/wpt/webauthn/public-key-credential-get-success-local.https-expected.txt patching file LayoutTests/http/wpt/webauthn/public-key-credential-get-success-local.https.html patching file LayoutTests/http/wpt/webauthn/public-key-credential-get-success.https.html patching file LayoutTests/http/wpt/webauthn/public-key-credential-is-user-verifying-platform-authenticator-available-expected.txt patching file LayoutTests/http/wpt/webauthn/public-key-credential-is-user-verifying-platform-authenticator-available.html patching file LayoutTests/http/wpt/webauthn/public-key-is-user-verifying-platform-authenticator-available-expected.txt rm 'LayoutTests/http/wpt/webauthn/public-key-is-user-verifying-platform-authenticator-available-expected.txt' patching file LayoutTests/http/wpt/webauthn/public-key-is-user-verifying-platform-authenticator-available.html rm 'LayoutTests/http/wpt/webauthn/public-key-is-user-verifying-platform-authenticator-available.html' patching file LayoutTests/http/wpt/webauthn/resources/util.js patching file LayoutTests/platform/mac-wk2/TestExpectations Hunk #1 FAILED at 881. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac-wk2/TestExpectations.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/9348240
Created attachment 350794 [details] Patch for landing
Comment on attachment 350794 [details] Patch for landing Clearing flags on attachment: 350794 Committed r236481: <https://trac.webkit.org/changeset/236481>
A build fix is committed: Committed r236486: <https://trac.webkit.org/changeset/236486>
Comment on attachment 350794 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=350794&action=review Some headers are missing from LocalConnection.h, causing build failures when the UnitedBuild sources rotate. I'll try and land a fix. > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h:42 > + WTF_MAKE_FAST_ALLOCATED; This requires wtf/FastMalloc.h > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h:43 > + WTF_MAKE_NONCOPYABLE(LocalConnection); This requires <wtf/Noncopyable.h > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h:58 > + virtual void getUserConsent(const String& reason, UserConsentCallback&&) const; wtf/text/WTFString.h > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h:60 > + virtual void getAttestation(const String& rpId, const String& username, const Vector<uint8_t>& hash, AttestationCallback&&) const; wtf/Vector.h
Committed r237674: <https://trac.webkit.org/changeset/237674>
(In reply to Frédéric Wang (:fredw) from comment #32) > Committed r237674: <https://trac.webkit.org/changeset/237674> Thanks, Frederic.
Comment on attachment 350794 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=350794&action=review > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:227 > + ASSERT(RunLoop::isMain()); This requires <wtf/runLoop.h> > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:46 > + ASSERT(!RunLoop::isMain()); This requires <wtf/RunLoop.h> > Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp:58 > + ASSERT(RunLoop::isMain()); This requires <wtf/RunLoop.h>
Committed r238221: <https://trac.webkit.org/changeset/238221>