[webkit-reviews] review granted: [Bug 224639] Allow using the platform authenticator on non-Touch ID Macs according to Internal requirements : [Attachment 426174] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 224639: Allow using the platform authenticator on non-Touch ID Macs
according to Internal requirements
https://bugs.webkit.org/show_bug.cgi?id=224639

Attachment 426174: Patch

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




--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 426174
  --> https://bugs.webkit.org/attachment.cgi?id=426174
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=426174&action=review

Patch looks good.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:67
> +const uint8_t otherMakeCredentialFlags = 0b01000001; // UP and AT are set.

Ok as-is. Consider using constexpr as it has guaranteed compile time semantics.
Also mostly aesthetics, consider using uniform initializer syntax. Both of
these suggestions are optional.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:430
> +    auto flags = makeCredentialFlags;

Ok as-is. Consider writing using ternary operator to make this a tiny bit more
efficient. This suggestion is optional.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:64
> +void LocalConnection::verifyUser(const String& rpId, ClientDataType type,
SecAccessControlRef accessControl, UserVerificationRequirement uv,
UserVerificationCallback&& completionHandler)

Ok as is. Consider a more descriptive name than "uv". This suggestion is
optional.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:96
> +	   if (information && information[@"UserPresence"])

Ok as-is. Consider removing first conjunct as it's ok to send a message to nil.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalConnection.mm:109
> +	   if (uv == UserVerificationRequirement::Required || [m_context
canEvaluatePolicy:LAPolicyDeviceOwnerAuthenticationWithBiometrics
error:nullptr]) {

Ok as is. Consider passing nil instead of nullptr.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:66
> +	       userVerification = UserVerification::Presence;

Ok as is. Consider adding a break statement.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:90
> +	       userVerification = UserVerification::Presence;

Ditto