Patch #1 of multiple. Add webauth .mojom and initial usage of makeCredential.

third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp

Index: third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp
diff --git a/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp b/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp
index baecd1d0a595b24722d66a3c81d7288167138384..1eefbd0d37dd459154f931072976a9ad319f44e0 100644
--- a/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp
+++ b/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp
@@ -5,12 +5,124 @@
 #include "modules/webauth/WebAuthentication.h"
 
 #include "bindings/core/v8/ScriptPromise.h"
+#include "bindings/core/v8/ScriptPromiseResolver.h"
+#include "core/dom/DOMException.h"
+#include "core/dom/Document.h"
+#include "core/dom/ExceptionCode.h"
+#include "core/frame/LocalFrame.h"
+#include "core/frame/UseCounter.h"
+#include "modules/webauth/RelyingPartyAccount.h"
+#include "modules/webauth/ScopedCredential.h"
+#include "modules/webauth/ScopedCredentialOptions.h"
+#include "modules/webauth/ScopedCredentialParameters.h"
+#include "public/platform/InterfaceProvider.h"
+
+namespace {
+const char kNoAuthenticatorError[] = "Authenticator unavailable.";
+}  // anonymous namespace
+
+namespace mojo {
+
+using webauth::mojom::blink::RelyingPartyAccount;
+using webauth::mojom::blink::RelyingPartyAccountPtr;
+using webauth::mojom::blink::ScopedCredentialOptions;
+using webauth::mojom::blink::ScopedCredentialOptionsPtr;
+using webauth::mojom::blink::ScopedCredentialParameters;
+using webauth::mojom::blink::ScopedCredentialParametersPtr;
+using webauth::mojom::blink::ScopedCredentialDescriptor;
+using webauth::mojom::blink::ScopedCredentialType;
+using webauth::mojom::blink::Transport;
+
+Vector<uint8_t> convertBufferSource(const blink::BufferSource& buffer) {
+  DCHECK(buffer.isNull());
+  Vector<uint8_t> vector;
+  if (buffer.isArrayBuffer()) {
+    vector.append(static_cast<uint8_t*>(buffer.getAsArrayBuffer()->data()),
+                  buffer.getAsArrayBuffer()->byteLength());
+  } else {
+    vector.append(
+        static_cast<uint8_t*>(buffer.getAsArrayBufferView()->baseAddress()),
+        buffer.getAsArrayBufferView()->byteLength());
+  }
+  return vector;
+}
+
+ScopedCredentialType convertScopedCredentialType(const WTF::String& credType) {

[ikilpatrick, 2017/02/28 19:18:18]

is there a standard naming for these methods in the codebase? What does
bluetooth do?

[kpaulhamus, 2017/03/01 01:56:35]

It doesn't really seem like it. Bluetooth doesn't have a completely analogous
case; ImageCapture calls it "ParseX" for string-to-enum conversions, webusb uses
"ConvertX", nfc says "toXType". Do you have a preference?

+  if (credType == "ScopedCred")

[dcheng, 2017/02/28 07:25:56]

Unfortunately, mapping strings to enums is the best we can do atm: see
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imagec...
for other code that has to do this.

[kpaulhamus, 2017/03/01 01:56:35]

Acknowledged.

+    return ScopedCredentialType::SCOPEDCRED;
+  NOTREACHED();
+  return ScopedCredentialType::SCOPEDCRED;
+}
+
+Transport convertTransport(const WTF::String& transport) {
+  if (transport == "usb")
+    return Transport::USB;
+  if (transport == "nfc")
+    return Transport::NFC;
+  if (transport == "ble")
+    return Transport::BLE;
+  NOTREACHED();
+  return Transport::USB;
+}
+
+RelyingPartyAccountPtr convertRelyingPartyAccount(
+    const blink::RelyingPartyAccount& accountInformation,

[dcheng, 2017/02/28 07:25:56]

However, this can be implemented by using something called a typemap. See
https://www.chromium.org/developers/design-documents/mojo/type-mapping. A simple
typemap example to look at is the Blink typemap for KURL:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/mojo/...

Ditto for the conversion helpers below.

[kpaulhamus, 2017/03/01 01:56:34]

Got it, I'll take a closer look.

+    blink::ScriptPromiseResolver* resolver) {
+  auto mojoAccount = RelyingPartyAccount::New();
+
+  mojoAccount->rpDisplayName = accountInformation.rpDisplayName();
+  mojoAccount->displayName = accountInformation.displayName();
+  mojoAccount->id = accountInformation.id();
+  mojoAccount->name = accountInformation.name();
+  mojoAccount->imageURL = accountInformation.imageURL();
+  return mojoAccount;
+}
+
+ScopedCredentialOptionsPtr convertScopedCredentialOptions(
+    const blink::ScopedCredentialOptions options,
+    blink::ScriptPromiseResolver* resolver) {
+  auto mojoOptions = ScopedCredentialOptions::New();
+  mojoOptions->timeoutSeconds = options.timeoutSeconds();
+  mojoOptions->rpId = options.rpId();
+
+  // Adds the excludeList members (which are ScopedCredentialDescriptors)
+  for (const auto& descriptor : options.excludeList()) {
+    auto mojoDescriptor = ScopedCredentialDescriptor::New();
+    mojoDescriptor->type = convertScopedCredentialType(descriptor.type());
+    mojoDescriptor->id = convertBufferSource(descriptor.id());
+    for (const auto& transport : descriptor.transports())
+      mojoDescriptor->transports.push_back(convertTransport(transport));
+    mojoOptions->excludeList.push_back(std::move(mojoDescriptor));
+  }
+  // TODO (kpaulhamus) add AuthenticationExtensions;
+  return mojoOptions;
+}
+
+ScopedCredentialParametersPtr convertScopedCredentialParameter(
+    const blink::ScopedCredentialParameters parameter,
+    blink::ScriptPromiseResolver* resolver) {
+  auto mojoParameter = ScopedCredentialParameters::New();
+  mojoParameter->type = convertScopedCredentialType(parameter.type());
+  // TODO (kpaulhamus) add AlgorithmIdentifier
+  return mojoParameter;
+}
+}  // namespace mojo
 
 namespace blink {
 
-WebAuthentication::WebAuthentication(LocalFrame& frame) {}
+WebAuthentication::WebAuthentication(LocalFrame& frame)
+    : ContextLifecycleObserver(frame.document()) {
+  frame.interfaceProvider()->getInterface(mojo::MakeRequest(&m_authenticator));
+  m_authenticator.set_connection_error_handler(convertToBaseCallback(
+      WTF::bind(&WebAuthentication::onAuthenticatorConnectionError,

[dcheng, 2017/02/28 07:25:56]

Usually, the renderer won't ever see a connection error--if it sees one, that
means the browser side broke the pipe: because the browser process crashed (in
which case any further work in the renderer is meaningless), or the document is
going away (in which case we'll call contextDestroyed). Is it necessary to have
this here?

(Also, including the service side implementation in //components/webauth will
help with this review)

[kpaulhamus, 2017/03/01 01:56:34]

I don't know enough about the pipe life cycle to make a determination, but it
did look like every example I found is actively using
set_connection_error_handle. (http://shortn/_HCSt7qtOy1)

+                wrapWeakPersistent(this))));
+}
 
-WebAuthentication::~WebAuthentication() {}
+WebAuthentication::~WebAuthentication() {
+  // |m_authenticator| may still be valid but there should be no more
+  // outstanding requests because each holds a persistent handle to this object.
+  DCHECK(m_authenticatorRequests.isEmpty());
+}
 
 void WebAuthentication::dispose() {}
 
@@ -20,8 +132,38 @@ ScriptPromise WebAuthentication::makeCredential(
     const HeapVector<ScopedCredentialParameters> cryptoParameters,
     const BufferSource& attestationChallenge,
     ScopedCredentialOptions& options) {
-  NOTREACHED();
-  return ScriptPromise();
+  ExecutionContext* executionContext = scriptState->getExecutionContext();
+  UseCounter::count(executionContext, UseCounter::AuthenticationMakeCredential);
+
+  ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
+  ScriptPromise promise = resolver->promise();
+  if (!m_authenticator) {

[ikilpatrick, 2017/02/28 19:18:18]

can you un-nest these if stmts?, e.g.

if (!m_authenticator)
  return ScriptPromise::rejectWithDOMException(...);

String errorMessage;
if (!executionContext->isSecureContext(...))
   return return ScriptPromise::rejectWithDOMException(...);

ScriptPromiseResolver* resolver = ...
...;

return promise;

[kpaulhamus, 2017/03/01 01:56:34]

Done.

+    resolver->reject(DOMException::create(NotSupportedError));
+  } else {
+    String errorMessage;
+    if (!executionContext->isSecureContext(errorMessage)) {

[dcheng, 2017/02/28 07:25:56]

Do you know if there any plan to implement the [SecureContext] attribute?

[kpaulhamus, 2017/03/01 01:56:34]

It's a work in progress:
https://bugs.chromium.org/p/chromium/issues/detail?id=634270

+      resolver->reject(DOMException::create(SecurityError, errorMessage));
+    } else {
+      // TODO(kpaulhamus) validate parameters according to spec
+      auto account =
+          mojo::convertRelyingPartyAccount(accountInformation, resolver);
+      Vector<uint8_t> buffer = mojo::convertBufferSource(attestationChallenge);
+      auto opts = mojo::convertScopedCredentialOptions(options, resolver);
+      Vector<webauth::mojom::blink::ScopedCredentialParametersPtr> parameters;
+      for (const auto& parameter : cryptoParameters) {
+        parameters.push_back(
+            mojo::convertScopedCredentialParameter(parameter, resolver));
+      }
+
+      m_authenticatorRequests.insert(resolver);
+      m_authenticator->makeCredential(
+          std::move(account), std::move(parameters), buffer, std::move(opts),
+          convertToBaseCallback(WTF::bind(&WebAuthentication::onMakeCredential,
+                                          wrapPersistent(this),

[dcheng, 2017/02/28 07:25:56]

Should this be a weak persistent? If this object would otherwise be destroyed,
it doesn't seem like the callback should keep this object alive.

[kpaulhamus, 2017/03/01 01:56:34]

I talked with reillyg about this, just because I wasn't sure myself. It seems to
be a rule of thumb that you use wrapPersistent() for both 'this' and the
ScriptPromiseResolver when building a Mojo callback, and use
wrapWeakPersistent() for connection error callbacks. 

The subtlety apparently is that promises don't retain a reference to the object
that created them, so if the script doesn't keep a reference to a
WebAuthentication object, then the promise will never be resolved.

He did say - "The question of whether the promise should be resolved if the
object you called the method on that returned it is GCed is one that makes
intuitive sense to me but I have never seen anything specifying the behavior."

+                                          wrapPersistent(resolver))));
+    }
+  }
+  return promise;
 }
 
 ScriptPromise WebAuthentication::getAssertion(
@@ -32,4 +174,58 @@ ScriptPromise WebAuthentication::getAssertion(
   return ScriptPromise();
 }
 
+void WebAuthentication::contextDestroyed(ExecutionContext*) {
+  m_authenticator.reset();
+  m_authenticatorRequests.clear();
+}
+
+void WebAuthentication::onAuthenticatorConnectionError() {
+  m_authenticator.reset();
+  for (ScriptPromiseResolver* resolver : m_authenticatorRequests) {
+    resolver->reject(
+        DOMException::create(NotFoundError, kNoAuthenticatorError));
+  }
+  m_authenticatorRequests.clear();
+}
+
+void WebAuthentication::onMakeCredential(
+    ScriptPromiseResolver* resolver,
+    Vector<webauth::mojom::blink::ScopedCredentialInfoPtr> credentials) {
+  if (!markRequestComplete(resolver))

[ikilpatrick, 2017/02/28 19:18:18]

Should this reject promise? Or DCHECK that this never happens?

[kpaulhamus, 2017/03/01 01:56:34]

markRequestCompleted has the additional action of removing the request from
tracking if the call was successful - similar to how m_requests in 
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/nfc/NF...
or m_deviceRequests in 
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webusb...

+    return;
+
+  HeapVector<Member<ScopedCredentialInfo>> scopedCredentials;
+  for (auto& credential : credentials) {
+    if (credential->clientData.isEmpty() || credential->attestation.isEmpty()) {
+      resolver->reject(

[ikilpatrick, 2017/02/28 19:18:18]

just rejectWithDOMException?

[kpaulhamus, 2017/03/01 01:56:34]

Do you have an example for this where the callback already has a
ScriptPromiseResolver? The examples for rejectWithDOMException seemed to use it
only in cases without an existing resolver. Internally, this seems to just call
resolver.reject() anyways?

+          DOMException::create(NotFoundError, "No credentials returned."));
+    }
+    DOMArrayBuffer* clientDataBuffer = DOMArrayBuffer::create(
+        static_cast<void*>(&credential->clientData.front()),
+        credential->clientData.size());
+
+    DOMArrayBuffer* attestationBuffer = DOMArrayBuffer::create(
+        static_cast<void*>(&credential->attestation.front()),
+        credential->attestation.size());
+
+    scopedCredentials.push_back(
+        ScopedCredentialInfo::create(clientDataBuffer, attestationBuffer));
+  }
+  resolver->resolve(scopedCredentials);
+  m_authenticatorRequests.erase(resolver);
+}
+
+bool WebAuthentication::markRequestComplete(ScriptPromiseResolver* resolver) {
+  auto requestEntry = m_authenticatorRequests.find(resolver);
+  if (requestEntry == m_authenticatorRequests.end())
+    return false;
+  m_authenticatorRequests.erase(requestEntry);
+  return true;
+}
+
+DEFINE_TRACE(WebAuthentication) {
+  visitor->trace(m_authenticatorRequests);
+  ContextLifecycleObserver::trace(visitor);
+}
+
 }  // namespace blink