Bug 189279

Summary: [WebAuthN] Make AuthenticatorManager
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: alex.gaynor, bfulgham, cdumez, commit-queue, ews-watchlist, fred.wang, jiewen_tan, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=189876
Bug Depends on:    
Bug Blocks: 181943, 191148    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
cdumez: review+, cdumez: commit-queue-
Patch for landing
commit-queue: commit-queue-
Patch for landing none

Jiewen Tan
Reported 2018-09-04 16:03:44 PDT
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.
Attachments
Patch (350.87 KB, patch)
2018-09-23 03:54 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Patch (351.57 KB, patch)
2018-09-23 13:30 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.94 MB, application/zip)
2018-09-23 14:51 PDT, EWS Watchlist 		no flags
Details
Patch (353.17 KB, patch)
2018-09-23 16:08 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Patch (353.25 KB, patch)
2018-09-23 18:31 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Patch (354.95 KB, patch)
2018-09-24 18:19 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Patch (352.99 KB, patch)
2018-09-24 18:28 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Patch (353.03 KB, patch)
2018-09-25 10:49 PDT, Jiewen Tan 		cdumez: review+
cdumez: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (352.10 KB, patch)
2018-09-25 14:20 PDT, Jiewen Tan 		commit-queue: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (352.30 KB, patch)
2018-09-25 14:40 PDT, Jiewen Tan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2018-09-04 16:04:32 PDT
<rdar://problem/44116792>
Jiewen Tan
Comment 2 2018-09-23 03:54:59 PDT
Created attachment 350561 [details] Patch
EWS Watchlist
Comment 3 2018-09-23 03:58:14 PDT Comment hidden (obsolete)
Jiewen Tan
Comment 4 2018-09-23 13:30:25 PDT
Created attachment 350588 [details] Patch
EWS Watchlist
Comment 5 2018-09-23 13:33:05 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 6 2018-09-23 14:51:51 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 7 2018-09-23 14:51:53 PDT Comment hidden (obsolete)
Jiewen Tan
Comment 8 2018-09-23 16:08:05 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 9 2018-09-23 16:11:24 PDT Comment hidden (obsolete)
Jiewen Tan
Comment 10 2018-09-23 18:31:22 PDT
Created attachment 350595 [details] Patch
EWS Watchlist
Comment 11 2018-09-23 18:34:16 PDT
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.
Chris Dumez
Comment 12 2018-09-24 09:54:57 PDT
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]));
Chris Dumez
Comment 13 2018-09-24 12:23:31 PDT
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
Jiewen Tan
Comment 14 2018-09-24 18:14:34 PDT
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.
Jiewen Tan
Comment 15 2018-09-24 18:19:00 PDT
Created attachment 350724 [details] Patch
Jiewen Tan
Comment 16 2018-09-24 18:28:07 PDT
Created attachment 350725 [details] Patch
EWS Watchlist
Comment 17 2018-09-24 18:31:51 PDT Comment hidden (obsolete)
Chris Dumez
Comment 18 2018-09-25 08:59:45 PDT
Build fails on GTK / WPE.
Jiewen Tan
Comment 19 2018-09-25 10:49:24 PDT
Created attachment 350759 [details] Patch
Jiewen Tan
Comment 20 2018-09-25 10:50:27 PDT
(In reply to Chris Dumez from comment #18) > Build fails on GTK / WPE. Fixed. Forgot to update the signature of GTK ports...
EWS Watchlist
Comment 21 2018-09-25 10:53:32 PDT
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.
Chris Dumez
Comment 22 2018-09-25 10:58:56 PDT
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.
Jiewen Tan
Comment 23 2018-09-25 11:08:01 PDT
(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.
Chris Dumez
Comment 24 2018-09-25 11:53:57 PDT
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*?
Jiewen Tan
Comment 25 2018-09-25 13:12:18 PDT
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.
Jiewen Tan
Comment 26 2018-09-25 14:20:08 PDT
Created attachment 350792 [details] Patch for landing
WebKit Commit Bot
Comment 27 2018-09-25 14:34:23 PDT Comment hidden (obsolete)
Jiewen Tan
Comment 28 2018-09-25 14:40:29 PDT
Created attachment 350794 [details] Patch for landing
WebKit Commit Bot
Comment 29 2018-09-25 15:22:45 PDT
Comment on attachment 350794 [details] Patch for landing Clearing flags on attachment: 350794 Committed r236481: <https://trac.webkit.org/changeset/236481>
Jiewen Tan
Comment 30 2018-09-25 16:21:44 PDT
A build fix is committed: Committed r236486: <https://trac.webkit.org/changeset/236486>
Frédéric Wang (:fredw)
Comment 31 2018-11-01 01:46:28 PDT
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
Frédéric Wang (:fredw)
Comment 32 2018-11-01 01:51:54 PDT
Committed r237674: <https://trac.webkit.org/changeset/237674>
Jiewen Tan
Comment 33 2018-11-01 11:40:18 PDT
(In reply to Frédéric Wang (:fredw) from comment #32) > Committed r237674: <https://trac.webkit.org/changeset/237674> Thanks, Frederic.
Frédéric Wang (:fredw)
Comment 34 2018-11-15 05:42:40 PST
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>
Frédéric Wang (:fredw)
Comment 35 2018-11-15 05:43:19 PST
Committed r238221: <https://trac.webkit.org/changeset/238221>
Note You need to log in before you can comment on or make changes to this bug.