Blink-layer update to match WebAuthN spec

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

-// Copyright 2017 The Chromium Authors. All rights reserved.
+// Copyright 2016 The Chromium Authors. All rights reserved.

[engedy, 2017/07/05 18:51:07]

nit: Time machine? :)

[kpaulhamus, 2017/07/12 21:21:47]

:-)

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "modules/webauth/WebAuthentication.h"
 
 #include <stdint.h>
 
 #include "bindings/core/v8/ScriptPromise.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "core/dom/DOMException.h"
 #include "core/dom/Document.h"

[engedy, 2017/07/05 18:51:07]

nit: Will be unneeded once the ContextLifecycleObserver is gone.

[kpaulhamus, 2017/07/12 21:21:48]

It's still needed for GetFrame.

[engedy, 2017/07/13 11:33:53]

Acknowledged.

 #include "core/dom/ExceptionCode.h"
 #include "core/frame/LocalFrame.h"
-#include "modules/webauth/RelyingPartyAccount.h"
-#include "modules/webauth/ScopedCredential.h"
-#include "modules/webauth/ScopedCredentialOptions.h"
-#include "modules/webauth/ScopedCredentialParameters.h"
+#include "modules/webauth/AuthenticatorAssertionResponse.h"
+#include "modules/webauth/AuthenticatorAttestationResponse.h"
+#include "modules/webauth/PublicKeyCredential.h"

[engedy, 2017/07/05 18:51:07]

nit: AuthenticatorAssertionResponse and PublicKeyCredential look unused?

[kpaulhamus, 2017/07/12 21:21:47]

Done.

 #include "public/platform/InterfaceProvider.h"
 
 namespace {
 const char kNoAuthenticatorError[] = "Authenticator unavailable.";
 // Time to wait for an authenticator to successfully complete an operation.
-const int kAdjustedTimeoutLowerInSeconds = 60;
-const int kAdjustedTimeoutUpperInSeconds = 120;
+const int64_t kAdjustedTimeoutLowerInMilliseconds = 60000;
+const int64_t kAdjustedTimeoutUpperInMilliseconds = 12000;

[vasilii, 2017/07/05 14:36:13]

constexpr

[engedy, 2017/07/05 18:51:07]

Also, could this directly be `constexpr WTF::TimeDelta`? That would allow using
much shorter names, using FromMinutes, and not including stdint.h.

[engedy, 2017/07/05 18:51:08]

Missing trailing 0? i.e. 12 sec instead of 2 minutes?

[kpaulhamus, 2017/07/12 21:21:48]

Done.

[kpaulhamus, 2017/07/12 21:21:48]

good catch

[kpaulhamus, 2017/07/12 21:21:48]

Yep, that's a good idea.

 }  // anonymous namespace
 
 namespace mojo {
-using webauth::mojom::blink::RelyingPartyAccount;
-using webauth::mojom::blink::RelyingPartyAccountPtr;
 using webauth::mojom::blink::AuthenticatorStatus;
-using webauth::mojom::blink::ScopedCredentialDescriptor;
-using webauth::mojom::blink::ScopedCredentialOptions;
-using webauth::mojom::blink::ScopedCredentialOptionsPtr;
-using webauth::mojom::blink::ScopedCredentialParameters;
-using webauth::mojom::blink::ScopedCredentialParametersPtr;
-using webauth::mojom::blink::ScopedCredentialType;
+using webauth::mojom::blink::MakeCredentialOptionsPtr;
+using webauth::mojom::blink::PublicKeyCredentialEntityPtr;
+using webauth::mojom::blink::PublicKeyCredentialParametersPtr;
+using webauth::mojom::blink::PublicKeyCredentialType;
 using webauth::mojom::blink::Transport;
 
 // TODO(kpaulhamus): Make this a TypeConverter
 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*>(

[engedy, 2017/07/05 18:51:06]

nit: DCHECK(buffer.isArrayBufferView());

[kpaulhamus, 2017/07/12 21:21:47]

Done.

                       buffer.getAsArrayBufferView().View()->BaseAddress()),
                   buffer.getAsArrayBufferView().View()->byteLength());
   }
   return vector;
 }
 
 // TODO(kpaulhamus): Make this a TypeConverter
