Use new SafeBrowsing redirect tracking code in CWS pings.

chrome/browser/extensions/webstore_inline_installer_browsertest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/webstore_inline_installer.h"
 
 #include "base/json/json_reader.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 98 matching lines @@
     ProgrammableInstallPrompt::g_done_callback = nullptr;
 
 // Fake inline installer which creates a programmable prompt in place of
 // the normal dialog UI.
 class WebstoreInlineInstallerForTest : public WebstoreInlineInstaller {
  public:
   WebstoreInlineInstallerForTest(WebContents* contents,
                                  content::RenderFrameHost* host,
                                  const std::string& extension_id,
                                  const GURL& requestor_url,
-                                 const Callback& callback)
+                                 const Callback& callback,
+                                 bool enable_safebrowsing_redirects)
       : WebstoreInlineInstaller(
             contents,
             host,
             kTestExtensionId,
             requestor_url,
             base::Bind(&WebstoreInlineInstallerForTest::InstallCallback,
                        base::Unretained(this))),
         install_result_target_(nullptr),
         programmable_prompt_(nullptr) {}
 
   std::unique_ptr<ExtensionInstallPrompt> CreateInstallUI() override {
     programmable_prompt_ = new ProgrammableInstallPrompt(web_contents());
     return base::WrapUnique(programmable_prompt_);
   }
 
   // Added here to make it public so that test cases can call it below.
   bool CheckRequestorAlive() const override {
     return WebstoreInlineInstaller::CheckRequestorAlive();
   }
 
+  bool SafeBrowsingNavigationEventsEnabled() const override {
+    return enable_safebrowsing_redirects_;
+  }
+
   // Tests that care about the actual arguments to the install callback can use
   // this to receive a copy in |install_result_target|.
   void set_install_result_target(
       std::unique_ptr<InstallResult>* install_result_target) {
     install_result_target_ = install_result_target;
   }
 
  private:
   ~WebstoreInlineInstallerForTest() override {}
 
   friend class base::RefCountedThreadSafe<WebstoreStandaloneInstaller>;
 
   void InstallCallback(bool success,
                        const std::string& error,
                        webstore_install::Result result) {
     if (install_result_target_) {
       install_result_target_->reset(new InstallResult);
       (*install_result_target_)->success = success;
       (*install_result_target_)->error = error;
       (*install_result_target_)->result = result;
     }
   }
 
   // This can be set by tests that want to get the actual install callback
   // arguments.
   std::unique_ptr<InstallResult>* install_result_target_;
 
+  // This can be set by tests that want to use the new SafeBrowsing redirect
+  // tracker.
+  bool enable_safebrowsing_redirects_;
+
   ProgrammableInstallPrompt* programmable_prompt_;
 };
 
 class WebstoreInlineInstallerForTestFactory :
     public WebstoreInlineInstallerFactory {
  public:
-  WebstoreInlineInstallerForTestFactory() : last_installer_(nullptr) {}
+  WebstoreInlineInstallerForTestFactory()
+      : last_installer_(nullptr), enable_safebrowsing_redirects_(false) {}
+  explicit WebstoreInlineInstallerForTestFactory(
+      bool enable_safebrowsing_redirects)
+      : last_installer_(nullptr),
+        enable_safebrowsing_redirects_(enable_safebrowsing_redirects) {}
   ~WebstoreInlineInstallerForTestFactory() override {}
 
   WebstoreInlineInstallerForTest* last_installer() { return last_installer_; }
 
   WebstoreInlineInstaller* CreateInstaller(
       WebContents* contents,
       content::RenderFrameHost* host,
       const std::string& webstore_item_id,
       const GURL& requestor_url,
       const WebstoreStandaloneInstaller::Callback& callback) override {
     last_installer_ = new WebstoreInlineInstallerForTest(
-        contents, host, webstore_item_id, requestor_url, callback);
+        contents, host, webstore_item_id, requestor_url, callback,
+        enable_safebrowsing_redirects_);
     return last_installer_;
   }
 
  private:
   // The last installer that was created.
   WebstoreInlineInstallerForTest* last_installer_;
