Bug 189279

<rdar://problem/44116792>

Created attachment 350561 [details]
Patch

Created attachment 350588 [details]
Patch

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

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

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>