-ScopedCredentialType ConvertScopedCredentialType(const String& cred_type) {
-  if (cred_type == "ScopedCred")
-    return ScopedCredentialType::SCOPEDCRED;
+PublicKeyCredentialType ConvertPublicKeyCredentialType(
+    const String& cred_type) {

[engedy, 2017/07/05 18:51:06]

nit: Let's avoid abbreviations, and make this either credential_type or type.

[kpaulhamus, 2017/07/12 21:21:47]

Done.

+  if (cred_type == "public-key")
+    return PublicKeyCredentialType::PUBLICKEY;
   NOTREACHED();
-  return ScopedCredentialType::SCOPEDCRED;
+  return PublicKeyCredentialType::PUBLICKEY;
 }
 
 // TODO(kpaulhamus): Make this a TypeConverter
 Transport ConvertTransport(const String& transport) {
   if (transport == "usb")
     return Transport::USB;
   if (transport == "nfc")
     return Transport::NFC;
   if (transport == "ble")
     return Transport::BLE;
   NOTREACHED();
   return Transport::USB;
 }
 
 // TODO(kpaulhamus): Make this a TypeConverter
-RelyingPartyAccountPtr ConvertRelyingPartyAccount(
-    const blink::RelyingPartyAccount& account_information,
-    blink::ScriptPromiseResolver* resolver) {
-  auto mojo_account = RelyingPartyAccount::New();
-
-  mojo_account->relying_party_display_name =
-      account_information.rpDisplayName();
-  mojo_account->display_name = account_information.displayName();
-  mojo_account->id = account_information.id();
-  mojo_account->name = account_information.name();
-  mojo_account->image_url = account_information.imageURL();
-  return mojo_account;
+PublicKeyCredentialEntityPtr ConvertPublicKeyCredentialUserEntity(
+    const blink::PublicKeyCredentialUserEntity& user) {
+  auto entity = webauth::mojom::blink::PublicKeyCredentialEntity::New();
+  entity->id = user.id();
+  entity->name = user.name();
+  if (user.hasIcon()) {
+    entity->icon = blink::KURL(blink::KURL(), user.icon());
+  }
+  entity->display_name = user.displayName();
+  return entity;
 }
 
 // TODO(kpaulhamus): Make this a TypeConverter
-ScopedCredentialOptionsPtr ConvertScopedCredentialOptions(
-    const blink::ScopedCredentialOptions options,
-    blink::ScriptPromiseResolver* resolver) {
-  auto mojo_options = ScopedCredentialOptions::New();
-  if (options.hasRpId()) {
-    // if rpID is missing, it will later be set to the origin of the page
-    // in the secure browser process.
-    mojo_options->relying_party_id = options.rpId();
+PublicKeyCredentialEntityPtr ConvertPublicKeyCredentialEntity(
+    const blink::PublicKeyCredentialEntity& rp) {
+  auto entity = webauth::mojom::blink::PublicKeyCredentialEntity::New();
+  entity->id = rp.id();
+  entity->name = rp.name();
+  if (rp.hasIcon()) {
+    entity->icon = blink::KURL(blink::KURL(), rp.icon());
+  }
+  return entity;
+}
+
+// TODO(kpaulhamus): Make this a TypeConverter
+PublicKeyCredentialParametersPtr ConvertPublicKeyCredentialParameters(
+    const blink::PublicKeyCredentialParameters parameter) {
+  auto mojo_parameter =
+      webauth::mojom::blink::PublicKeyCredentialParameters::New();
+  mojo_parameter->type = ConvertPublicKeyCredentialType(parameter.type());
+  // TODO(kpaulhamus): add AlgorithmIdentifier
+  return mojo_parameter;
+}
+
+// TODO(kpaulhamus): Make this a TypeConverter
+MakeCredentialOptionsPtr ConvertMakeCredentialOptions(
+    const blink::MakeCredentialOptions options) {
+  auto mojo_options = webauth::mojom::blink::MakeCredentialOptions::New();
+  mojo_options->relying_party = ConvertPublicKeyCredentialEntity(options.rp());
+  mojo_options->user = ConvertPublicKeyCredentialUserEntity(options.user());
+  mojo_options->challenge = ConvertBufferSource(options.challenge());
+
+  Vector<webauth::mojom::blink::PublicKeyCredentialParametersPtr> parameters;
+  for (const auto& parameter : options.parameters()) {
+    if (parameter.hasType()) {

[engedy, 2017/07/05 18:51:07]

nit: I'm not very familiar with this, but I wonder if this can really be false,
given everything is `required` in the IDLs?

[kpaulhamus, 2017/07/12 21:21:47]

Good point.

+      parameters.push_back(ConvertPublicKeyCredentialParameters(parameter));
+    }
+  }
+  mojo_options->crypto_parameters = std::move(parameters);
+
+  // Step 4 of https://w3c.github.io/webauthn/#createCredential
+  int64_t predicted_timeout;

[engedy, 2017/07/05 18:51:06]

nit: What does predicted mean in this context? I don't find a mention to it in
the spec.

[kpaulhamus, 2017/07/12 21:21:48]

It's essentially a temp variable. I'll change the name so it's not confusing.

[engedy, 2017/07/13 11:33:54]

Much clearer, thank you!

+  if (options.hasTimeout()) {
+    predicted_timeout = options.timeout();
+  } else {
+    predicted_timeout = kAdjustedTimeoutLowerInMilliseconds;
   }
 
-  // Step 4 of https://w3c.github.io/webauthn/#createCredential
-  int predicted_timeout = kAdjustedTimeoutLowerInSeconds;
-  if (options.hasTimeoutSeconds()) {
-    predicted_timeout = static_cast<int>(options.timeoutSeconds());
-  }
-
-  mojo_options->adjusted_timeout = static_cast<double>(
-      std::max(kAdjustedTimeoutLowerInSeconds,
-               std::min(kAdjustedTimeoutUpperInSeconds, predicted_timeout)));
+  mojo_options->adjusted_timeout = WTF::TimeDelta::FromMilliseconds(std::max(

[engedy, 2017/07/05 18:51:07]

nit: Can we use WTF::clampTo here?

[kpaulhamus, 2017/07/12 21:21:47]

I didn't know about clampTo, that's nifty. I tried using it, but it threw errors
over WTF::TimeDelta types. I think it's an either-or situation between clampTo
and using WTF::TimeDelta for the kAdjustedTimeout* types. The max/min comparison
as-is seems fine to me?

[engedy, 2017/07/13 11:33:53]

That's a shame. And yes, I agree, using WTF::TimeDelta trumps using clampTo, so
let's stick to min/max comparison. Ack.

+      kAdjustedTimeoutLowerInMilliseconds,
+      std::min(kAdjustedTimeoutUpperInMilliseconds, predicted_timeout)));
 
   if (options.hasExcludeList()) {
-    // Adds the excludeList members (which are ScopedCredentialDescriptors)
+    // Adds the excludeList members (which are PublicKeyCredentialDescriptors)
     for (const auto& descriptor : options.excludeList()) {
-      auto mojo_descriptor = ScopedCredentialDescriptor::New();
-      mojo_descriptor->type = ConvertScopedCredentialType(descriptor.type());
+      auto mojo_descriptor =
+          webauth::mojom::blink::PublicKeyCredentialDescriptor::New();
+      mojo_descriptor->type = ConvertPublicKeyCredentialType(descriptor.type());
       mojo_descriptor->id = ConvertBufferSource(descriptor.id());
       for (const auto& transport : descriptor.transports())
         mojo_descriptor->transports.push_back(ConvertTransport(transport));
       mojo_options->exclude_list.push_back(std::move(mojo_descriptor));
     }
   }
-  // TODO(kpaulhamus): add AuthenticationExtensions;
   return mojo_options;
 }
 
-// TODO(kpaulhamus): Make this a TypeConverter
-ScopedCredentialParametersPtr ConvertScopedCredentialParameter(
-    const blink::ScopedCredentialParameters parameter,
-    blink::ScriptPromiseResolver* resolver) {
-  auto mojo_parameter = ScopedCredentialParameters::New();
-  mojo_parameter->type = ConvertScopedCredentialType(parameter.type());
-  // TODO(kpaulhamus): add AlgorithmIdentifier
-  return mojo_parameter;
-}
-
 blink::DOMException* CreateExceptionFromStatus(AuthenticatorStatus status) {
   switch (status) {
     case AuthenticatorStatus::NOT_IMPLEMENTED:
       return blink::DOMException::Create(blink::kNotSupportedError,
                                          "Not implemented.");
     case AuthenticatorStatus::NOT_ALLOWED_ERROR:
       return blink::DOMException::Create(blink::kNotAllowedError,
                                          "Not allowed.");
     case AuthenticatorStatus::NOT_SUPPORTED_ERROR:
       return blink::DOMException::Create(
           blink::kNotSupportedError,
           "Parameters for this operation are not supported.");
     case AuthenticatorStatus::SECURITY_ERROR:
       return blink::DOMException::Create(blink::kSecurityError,
                                          "The operation was not allowed.");
     case AuthenticatorStatus::UNKNOWN_ERROR:
       return blink::DOMException::Create(blink::kUnknownError,
                                          "Request failed.");
     case AuthenticatorStatus::CANCELLED:
       return blink::DOMException::Create(blink::kNotAllowedError,
                                          "User canceled the operation.");
     case AuthenticatorStatus::SUCCESS:
       return nullptr;
     default:
       NOTREACHED();
       return nullptr;
   }
 }

[engedy, 2017/07/05 18:51:07]

nit: Add blank line after

[kpaulhamus, 2017/07/12 21:21:48]

Done.

 }  // namespace mojo
 
 namespace blink {
+
 WebAuthentication::WebAuthentication(LocalFrame& frame)
-    : ContextLifecycleObserver(frame.GetDocument()) {}
+    : ContextLifecycleObserver(frame.GetDocument()) {
+  frame.GetInterfaceProvider()->GetInterface(
+      mojo::MakeRequest(&authenticator_));
+  authenticator_.set_connection_error_handler(ConvertToBaseCallback(
+      WTF::Bind(&WebAuthentication::OnAuthenticatorConnectionError,
+                WrapWeakPersistent(this))));
+}
 
 WebAuthentication::~WebAuthentication() {
   // |authenticator_| may still be valid but there should be no more
   // outstanding requests because each holds a persistent handle to this object.
   DCHECK(authenticator_requests_.IsEmpty());
 }
 
 ScriptPromise WebAuthentication::makeCredential(
     ScriptState* script_state,
-    const RelyingPartyAccount& account_information,
-    const HeapVector<ScopedCredentialParameters> crypto_parameters,
-    const BufferSource& attestation_challenge,
-    ScopedCredentialOptions& options) {
-  ScriptPromise promise = RejectIfNotSupported(script_state);
-  if (!promise.IsEmpty())
-    return promise;
+    MakeCredentialOptions& publicKey) {
+  if (!authenticator_) {
+    return ScriptPromise::RejectWithDOMException(
+        script_state, DOMException::Create(kNotSupportedError));
+  }
 
   ScriptPromiseResolver* resolver = ScriptPromiseResolver::Create(script_state);
+  ScriptPromise promise = resolver->Promise();

[engedy, 2017/07/05 18:51:07]

nit: Do we need to make this call here because of side-effects of calling
Promise()? If so, could you please add a comment to this effect?

[kpaulhamus, 2017/07/12 21:21:48]

Could you clarify what kind of comment you mean? This is how promises are
conventionally returned. I could just "return resolver->Promise()" at the end
like before, if that would be clearer.

[engedy, 2017/07/13 11:33:54]

Sorry for the ambiguous phrasing. I was indeed just wondering why we don't just
call resolver->Promise() in the return statement, as you are doing now. Ack.

 
-  Vector<uint8_t> buffer = mojo::ConvertBufferSource(attestation_challenge);
-  auto opts = mojo::ConvertScopedCredentialOptions(options, resolver);
-  Vector<webauth::mojom::blink::ScopedCredentialParametersPtr> parameters;
-  for (const auto& parameter : crypto_parameters) {
-    if (parameter.hasType()) {
-      parameters.push_back(
-          mojo::ConvertScopedCredentialParameter(parameter, resolver));
-    }
-  }
-  auto account =
-      mojo::ConvertRelyingPartyAccount(account_information, resolver);
+  auto options = mojo::ConvertMakeCredentialOptions(publicKey);
   authenticator_requests_.insert(resolver);
   authenticator_->MakeCredential(
-      std::move(account), std::move(parameters), buffer, std::move(opts),
-      ConvertToBaseCallback(WTF::Bind(&WebAuthentication::OnMakeCredential,
-                                      WrapPersistent(this),
-                                      WrapPersistent(resolver))));
-  return resolver->Promise();
+      std::move(options), ConvertToBaseCallback(WTF::Bind(
+                              &WebAuthentication::OnMakeCredential,
+                              WrapPersistent(this), WrapPersistent(resolver))));
+  return promise;
 }
 
 ScriptPromise WebAuthentication::getAssertion(
     ScriptState* script_state,
-    const BufferSource& assertion_challenge,
-    const AuthenticationAssertionOptions& options) {
+    const PublicKeyCredentialRequestOptions& publicKey) {
   NOTREACHED();
   return ScriptPromise();
 }
 
-void WebAuthentication::ContextDestroyed(ExecutionContext*) {

[engedy, 2017/07/05 18:51:07]

Hmm, how come we don't need this anymore?

[kpaulhamus, 2017/07/12 21:21:48]

I was going to put it back, but it's a ContextLifecycleObserver override, which
we don't use anymore.

[engedy, 2017/07/13 11:33:53]

Could you please explain to me why we don't need it anymore? I don't see the
lifetime management changes. What am I missing?

[kpaulhamus, 2017/07/13 16:00:53]

Yeah, in WebAuthentication.h/.cpp in Patch 2, you had comments about not needing
ContextLifecycleObserver. Once I stripped that out, it made no sense to have
ContextDestroyed(). It certainly doesn't matter once I move this stuff to
CredMan because WebAuthentication will stop controlling the lifecycle of the
blink-layer implementation anyways.

[engedy, 2017/07/13 20:27:12]

Sorry, misunderstanding. I suggested removing only because it seemed it was not
used anymore, but if we needed it, then we still need it. Could you please
re-add it?

-  Cleanup();
-}
-
 void WebAuthentication::OnMakeCredential(
     ScriptPromiseResolver* resolver,
     webauth::mojom::blink::AuthenticatorStatus status,
-    webauth::mojom::blink::ScopedCredentialInfoPtr credential) {
+    webauth::mojom::blink::PublicKeyCredentialInfoPtr credential) {
   if (!MarkRequestComplete(resolver))
     return;
 
   DOMException* error = mojo::CreateExceptionFromStatus(status);
   if (error) {
     DCHECK(!credential);
     resolver->Reject(error);
     Cleanup();
     return;
   }
 
-  if (credential->client_data.IsEmpty() || credential->attestation.IsEmpty()) {
+  if (credential->client_data_json.IsEmpty() ||
+      credential->response->attestation_object.IsEmpty()) {
     resolver->Reject(
-        DOMException::Create(kNotFoundError, "No credential returned."));
-    return;
+        DOMException::Create(kNotFoundError, "No credentials returned."));
   }
 
-  DOMArrayBuffer* clientDataBuffer = DOMArrayBuffer::Create(
-      static_cast<void*>(&credential->client_data.front()),
-      credential->client_data.size());
+  DOMArrayBuffer* client_data_buffer = DOMArrayBuffer::Create(
+      static_cast<void*>(&credential->client_data_json.front()),
+      credential->client_data_json.size());
 
-  DOMArrayBuffer* attestationBuffer = DOMArrayBuffer::Create(
-      static_cast<void*>(&credential->attestation.front()),
-      credential->attestation.size());
+  // Return AuthenticatorAttestationResponse
+  DOMArrayBuffer* attestation_buffer = DOMArrayBuffer::Create(
+      static_cast<void*>(&credential->response->attestation_object.front()),
+      credential->response->attestation_object.size());
 
-  ScopedCredentialInfo* scopedCredential =
-      ScopedCredentialInfo::Create(clientDataBuffer, attestationBuffer);
-  resolver->Resolve(scopedCredential);
+  AuthenticatorAttestationResponse* attestation_response =
+      AuthenticatorAttestationResponse::Create(client_data_buffer,
+                                               attestation_buffer);
+  resolver->Resolve(attestation_response);
 }
 
 ScriptPromise WebAuthentication::RejectIfNotSupported(

[engedy, 2017/07/05 18:51:06]

Looks like this is not used anymore?

[kpaulhamus, 2017/07/12 21:21:48]

Ah, it's supposed to be used. Bad merge/rebase.

     ScriptState* script_state) {
   if (!authenticator_) {
     if (!GetFrame()) {
       return ScriptPromise::RejectWithDOMException(
           script_state, DOMException::Create(kNotSupportedError));
     }
     GetFrame()->GetInterfaceProvider()->GetInterface(
         mojo::MakeRequest(&authenticator_));
 
     authenticator_.set_connection_error_handler(ConvertToBaseCallback(
@@ skipping 24 matching lines @@
   ContextLifecycleObserver::Trace(visitor);
 }
 
 // Clears the promise resolver, timer, and closes the Mojo connection.
 void WebAuthentication::Cleanup() {
   authenticator_.reset();
   authenticator_requests_.clear();
 }
 
 }  // namespace blink