Remove GetAssociatedWebContents from echoPrivate implementation

GetAssociatedWebContents has been used in echoPrivate.getUserConsent
to determine to which window to attach the user consent dialog.
The API function is used from a component echo extension's event page,
and was relying on GetAssociatedWebContents to select the active tab as
the target for the dialog (note that this was not always the right
tab/window to use).

To address this issue, the echoPrivate.getUserConsent has been updated
to accept tabId, which identifies the tab to use for the user constent
dialog, and the extension was updated to pass the target tabId in. It
should now be OK to stop depending on GetAssociatedWebContentsDeprecated
to select the target tab.

Calls from extension contexts will now require the tabId to be
specified. The only circumstance the tabId can be omitted is when the
function is called from a platform app window (in which the sender web
contents will point to the desired target for the user consent dialog).

BUG=461394

Change-Id: I49c9dd5a8ffd9543bb441bc694dbc929ac8c2782
Reviewed-on: https://chromium-review.googlesource.com/1054489
Commit-Queue: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558057}

chrome/browser/chromeos/extensions/echo_private_api.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/chromeos/extensions/echo_private_api.h"
 
+#include <memory>
 #include <string>
 #include <utility>
 
@@ -29,6 +30,7 @@
 #include "components/prefs/scoped_user_pref_update.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/extension_file_task_runner.h"
+#include "extensions/browser/view_type_utils.h"
 #include "extensions/common/extension.h"
 
 namespace echo_api = extensions::api::echo_private;
@@ -242,7 +244,16 @@ void EchoPrivateGetUserConsentFunction::OnRedeemOffersAllowedChecked(
   }
 
   content::WebContents* web_contents = nullptr;
