Bug 181928 - [WebAuthN] Implement PublicKeyCredential's [[Create]] with a dummy authenticator
Summary: [WebAuthN] Implement PublicKeyCredential's [[Create]] with a dummy authenticator
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-01-22 02:04 PST by Jiewen Tan
Modified: 2018-01-22 19:05 PST (History)
8 users (show)

See Also:


Attachments
Patch (100.47 KB, patch)
2018-01-22 02:25 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (99.12 KB, patch)
2018-01-22 13:05 PST, Jiewen Tan
bfulgham: review+
Details | Formatted Diff | Diff
Patch for landing (100.14 KB, patch)
2018-01-22 15:02 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for landing (100.17 KB, patch)
2018-01-22 15:47 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for landing (100.17 KB, patch)
2018-01-22 17:26 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-01-22 02:04:08 PST
Implement PublicKeyCredential's [[Create]] with a dummy authenticator
Comment 1 Jiewen Tan 2018-01-22 02:04:35 PST
<rdar://problem/36459893>
Comment 2 Jiewen Tan 2018-01-22 02:25:02 PST
Created attachment 331907 [details]
Patch
Comment 3 Brent Fulgham 2018-01-22 13:04:40 PST
Did you forget to include some changes in your patch?

In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource152.cpp:1:
/Volumes/Data/EWS/WebKit/Source/WebCore/Modules/webauthn/Authenticator.cpp:53:12: error: no viable conversion from returned value of type 'Vector<uint8_t>' (aka 'Vector<unsigned char>') to function return type 'ExceptionOr<Vector<uint8_t> >' (aka 'ExceptionOr<Vector<unsigned char> >')
    return attestationObject;
           ^~~~~~~~~~~~~~~~~
In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource152.cpp:1:
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/Modules/webauthn/Authenticator.cpp:27:
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/Modules/webauthn/Authenticator.h:28:
/Volumes/Data/EWS/WebKit/Source/WebCore/dom/ExceptionOr.h:35:37: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'Vector<uint8_t>' (aka 'Vector<unsigned char>') to 'WebCore::ExceptionOr<WTF::Vector<unsigned char, 0, WTF::CrashOnOverflow, 16, WTF::FastMalloc> > &&' for 1st argument
template<typename ReturnType> class ExceptionOr {
                                    ^
/Volumes/Data/EWS/WebKit/Source/WebCore/dom/ExceptionOr.h:35:37: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'Vector<uint8_t>' (aka 'Vector<unsigned char>') to 'const WebCore::ExceptionOr<WTF::Vector<unsigned char, 0, WTF::CrashOnOverflow, 16, WTF::FastMalloc> > &' for 1st argument
/Volumes/Data/EWS/WebKit/Source/WebCore/dom/ExceptionOr.h:37:5: note: candidate constructor not viable: no known conversion from 'Vector<uint8_t>' (aka 'Vector<unsigned char>') to 'WebCore::Exception &&' for 1st argument
    ExceptionOr(Exception&&);
    ^
/Volumes/Data/EWS/WebKit/Source/WebCore/dom/ExceptionOr.h:38:5: note: candidate constructor not viable: no known conversion from 'Vector<uint8_t>' (aka 'Vector<unsigned char>') to 'WTF::Vector<unsigned char, 0, WTF::CrashOnOverflow, 16, WTF::FastMalloc> &&' for 1st argument
    ExceptionOr(ReturnType&&);
    ^
/Volumes/Data/EWS/WebKit/Source/WebCore/dom/ExceptionOr.h:39:88: note: candidate template ignored: disabled by 'enable_if' [with OtherType = WTF::Vector<unsigned char, 0, WTF::CrashOnOverflow, 16, WTF::FastMalloc>]
    template<typename OtherType> ExceptionOr(const OtherType&, typename std::enable_if<std::is_scalar<OtherType>::value && std::is_convertible<OtherType, ReturnType>::value>::type* = nullptr);
                                                                                       ^
1 error generated.
Comment 4 Brent Fulgham 2018-01-22 13:05:27 PST
Comment on attachment 331907 [details]
Patch

r- because it is failing to build on just about every platform. I think the patch is either dependent on other work, or is missing some code.
Comment 5 Jiewen Tan 2018-01-22 13:05:33 PST
Created attachment 331954 [details]
Patch
Comment 6 Brent Fulgham 2018-01-22 14:04:43 PST
Comment on attachment 331954 [details]
Patch

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

I think this looks good for initial code. r=me.

> Source/WebCore/ChangeLog:11
> +        and a pass paths. A numbers of dependencies need to be resolved later in order to comply with the spec.

... "and a pass path. A number of"

> Source/WebCore/ChangeLog:12
> +        Also, the current architecture of handling async operations including dispatching, timeout, and aborting

I would say, "of handling async WebAuthN operations", otherwise it sounds like you think the entire async model of WebKit needs to be replaced :-)

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:77
> +// since it doesn't support observers (or other means) to trigger the actual abort action. Enhancement to AbortSignal is needed.

