Bug 181928 – [WebAuthN] Implement PublicKeyCredential's [[Create]] with a dummy authenticator

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Implement PublicKeyCredential's [[Create]] with a dummy authenticator

[Jiewen Tan]

<rdar://problem/36459893>

[Jiewen Tan]

Created attachment 331907 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Jiewen Tan]

Created attachment 331954 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

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!

[Jiewen Tan]

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!

[Jiewen Tan]

Created attachment 331968 [details]
Patch for landing

[Jiewen Tan]

Created attachment 331970 [details]
Patch for landing

[Jiewen Tan]

Created attachment 331987 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 331987 [details]
Patch for landing

Clearing flags on attachment: 331987

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