+
+  bool enable_safebrowsing_redirects_;
 };
 
 IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerTest,
                        CloseTabBeforeInstallConfirmation) {
   GURL install_url = GenerateTestServerUrl(kAppDomain, "install.html");
   ui_test_utils::NavigateToURL(browser(), install_url);
   WebContents* web_contents =
       browser()->tab_strip_model()->GetActiveWebContents();
   TabHelper* tab_helper = TabHelper::FromWebContents(web_contents);
   tab_helper->SetWebstoreInlineInstallerFactoryForTests(
@@ skipping 162 matching lines @@
 }
 
 // Test calling chrome.webstore.install() twice without waiting for the first to
 // finish.
 IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerTest, DoubleInlineInstallTest) {
   ui_test_utils::NavigateToURL(
       browser(), GenerateTestServerUrl(kAppDomain, "double_install.html"));
   RunTest("runTest");
 }
 
-class WebstoreInlineInstallerRedirectTest : public WebstoreInlineInstallerTest {
+class WebstoreInlineInstallerRedirectTest
+    : public WebstoreInlineInstallerTest,
+      public ::testing::WithParamInterface<bool> {
  public:
   WebstoreInlineInstallerRedirectTest() : cws_request_received_(false) {}
   ~WebstoreInlineInstallerRedirectTest() override {}
 
   void SetUpInProcessBrowserTestFixture() override {
     WebstoreInstallerTest::SetUpInProcessBrowserTestFixture();
     host_resolver()->AddRule(kRedirect1Domain, "127.0.0.1");
     host_resolver()->AddRule(kRedirect2Domain, "127.0.0.1");
   }
 
   void ProcessServerRequest(
       const net::test_server::HttpRequest& request) override {
     if (request.content.find("redirect_chain") != std::string::npos) {
       cws_request_received_ = true;
 
       std::unique_ptr<base::Value> contents =
           base::JSONReader::Read(request.content);
       ASSERT_EQ(base::Value::Type::DICTIONARY, contents->GetType());
       cws_request_json_data_ = base::DictionaryValue::From(std::move(contents));
     }
   }
 
   bool cws_request_received_;
   std::unique_ptr<base::DictionaryValue> cws_request_json_data_;
 };
 
 // Test that an install from a page arrived at via redirects includes the
 // redirect information in the webstore request.
-IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerRedirectTest,
+IN_PROC_BROWSER_TEST_P(WebstoreInlineInstallerRedirectTest,
                        IncludesRedirectData) {
+  const bool using_safe_browsing_tracker = GetParam();
+
+  // GURL pre_navigate_url = GenerateTestServerUrl(kAppDomain, "install.html");

[Devlin, 2017/04/13 15:03:07]

dead code?

[robertshield, 2017/04/21 00:40:41]

Oops, yes, thanks.

+  // ui_test_utils::NavigateToURL(browser(), pre_navigate_url);
+  WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  TabHelper* tab_helper = TabHelper::FromWebContents(web_contents);
+  WebstoreInlineInstallerForTestFactory* factory =
+      new WebstoreInlineInstallerForTestFactory(using_safe_browsing_tracker);
+  tab_helper->SetWebstoreInlineInstallerFactoryForTests(factory);
+
   // Hand craft a url that will cause the test server to issue redirects.
   const std::vector<std::string> redirects = {kRedirect1Domain,
                                               kRedirect2Domain};
   net::HostPortPair host_port = embedded_test_server()->host_port_pair();
   std::string redirect_chain;
   for (const auto& redirect : redirects) {
     std::string redirect_url = base::StringPrintf(
         "http://%s:%d/server-redirect?", redirect.c_str(), host_port.port());
     redirect_chain += redirect_url;
   }
   const GURL install_url =
       GURL(redirect_chain +
            GenerateTestServerUrl(kAppDomain, "install.html").spec());
 
   AutoAcceptInstall();
   ui_test_utils::NavigateToURL(browser(), install_url);
-  RunTest("runTest");
+
+  RunTestAsync("runTest");

[Devlin, 2017/04/13 15:03:07]

Do we need to make this an async run?

[robertshield, 2017/04/21 00:40:41]

It seems to hang if we don't. This became true when I added the
WebstoreInlineInstallerForTestFactory above. 

+  while (!ProgrammableInstallPrompt::Ready())
+    base::RunLoop().RunUntilIdle();
+  web_contents->Close();
 
   EXPECT_TRUE(cws_request_received_);
   ASSERT_NE(nullptr, cws_request_json_data_);
 
   base::ListValue* redirect_list = nullptr;
   cws_request_json_data_->GetList("redirect_chain", &redirect_list);
   ASSERT_NE(nullptr, redirect_list);
 
   // Check that the expected domains are in the redirect list.
-  const std::vector<std::string> expected_redirect_domains = {
+  const std::set<std::string> expected_redirect_domains = {
       kRedirect1Domain, kRedirect2Domain, kAppDomain};
-  ASSERT_EQ(expected_redirect_domains.size(), redirect_list->GetSize());
-  int i = 0;
+
+  // The SafeBrowsing tracker has a much more liberal definition of "redirect"
+  // and it may (based on timing) pick up additional navigations that occur
+  // shortly before the navigation we mainly care about here. Be somewhat
+  // permissive in what we accept as redirect results.
+  ASSERT_LE(expected_redirect_domains.size(), redirect_list->GetSize());
+
   for (const auto& value : *redirect_list) {
     std::string value_string;
     ASSERT_TRUE(value->GetAsString(&value_string));
     GURL redirect_url(value_string);
-    EXPECT_EQ(expected_redirect_domains[i++], redirect_url.host());
+    EXPECT_TRUE(expected_redirect_domains.find(redirect_url.host()) !=
+                expected_redirect_domains.end());
   }
 }
 
+INSTANTIATE_TEST_CASE_P(NetRedirectTracking,
+                        WebstoreInlineInstallerRedirectTest,
+                        testing::Values(false));

[Devlin, 2017/04/13 15:03:07]

Optional: If you don't care much about the naming, I think you can use a single
call to instantiate the tests with testing::Bool() instead of
testing::Values(true) and testing::Values(false).  Up to you.

[robertshield, 2017/04/21 00:40:41]

Ah, true.. I think on the balance having the two more explicit names here makes
it slightly easier to parse test log output. 

+INSTANTIATE_TEST_CASE_P(SafeBrowsingRedirectTracking,
+                        WebstoreInlineInstallerRedirectTest,
+                        testing::Values(true));
+
 class WebstoreInlineInstallerListenerTest : public WebstoreInlineInstallerTest {
  public:
   WebstoreInlineInstallerListenerTest() {}
   ~WebstoreInlineInstallerListenerTest() override {}
 
  protected:
   void RunTest(const std::string& file_name) {
     AutoAcceptInstall();
     ui_test_utils::NavigateToURL(browser(),
                                  GenerateTestServerUrl(kAppDomain, file_name));
@@ skipping 30 matching lines @@
       WindowOpenDisposition::NEW_FOREGROUND_TAB,
       ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
   DCHECK_NE(old_tab_index, browser()->tab_strip_model()->active_index());
   browser()->tab_strip_model()->CloseWebContentsAt(old_tab_index,
                                                    TabStripModel::CLOSE_NONE);
   WebstoreInstallerTest::RunTest("runTest");
   EXPECT_TRUE(registry->enabled_extensions().GetByID(kTestExtensionId));
 }
 
 }  // namespace extensions