Bugzilla for AbortSIgnal enhancement request?

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:79
> +    void CredentialsContainer::dispatchTask(OperationType&& operation, Ref<DeferredPromise>&& promise, std::optional<unsigned long> timeOutInMs)

I don't think you want this whitespace.

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:86
> +    // FIXME: We should probably trim timeOutInMs to some max allowable number.

Doe the WebAuthN spec indicate a performance level? Do we have other authentication tasks that have a timeout value?

> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:96
> +        pendingPromise->timer->startOneShot(Seconds(timeOutInMs.value() / 1000.0));

Do we need to assert upon entering 'dispatchTask' that we are on the MainThread? Otherwise, couldn't the timer be created on some background thread, and we will only assert when the timer fires?

> Source/WebCore/Modules/webauthn/Authenticator.cpp:41
> +    // User cancellation is effecively NotAllowedError.

Can you add a comment here that this is just a  dummy function to support initial development?

> Source/WebCore/Modules/webauthn/Authenticator.cpp:42
> +    if (user.displayName == "John")

LOL

> Source/WebCore/Modules/webauthn/PublicKeyCredential.cpp:128
> +    // FIXME: We lack fundamental support from SecurityOrigin to determine if a host is a valid domain or not.

Can you file a Radar/Bugzilla about this? We may not be able to take immediate action, but I'd like it on the task list.
Comment 7 Brent Fulgham 2018-01-22 14:05:14 PST
Comment on attachment 331954 [details]
Patch

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

> LayoutTests/http/wpt/webauthn/idl.https-expected.txt:54
> +PASS AuthenticatorAssertionResponse interface: attribute userHandle 

Are these official WebAuthN tests? Great!
Comment 8 Jiewen Tan 2018-01-22 15:00:29 PST
Comment on attachment 331954 [details]
Patch

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

Thanks Brent for r+ the patch.

>> Source/WebCore/ChangeLog:11
>> +        and a pass paths. A numbers of dependencies need to be resolved later in order to comply with the spec.
> 
> ... "and a pass path. A number of"

Fixed.

>> Source/WebCore/ChangeLog:12
>> +        Also, the current architecture of handling async operations including dispatching, timeout, and aborting
> 
> I would say, "of handling async WebAuthN operations", otherwise it sounds like you think the entire async model of WebKit needs to be replaced :-)

Yep. I would be more specific.

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:77
>> +// since it doesn't support observers (or other means) to trigger the actual abort action. Enhancement to AbortSignal is needed.
> 
> Bugzilla for AbortSIgnal enhancement request?

Bugzilla for every FIXME has been updated.

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:79
>> +    void CredentialsContainer::dispatchTask(OperationType&& operation, Ref<DeferredPromise>&& promise, std::optional<unsigned long> timeOutInMs)
> 
> I don't think you want this whitespace.

Fixed.

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:86
>> +    // FIXME: We should probably trim timeOutInMs to some max allowable number.
> 
> Doe the WebAuthN spec indicate a performance level? Do we have other authentication tasks that have a timeout value?

The spec only suggests user agents should trim the timeout to some platform dependent max value. Both attestation and assertion have a timeout value. So far, I didn't have any speculation on what the value should be.

>> Source/WebCore/Modules/credentialmanagement/CredentialsContainer.cpp:96
>> +        pendingPromise->timer->startOneShot(Seconds(timeOutInMs.value() / 1000.0));
> 
> Do we need to assert upon entering 'dispatchTask' that we are on the MainThread? Otherwise, couldn't the timer be created on some background thread, and we will only assert when the timer fires?

Sure, an assertion is added at the very beginning of this method.

>> Source/WebCore/Modules/webauthn/Authenticator.cpp:41
>> +    // User cancellation is effecively NotAllowedError.
> 
> Can you add a comment here that this is just a  dummy function to support initial development?

Added.

>> Source/WebCore/Modules/webauthn/PublicKeyCredential.cpp:128
>> +    // FIXME: We lack fundamental support from SecurityOrigin to determine if a host is a valid domain or not.
> 
> Can you file a Radar/Bugzilla about this? We may not be able to take immediate action, but I'd like it on the task list.

Fixed.

>> LayoutTests/http/wpt/webauthn/idl.https-expected.txt:54
>> +PASS AuthenticatorAssertionResponse interface: attribute userHandle 
> 
> Are these official WebAuthN tests? Great!

Let's make them official!
Comment 9 Jiewen Tan 2018-01-22 15:02:40 PST
Created attachment 331968 [details]
Patch for landing
Comment 10 Jiewen Tan 2018-01-22 15:47:48 PST
Created attachment 331970 [details]
Patch for landing
Comment 11 Jiewen Tan 2018-01-22 17:26:47 PST
Created attachment 331987 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2018-01-22 19:01:05 PST
Comment on attachment 331987 [details]
Patch for landing

Clearing flags on attachment: 331987

Committed r227382: <https://trac.webkit.org/changeset/227382>