Bug 182771 - [WebAuthN] Implement PublicKeyCredential.isUserVerifyingPlatformAuthenticatorAvailable()
Summary: [WebAuthN] Implement PublicKeyCredential.isUserVerifyingPlatformAuthenticator...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency treegraph
 
Reported: 2018-02-13 22:04 PST by Jiewen Tan
Modified: 2018-02-16 12:43 PST (History)
4 users (show)

See Also:


Attachments
Patch (57.62 KB, patch)
2018-02-13 22:19 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (54.37 KB, patch)
2018-02-15 14:38 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (54.53 KB, patch)
2018-02-15 16:52 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2018-02-13 22:04:30 PST
Implement PublicKeyCredential.isUserVerifyingPlatformAuthenticatorAvailable() per https://www.w3.org/TR/webauthn/#isUserVerifyingPlatformAuthenticatorAvailable.
Comment 1 Jiewen Tan 2018-02-13 22:04:59 PST
<rdar://problem/36459988>
Comment 2 Jiewen Tan 2018-02-13 22:19:00 PST
Created attachment 333769 [details]
Patch
Comment 3 Brent Fulgham 2018-02-14 09:40:59 PST
Comment on attachment 333769 [details]
Patch

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

This looks good, but I think you should try to implement on macOS as well. The SDKs exist! r- because of lack of macOS support.

> Source/WebCore/ChangeLog:13
> +        Besides, it changes DeferredPromise to DOMPromiseDeferred<> for all CredentialsManagement and

I would say, "In addition, it changes ..."

> Source/WebKit/ChangeLog:9
> +        This patch uitlizes LocalAuthentication Framework to determine if biometrics

utilizes

> Source/WebKit/ChangeLog:14
> +        Corresponding macOS implementations are marked as unimplemented as a result.

I don't think this is true. <https://developer.apple.com/documentation/localauthentication?language=objc> claims that it is present in iOS 8+ and macOS 10.10+

> Source/WebKit/UIProcess/CredentialManagement/ios/WebCredentialsMessengerProxyIOS.mm:44
> +        LOG_ERROR("Couldn't evaluate policy: %@", error);

This might be better as "Couldn't evaluate authentication with biometrics policy: %@"

> Source/WebKit/UIProcess/CredentialManagement/mac/WebCredentialsMessengerProxyMac.mm:37
> +    notImplemented();

I think this can be implemented, based on <https://developer.apple.com/documentation/localauthentication?language=objc>.
Comment 4 Jiewen Tan 2018-02-15 11:14:00 PST
Comment on attachment 333769 [details]
Patch

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

Thanks Brent for reviewing the patch.

>> Source/WebCore/ChangeLog:13
>> +        Besides, it changes DeferredPromise to DOMPromiseDeferred<> for all CredentialsManagement and
> 
> I would say, "In addition, it changes ..."

Fixed.

>> Source/WebKit/ChangeLog:9
>> +        This patch uitlizes LocalAuthentication Framework to determine if biometrics
> 
> utilizes

Fixed.

>> Source/WebKit/ChangeLog:14
>> +        Corresponding macOS implementations are marked as unimplemented as a result.
> 
> I don't think this is true. <https://developer.apple.com/documentation/localauthentication?language=objc> claims that it is present in iOS 8+ and macOS 10.10+

Sure. I will try a TouchID equipped device and see what the real effect is.

>> Source/WebKit/UIProcess/CredentialManagement/ios/WebCredentialsMessengerProxyIOS.mm:44
>> +        LOG_ERROR("Couldn't evaluate policy: %@", error);
> 
> This might be better as "Couldn't evaluate authentication with biometrics policy: %@"

Fixed.

>> Source/WebKit/UIProcess/CredentialManagement/mac/WebCredentialsMessengerProxyMac.mm:37
>> +    notImplemented();
> 
> I think this can be implemented, based on <https://developer.apple.com/documentation/localauthentication?language=objc>.

Ditto.
Comment 5 Jiewen Tan 2018-02-15 13:21:59 PST
Confirmed LocalAuthentication does distinguish TouchID equipped macs and TouchIDless ones.
Comment 6 Jiewen Tan 2018-02-15 14:38:55 PST
Created attachment 333949 [details]
Patch
Comment 7 Jiewen Tan 2018-02-15 16:52:29 PST
Created attachment 333970 [details]
Patch
Comment 8 Brent Fulgham 2018-02-16 12:01:36 PST
Comment on attachment 333970 [details]
Patch

Looks great! r=me.
Comment 9 Jiewen Tan 2018-02-16 12:17:49 PST
(In reply to Brent Fulgham from comment #8)
> Comment on attachment 333970 [details]
> Patch
> 
> Looks great! r=me.

Thanks Brent for r+ the patch.
Comment 10 WebKit Commit Bot 2018-02-16 12:43:32 PST
Comment on attachment 333970 [details]
Patch

Clearing flags on attachment: 333970

Committed r228572: <https://trac.webkit.org/changeset/228572>
Comment 11 WebKit Commit Bot 2018-02-16 12:43:34 PST
All reviewed patches have been landed.  Closing bug.