[Extensions] Don't show the settings overridden dialog for same-domains

An extension controlling settings, like search provider, could
potentially have the same settings as the user previously set. For
instance, a user could:
1) set search provider to example.com
2) install the example.com search extension

In this case, we shouldn't display the settings overridden dialog,
because there's no effective change.

Add logic to not display the dialog if the fallback search provider
is not extension provided, and matches the origin of the new
provider. If the previous provider was an extension, we'll still
show the dialog - this simplifies the implementation, and can also
be useful to the user because the controlling extension changed.

Add tests for the same.

Bug: 940923

Change-Id: I3ab9ba91d8bb473e44892a6bcc9d746d8472a465
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2313997Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791001}

chrome/browser/ui/extensions/settings_overridden_params_providers.cc

@@ -79,8 +79,12 @@ struct SecondarySearchInfo {
 
   Type type;
 
+  // The origin of the search engine. Only populated if the secondary search
+  // is not from another extension.
+  GURL origin;
+
   // The name of the search engine; only populated when |type| is
-  // kOtherDefaultOption.
+  // kNonGoogleInDefaultList.
   base::string16 name;
 };
 
@@ -94,8 +98,15 @@ SecondarySearchInfo GetSecondarySearchInfo(Profile* profile) {
   // the search engine.
   DCHECK_GE(num_overriding_extensions, 1u);
 
-  if (num_overriding_extensions > 1)  // Another extension would take over.
+  if (num_overriding_extensions > 1) {
+    // Another extension would take over.
+    // NOTE(devlin): Theoretically, we could try and figure out exactly which
+    // extension would take over, and include the origin of the secondary
+    // search. However, this (>1 overriding extension) is an uncommon case, and
+    // all that will happen is that we'll prompt the user that the new extension
+    // is overriding search.
     return {SecondarySearchInfo::Type::kOther};
+  }
 
   const TemplateURLService* const template_url_service =
       TemplateURLServiceFactory::GetForProfile(profile);
@@ -109,18 +120,19 @@ SecondarySearchInfo GetSecondarySearchInfo(Profile* profile) {
     return {SecondarySearchInfo::Type::kOther};
   }
 
-  if (IsGoogleSearch(*secondary_search, *template_url_service))
-    return {SecondarySearchInfo::Type::kGoogle};
+  const GURL search_url = secondary_search->GenerateSearchURL(
+      template_url_service->search_terms_data());
+  const GURL origin = search_url.GetOrigin();
+  if (google_util::IsGoogleSearchUrl(search_url))
+    return {SecondarySearchInfo::Type::kGoogle, origin};
 
   if (!template_url_service->ShowInDefaultList(secondary_search)) {
     // Found another search engine, but it's not one of the default options.
-    return {SecondarySearchInfo::Type::kOther};
+    return {SecondarySearchInfo::Type::kOther, origin};
   }
 
-  DCHECK(!secondary_search->short_name().empty());
-
   // The secondary search engine is another of the defaults.
-  return {SecondarySearchInfo::Type::kNonGoogleInDefaultList,
+  return {SecondarySearchInfo::Type::kNonGoogleInDefaultList, origin,
           secondary_search->short_name()};
 }
 
@@ -230,6 +242,20 @@ GetSearchOverriddenParams(Profile* profile) {
   GURL search_url(default_search->url());
   DCHECK(search_url.is_valid()) << default_search->url();
 
+  // Check whether the secondary search is the same search the extension set.
+  // This can happen if the user set a search engine, and then installed an
+  // extension that set the same one.
+  SecondarySearchInfo secondary_search = GetSecondarySearchInfo(profile);
+  // NOTE: Normally, we wouldn't want to use direct equality comparison of
+  // GURL::GetOrigin() because of edge cases like inner URLs with filesystem,
+  // etc. This okay here, because if the origins don't match, we'll show the
+  // dialog to the user. That's likely good if any extension is doing something
+  // as crazy as using filesystem: URLs as a search engine.
+  if (!secondary_search.origin.is_empty() &&
+      secondary_search.origin == search_url.GetOrigin()) {
+    return base::nullopt;
+  }
+
   // Format the URL for display.
   const url_formatter::FormatUrlTypes kFormatRules =
       url_formatter::kFormatUrlOmitTrivialSubdomains |
@@ -246,8 +272,6 @@ GetSearchOverriddenParams(Profile* profile) {
   constexpr char kBackToGoogleHistogramName[] =
       "Extensions.SettingsOverridden.BackToGoogleSearchOverriddenDialogResult";
 
-  SecondarySearchInfo secondary_search = GetSecondarySearchInfo(profile);
-
   const char* histogram_name = nullptr;
   const gfx::VectorIcon* icon = nullptr;
   base::string16 dialog_title;

chrome/browser/ui/extensions/settings_overridden_params_providers_browsertest.cc

@@ -20,6 +20,7 @@
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/extension_builder.h"
 #include "extensions/common/value_builder.h"
+#include "extensions/test/test_extension_dir.h"
 
 class SettingsOverriddenParamsProvidersBrowserTest
     : public extensions::ExtensionBrowserTest {
@@ -57,10 +58,9 @@ class SettingsOverriddenParamsProvidersBrowserTest
   // |new_search_name_out| will be populated with the new search provider's
   // name.
   void SetNewDefaultSearch(bool new_search_shows_in_default_list,
-                           std::string* new_search_name_out) {
+                           const TemplateURL** new_turl_out) {
     // Find a search provider that isn't Google, and set it as the default.
-    TemplateURLService* const template_url_service =
-        TemplateURLServiceFactory::GetForProfile(profile());
+    TemplateURLService* const template_url_service = GetTemplateURLService();
     TemplateURLService::TemplateURLVector template_urls =
         template_url_service->GetTemplateURLs();
     auto iter =
@@ -75,8 +75,12 @@ class SettingsOverriddenParamsProvidersBrowserTest
     ASSERT_NE(template_urls.end(), iter);
     // iter != template_urls.end());
     template_url_service->SetUserSelectedDefaultSearchProvider(*iter);
-    if (new_search_name_out)
-      *new_search_name_out = base::UTF16ToUTF8((*iter)->short_name());
+    if (new_turl_out)
+      *new_turl_out = *iter;
+  }
+
+  TemplateURLService* GetTemplateURLService() {
+    return TemplateURLServiceFactory::GetForProfile(profile());
   }
 };
 
@@ -122,8 +126,10 @@ IN_PROC_BROWSER_TEST_F(SettingsOverriddenParamsProvidersBrowserTest,
 IN_PROC_BROWSER_TEST_F(SettingsOverriddenParamsProvidersBrowserTest,
                        GetExtensionControllingSearch_NonGoogleSearch) {
   constexpr bool kNewSearchShowsInDefaultList = true;
-  std::string new_search_name;
-  SetNewDefaultSearch(kNewSearchShowsInDefaultList, &new_search_name);
+  const TemplateURL* new_turl = nullptr;
+  SetNewDefaultSearch(kNewSearchShowsInDefaultList, &new_turl);
+  ASSERT_TRUE(new_turl);
+  std::string new_search_name = base::UTF16ToUTF8(new_turl->short_name());
 
   const extensions::Extension* extension = AddExtensionControllingSearch();
   ASSERT_TRUE(extension);
@@ -139,8 +145,7 @@ IN_PROC_BROWSER_TEST_F(SettingsOverriddenParamsProvidersBrowserTest,
                        GetExtensionControllingSearch_NonDefaultSearch) {
   // Create and set a search provider that isn't one of the built-in default
   // options.
-  TemplateURLService* const template_url_service =
-      TemplateURLServiceFactory::GetForProfile(profile());
+  TemplateURLService* const template_url_service = GetTemplateURLService();
   template_url_service->Add(
       std::make_unique<TemplateURL>(*GenerateDummyTemplateURLData("test")));
 
@@ -174,6 +179,99 @@ IN_PROC_BROWSER_TEST_F(
             base::UTF16ToUTF8(params->dialog_title));
 }
 
+// Tests that null params are returned (indicating no dialog should be shown)
+// when an extension overrides search to the same domain that was previously
+// used using a prepopulated id.
+IN_PROC_BROWSER_TEST_F(SettingsOverriddenParamsProvidersBrowserTest,
+                       SearchOverriddenToSameSearch_PrepopulatedId) {
+  constexpr bool kNewSearchShowsInDefaultList = true;
+  const TemplateURL* new_turl = nullptr;
+  SetNewDefaultSearch(kNewSearchShowsInDefaultList, &new_turl);
+  ASSERT_TRUE(new_turl);
+  // Google's ID is the lowest valid ID (1); the new engine must be greater.
+  constexpr int kGooglePrepopulateId = 1;
+  EXPECT_GT(new_turl->prepopulate_id(), kGooglePrepopulateId);
+
+  constexpr char kManifestTemplate[] =
+      R"({
+           "name": "Search Override Extension",
+           "version": "0.1",
+           "manifest_version": 2,
+           "chrome_settings_overrides": {
+             "search_provider": {
+               "search_url": "%s/?q={searchTerms}",
+               "prepopulated_id": %d,
+               "is_default": true
+             }
+           }
+         })";
+  extensions::TestExtensionDir test_dir;
+
+  GURL search_url =
+      new_turl->GenerateSearchURL(GetTemplateURLService()->search_terms_data());
+  test_dir.WriteManifest(base::StringPrintf(
+      kManifestTemplate, search_url.GetOrigin().spec().c_str(),
+      new_turl->prepopulate_id()));
+
+  const extensions::Extension* extension =
+      InstallExtensionWithPermissionsGranted(test_dir.UnpackedPath(), 1);
+  ASSERT_TRUE(extension);
+  EXPECT_EQ(extension,
+            extensions::GetExtensionOverridingSearchEngine(profile()));
+
+  base::Optional<ExtensionSettingsOverriddenDialog::Params> params =
+      settings_overridden_params::GetSearchOverriddenParams(profile());
+  EXPECT_FALSE(params) << "Unexpected params: " << params->dialog_title;
+}
+
+// Tests that null params are returned (indicating no dialog should be shown)
+// when an extension overrides search to the same domain that was previously
+// used using a custom search definition.
+IN_PROC_BROWSER_TEST_F(SettingsOverriddenParamsProvidersBrowserTest,
+                       SearchOverriddenToSameSearch_SameDomain) {
+  constexpr bool kNewSearchShowsInDefaultList = true;
+  const TemplateURL* new_turl = nullptr;
+  SetNewDefaultSearch(kNewSearchShowsInDefaultList, &new_turl);
+  ASSERT_TRUE(new_turl);
+  // Google's ID is the lowest valid ID (1); the new engine must be greater.
+  constexpr int kGooglePrepopulateId = 1;
+  EXPECT_GT(new_turl->prepopulate_id(), kGooglePrepopulateId);
+
+  constexpr char kManifestTemplate[] =
+      R"({
+           "name": "Search Override Extension",
+           "version": "0.1",
+           "manifest_version": 2,
+           "chrome_settings_overrides": {
+             "search_provider": {
+               "search_url": "%s/?q={searchTerms}",
+               "name": "New Search",
+               "keyword": "word",
+               "encoding": "UTF-8",
+               "favicon_url": "https://example.com/favicon.ico",
+               "is_default": true
+             }
+           }
+         })";
+
+  extensions::TestExtensionDir test_dir;
+
+  GURL search_url =
+      new_turl->GenerateSearchURL(GetTemplateURLService()->search_terms_data());
+  test_dir.WriteManifest(base::StringPrintf(
+      kManifestTemplate, search_url.GetOrigin().spec().c_str()));
+
+  const extensions::Extension* extension =
+      InstallExtensionWithPermissionsGranted(test_dir.UnpackedPath(), 1);
+  ASSERT_TRUE(extension);
+  EXPECT_EQ(extension,
+            extensions::GetExtensionOverridingSearchEngine(profile()));
+
+  base::Optional<ExtensionSettingsOverriddenDialog::Params> params =
+      settings_overridden_params::GetSearchOverriddenParams(profile());
+  EXPECT_FALSE(params) << "Unexpected params: " << params->dialog_title;
+}
+
 #endif  // defined(OS_WIN) || defined(OS_MACOSX)
 
 // Tests the dialog display when the default search engine has changed; in this
@@ -181,8 +279,7 @@ IN_PROC_BROWSER_TEST_F(
 IN_PROC_BROWSER_TEST_F(SettingsOverriddenParamsProvidersBrowserTest,
                        DialogParamsWithNonDefaultSearch) {
   // Find a search provider that isn't Google, and set it as the default.
-  TemplateURLService* const template_url_service =
-      TemplateURLServiceFactory::GetForProfile(profile());
+  TemplateURLService* const template_url_service = GetTemplateURLService();
   TemplateURLService::TemplateURLVector template_urls =
       template_url_service->GetTemplateURLs();
   auto iter = std::find_if(template_urls.begin(), template_urls.end(),