Summary: | Allow using the platform authenticator on non-Touch ID Macs according to Internal requirements | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||
Component: | WebKit Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bfulgham, jiewen_tan, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Jiewen Tan
2021-04-15 18:54:25 PDT
Created attachment 426174 [details]
Patch
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 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. Created attachment 426286 [details]
Patch for landing
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]. |