Bug 189279 – [WebAuthN] Make AuthenticatorManager

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

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.

[Radar WebKit Bug Importer]

<rdar://problem/44116792>

[Jiewen Tan]

Created attachment 350561 [details]
Patch

[EWS Watchlist]

Attachment 350561 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:174:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:449:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 77 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Created attachment 350588 [details]
Patch

[EWS Watchlist]

Attachment 350588 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:174:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:449:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 78 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 350588 [details]
Patch

Attachment 350588 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9323522

New failing tests:
http/wpt/webauthn/public-key-credential-create-failure-local.https.html
http/wpt/webauthn/public-key-credential-create-success-local.https.html
http/wpt/webauthn/public-key-credential-get-success-local.https.html

[EWS Watchlist]

Created attachment 350590 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Jiewen Tan]

Created attachment 350593 [details]
Patch

[EWS Watchlist]

Attachment 350593 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:174:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:449:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Created attachment 350595 [details]
Patch

[EWS Watchlist]

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 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 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 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]

Created attachment 350724 [details]
Patch

[Jiewen Tan]

Created attachment 350725 [details]
Patch

[EWS Watchlist]

Attachment 350725 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:173:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:445:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 80 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Build fails on GTK / WPE.

[Jiewen Tan]

Created attachment 350759 [details]
Patch

[Jiewen Tan]

(In reply to Chris Dumez from comment #18)
> Build fails on GTK / WPE.

Fixed. Forgot to update the signature of GTK ports...

[EWS Watchlist]

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 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]

(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 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 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]

Created attachment 350792 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 350792 [details]
Patch for landing

Rejecting attachment 350792 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 350792, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
 file Source/WebKit/UIProcess/WebAuthentication/Authenticator.h
patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp
patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.h
patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorTransportService.cpp
patching file Source/WebKit/UIProcess/WebAuthentication/AuthenticatorTransportService.h
patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticationSoftLink.h
patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h
patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm
patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.h
patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm
patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalService.h
patching file Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalService.mm
patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.cpp
patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockAuthenticatorManager.h
patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.h
patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm
patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalService.cpp
patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalService.h
patching file Source/WebKit/UIProcess/WebAuthentication/Mock/MockWebAuthenticationConfiguration.h
patching file Source/WebKit/UIProcess/WebAuthentication/WebAuthenticationRequestData.h
patching file Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.cpp
patching file Source/WebKit/UIProcess/WebAuthentication/WebAuthenticatorCoordinatorProxy.h
patching file Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
patching file Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h
patching file Source/WebKit/WebKit.xcodeproj/project.pbxproj
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
patching file Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm
rm 'Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm'
patching file Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl
patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp
patching file Tools/WebKitTestRunner/InjectedBundle/TestRunner.h
patching file Tools/WebKitTestRunner/TestController.cpp
patching file Tools/WebKitTestRunner/TestController.h
patching file Tools/WebKitTestRunner/TestInvocation.cpp
patching file Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj
patching file Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/TestExpectations
patching file LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local.https-expected.txt
patching file LayoutTests/http/wpt/webauthn/public-key-credential-create-failure-local.https.html
patching file LayoutTests/http/wpt/webauthn/public-key-credential-create-success-local.https-expected.txt
patching file LayoutTests/http/wpt/webauthn/public-key-credential-create-success-local.https.html
patching file LayoutTests/http/wpt/webauthn/public-key-credential-create-success.https.html
patching file LayoutTests/http/wpt/webauthn/public-key-credential-get-failure-local.https-expected.txt
patching file LayoutTests/http/wpt/webauthn/public-key-credential-get-failure-local.https.html
patching file LayoutTests/http/wpt/webauthn/public-key-credential-get-success-local.https-expected.txt
patching file LayoutTests/http/wpt/webauthn/public-key-credential-get-success-local.https.html
patching file LayoutTests/http/wpt/webauthn/public-key-credential-get-success.https.html
patching file LayoutTests/http/wpt/webauthn/public-key-credential-is-user-verifying-platform-authenticator-available-expected.txt
patching file LayoutTests/http/wpt/webauthn/public-key-credential-is-user-verifying-platform-authenticator-available.html
patching file LayoutTests/http/wpt/webauthn/public-key-is-user-verifying-platform-authenticator-available-expected.txt
rm 'LayoutTests/http/wpt/webauthn/public-key-is-user-verifying-platform-authenticator-available-expected.txt'
patching file LayoutTests/http/wpt/webauthn/public-key-is-user-verifying-platform-authenticator-available.html
rm 'LayoutTests/http/wpt/webauthn/public-key-is-user-verifying-platform-authenticator-available.html'
patching file LayoutTests/http/wpt/webauthn/resources/util.js
patching file LayoutTests/platform/mac-wk2/TestExpectations
Hunk #1 FAILED at 881.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac-wk2/TestExpectations.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/9348240

[Jiewen Tan]

Created attachment 350794 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 350794 [details]
Patch for landing

Clearing flags on attachment: 350794

Committed r236481: <https://trac.webkit.org/changeset/236481>

[Jiewen Tan]

A build fix is committed:
Committed r236486: <https://trac.webkit.org/changeset/236486>

[Frédéric Wang (:fredw)]

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)]

Committed r237674: <https://trac.webkit.org/changeset/237674>

[Jiewen Tan]

(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 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)]

Committed r238221: <https://trac.webkit.org/changeset/238221>