Bug 189279 - [WebAuthN] Make AuthenticatorManager
Summary: [WebAuthN] Make AuthenticatorManager
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943 191148
  Show dependency treegraph
 
Reported: 2018-09-04 16:03 PDT by Jiewen Tan
Modified: 2018-11-15 05:43 PST (History)
10 users (show)

See Also:


Attachments
Patch (350.87 KB, patch)
2018-09-23 03:54 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (351.57 KB, patch)
2018-09-23 13:30 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Patch (353.25 KB, patch)
2018-09-23 18:31 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (354.95 KB, patch)
2018-09-24 18:19 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (352.99 KB, patch)
2018-09-24 18:28 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (353.03 KB, patch)
2018-09-25 10:49 PDT, Jiewen Tan
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (352.10 KB, patch)
2018-09-25 14:20 PDT, Jiewen Tan
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (352.30 KB, patch)
2018-09-25 14:40 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 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.
Comment 1 Radar WebKit Bug Importer 2018-09-04 16:04:32 PDT
<rdar://problem/44116792>
Comment 2 Jiewen Tan 2018-09-23 03:54:59 PDT
Created attachment 350561 [details]
Patch
Comment 3 EWS Watchlist 2018-09-23 03:58:14 PDT Comment hidden (obsolete)
Comment 4 Jiewen Tan 2018-09-23 13:30:25 PDT
Created attachment 350588 [details]
Patch
Comment 5 EWS Watchlist 2018-09-23 13:33:05 PDT Comment hidden (obsolete)
Comment 6 EWS Watchlist 2018-09-23 14:51:51 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-09-23 14:51:53 PDT Comment hidden (obsolete)
Comment 8 Jiewen Tan 2018-09-23 16:08:05 PDT Comment hidden (obsolete)
Comment 9 EWS Watchlist 2018-09-23 16:11:24 PDT Comment hidden (obsolete)
Comment 10 Jiewen Tan 2018-09-23 18:31:22 PDT
Created attachment 350595 [details]
Patch
Comment 11 EWS Watchlist 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.
Comment 12 Chris Dumez 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]));
Comment 13 Chris Dumez 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
Comment 14 Jiewen Tan 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.
Comment 15 Jiewen Tan 2018-09-24 18:19:00 PDT
Created attachment 350724 [details]
Patch
Comment 16 Jiewen Tan 2018-09-24 18:28:07 PDT
Created attachment 350725 [details]
Patch
Comment 17 EWS Watchlist 2018-09-24 18:31:51 PDT Comment hidden (obsolete)
Comment 18 Chris Dumez 2018-09-25 08:59:45 PDT
Build fails on GTK / WPE.
Comment 19 Jiewen Tan 2018-09-25 10:49:24 PDT
Created attachment 350759 [details]
Patch
Comment 20 Jiewen Tan 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...
Comment 21 EWS Watchlist 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.
Comment 22 Chris Dumez 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.
Comment 23 Jiewen Tan 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.
Comment 24 Chris Dumez 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*?
Comment 25 Jiewen Tan 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.
Comment 26 Jiewen Tan 2018-09-25 14:20:08 PDT
Created attachment 350792 [details]
Patch for landing
Comment 27 WebKit Commit Bot 2018-09-25 14:34:23 PDT Comment hidden (obsolete)
Comment 28 Jiewen Tan 2018-09-25 14:40:29 PDT
Created attachment 350794 [details]
Patch for landing
Comment 29 WebKit Commit Bot 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>
Comment 30 Jiewen Tan 2018-09-25 16:21:44 PDT
A build fix is committed:
Committed r236486: <https://trac.webkit.org/changeset/236486>
Comment 31 Frédéric Wang (:fredw) 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
Comment 32 Frédéric Wang (:fredw) 2018-11-01 01:51:54 PDT
Committed r237674: <https://trac.webkit.org/changeset/237674>
Comment 33 Jiewen Tan 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.
Comment 34 Frédéric Wang (:fredw) 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>
Comment 35 Frédéric Wang (:fredw) 2018-11-15 05:43:19 PST
Committed r238221: <https://trac.webkit.org/changeset/238221>