Bug 224639 – Allow using the platform authenticator on non-Touch ID Macs according to Internal requirements

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Allow using the platform authenticator on non-Touch ID Macs according to Internal requirements.

[Jiewen Tan]

rdar://74698346

[Jiewen Tan]

Created attachment 426174 [details]
Patch

[Daniel Bates]

Comment on attachment 426174 [details]
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

[Jiewen Tan]

Comment on attachment 426174 [details]
Patch

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

Thanks Dan for r+ this patch.

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

Fixed.

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

Fixed.

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

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockLocalConnection.mm:66
>> +            userVerification = UserVerification::Presence;
> 
> Ok as is. Consider adding a break statement.

Fixed.

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

Fixed.

[Jiewen Tan]

Created attachment 426286 [details]
Patch for landing

[EWS]

Committed r276180 (236663@main): <https://commits.webkit.org/236663@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426286 [details].