Bug 181928

<rdar://problem/36459893>

Created attachment 331907 [details] Patch

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

Created attachment 331954 [details] Patch

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

Created attachment 331968 [details] Patch for landing

Created attachment 331970 [details] Patch for landing

Created attachment 331987 [details] Patch for landing

Comment on attachment 331987 [details] Patch for landing Clearing flags on attachment: 331987 Committed r227382: <https://trac.webkit.org/changeset/227382>