Add Digital Asset Links verification for postMessage API

chrome/browser/android/customtabs/origin_verifier.cc

+// 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 "chrome/browser/android/customtabs/origin_verifier.h"
+
+#include "base/android/jni_android.h"
+#include "base/android/jni_string.h"
+#include "base/strings/string_number_conversions.h"

[nyquist, 2017/04/19 06:35:56]

Nit: Which of the conversions is this for again?

[Yusuf, 2017/04/26 00:51:36]

Removed. I dont think we need this anymore.

+#include "base/values.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "jni/OriginVerifier_jni.h"
+
+using base::android::ConvertJavaStringToUTF16;
+using base::android::JavaParamRef;
+
+namespace customtabs {
+
+OriginVerifier::OriginVerifier(JNIEnv* env, jobject obj) {
+  jobject_.Reset(env, obj);
+  Profile* profile = ProfileManager::GetLastUsedProfile()->GetOriginalProfile();

[nyquist, 2017/04/19 06:35:56]

I don't know how profiles interact with custom tabs, etc., but if this is
possibly becoming a generic tool in the future, should we get this Profile from
Java instead? Right now it's hidden far away from the caller, so it might be
hard to figure out which context they're using for their request.

[Yusuf, 2017/04/26 00:51:36]

Done.

+  DCHECK(profile);
+  asset_link_handler_ =
+      base::MakeUnique<digital_asset_links::DigitalAssetLinksHandler>(
+          profile->GetRequestContext());
+}
+
+OriginVerifier::~OriginVerifier() {}

[nyquist, 2017/04/19 06:35:56]

Nit: = default?

[Yusuf, 2017/04/26 00:51:36]

Done.

+
+void OriginVerifier::VerifyOrigin(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    const JavaParamRef<jstring>& j_package_name,
+    const base::android::JavaParamRef<jstring>& j_fingerprint,
+    const JavaParamRef<jstring>& j_origin) {
+  if (!j_package_name || !j_fingerprint || !j_origin)
+    return;

[nyquist, 2017/04/19 06:35:56]

It looks like this leads to the Java listener never being invoked, but that is
not documented clearly in the interface description. Could you update that or
change that or change how this behaves?

Nit: Maybe an empty newline after this early return for clarity?

[Yusuf, 2017/04/26 00:51:36]

Done.

+  std::string package_name = ConvertJavaStringToUTF8(env, j_package_name);
+  std::string fingerprint = ConvertJavaStringToUTF8(env, j_fingerprint);
+  std::string origin = ConvertJavaStringToUTF8(env, j_origin);
+
+  asset_link_handler_->CheckDigitalAssetLinkRelationship(
+      base::Bind(&customtabs::OriginVerifier::OnRelationshipCheckComplete,
+                 base::Unretained(this)),

[nyquist, 2017/04/19 06:35:56]

Should you be giving away a WeakPtr instead here?  Here you're assuming total
ownership over the lifetime and guaranteeing that it will always be there when
the callback happens, which I believe is not the case if you call start() again
in Java (where you cleanUp and delete |this|).

Or is the idea here that you will either be a live, or you will be dead, but in
the latter case, you'll delete the DigitalAssetLinksHandler, which in turn will
delete the URLFetcher which will lead to the callback not happening? If that's
the plan, could you document that here so future readers can easily figure out
what the lifetime guarantees are?

I guess this might be affected about whether you end up using the safe JSON
parser, or if you continue to parse JSON retrieved from the network in the main
thread of the browser process.

Also, you might find that
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md is a
great source for inspiration! :-)

[Yusuf, 2017/04/26 00:51:36]

The latter and added a comment here as well.

[nyquist, 2017/04/27 04:38:25]

Acknowledged.

+      origin, package_name, fingerprint,
+      "delegate_permission/common.use_as_origin");
+}
+
+void OriginVerifier::OnRelationshipCheckComplete(
+    std::unique_ptr<base::DictionaryValue> response) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+
+  bool verified = false;
+
+  if (response) {
+    response->GetBoolean(
+        digital_asset_links::kDigitalAssetLinksCheckResponseKeyLinked,
+        &verified);
+  }
+  Java_OriginVerifier_originVerified(env, jobject_, verified);
+}
+
+void OriginVerifier::Destroy(JNIEnv* env,
+                             const base::android::JavaRef<jobject>& obj) {
+  delete this;
+}
+
+static jlong Init(JNIEnv* env,
+                  const base::android::JavaParamRef<jobject>& obj) {
+  if (!g_browser_process)
+    return 0;

[nyquist, 2017/04/19 06:35:56]

Nit: Empty line after early return?

[Yusuf, 2017/04/26 00:51:35]

Done.

+  OriginVerifier* native_verifier = new OriginVerifier(env, obj);
+  return reinterpret_cast<intptr_t>(native_verifier);
+}
+
+bool RegisterOriginVerifier(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+}  // namespace customtabs