Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2767333006:
Add Digital Asset Links verification for postMessage API (Closed)
DescriptionAdd Digital Asset Links verification for postMessage API
Adds DigitalAssetLinks handler which queries digitalassetlinks.googleapis.com/
for verifying the relationship between an Android app and a web domain.
This is added in a new component digital_asset_links since this API is
a generic web API for checking Android-Web relationships. It may be
useful for iOS and ChromeOS as well. The component currently depends on
base and net.
Then custom tabs uses this handler for verifying postMessage origin declared by
the client app when they send requestPostMessageChannel. This enabled
third party apps to use postMessage related APIs with secure and
verified origin declaration.
BUG=704975
Review-Url: https://codereview.chromium.org/2767333006
Cr-Commit-Position: refs/heads/master@{#467791}
Committed: https://chromium.googlesource.com/chromium/src/+/2f2c75134816ad67b3bab94cfff8347b53cbc7d3
Patch Set 1 #Patch Set 2 : FindBugs fixes #Patch Set 3 : Added caching #Patch Set 4 : updated origin #Patch Set 5 : moved digital_asset_links to a component #
Total comments: 32
Patch Set 6 : lizeb@ commetns #
Total comments: 22
Patch Set 7 : made clang happy and also added a test #Patch Set 8 : lizeb@ comments #
Total comments: 14
Patch Set 9 : java comments #Patch Set 10 : fix hex conversion logic #
Total comments: 16
Patch Set 11 : lizeb nits #Patch Set 12 : added native unit tests #
Total comments: 8
Patch Set 13 : test fix and DEPS update #Patch Set 14 : lizeb@ test comments #
Total comments: 106
Patch Set 15 : nyquist@ changes #Patch Set 16 : fix OriginVerifierTest #Patch Set 17 : tests fixed and the network request verified #Patch Set 18 : unbreak other ClientManagerTests #Patch Set 19 : unittests #Patch Set 20 : moar unittests #
Total comments: 14
Patch Set 21 : nyquist final comments #Patch Set 22 : destructor #Messages
Total messages: 82 (53 generated)
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yusufo@chromium.org changed reviewers: + lizeb@chromium.org
Hi Benoit, PTAL at the current state of this CL. TODOs and things that would benefit from suggestions: -Caching logic for verification on Java side. (I haven't added this yet. We may benefit from web cache depending on how often these requests occur, but maybe we should also cache and query per day/week/chrome lifecycle?) -New relationship to declare in digital asset links. We won't use delegate_permission/common.handle_all_urls (See origin_verifier.cc for context), but need a new one. -New place for DigitalAssetLinksHandler. Should this be a component?
I am now leaning towards a per app lifecycle caching on the ClientManager side.
This is now ready for review. @lizeb for customtabs/ bits and overall approach
Description was changed from ========== Add Digital Asset Links verification for postMessage API Adds DigitalAssetLinks handler which queries digitalassetlinks.googleapis.com/ for verifying the relationship between an Android app and a web domain. Then custom tabs uses this handler for verifying postMessage origin declared by the client app when they send requestPostMessageChannel BUG=704975 ========== to ========== Add Digital Asset Links verification for postMessage API Adds DigitalAssetLinks handler which queries digitalassetlinks.googleapis.com/ for verifying the relationship between an Android app and a web domain. This is added in a new component digital_asset_links since this API is a generic web API for checking Android-Web relationships. It may be useful for iOS and ChromeOS as well. The component currently depends on base and net. Then custom tabs uses this handler for verifying postMessage origin declared by the client app when they send requestPostMessageChannel. This enabled third party apps to use postMessage related APIs with secure and verified origin declaration. BUG=704975 ==========
Description was changed from ========== Add Digital Asset Links verification for postMessage API Adds DigitalAssetLinks handler which queries digitalassetlinks.googleapis.com/ for verifying the relationship between an Android app and a web domain. This is added in a new component digital_asset_links since this API is a generic web API for checking Android-Web relationships. It may be useful for iOS and ChromeOS as well. The component currently depends on base and net. Then custom tabs uses this handler for verifying postMessage origin declared by the client app when they send requestPostMessageChannel. This enabled third party apps to use postMessage related APIs with secure and verified origin declaration. BUG=704975 ========== to ========== Add Digital Asset Links verification for postMessage API Adds DigitalAssetLinks handler which queries digitalassetlinks.googleapis.com/ for verifying the relationship between an Android app and a web domain. This is added in a new component digital_asset_links since this API is a generic web API for checking Android-Web relationships. It may be useful for iOS and ChromeOS as well. The component currently depends on base and net. Then custom tabs uses this handler for verifying postMessage origin declared by the client app when they send requestPostMessageChannel. This enabled third party apps to use postMessage related APIs with secure and verified origin declaration. BUG=704975 ==========
Description was changed from ========== Add Digital Asset Links verification for postMessage API Adds DigitalAssetLinks handler which queries digitalassetlinks.googleapis.com/ for verifying the relationship between an Android app and a web domain. This is added in a new component digital_asset_links since this API is a generic web API for checking Android-Web relationships. It may be useful for iOS and ChromeOS as well. The component currently depends on base and net. Then custom tabs uses this handler for verifying postMessage origin declared by the client app when they send requestPostMessageChannel. This enabled third party apps to use postMessage related APIs with secure and verified origin declaration. BUG=704975 ========== to ========== Add Digital Asset Links verification for postMessage API Adds DigitalAssetLinks handler which queries digitalassetlinks.googleapis.com/ for verifying the relationship between an Android app and a web domain. This is added in a new component digital_asset_links since this API is a generic web API for checking Android-Web relationships. It may be useful for iOS and ChromeOS as well. The component currently depends on base and net. Then custom tabs uses this handler for verifying postMessage origin declared by the client app when they send requestPostMessageChannel. This enabled third party apps to use postMessage related APIs with secure and verified origin declaration. BUG=704975 ==========
Sorry for the delay. Happy that we can add this! I have only looked at the C++ side. Most comments are nits, the most important question being around the behavior of URLFetcher with redirects and certificate validation errors. I believe it's fine, but don't want to assume, since this is security-sensitive. https://codereview.chromium.org/2767333006/diff/80001/chrome/browser/android/... File chrome/browser/android/customtabs/origin_verifier.cc (right): https://codereview.chromium.org/2767333006/diff/80001/chrome/browser/android/... chrome/browser/android/customtabs/origin_verifier.cc:49: if (!response) { What about: bool verified = false; if (response) { response->GetBoolean( digital_asset_links::kDigitalAssetLinksCheckResponseKeyLinked, &verified); } Java_OriginVerifier_originVerified(env, jobject_, verified); https://codereview.chromium.org/2767333006/diff/80001/chrome/browser/android/... chrome/browser/android/customtabs/origin_verifier.cc:61: asset_link_handler_.reset(); since there is delete this below, this is unnecessary. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... File components/digital_asset_links/digital_asset_links_handler.cc (right): https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:20: static const char kDigitalAssetLinksBaseURL[] = nit: "static" is unnecessary here and below (due to the anonymous namespace). https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:22: static const char kDigitalAssetLinksCheckAPI[] = "/v1/assetlinks:check?"; nit: Should the path include the "?" at the end? Wouldn't it be automatically added by AppendQueryParameters below? https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:29: GURL GetUrlForCheckingRelationship(std::string web_domain, nit: make all parameters const std::string& param https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:43: return request_url; nit: Add a DCHECK() above to check that the URL is valid? https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:45: } nit: Add // namespace https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:50: const scoped_refptr<net::URLRequestContextGetter>& request_context) { nit: Initializer list instead of inline in the constructor? DigitalAssetLinksHandler(...) : request_context_(request_context) {} https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:64: listener_.get()->OnRelationshipCheckComplete(dict); nit: listener_->OnRelationshipCheckComplete(nullptr) ? Here and below, replace listener_.get()->Foo() with listener->Foo() https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:71: if (!value.get() || !value->GetAsDictionary(&dict)) { 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. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:80: std::string web_domain, nit: make the std::string arguments const std::string& https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:89: listener_.reset(listener); 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. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:91: url_fetcher_.reset(); nit: url_fetcher_ = std::move(net::URLFetcher::Create(....)) Then you don't need the reset() call. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:93: url_fetcher_ = Question: What is the behavior of URLFetcher with redirects and failed certificate validation? https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... File components/digital_asset_links/digital_asset_links_handler.h (right): https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.h:18: static const char kDigitalAssetLinksCheckResponseKeyLinked[] = "linked"; nit: Make this constexpr (and hope for the best) or only declare the constant here, and define it in the .cc file. Otherwise it will be duplicated at each inclusion of the file. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.h:20: class AssetLinkListener { Since it is a simple Callback for now, what about just a typedef with base::Callback? Something like (in DigitalAssetLinkHandler): typedef base::Callback<void(std::unique_ptr<base::DictionaryValue>)> RelationshipCheckResultCallback; ? Independently, you need to pass a unique_ptr<> here, otherwise the dictionary will be leaked, unless the client knows it has to delete it (and in the android code the dictionary is indeed leaked).
Moved this to chrome/android with a folder specific DEPS file, after a chat with jam@. For now this won't be a component. https://codereview.chromium.org/2767333006/diff/80001/chrome/browser/android/... File chrome/browser/android/customtabs/origin_verifier.cc (right): https://codereview.chromium.org/2767333006/diff/80001/chrome/browser/android/... chrome/browser/android/customtabs/origin_verifier.cc:49: if (!response) { On 2017/03/31 15:38:42, Benoit L wrote: > What about: > > bool verified = false; > > if (response) { > response->GetBoolean( > digital_asset_links::kDigitalAssetLinksCheckResponseKeyLinked, > &verified); > } > Java_OriginVerifier_originVerified(env, jobject_, verified); Done. https://codereview.chromium.org/2767333006/diff/80001/chrome/browser/android/... chrome/browser/android/customtabs/origin_verifier.cc:61: asset_link_handler_.reset(); On 2017/03/31 15:38:42, Benoit L wrote: > since there is delete this below, this is unnecessary. Done. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... File components/digital_asset_links/digital_asset_links_handler.cc (right): https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:20: static const char kDigitalAssetLinksBaseURL[] = On 2017/03/31 15:38:43, Benoit L wrote: > nit: "static" is unnecessary here and below (due to the anonymous namespace). Done. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:22: static const char kDigitalAssetLinksCheckAPI[] = "/v1/assetlinks:check?"; On 2017/03/31 15:38:42, Benoit L wrote: > nit: Should the path include the "?" at the end? > Wouldn't it be automatically added by AppendQueryParameters below? I might be missing something but it looks like no. AppendQueryParameters only adds the = and &. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:29: GURL GetUrlForCheckingRelationship(std::string web_domain, On 2017/03/31 15:38:42, Benoit L wrote: > nit: make all parameters > const std::string& param Done. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:43: return request_url; On 2017/03/31 15:38:43, Benoit L wrote: > nit: Add a DCHECK() above to check that the URL is valid? Done. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:45: } On 2017/03/31 15:38:42, Benoit L wrote: > nit: Add > // namespace Done. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:50: const scoped_refptr<net::URLRequestContextGetter>& request_context) { On 2017/03/31 15:38:42, Benoit L wrote: > nit: Initializer list instead of inline in the constructor? > > DigitalAssetLinksHandler(...) : request_context_(request_context) {} Done. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:64: listener_.get()->OnRelationshipCheckComplete(dict); On 2017/03/31 15:38:42, Benoit L wrote: > nit: listener_->OnRelationshipCheckComplete(nullptr) > ? > > Here and below, replace > listener_.get()->Foo() > > with > listener->Foo() Done. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:71: if (!value.get() || !value->GetAsDictionary(&dict)) { On 2017/03/31 15:38:42, Benoit L wrote: > 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. Done. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:80: std::string web_domain, On 2017/03/31 15:38:42, Benoit L wrote: > nit: make the std::string arguments > const std::string& Done. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:89: listener_.reset(listener); On 2017/03/31 15:38:42, Benoit L wrote: > 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. After the conversion to Callback, it made sense to give the ownership to handler. So changed the param to unique_ptr https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:91: url_fetcher_.reset(); On 2017/03/31 15:38:42, Benoit L wrote: > nit: > > url_fetcher_ = std::move(net::URLFetcher::Create(....)) > > Then you don't need the reset() call. Done. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.cc:93: url_fetcher_ = On 2017/03/31 15:38:42, Benoit L wrote: > Question: What is the behavior of URLFetcher with redirects and failed > certificate validation? 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. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... File components/digital_asset_links/digital_asset_links_handler.h (right): https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.h:18: static const char kDigitalAssetLinksCheckResponseKeyLinked[] = "linked"; On 2017/03/31 15:38:43, Benoit L wrote: > nit: Make this constexpr (and hope for the best) or only declare the constant > here, and define it in the .cc file. Otherwise it will be duplicated at each > inclusion of the file. Done. https://codereview.chromium.org/2767333006/diff/80001/components/digital_asse... components/digital_asset_links/digital_asset_links_handler.h:20: class AssetLinkListener { On 2017/03/31 15:38:43, Benoit L wrote: > Since it is a simple Callback for now, what about just a typedef with > base::Callback? > > Something like (in DigitalAssetLinkHandler): > > typedef base::Callback<void(std::unique_ptr<base::DictionaryValue>)> > RelationshipCheckResultCallback; > > > ? > > > Independently, you need to pass a unique_ptr<> here, otherwise the dictionary > will be leaked, unless the client knows it has to delete it (and in the android > code the dictionary is indeed leaked). Done.
Sorry for the delay, and for things I missed initially. A few minor comments about the C++ part. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.cc (right): https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:21: Profile* profile = ProfileManager::GetPrimaryUserProfile(); The documentation for this function says: // This function is temporary and will soon be moved to ash. As such avoid // using it at all cost. What about static Profile* GetLastUsedProfile(); ? Also, can this ever return nullptr? I would tend to say given the current way custom tabs is set up, but it would probably be best to add a null check here. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:22: asset_link_handler_.reset(new digital_asset_links::DigitalAssetLinksHandler( tiny nit: What about: asset_link_handler_ = base::MakeUnique<digital_asset_links::DigitalAssetLinksHandler>(profile->GetRequestContext()); https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:43: asset_link_handler_.get()->CheckDigitalAssetLinkRelationship( nit: asset_link_handler->Check... https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:45: &callback), This doesn't work, as this is taking the address of a local variable. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.h (right): https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:22: explicit OriginVerifier(JNIEnv* env, jobject obj); nit: explicit is only needed for single-parameter constructors. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc (right): https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:72: std::unique_ptr<base::Value> value = base::JSONReader::Read(response_body); DictionaryValue::From() gives a unique_ptr<> directly, from the documentation of GetAsDictionary. It also does the nullptr check for you: std::unique_ptr<DictionarValue> dict = DictionaryValue::From(value); if (dict) { ... } Actually, you don't even need the if condition in this case, since if dict is nullptr, you call callback with nullptr as a parameter. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:78: callback_->Run(nullptr); Should we delete the URLFetcher here? https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:92: callback_ = std::move(listener); nit: as suggested above, passing the callback by value would simplify the code. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:94: url_fetcher_ = std::move( dumb question: is it OK to delete a URLFetcher while the request is in flight? That may happen if we get two requests back to back. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.h (right): https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:45: // The callback for receiving a URLFether result. This handler owns this. nit: ownership is implied by the unique_ptr. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:46: std::unique_ptr<RelationshipCheckResultCallback> callback_; And actually, since this is a callback, why not taking it by value, so that you don't have to care about the unique_ptr?
https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.cc (right): https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:21: Profile* profile = ProfileManager::GetPrimaryUserProfile(); On 2017/04/04 18:44:12, Benoit L wrote: > The documentation for this function says: > > // This function is temporary and will soon be moved to ash. As such avoid > // using it at all cost. > > > What about > static Profile* GetLastUsedProfile(); > > ? > > Also, can this ever return nullptr? > I would tend to say given the current way custom tabs is set up, but it would > probably be best to add a null check here. Switched to using GetLastUsedProfile->GetOriginalProfile to avoid getting the incognito profile. It can't return null, since we check for g_browser_process on Init below, we should be fine. Added a dcheck to be sure. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:22: asset_link_handler_.reset(new digital_asset_links::DigitalAssetLinksHandler( On 2017/04/04 18:44:11, Benoit L wrote: > tiny nit: > > What about: > > asset_link_handler_ = > base::MakeUnique<digital_asset_links::DigitalAssetLinksHandler>(profile->GetRequestContext()); Done. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:43: asset_link_handler_.get()->CheckDigitalAssetLinkRelationship( On 2017/04/04 18:44:11, Benoit L wrote: > nit: asset_link_handler->Check... Done. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:45: &callback), On 2017/04/04 18:44:11, Benoit L wrote: > This doesn't work, as this is taking the address of a local variable. not using unique ptr anymore. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.h (right): https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:22: explicit OriginVerifier(JNIEnv* env, jobject obj); On 2017/04/04 18:44:12, Benoit L wrote: > nit: explicit is only needed for single-parameter constructors. Done. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc (right): https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:72: std::unique_ptr<base::Value> value = base::JSONReader::Read(response_body); On 2017/04/04 18:44:12, Benoit L wrote: > DictionaryValue::From() gives a unique_ptr<> directly, from the documentation of > GetAsDictionary. > It also does the nullptr check for you: > > std::unique_ptr<DictionarValue> dict = DictionaryValue::From(value); > > if (dict) { > ... > } > > Actually, you don't even need the if condition in this case, since if dict is > nullptr, you call callback with nullptr as a parameter. Done. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:78: callback_->Run(nullptr); On 2017/04/04 18:44:12, Benoit L wrote: > Should we delete the URLFetcher here? Resetted. Does that work? :) https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:92: callback_ = std::move(listener); On 2017/04/04 18:44:12, Benoit L wrote: > nit: as suggested above, passing the callback by value would simplify the code. Done. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:94: url_fetcher_ = std::move( On 2017/04/04 18:44:12, Benoit L wrote: > dumb question: is it OK to delete a URLFetcher while the request is in flight? > > That may happen if we get two requests back to back. I think so yes. At least I have seen examples of the same pattern. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.h (right): https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:45: // The callback for receiving a URLFether result. This handler owns this. On 2017/04/04 18:44:12, Benoit L wrote: > nit: ownership is implied by the unique_ptr. Done. https://codereview.chromium.org/2767333006/diff/100001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:46: std::unique_ptr<RelationshipCheckResultCallback> callback_; On 2017/04/04 18:44:12, Benoit L wrote: > And actually, since this is a callback, why not taking it by value, so that you > don't have to care about the unique_ptr? Done.
Looked at the Java part as well. That's quite unfortunate that we have to deal with certificate... but code looks fine. A few comments. https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java (right): https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:357: Uri getPostMessageOriginForSessionForTesting(CustomTabsSessionToken session) { Just for consistency, should this be synchronized as well? https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java (right): https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:51: static void prePopulateVerifiedOriginForTesting(String packageName, Uri origin) { nit: @VisibleForTesting? https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:71: return sCachedOriginMap.get(mPackageName); nit: Map#get() returns null if the key is not in the map, so you don't need the if condition above. https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:122: Log.w(TAG, "Given package name was not found in installed packages"); Can this actually happen? Since the package name comes from the system, we should not have a non-existent package. If it's the case, then maybe remove the logging call? https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:132: Log.w(TAG, "Certificate type X509 is not available"); nit: I think that this exception (and the one below for X509) cannot happen in practice, so the value of logging a message decreases. Maybe just return null? Or even something like a larger try block with: try { // certificate and encoding stuff } catch (CertificateException | NoSuchAlgorithmException e) { return null; } catch (CertificateEncodingException e) { Log.w(TAG, "Certificate type X509 encoding failed"); } https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:144: return hexString; This function can return null or "" in case of error. Can it return null in all cases? https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:147: private static String byteArrayToHexString(byte[] byteArray) { I found static String toHexString(byte[] bytes) in MediaDrmSessionManager.java that seems simpler (although it doesn't do the padding you want). Can the same approach be used here?
https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java (right): https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:357: Uri getPostMessageOriginForSessionForTesting(CustomTabsSessionToken session) { On 2017/04/05 16:32:03, Benoit L wrote: > Just for consistency, should this be synchronized as well? Done. https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java (right): https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:51: static void prePopulateVerifiedOriginForTesting(String packageName, Uri origin) { On 2017/04/05 16:32:03, Benoit L wrote: > nit: @VisibleForTesting? Done. https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:71: return sCachedOriginMap.get(mPackageName); On 2017/04/05 16:32:03, Benoit L wrote: > nit: Map#get() returns null if the key is not in the map, so you don't need the > if condition above. Done. https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:122: Log.w(TAG, "Given package name was not found in installed packages"); On 2017/04/05 16:32:03, Benoit L wrote: > Can this actually happen? > Since the package name comes from the system, we should not have a non-existent > package. If it's the case, then maybe remove the logging call? Done. https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:132: Log.w(TAG, "Certificate type X509 is not available"); On 2017/04/05 16:32:03, Benoit L wrote: > nit: I think that this exception (and the one below for X509) cannot happen in > practice, so the value of logging a message decreases. Maybe just return null? > > Or even something like a larger try block with: > > try { > // certificate and encoding stuff > } catch (CertificateException | NoSuchAlgorithmException e) { > return null; > } catch (CertificateEncodingException e) { > Log.w(TAG, "Certificate type X509 encoding failed"); > } Gone with a slight variation since CertificateEncodingException is a CertificateException subclass https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:144: return hexString; On 2017/04/05 16:32:03, Benoit L wrote: > This function can return null or "" in case of error. > Can it return null in all cases? Done. https://codereview.chromium.org/2767333006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:147: private static String byteArrayToHexString(byte[] byteArray) { On 2017/04/05 16:32:03, Benoit L wrote: > I found > static String toHexString(byte[] bytes) > > in MediaDrmSessionManager.java that seems simpler (although it doesn't do the > padding you want). > Can the same approach be used here? Done.
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
lgtm, with a few nits. Is there a way to test the native side? https://codereview.chromium.org/2767333006/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java (right): https://codereview.chromium.org/2767333006/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:33: * Uses Digital Asset Links to confirm that the given origin is associated with the package name as nit: Add a comment about caching? https://codereview.chromium.org/2767333006/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:135: if (e instanceof CertificateEncodingException) { You can put multiple catch blocks, provided that the first one is for the derived exception, not the base one. Seems cleaner than using instanceof. https://codereview.chromium.org/2767333006/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:144: StringBuilder hexString = new StringBuilder(byteArray.length * 2); This length computation doesn't account for the delimiter. Should it be byteArray.length * 3 - 1 ? https://codereview.chromium.org/2767333006/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/ClientManagerTest.java (right): https://codereview.chromium.org/2767333006/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/ClientManagerTest.java:170: // If there is a prepopulated origin, we should get a syncronous verification. nit: Synchronous. https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.cc (right): https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:43: digital_asset_links ::RelationshipCheckResultCallback callback( nit: extra " " https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:47: callback, origin, package_name, fingerprint, nit: Can you inline the base::Bind() call here? That's one less copy. https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.h (right): https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:40: // URL request context. nit: Remove this comment? (seems redundant with the code). https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:45: // The callback for receiving a URLFether result. nit: URLFetcher.
https://codereview.chromium.org/2767333006/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java (right): https://codereview.chromium.org/2767333006/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:33: * Uses Digital Asset Links to confirm that the given origin is associated with the package name as On 2017/04/07 15:04:54, Benoit L wrote: > nit: Add a comment about caching? Done. https://codereview.chromium.org/2767333006/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:135: if (e instanceof CertificateEncodingException) { On 2017/04/07 15:04:54, Benoit L wrote: > You can put multiple catch blocks, provided that the first one is for the > derived exception, not the base one. > Seems cleaner than using instanceof. Done. https://codereview.chromium.org/2767333006/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:144: StringBuilder hexString = new StringBuilder(byteArray.length * 2); On 2017/04/07 15:04:54, Benoit L wrote: > This length computation doesn't account for the delimiter. > Should it be > byteArray.length * 3 - 1 > ? Done. https://codereview.chromium.org/2767333006/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/ClientManagerTest.java (right): https://codereview.chromium.org/2767333006/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/ClientManagerTest.java:170: // If there is a prepopulated origin, we should get a syncronous verification. On 2017/04/07 15:04:54, Benoit L wrote: > nit: Synchronous. Done. https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.cc (right): https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:43: digital_asset_links ::RelationshipCheckResultCallback callback( On 2017/04/07 15:04:54, Benoit L wrote: > nit: extra " " Done. https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:47: callback, origin, package_name, fingerprint, On 2017/04/07 15:04:54, Benoit L wrote: > nit: Can you inline the base::Bind() call here? > That's one less copy. Done. https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.h (right): https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:40: // URL request context. On 2017/04/07 15:04:55, Benoit L wrote: > nit: Remove this comment? (seems redundant with the code). Done. https://codereview.chromium.org/2767333006/diff/180001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:45: // The callback for receiving a URLFether result. On 2017/04/07 15:04:54, Benoit L wrote: > nit: URLFetcher. Done.
yusufo@chromium.org changed reviewers: + nyquist@chromium.org
Added native unit tests lizeb@ feel free to take another look nyquist@ for chrome/browser/android OWNERS
Thanks for adding the tests! still lgtm. https://codereview.chromium.org/2767333006/diff/220001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc (right): https://codereview.chromium.org/2767333006/diff/220001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:40: CHECK(fetcher); nit: ASSERT_TRUE()? https://codereview.chromium.org/2767333006/diff/220001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:45: "{\n" nit: What about using the new hotness, raw string literals? R"({ "linked": true, "maxAge": .... "})" See https://chromium-cpp.appspot.com/ Looks nicer, here and below. https://codereview.chromium.org/2767333006/diff/220001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:72: int num_invocations_; Should this be reset in SetUp()? https://codereview.chromium.org/2767333006/diff/220001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:102: EXPECT_FALSE(!response_); nit: Why not EXPECT_TRUE?
https://codereview.chromium.org/2767333006/diff/220001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc (right): https://codereview.chromium.org/2767333006/diff/220001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:40: CHECK(fetcher); On 2017/04/18 16:14:31, Benoit L wrote: > nit: ASSERT_TRUE()? Done. https://codereview.chromium.org/2767333006/diff/220001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:45: "{\n" On 2017/04/18 16:14:31, Benoit L wrote: > nit: What about using the new hotness, raw string literals? > > R"({ > "linked": true, > "maxAge": .... > > "})" > > > See https://chromium-cpp.appspot.com/ > > Looks nicer, here and below. Done. And thanks for letting me learn about them! Had no clue! https://codereview.chromium.org/2767333006/diff/220001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:72: int num_invocations_; On 2017/04/18 16:14:31, Benoit L wrote: > Should this be reset in SetUp()? Done. https://codereview.chromium.org/2767333006/diff/220001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:102: EXPECT_FALSE(!response_); On 2017/04/18 16:14:31, Benoit L wrote: > nit: Why not EXPECT_TRUE? Done.
yusufo@chromium.org changed reviewers: + gab@chromium.org, rdsmith@chromium.org
Adding OWNERS from base/ and net/ because of the DEPS change I am making. For context, this folder I am adding (digital_asset_links) is pretty generic code (Calling REST APIs on a google web endpoint to check relationships between apps and web pages), but will be used for android only for now. After chatting with jam@, we decided to put this in chrome/browser/android for now and not make it a component, but limit DEPS so that dependencies don't get out of end. I need OWNERS approvals for deps I am adding. So: gab@ for base/ rdsmith@ for net/ sky@ for content/public/test
yusufo@chromium.org changed reviewers: + sky@chromium.org
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Mostly just a few questions since I'm not super familiar with this area yet. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:346: public synchronized void verifyAndInitializeWithPostMessageOriginForSession( OK, I see this is used other places in this class as well, but this scares me a little bit. By putting this on the public API of the class, you loose control of when the lock is held and from which thread, etc. Other classes with a reference to an instance of this object can now do: synchronized(mClientManager) { ... } and take the same lock as this one. Very often, you rather want to have something like: ### private final Object mLock = new Object(); public void something() { synchronized(mLock) { // do stuff } } ### Anyway, that sounds like something to either address or not in a follow-up, just figured I'd mention it since I was looking :-) https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:350: params.postMessageHandler.verifyAndInitializeWithOrigin(params.packageName, origin); the |postMessageHandler| member of SessionParams seems to for example come from #newSession(...), which is a public method, but we seem to never check whether the PostMessageHandler reference is null. Is that because this class is package protected, and we're guaranteed that the caller will never pass in null? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:38: class OriginVerifier { What is the expected lifetime of this object? It deals with native, so when should it be cleaned up by the owner? It looks like after it's created, it can never be deleted, given that start(...) is the only public method? I guess in some cases it might be deleted by native after a successful attempt, but since that's not guaranteed, this feels a little bit iffy. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:42: private long mNativeOriginVerifier = 0; Nit: Could you move this to after the final fields, to ensure it's easy to figure out what can change and not over the lifetime of an instance of OriginVerifier? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:71: private Uri getCachedOriginIfExists() { Nit: This is non-static. Any particular reason that this is all the way up here before the constructor and public interfaces? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:77: void onOriginVerified(String packageName, Uri origin, boolean result); When is this called? Is this guaranteed to be posted, or can this be invoked before the call to #start(...) finishes? From the sound of the description of this CL we really want this result to always be posted back. Also, it seems like there are cases where this is not called at all, which might be confusing for the caller (params passed to native are bad for example) Also, could you add JavaDoc to the params? Particularly the last one has an unhelpful name. Or, if you don't want that, maybe just rename the variable to |verified| or |success|? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:93: * Verify the claimed origin for the cached package name asynchronously. Nit: Could you mention that this will end up doing a network request? And also mention with context that is used for the URLFetcher? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:94: * @param origin The postMessage origin the application is claiming to have. Can't be null Nit: Missing period at the end. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:96: public void start(@NonNull Uri origin) { Should this only be called from the main thread? It's changing internal state that's later read in a callback from native (|mOrigin| for example). Or is the plan here to protect this only by the fact that the class is package protected? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:102: originVerified(true); This looks like it's re-entrant. Should this be posted to the caller handler instead to ensure that the API is consistent? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:108: if (mNativeOriginVerifier == 0) return; This ends up not invoking the callback / listener at all. Is that intentional? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:118: private String getCertificateSHA256FingerprintForPackage(String packageName) { Nit: What does this method return? I.e. what's the type of content? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:119: PackageManager pm = ContextUtils.getApplicationContext().getPackageManager(); Nit: Could you extract the package stuff into a private static PackageInfo getPackageInfo(String packageName) {...} ? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:126: InputStream input = new ByteArrayInputStream(packageInfo.signatures[0].toByteArray()); Could you extract out a method that takes packageInfo.signatures[0] (or the byte[] if you want) as an argument to make it generic and easy to review for security peeps? Maybe even make it package protected so you can easily test that part? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:131: (X509Certificate) CertificateFactory.getInstance("X509").generateCertificate( Do you think it would be helpful for someone from chrome security to take a quick peek at this part? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:143: private static String byteArrayToHexString(byte[] byteArray) { What do you think about making this package protected and adding a test for it? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:135: * it if the result is a success. Can be called multiple times. If so, the previous requests If I call this with: verifyAndInitializeWithOrigin("com.chrome.canary", Uri.parse("https://chrome.com/")); verifyAndInitializeWithOrigin("com.android.chrome", Uri.parse("https://google.com/")); It looks like the origin verifier will still use the initial package name. Is that intentional? And if it is, could you please document that as it's not really intuitive. If the package name is supposed to never change for a given PostMessageHandler, maybe put it as a constructor parameter instead? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.cc (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:9: #include "base/strings/string_number_conversions.h" Nit: Which of the conversions is this for again? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:22: Profile* profile = ProfileManager::GetLastUsedProfile()->GetOriginalProfile(); 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. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:29: OriginVerifier::~OriginVerifier() {} Nit: = default? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:38: return; 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? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:45: base::Unretained(this)), 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! :-) https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:72: return 0; Nit: Empty line after early return? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.h (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:8: #include "base/android/jni_weak_ref.h" Nit: Did you mean base/android/scoped_java_ref.h ? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:9: #include "chrome/browser/android/digital_asset_links/digital_asset_links_handler.h" Nit: Could digital_asset_links::DigitalAssetLinksHandler be forward declared instead here in the header, and just include the header in the .cc file? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:20: class OriginVerifier { Nit: Could you add a class-level comment to refer to to the Java class? Basically just say that this is the JNI bridge? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:30: void Destroy(JNIEnv* env, const base::android::JavaRef<jobject>& obj); Nit: Empty line before this? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:32: void OnRelationshipCheckComplete( Nit: Should this public method be documented to clarify who should call it and what it does? Also, does it need to be public? (I think it's for the callback?) https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. Nit: Remove (c) for all these files https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:9: #include "base/strings/string_number_conversions.h" Nit: Which conversion is this needed for again? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:56: DigitalAssetLinksHandler::~DigitalAssetLinksHandler() {} Nit: = default? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:73: base::DictionaryValue::From(base::JSONReader::Read(response_body))); You're running in the main thread of the browser process and parsing external JSON data. Should you instead use //components/safe_json/safe_json_parser.h ? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:79: RelationshipCheckResultCallback listener, Nit: Should this be called |callback|? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:84: GURL request_url = GetUrlForCheckingRelationship(web_domain, package_name, Nit: Should we verify that this happens on a particular thread? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:89: callback_ = listener; What if I call this method multiple times, particularly while the request is outstanding? Wouldn't that lead to the wrong callback being invoked in OnURLFetchComplete()? Or is the plan that since the url_fetcher_ on the next line resets the url_fetcher_ the OnURLFetchComplete will never happen since they both happen on the same thread? If so, could you clarify why this is safe with a comment? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:92: net::URLFetcher::Create(0, request_url, net::URLFetcher::GET, this); Should this have a traffic annotation attached to it? It would probably be helpful for figuring out where the traffic is from when doing attribution of power and network ,etc. See https://codereview.chromium.org/2703283004 as an example. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.h (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:20: typedef base::Callback<void(std::unique_ptr<base::DictionaryValue>)> Nit: On a failure what will the parameter to this callback be? (nullptr) What about on success? What are the expected fields in the dictionary? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:23: class DigitalAssetLinksHandler : public net::URLFetcherDelegate { Nit: What is this class used for? What is the lifetime for this class? On which thread should it be constructed and destroyed on? Also, is there some spec you can refer to, either here or in the implementation? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:25: DigitalAssetLinksHandler( Nit: Single argument constructors should be marked as explicit. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:29: bool CheckDigitalAssetLinkRelationship( Nit: Could you explain what this method does? What does this return? From which thread can it be called? What are the guarantees for invoking the callback? Will it be re-entrant, or will the result always be posted? What are the different params? Like what's |relationship|? What type of value is |fingerprint|? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:45: RelationshipCheckResultCallback callback_; So, this is a little bit confusing. It's passed in to the constructor, and stored for re-use, but it's still called a Callback. I think it'd be super helpful to clarify how this class is supposed to be used, i.e. is it a one-off or something that you can continuously call? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:5: #include "chrome/browser/android/digital_asset_links/digital_asset_links_handler.h" Nit: Empty line after this? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:21: class DigitalAssetLinksHandlerTest : public ::testing::Test { Nit: Should this class be in an anonymous namespace? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:28: ~DigitalAssetLinksHandlerTest() override {} Nit: = default? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:69: base::MessageLoop message_loop_; Nit: Could some of these be private? https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:75: }; Nit: private: DISALLOW_COPY_AND_ASSIGN(DigitalAssetLinksHandlerTest); https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:93: TEST_F(DigitalAssetLinksHandlerTest, InvalidResponse) { Nit: Is the response invalid, or is it just that the result was that there was no link verified?
chrome/browser/android/digital_asset_links/DEPS LGTM.
lgtm for chrome/browser/android/digital_asset_links/DEPS -> +base
https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:346: public synchronized void verifyAndInitializeWithPostMessageOriginForSession( On 2017/04/19 06:35:55, nyquist wrote: > OK, I see this is used other places in this class as well, but this scares me a > little bit. By putting this on the public API of the class, you loose control of > when the lock is held and from which thread, etc. Other classes with a reference > to an instance of this object can now do: > synchronized(mClientManager) { ... } > and take the same lock as this one. > > Very often, you rather want to have something like: > > ### > private final Object mLock = new Object(); > > public void something() { > synchronized(mLock) { > // do stuff > } > } > ### > > Anyway, that sounds like something to either address or not in a follow-up, just > figured I'd mention it since I was looking :-) This is more of a lizeb@ comment, because I know he likes the current structure. But acked, I will mention this to him. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/ClientManager.java:350: params.postMessageHandler.verifyAndInitializeWithOrigin(params.packageName, origin); On 2017/04/19 06:35:55, nyquist wrote: > the |postMessageHandler| member of SessionParams seems to for example come from > #newSession(...), which is a public method, but we seem to never check whether > the PostMessageHandler reference is null. > Is that because this class is package protected, and we're guaranteed that the > caller will never pass in null? Yes, the assumption is that is never null. Would rather resolve this by adding @NonNull to newSession. (which is the case for the current only caller.) https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:38: class OriginVerifier { On 2017/04/19 06:35:55, nyquist wrote: > What is the expected lifetime of this object? It deals with native, so when > should it be cleaned up by the owner? It looks like after it's created, it can > never be deleted, given that start(...) is the only public method? I guess in > some cases it might be deleted by native after a successful attempt, but since > that's not guaranteed, this feels a little bit iffy. Made cleanup package protected and started calling it during session cleanup. Added comments to reflect the current ownership. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:42: private long mNativeOriginVerifier = 0; On 2017/04/19 06:35:55, nyquist wrote: > Nit: Could you move this to after the final fields, to ensure it's easy to > figure out what can change and not over the lifetime of an instance of > OriginVerifier? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:71: private Uri getCachedOriginIfExists() { On 2017/04/19 06:35:55, nyquist wrote: > Nit: This is non-static. Any particular reason that this is all the way up here > before the constructor and public interfaces? Nope. this was static before I added the mPackageName dependency. Moved it down. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:77: void onOriginVerified(String packageName, Uri origin, boolean result); On 2017/04/19 06:35:55, nyquist wrote: > When is this called? Is this guaranteed to be posted, or can this be invoked > before the call to #start(...) finishes? From the sound of the description of > this CL we really want this result to always be posted back. > > Also, it seems like there are cases where this is not called at all, which might > be confusing for the caller (params passed to native are bad for example) > > Also, could you add JavaDoc to the params? Particularly the last one has an > unhelpful name. Or, if you don't want that, maybe just rename the variable to > |verified| or |success|? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:93: * Verify the claimed origin for the cached package name asynchronously. On 2017/04/19 06:35:55, nyquist wrote: > Nit: Could you mention that this will end up doing a network request? And also > mention with context that is used for the URLFetcher? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:94: * @param origin The postMessage origin the application is claiming to have. Can't be null On 2017/04/19 06:35:56, nyquist wrote: > Nit: Missing period at the end. Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:96: public void start(@NonNull Uri origin) { On 2017/04/19 06:35:55, nyquist wrote: > Should this only be called from the main thread? It's changing internal state > that's later read in a callback from native (|mOrigin| for example). Or is the > plan here to protect this only by the fact that the class is package protected? Yes, it should be on main thread only (Actually I might be crashing on the current test because of that). Made changes to that effect in the caller as well. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:102: originVerified(true); On 2017/04/19 06:35:56, nyquist wrote: > This looks like it's re-entrant. Should this be posted to the caller handler > instead to ensure that the API is consistent? I posted back on the UI thread. But the native side worries me. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:108: if (mNativeOriginVerifier == 0) return; On 2017/04/19 06:35:55, nyquist wrote: > This ends up not invoking the callback / listener at all. Is that intentional? Yes, this is for non-integration tests to still create OriginVerifier with no native component and test the logic for caching. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:118: private String getCertificateSHA256FingerprintForPackage(String packageName) { On 2017/04/19 06:35:56, nyquist wrote: > Nit: What does this method return? I.e. what's the type of content? The certificate for the apk in hex format: Something like : 0F:D9:A0:CF:B0:7B:65:95:09..... Were you saying we should document this? I would say the caller to OriginVerifier shouldn't care much about these internals. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:119: PackageManager pm = ContextUtils.getApplicationContext().getPackageManager(); On 2017/04/19 06:35:55, nyquist wrote: > Nit: Could you extract the package stuff into a private static PackageInfo > getPackageInfo(String packageName) {...} ? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:126: InputStream input = new ByteArrayInputStream(packageInfo.signatures[0].toByteArray()); On 2017/04/19 06:35:55, nyquist wrote: > Could you extract out a method that takes packageInfo.signatures[0] (or the > byte[] if you want) as an argument to make it generic and easy to review for > security peeps? Maybe even make it package protected so you can easily test that > part? See below, the one below already takes byte[]. Making that package protected. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:131: (X509Certificate) CertificateFactory.getInstance("X509").generateCertificate( On 2017/04/19 06:35:56, nyquist wrote: > Do you think it would be helpful for someone from chrome security to take a > quick peek at this part? I can do that after you are comfortable for the state of the CL, but didn't understand the security concerns here. We are calling PackageManager APIs for other package names, so this is stuff pretty much all apps have access to. The signature is not the developer signature, it is the APK signature which is unique but public. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:143: private static String byteArrayToHexString(byte[] byteArray) { On 2017/04/19 06:35:55, nyquist wrote: > What do you think about making this package protected and adding a test for it? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:135: * it if the result is a success. Can be called multiple times. If so, the previous requests On 2017/04/19 06:35:56, nyquist wrote: > If I call this with: > verifyAndInitializeWithOrigin("com.chrome.canary", > Uri.parse("https://chrome.com/")); > verifyAndInitializeWithOrigin("com.android.chrome", > Uri.parse("https://google.com/")); > It looks like the origin verifier will still use the initial package name. Is > that intentional? And if it is, could you please document that as it's not > really intuitive. If the package name is supposed to never change for a given > PostMessageHandler, maybe put it as a constructor parameter instead? For technical reasons, we can't get access to the package name at the time this is constructed. But it should be unique which means we should probably set it only once. Moved it to a separate setter and started calling it as close as possible to construction https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.cc (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:9: #include "base/strings/string_number_conversions.h" On 2017/04/19 06:35:56, nyquist wrote: > Nit: Which of the conversions is this for again? Removed. I dont think we need this anymore. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:22: Profile* profile = ProfileManager::GetLastUsedProfile()->GetOriginalProfile(); On 2017/04/19 06:35:56, nyquist wrote: > 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. Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:29: OriginVerifier::~OriginVerifier() {} On 2017/04/19 06:35:56, nyquist wrote: > Nit: = default? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:38: return; On 2017/04/19 06:35:56, nyquist wrote: > 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? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:45: base::Unretained(this)), On 2017/04/19 06:35:56, nyquist wrote: > 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! :-) The latter and added a comment here as well. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:72: return 0; On 2017/04/19 06:35:56, nyquist wrote: > Nit: Empty line after early return? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.h (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:8: #include "base/android/jni_weak_ref.h" On 2017/04/19 06:35:56, nyquist wrote: > Nit: Did you mean base/android/scoped_java_ref.h ? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:9: #include "chrome/browser/android/digital_asset_links/digital_asset_links_handler.h" On 2017/04/19 06:35:56, nyquist wrote: > Nit: Could digital_asset_links::DigitalAssetLinksHandler be forward declared > instead here in the header, and just include the header in the .cc file? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:20: class OriginVerifier { On 2017/04/19 06:35:56, nyquist wrote: > Nit: Could you add a class-level comment to refer to to the Java class? > Basically just say that this is the JNI bridge? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:30: void Destroy(JNIEnv* env, const base::android::JavaRef<jobject>& obj); On 2017/04/19 06:35:56, nyquist wrote: > Nit: Empty line before this? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.h:32: void OnRelationshipCheckComplete( On 2017/04/19 06:35:56, nyquist wrote: > Nit: Should this public method be documented to clarify who should call it and > what it does? Also, does it need to be public? (I think it's for the callback?) Moved to private https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/04/19 06:35:56, nyquist wrote: > Nit: Remove (c) for all these files Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:9: #include "base/strings/string_number_conversions.h" On 2017/04/19 06:35:57, nyquist wrote: > Nit: Which conversion is this needed for again? You know... That one.. Removed. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:56: DigitalAssetLinksHandler::~DigitalAssetLinksHandler() {} On 2017/04/19 06:35:57, nyquist wrote: > Nit: = default? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:73: base::DictionaryValue::From(base::JSONReader::Read(response_body))); On 2017/04/19 06:35:57, nyquist wrote: > You're running in the main thread of the browser process and parsing external > JSON data. Should you instead use //components/safe_json/safe_json_parser.h ? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:79: RelationshipCheckResultCallback listener, On 2017/04/19 06:35:57, nyquist wrote: > Nit: Should this be called |callback|? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:84: GURL request_url = GetUrlForCheckingRelationship(web_domain, package_name, On 2017/04/19 06:35:56, nyquist wrote: > Nit: Should we verify that this happens on a particular thread? Added a DCHECK to OriginVerifier side of the caller. Cant check it here because we dont know what BrowserThread is here. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:89: callback_ = listener; On 2017/04/19 06:35:57, nyquist wrote: > What if I call this method multiple times, particularly while the request is > outstanding? Wouldn't that lead to the wrong callback being invoked in > OnURLFetchComplete()? > Or is the plan that since the url_fetcher_ on the next line resets the > url_fetcher_ the OnURLFetchComplete will never happen since they both happen on > the same thread? If so, could you clarify why this is safe with a comment? The latter. And I added a comment to this effect. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:92: net::URLFetcher::Create(0, request_url, net::URLFetcher::GET, this); On 2017/04/19 06:35:56, nyquist wrote: > Should this have a traffic annotation attached to it? It would probably be > helpful for figuring out where the traffic is from when doing attribution of > power and network ,etc. See https://codereview.chromium.org/2703283004 as an > example. Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.h (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:20: typedef base::Callback<void(std::unique_ptr<base::DictionaryValue>)> On 2017/04/19 06:35:57, nyquist wrote: > Nit: On a failure what will the parameter to this callback be? (nullptr) > What about on success? > What are the expected fields in the dictionary? Documented the failure case. On success depends on what API they used. Check and List returns different things. Added a top level reference to APIs. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:23: class DigitalAssetLinksHandler : public net::URLFetcherDelegate { On 2017/04/19 06:35:57, nyquist wrote: > Nit: What is this class used for? What is the lifetime for this class? On which > thread should it be constructed and destroyed on? > > Also, is there some spec you can refer to, either here or in the implementation? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:25: DigitalAssetLinksHandler( On 2017/04/19 06:35:57, nyquist wrote: > Nit: Single argument constructors should be marked as explicit. Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:29: bool CheckDigitalAssetLinkRelationship( On 2017/04/19 06:35:57, nyquist wrote: > Nit: Could you explain what this method does? > What does this return? From which thread can it be called? > What are the guarantees for invoking the callback? Will it be re-entrant, or > will the result always be posted? > > What are the different params? Like what's |relationship|? > What type of value is |fingerprint|? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:45: RelationshipCheckResultCallback callback_; On 2017/04/19 06:35:57, nyquist wrote: > So, this is a little bit confusing. It's passed in to the constructor, and > stored for re-use, but it's still called a Callback. > I think it'd be super helpful to clarify how this class is supposed to be used, > i.e. is it a one-off or something that you can continuously call? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:5: #include "chrome/browser/android/digital_asset_links/digital_asset_links_handler.h" On 2017/04/19 06:35:57, nyquist wrote: > Nit: Empty line after this? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:21: class DigitalAssetLinksHandlerTest : public ::testing::Test { On 2017/04/19 06:35:57, nyquist wrote: > Nit: Should this class be in an anonymous namespace? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:28: ~DigitalAssetLinksHandlerTest() override {} On 2017/04/19 06:35:57, nyquist wrote: > Nit: = default? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:69: base::MessageLoop message_loop_; On 2017/04/19 06:35:57, nyquist wrote: > Nit: Could some of these be private? Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:75: }; On 2017/04/19 06:35:57, nyquist wrote: > Nit: > private: > DISALLOW_COPY_AND_ASSIGN(DigitalAssetLinksHandlerTest); Done. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:93: TEST_F(DigitalAssetLinksHandlerTest, InvalidResponse) { On 2017/04/19 06:35:57, nyquist wrote: > Nit: Is the response invalid, or is it just that the result was that there was > no link verified? Renamed to Positive/NegativeResponse.
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
This should be good to go now. Thanks a lot for the review nyquist@
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm % fixing OriginVerifier::VerifyOrigin callback when stuff fails before URLRequest starts. I think the fix should be quick if you return false from native and do it in Java though. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:96: public void start(@NonNull Uri origin) { On 2017/04/26 00:51:35, Yusuf wrote: > On 2017/04/19 06:35:55, nyquist wrote: > > Should this only be called from the main thread? It's changing internal state > > that's later read in a callback from native (|mOrigin| for example). Or is the > > plan here to protect this only by the fact that the class is package > protected? > > Yes, it should be on main thread only (Actually I might be crashing on the > current test because of that). > > Made changes to that effect in the caller as well. Acknowledged. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:102: originVerified(true); On 2017/04/26 00:51:35, Yusuf wrote: > On 2017/04/19 06:35:56, nyquist wrote: > > This looks like it's re-entrant. Should this be posted to the caller handler > > instead to ensure that the API is consistent? > > I posted back on the UI thread. But the native side worries me. I think you've fixed the native side too? https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:108: if (mNativeOriginVerifier == 0) return; On 2017/04/26 00:51:35, Yusuf wrote: > On 2017/04/19 06:35:55, nyquist wrote: > > This ends up not invoking the callback / listener at all. Is that intentional? > > Yes, this is for non-integration tests to still create OriginVerifier with no > native component and test the logic for caching. Acknowledged. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:118: private String getCertificateSHA256FingerprintForPackage(String packageName) { On 2017/04/26 00:51:35, Yusuf wrote: > On 2017/04/19 06:35:56, nyquist wrote: > > Nit: What does this method return? I.e. what's the type of content? > > The certificate for the apk in hex format: > > Something like : 0F:D9:A0:CF:B0:7B:65:95:09..... > > Were you saying we should document this? I would say the caller to > OriginVerifier shouldn't care much about these internals. What you did is fine. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:126: InputStream input = new ByteArrayInputStream(packageInfo.signatures[0].toByteArray()); On 2017/04/26 00:51:35, Yusuf wrote: > On 2017/04/19 06:35:55, nyquist wrote: > > Could you extract out a method that takes packageInfo.signatures[0] (or the > > byte[] if you want) as an argument to make it generic and easy to review for > > security peeps? Maybe even make it package protected so you can easily test > that > > part? > > See below, the one below already takes byte[]. Making that package protected. Acknowledged. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java:131: (X509Certificate) CertificateFactory.getInstance("X509").generateCertificate( On 2017/04/26 00:51:35, Yusuf wrote: > On 2017/04/19 06:35:56, nyquist wrote: > > Do you think it would be helpful for someone from chrome security to take a > > quick peek at this part? > > I can do that after you are comfortable for the state of the CL, but didn't > understand the security concerns here. We are calling PackageManager APIs for > other package names, so this is stuff pretty much all apps have access to. The > signature is not the developer signature, it is the APK signature which is > unique but public. Acknowledged. https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:135: * it if the result is a success. Can be called multiple times. If so, the previous requests On 2017/04/26 00:51:35, Yusuf wrote: > On 2017/04/19 06:35:56, nyquist wrote: > > If I call this with: > > verifyAndInitializeWithOrigin("com.chrome.canary", > > Uri.parse("https://chrome.com/")); > > verifyAndInitializeWithOrigin("com.android.chrome", > > Uri.parse("https://google.com/")); > > It looks like the origin verifier will still use the initial package name. Is > > that intentional? And if it is, could you please document that as it's not > > really intuitive. If the package name is supposed to never change for a given > > PostMessageHandler, maybe put it as a constructor parameter instead? > > For technical reasons, we can't get access to the package name at the time this > is constructed. But it should be unique which means we should probably set it > only once. Moved it to a separate setter and started calling it as close as > possible to construction Acknowledged. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.cc (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:45: base::Unretained(this)), On 2017/04/26 00:51:36, Yusuf wrote: > On 2017/04/19 06:35:56, nyquist wrote: > > 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! :-) > > The latter and added a comment here as well. Acknowledged. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:56: DigitalAssetLinksHandler::~DigitalAssetLinksHandler() {} On 2017/04/26 00:51:36, Yusuf wrote: > On 2017/04/19 06:35:57, nyquist wrote: > > Nit: = default? > > Done. Done. You keep using that word. I don't think it means what you think it means. ;-) https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:84: GURL request_url = GetUrlForCheckingRelationship(web_domain, package_name, On 2017/04/26 00:51:36, Yusuf wrote: > On 2017/04/19 06:35:56, nyquist wrote: > > Nit: Should we verify that this happens on a particular thread? > > Added a DCHECK to OriginVerifier side of the caller. Cant check it here because > we dont know what BrowserThread is here. Acknowledged. https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.cc:92: net::URLFetcher::Create(0, request_url, net::URLFetcher::GET, this); On 2017/04/26 00:51:36, Yusuf wrote: > On 2017/04/19 06:35:56, nyquist wrote: > > Should this have a traffic annotation attached to it? It would probably be > > helpful for figuring out where the traffic is from when doing attribution of > > power and network ,etc. See https://codereview.chromium.org/2703283004 as an > > example. > > Done. Awesome! Super duper helpful! https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.h (right): https://codereview.chromium.org/2767333006/diff/260001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:20: typedef base::Callback<void(std::unique_ptr<base::DictionaryValue>)> On 2017/04/26 00:51:36, Yusuf wrote: > On 2017/04/19 06:35:57, nyquist wrote: > > Nit: On a failure what will the parameter to this callback be? (nullptr) > > What about on success? > > What are the expected fields in the dictionary? > > Documented the failure case. On success depends on what API they used. Check and > List returns different things. Added a top level reference to APIs. Awesome with the referral to the REST API. Very helpful! https://codereview.chromium.org/2767333006/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java (right): https://codereview.chromium.org/2767333006/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:61: mPackageName = packageName; Nit: assert mPackageName == null ? https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.cc (right): https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:51: asset_link_handler_->CheckDigitalAssetLinkRelationship( DigitalAssetLinksHandler::CheckDigitalAssetLinkRelationship returns a bool return value. I think that probably should imply that you want to post a failing callback back to Java? Because when it returns null, the DigitalAssetLinksHandler doesn't post anything. I was just thinking something along the lines of: base::ThreadTaskRunnerHandle::Get()->PostTask( FROM_HERE, base::Bind(.... something like below ...)); One note though, is it safe to use base::Unretained(this) for that case? I mean in case this gets destroyed while that callback is pending. So I guess what I'm saying is that it's probably easier to return false from this nativeVerifyOrigin method, and then do the callback from Java instead, since you already do that for when it's cached (except of course with |false| this time :-p ). I think that's definitely the easiest. Or, if you're not going to check it, maybe make it void? https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.h (right): https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:36: // "web_domain" for the source Android app which is uniquely defined by the Nit: Use vertical bars instead of quotes, i.e. |web_domain| https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:37: // "package_name" and SHA256 "finger_print" (a string with 32 hexadecimals Nit: |fingerprint| https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:39: // here will result in a bad request and a null response to the callback. Nit: nullptr https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc (right): https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:79: DISALLOW_COPY_AND_ASSIGN(DigitalAssetLinksHandlerTest); Nit: I think we usually have an empty line before this one. https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:82: TEST_F(DigitalAssetLinksHandlerTest, PositiveResponse) { Nit: I think I'd end the anonymous namespace right before this method, but wrap the whole file (like before) in the digital_asset_links namespace. That way you don't have to prefix the class-under-test with the namespace.
https://codereview.chromium.org/2767333006/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java (right): https://codereview.chromium.org/2767333006/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/customtabs/PostMessageHandler.java:61: mPackageName = packageName; On 2017/04/27 04:38:25, nyquist (nychthemeron ping) wrote: > Nit: assert mPackageName == null ? added @Nonnull https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler.h (right): https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:36: // "web_domain" for the source Android app which is uniquely defined by the On 2017/04/27 04:38:25, nyquist (nychthemeron ping) wrote: > Nit: Use vertical bars instead of quotes, i.e. |web_domain| Done. https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:37: // "package_name" and SHA256 "finger_print" (a string with 32 hexadecimals On 2017/04/27 04:38:25, nyquist (nychthemeron ping) wrote: > Nit: |fingerprint| Done. https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler.h:39: // here will result in a bad request and a null response to the callback. On 2017/04/27 04:38:25, nyquist (nychthemeron ping) wrote: > Nit: nullptr Done. https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... File chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc (right): https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:79: DISALLOW_COPY_AND_ASSIGN(DigitalAssetLinksHandlerTest); On 2017/04/27 04:38:25, nyquist (nychthemeron ping) wrote: > Nit: I think we usually have an empty line before this one. Done. https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/digital_asset_links/digital_asset_links_handler_unittest.cc:82: TEST_F(DigitalAssetLinksHandlerTest, PositiveResponse) { On 2017/04/27 04:38:25, nyquist (nychthemeron ping) wrote: > Nit: I think I'd end the anonymous namespace right before this method, but wrap > the whole file (like before) in the digital_asset_links namespace. > That way you don't have to prefix the class-under-test with the namespace. Done.
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... File chrome/browser/android/customtabs/origin_verifier.cc (right): https://codereview.chromium.org/2767333006/diff/380001/chrome/browser/android... chrome/browser/android/customtabs/origin_verifier.cc:51: asset_link_handler_->CheckDigitalAssetLinkRelationship( On 2017/04/27 04:38:25, nyquist (nychthemeron ping) wrote: > DigitalAssetLinksHandler::CheckDigitalAssetLinkRelationship returns a bool > return value. I think that probably should imply that you want to post a failing > callback back to Java? Because when it returns null, the > DigitalAssetLinksHandler doesn't post anything. > > I was just thinking something along the lines of: > base::ThreadTaskRunnerHandle::Get()->PostTask( > FROM_HERE, base::Bind(.... something like below ...)); > > One note though, is it safe to use base::Unretained(this) for that case? I mean > in case this gets destroyed while that callback is pending. > > So I guess what I'm saying is that it's probably easier to return false from > this nativeVerifyOrigin method, and then do the callback from Java instead, > since you already do that for when it's cached (except of course with |false| > this time :-p ). I think that's definitely the easiest. > > Or, if you're not going to check it, maybe make it void? Done.
The CQ bit was checked by yusufo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org, sky@chromium.org, rdsmith@chromium.org, gab@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/2767333006/#ps420001 (title: "destructor")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by yusufo@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 420001, "attempt_start_ts": 1493322787173520,
"parent_rev": "69a1f75e22720b35e521bb3776cb3e0add382f74", "commit_rev":
"2f2c75134816ad67b3bab94cfff8347b53cbc7d3"}
Message was sent while issue was closed.
Description was changed from ========== Add Digital Asset Links verification for postMessage API Adds DigitalAssetLinks handler which queries digitalassetlinks.googleapis.com/ for verifying the relationship between an Android app and a web domain. This is added in a new component digital_asset_links since this API is a generic web API for checking Android-Web relationships. It may be useful for iOS and ChromeOS as well. The component currently depends on base and net. Then custom tabs uses this handler for verifying postMessage origin declared by the client app when they send requestPostMessageChannel. This enabled third party apps to use postMessage related APIs with secure and verified origin declaration. BUG=704975 ========== to ========== Add Digital Asset Links verification for postMessage API Adds DigitalAssetLinks handler which queries digitalassetlinks.googleapis.com/ for verifying the relationship between an Android app and a web domain. This is added in a new component digital_asset_links since this API is a generic web API for checking Android-Web relationships. It may be useful for iOS and ChromeOS as well. The component currently depends on base and net. Then custom tabs uses this handler for verifying postMessage origin declared by the client app when they send requestPostMessageChannel. This enabled third party apps to use postMessage related APIs with secure and verified origin declaration. BUG=704975 Review-Url: https://codereview.chromium.org/2767333006 Cr-Commit-Position: refs/heads/master@{#467791} Committed: https://chromium.googlesource.com/chromium/src/+/2f2c75134816ad67b3bab94cfff8... ==========
Message was sent while issue was closed.
Committed patchset #22 (id:420001) as https://chromium.googlesource.com/chromium/src/+/2f2c75134816ad67b3bab94cfff8... |
