Add Digital Asset Links verification for postMessage API

components/digital_asset_links/digital_asset_links_handler.cc

Index: components/digital_asset_links/digital_asset_links_handler.cc
diff --git a/components/digital_asset_links/digital_asset_links_handler.cc b/components/digital_asset_links/digital_asset_links_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..93e3956273be7217a18d6c5bf07b6134a287989e
--- /dev/null
+++ b/components/digital_asset_links/digital_asset_links_handler.cc
@@ -0,0 +1,101 @@
+// Copyright (c) 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/digital_asset_links/digital_asset_links_handler.h"
+
+#include "base/json/json_reader.h"
+#include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
+#include "base/values.h"
+#include "net/base/load_flags.h"
+#include "net/base/url_util.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_status_code.h"
+#include "net/http/http_util.h"
+#include "net/url_request/url_request_status.h"
+
+namespace {
+static const char kDigitalAssetLinksBaseURL[] =

[Benoit L, 2017/03/31 15:38:43]

nit: "static" is unnecessary here and below (due to the anonymous namespace).

[Yusuf, 2017/03/31 22:41:49]

Done.

+    "https://digitalassetlinks.googleapis.com";
+static const char kDigitalAssetLinksCheckAPI[] = "/v1/assetlinks:check?";

[Benoit L, 2017/03/31 15:38:42]

nit: Should the path include the "?" at the end?
Wouldn't it be automatically added by AppendQueryParameters below?

[Yusuf, 2017/03/31 22:41:50]

I might be missing something but it looks like no. AppendQueryParameters only
adds the = and &.

+static const char kTargetOriginParam[] = "source.web.site";
+static const char kSourcePackageNameParam[] = "target.androidApp.packageName";
+static const char kSourceFingerprintParam[] =
+    "target.androidApp.certificate.sha256Fingerprint";
+static const char kRelationshipParam[] = "relation";
+
+GURL GetUrlForCheckingRelationship(std::string web_domain,

[Benoit L, 2017/03/31 15:38:42]

nit: make all parameters
const std::string& param

[Yusuf, 2017/03/31 22:41:50]

Done.

+                                   std::string package_name,
+                                   std::string fingerprint,
+                                   std::string relationship) {
+  GURL request_url =
+      GURL(kDigitalAssetLinksBaseURL).Resolve(kDigitalAssetLinksCheckAPI);
+  request_url =
+      net::AppendQueryParameter(request_url, kTargetOriginParam, web_domain);
+  request_url = net::AppendQueryParameter(request_url, kSourcePackageNameParam,
+                                          package_name);
+  request_url = net::AppendQueryParameter(request_url, kSourceFingerprintParam,
+                                          fingerprint);
+  request_url =
+      net::AppendQueryParameter(request_url, kRelationshipParam, relationship);
+  return request_url;

[Benoit L, 2017/03/31 15:38:43]

nit: Add a DCHECK() above to check that the URL is valid?

[Yusuf, 2017/03/31 22:41:50]

Done.

+}
+}

[Benoit L, 2017/03/31 15:38:42]

nit: Add
  // namespace

[Yusuf, 2017/03/31 22:41:50]

Done.

+
+namespace digital_asset_links {
+
+DigitalAssetLinksHandler::DigitalAssetLinksHandler(
+    const scoped_refptr<net::URLRequestContextGetter>& request_context) {

[Benoit L, 2017/03/31 15:38:42]

nit: Initializer list instead of inline in the constructor?

DigitalAssetLinksHandler(...) : request_context_(request_context) {}

[Yusuf, 2017/03/31 22:41:49]

Done.

+  request_context_ = request_context;
+}
+
+DigitalAssetLinksHandler::~DigitalAssetLinksHandler() {}
+
+void DigitalAssetLinksHandler::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  base::DictionaryValue* dict = NULL;
+  if (!source->GetStatus().is_success() ||
+      source->GetResponseCode() != net::HTTP_OK) {
+    LOG(WARNING) << base::StringPrintf(
+        "Digital Asset Links endpoint responded with code %d.",
+        source->GetResponseCode());
+    listener_.get()->OnRelationshipCheckComplete(dict);

[Benoit L, 2017/03/31 15:38:42]

nit: listener_->OnRelationshipCheckComplete(nullptr)
?

Here and below, replace
listener_.get()->Foo()

with
listener->Foo()

[Yusuf, 2017/03/31 22:41:50]

Done.

+    return;
+  }
+  std::string response_body;
+  source->GetResponseAsString(&response_body);
+
+  std::unique_ptr<base::Value> value = base::JSONReader::Read(response_body);
+  if (!value.get() || !value->GetAsDictionary(&dict)) {

[Benoit L, 2017/03/31 15:38:42]

nit:
if (!value || !value->GetAsDictionary(...))

Also, explicitly calling OnRelationshipCheckComplete(nullptr)
makes the code easier to follow by showing the error cases more clearly.

It may also save you from pesky warnings from static analyzer tools, as
otherwise the two branch of the if() are identical.

[Yusuf, 2017/03/31 22:41:50]

Done.

+    listener_.get()->OnRelationshipCheckComplete(dict);
+    return;
+  }
+  listener_.get()->OnRelationshipCheckComplete(dict);
+}
+
+bool DigitalAssetLinksHandler::CheckDigitalAssetLinkRelationship(
+    AssetLinkListener* listener,
+    std::string web_domain,

[Benoit L, 2017/03/31 15:38:42]

nit: make the std::string arguments
const std::string&

[Yusuf, 2017/03/31 22:41:50]

Done.

+    std::string package_name,
+    std::string fingerprint,
+    std::string relationship) {
+  GURL request_url = GetUrlForCheckingRelationship(web_domain, package_name,
+                                                   fingerprint, relationship);
+  if (!request_url.is_valid())
+    return false;
+
+  listener_.reset(listener);

[Benoit L, 2017/03/31 15:38:42]

If this object takes ownership of the listener, then change the parameter to
std::unique_ptr<>.

Otherwise it's not clear that this class takes ownership of the pointer.

If it doesn't take ownership of the listener, then document that the listener
must outlive this class, and take a raw pointer as parameter.

[Yusuf, 2017/03/31 22:41:50]

After the conversion to Callback, it made sense to give the ownership to
handler. So changed the param to unique_ptr

+
+  url_fetcher_.reset();

[Benoit L, 2017/03/31 15:38:42]

nit:

url_fetcher_ = std::move(net::URLFetcher::Create(....))

Then you don't need the reset() call.

[Yusuf, 2017/03/31 22:41:50]

Done.

+
+  url_fetcher_ =

[Benoit L, 2017/03/31 15:38:42]

Question: What is the behavior of URLFetcher with redirects and failed
certificate validation?

[Yusuf, 2017/03/31 22:41:50]

From url_fetcher.h:
Normally, URLFetcher will abort loads that request SSL client certificate
authentication. But there is SetIgnoreCertificateRequests to ignore this
behavior which we don't set here.

For redirects, there is SetStopOnRedirect which says if set to true, 3xx
responses will cause the fetch to halt immediately rather than continue through
the redirect.

So I guess the default behavior which we are using right now will continue
through the redirect. If there are any concerns around that we can set this to
true. I don't think it causes any issues now, but only if the web endpoint moves
to another destination and starts redirecting.

+      net::URLFetcher::Create(0, request_url, net::URLFetcher::GET, this);
+  url_fetcher_->SetAutomaticallyRetryOn5xx(false);
+  url_fetcher_->SetRequestContext(request_context_.get());
+  url_fetcher_->Start();
+  return true;
+}
+
+}  // namespace digital_asset_links