-  if (params->consent_requester.tab_id) {
+  if (!params->consent_requester.tab_id) {
+    web_contents = GetSenderWebContents();
+
+    if (!web_contents || extensions::GetViewType(web_contents) !=
+                             extensions::VIEW_TYPE_APP_WINDOW) {
+      error_ = "Not called from an app window - the tabId is required.";
+      SendResponse(false);
+      return;
+    }
+  } else {
     TabStripModel* tab_strip = nullptr;
     int tab_index = -1;
     if (!extensions::ExtensionTabUtil::GetTabById(
@@ -262,17 +273,9 @@ void EchoPrivateGetUserConsentFunction::OnRedeemOffersAllowedChecked(
       SendResponse(false);
       return;
     }
-  } else {
-    // TODO(tbarzic): Change this to GetSenderWebContets once the echo extension
-    // code is updated to send tab ID information with the request.
-    web_contents = GetAssociatedWebContentsDeprecated();
   }
 
-  if (!web_contents) {
-    error_ = "No web contents.";
-    SendResponse(false);
-    return;
-  }
+  DCHECK(web_contents);
 
   // Add ref to ensure the function stays around until the dialog listener is
   // called. The reference is release in |Finalize|.

chrome/browser/chromeos/extensions/echo_private_api.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_CHROMEOS_EXTENSIONS_ECHO_PRIVATE_API_H_
 #define CHROME_BROWSER_CHROMEOS_EXTENSIONS_ECHO_PRIVATE_API_H_
 
+#include <string>
+
 #include "base/compiler_specific.h"
 #include "chrome/browser/chromeos/ui/echo_dialog_listener.h"
 #include "chrome/browser/extensions/chrome_extension_function.h"
@@ -89,8 +91,8 @@ class EchoPrivateGetUserConsentFunction : public ChromeAsyncExtensionFunction,
                                           public chromeos::EchoDialogListener {
  public:
   // Type for the dialog shown callback used in tests.
-  typedef base::Callback<void(chromeos::EchoDialogView* dialog)>
-          DialogShownTestCallback;
+  using DialogShownTestCallback =
+      base::RepeatingCallback<void(chromeos::EchoDialogView* dialog)>;
 
   EchoPrivateGetUserConsentFunction();
 

chrome/browser/chromeos/extensions/echo_private_apitest.cc

@@ -6,11 +6,15 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/ui/echo_dialog_view.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
+#include "chrome/browser/extensions/extension_tab_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chromeos/chromeos_switches.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -42,17 +46,19 @@ class ExtensionEchoPrivateApiTest : public ExtensionApiTest {
     command_line->AppendSwitch(switches::kStubCrosSettings);
   }
 
-  void RunDefaultGetUserFunctionAndExpectResultEquals(bool expected_result) {
+  void RunDefaultGetUserFunctionAndExpectResultEquals(int tab_id,
+                                                      bool expected_result) {
     scoped_refptr<EchoPrivateGetUserConsentFunction> function(
         EchoPrivateGetUserConsentFunction::CreateForTest(
-            base::Bind(&ExtensionEchoPrivateApiTest::OnDialogShown,
+            base::BindRepeating(&ExtensionEchoPrivateApiTest::OnDialogShown,
                                 base::Unretained(this))));
     function->set_has_callback(true);
 
+    const std::string arguments = base::StringPrintf(
+        R"([{"serviceName": "name", "origin": "https://test.com", "tabId": %d}])",
+        tab_id);
     std::unique_ptr<base::Value> result(utils::RunFunctionAndReturnSingleResult(
-        function.get(),
-        "[{\"serviceName\":\"some_name\",\"origin\":\"http://chromium.org\"}]",
-        browser()));
+        function.get(), arguments, browser()));
 
     ASSERT_TRUE(result.get());
     ASSERT_EQ(base::Value::Type::BOOLEAN, result->type());
@@ -98,6 +104,28 @@ class ExtensionEchoPrivateApiTest : public ExtensionApiTest {
     return dialog_invocation_count_;
   }
 
+  // Open and activates tab in the test browser. Returns the ID of the opened
+  // tab.
+  int OpenAndActivateTab() {
+    AddTabAtIndex(0, GURL("about:blank"), ui::PAGE_TRANSITION_LINK);
+    browser()->tab_strip_model()->ActivateTabAt(0, true);
+    return extensions::ExtensionTabUtil::GetTabId(
+        browser()->tab_strip_model()->GetActiveWebContents());
+  }
+
+  bool CloseTabWithId(int tab_id) {
+    TabStripModel* tab_strip = nullptr;
+    int tab_index = -1;
+    if (!extensions::ExtensionTabUtil::GetTabById(tab_id, profile(), false,
+                                                  nullptr, &tab_strip, nullptr,
+                                                  &tab_index)) {
+      ADD_FAILURE() << "Tab not found " << tab_id;
+      return false;
+    }
+
+    return tab_strip->CloseWebContentsAt(tab_index, 0);
+  }
+
  protected:
   int expected_dialog_buttons_;
   DialogTestAction dialog_action_;
@@ -113,68 +141,139 @@ IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest, EchoTest) {
 
 IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
                        GetUserConsent_InvalidOrigin) {
+  const int tab_id = OpenAndActivateTab();
+
   expected_dialog_buttons_ =  ui::DIALOG_BUTTON_NONE;
   dialog_action_ = DIALOG_TEST_ACTION_NONE;
 
   scoped_refptr<EchoPrivateGetUserConsentFunction> function(
-      EchoPrivateGetUserConsentFunction::CreateForTest(base::Bind(
-          &ExtensionEchoPrivateApiTest::OnDialogShown,
+      EchoPrivateGetUserConsentFunction::CreateForTest(
+          base::BindRepeating(&ExtensionEchoPrivateApiTest::OnDialogShown,
                               base::Unretained(this))));
 
   std::string error = utils::RunFunctionAndReturnError(
       function.get(),
-      "[{\"serviceName\":\"some name\",\"origin\":\"invalid\"}]",
+      base::StringPrintf(
+          R"([{"serviceName": "name", "origin": "invalid", "tabId": %d}])",
+          tab_id),
       browser());
 
   EXPECT_EQ("Invalid origin.", error);
   EXPECT_EQ(0, dialog_invocation_count());
 }
 
+IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest, GetUserConsent_NoTabIdSet) {
+  expected_dialog_buttons_ = ui::DIALOG_BUTTON_NONE;
+  dialog_action_ = DIALOG_TEST_ACTION_NONE;
+
+  scoped_refptr<EchoPrivateGetUserConsentFunction> function(
+      EchoPrivateGetUserConsentFunction::CreateForTest(
+          base::BindRepeating(&ExtensionEchoPrivateApiTest::OnDialogShown,
+                              base::Unretained(this))));
+
+  std::string error = utils::RunFunctionAndReturnError(
+      function.get(),
+      R"([{"serviceName": "name", "origin": "https://test.com"}])", browser());
+
+  EXPECT_EQ("Not called from an app window - the tabId is required.", error);
+  EXPECT_EQ(0, dialog_invocation_count());
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
+                       GetUserConsent_InactiveTab) {
+  const int tab_id = OpenAndActivateTab();
+  // Open and activate another tab.
+  OpenAndActivateTab();
+
+  expected_dialog_buttons_ = ui::DIALOG_BUTTON_NONE;
+  dialog_action_ = DIALOG_TEST_ACTION_NONE;
+
+  scoped_refptr<EchoPrivateGetUserConsentFunction> function(
+      EchoPrivateGetUserConsentFunction::CreateForTest(
+          base::BindRepeating(&ExtensionEchoPrivateApiTest::OnDialogShown,
+                              base::Unretained(this))));
+
+  const std::string arguments = base::StringPrintf(
+      R"([{"serviceName": "name", "origin": "https://test.com", "tabId": %d}])",
+      tab_id);
+  std::string error =
+      utils::RunFunctionAndReturnError(function.get(), arguments, browser());
+
+  EXPECT_EQ("Consent requested from an inactive tab.", error);
+  EXPECT_EQ(0, dialog_invocation_count());
+}
+
+IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest, GetUserConsent_ClosedTab) {
+  const int tab_id = OpenAndActivateTab();
+  ASSERT_TRUE(CloseTabWithId(tab_id));
+
+  expected_dialog_buttons_ = ui::DIALOG_BUTTON_NONE;
+  dialog_action_ = DIALOG_TEST_ACTION_NONE;
+
+  scoped_refptr<EchoPrivateGetUserConsentFunction> function(
+      EchoPrivateGetUserConsentFunction::CreateForTest(
+          base::BindRepeating(&ExtensionEchoPrivateApiTest::OnDialogShown,
+                              base::Unretained(this))));
+
+  const std::string arguments = base::StringPrintf(
+      R"([{"serviceName": "name", "origin": "https://test.com", "tabId": %d}])",
+      tab_id);
+  std::string error =
+      utils::RunFunctionAndReturnError(function.get(), arguments, browser());
+
+  EXPECT_EQ("Tab not found.", error);
+  EXPECT_EQ(0, dialog_invocation_count());
+}
+
 IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
                        GetUserConsent_AllowRedeemPrefNotSet) {
+  const int tab_id = OpenAndActivateTab();
   expected_dialog_buttons_ = ui::DIALOG_BUTTON_CANCEL | ui::DIALOG_BUTTON_OK;
   dialog_action_ = DIALOG_TEST_ACTION_ACCEPT;
 
-  RunDefaultGetUserFunctionAndExpectResultEquals(true);
+  RunDefaultGetUserFunctionAndExpectResultEquals(tab_id, true);
 
   EXPECT_EQ(1, dialog_invocation_count());
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
                        GetUserConsent_AllowRedeemPrefTrue) {
+  const int tab_id = OpenAndActivateTab();
   chromeos::CrosSettings::Get()->SetBoolean(
             chromeos::kAllowRedeemChromeOsRegistrationOffers, true);
 
   expected_dialog_buttons_ = ui::DIALOG_BUTTON_CANCEL | ui::DIALOG_BUTTON_OK;
   dialog_action_ = DIALOG_TEST_ACTION_ACCEPT;
 
-  RunDefaultGetUserFunctionAndExpectResultEquals(true);
+  RunDefaultGetUserFunctionAndExpectResultEquals(tab_id, true);
 
   EXPECT_EQ(1, dialog_invocation_count());
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
                        GetUserConsent_ConsentDenied) {
+  const int tab_id = OpenAndActivateTab();
   chromeos::CrosSettings::Get()->SetBoolean(
             chromeos::kAllowRedeemChromeOsRegistrationOffers, true);
 
   expected_dialog_buttons_ = ui::DIALOG_BUTTON_CANCEL | ui::DIALOG_BUTTON_OK;
   dialog_action_ = DIALOG_TEST_ACTION_CANCEL;
 
-  RunDefaultGetUserFunctionAndExpectResultEquals(false);
+  RunDefaultGetUserFunctionAndExpectResultEquals(tab_id, false);
 
   EXPECT_EQ(1, dialog_invocation_count());
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionEchoPrivateApiTest,
                        GetUserConsent_AllowRedeemPrefFalse) {
+  const int tab_id = OpenAndActivateTab();
   chromeos::CrosSettings::Get()->SetBoolean(
             chromeos::kAllowRedeemChromeOsRegistrationOffers, false);
 
   expected_dialog_buttons_ = ui::DIALOG_BUTTON_CANCEL;
   dialog_action_ = DIALOG_TEST_ACTION_CANCEL;
 
-  RunDefaultGetUserFunctionAndExpectResultEquals(false);
+  RunDefaultGetUserFunctionAndExpectResultEquals(tab_id, false);
 
   EXPECT_EQ(1, dialog_invocation_count());
 }