[webkit-reviews] review denied: [Bug 189279] [WebAuthN] Make AuthenticatorManager : [Attachment 350595] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Jiewen Tan <jiewen_tan at apple.com>'s
request for review:
Bug 189279: [WebAuthN] Make AuthenticatorManager
https://bugs.webkit.org/show_bug.cgi?id=189279

Attachment 350595: Patch

https://bugs.webkit.org/attachment.cgi?id=350595&action=review




--- Comment #13 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 350595
  --> https://bugs.webkit.org/attachment.cgi?id=350595
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