Added browser test for TemplateUrlService protected prefs

chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc

Index: chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
diff --git a/chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc b/chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..68011c621f7fb3de574de527e25086e1af50b9b2
--- /dev/null
+++ b/chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc
@@ -0,0 +1,227 @@
+// Copyright 2016 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 "base/base_switches.h"
+#include "base/command_line.h"
+#include "base/json/json_file_value_serializer.h"
+#include "base/json/json_reader.h"
+#include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/prefs/chrome_pref_service_factory.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/search_test_utils.h"
+#include "components/search_engines/template_url_service.h"
+#include "components/user_prefs/tracked/pref_hash_calculator.h"
+
+#if defined(OS_WIN)
+#include "rlz/lib/machine_id.h"
+#endif  // defined(OS_WIN)
+
+namespace {
+
+std::string GetSeed() {
+  // Seed is empty string in tests.

[Peter Kasting, 2016/11/23 18:49:13]

Nit: I would just pass std::string() directly in the caller.

[Alexander Yashkin, 2016/11/24 14:37:27]

Done

+  return std::string();
+}
+
+std::string GetDeviceID() {
+  std::string device_id;
+#if defined(OS_WIN) && defined(ENABLE_RLZ)
+  rlz_lib::GetMachineId(&device_id);

[Peter Kasting, 2016/11/23 18:49:13]

Why do we need to compute this?  Can't we just pass an empty string for both the
seed and the device ID?

[Alexander Yashkin, 2016/11/24 14:37:27]

Done

+#endif
+  return device_id;
+}
+
+}  // namespace
+
+class TURLServicePrefsProtectBrowsertest : public InProcessBrowserTest {
+ protected:
+  TURLServicePrefsProtectBrowsertest();
+
+  void SetUpCommandLine(base::CommandLine* command_line) override;
+  void SetUpInProcessBrowserTestFixture() override;
+  Profile* CreateProfileWithPrefs(
+      const std::string& prefs,
+      const std::vector<std::string>& names_to_update_hashes);
+
+ private:
+  PrefHashCalculator hash_calculator_;
+};

[Peter Kasting, 2016/11/23 18:49:13]

Nit: DISALLOW_COPY_AND_ASSIGN

[Alexander Yashkin, 2016/11/24 14:37:27]

Done

+
+TURLServicePrefsProtectBrowsertest::TURLServicePrefsProtectBrowsertest()
+    : hash_calculator_(GetSeed(), GetDeviceID()) {}
+
+// Enable protection of DSE prefs by turning on field trial in command line.
+void TURLServicePrefsProtectBrowsertest::SetUpCommandLine(
+    base::CommandLine* command_line) {
+  command_line->AppendSwitchASCII(
+      switches::kForceFieldTrials,
+      base::StringPrintf("%s/%s/",
+                         chrome_prefs::internals::kSettingsEnforcementTrialName,
+                         chrome_prefs::internals::
+                             kSettingsEnforcementGroupEnforceAlwaysWithDSE));
+}
+
+// Prefs protection is turned off in domain, so disable domain check.
+void TURLServicePrefsProtectBrowsertest::SetUpInProcessBrowserTestFixture() {
+  chrome_prefs::DisableDomainCheckForTesting();
+}
+
+Profile* TURLServicePrefsProtectBrowsertest::CreateProfileWithPrefs(
+    const std::string& prefs,
+    const std::vector<std::string>& names_to_update_hashes) {
+  // Create a directory for a new profile and get the path to the Preferences.
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  const base::FilePath profile_path =
+      profile_manager->GenerateNextProfileDirectoryPath();
+  const base::FilePath prefs_path =
+      profile_path.Append(chrome::kPreferencesFilename);
+  bool dir_exists = base::CreateDirectory(prefs_path.DirName());
+  DCHECK(dir_exists);

[Peter Kasting, 2016/11/23 18:49:13]

Nit: Avoid [D]CHECK in test code (failed checks cause tests to terminate
ungracefully and leave bots in a bad state).  Some ways to do this:

* Just omit the DCHECK
* Return nullptr if !dir_exists (caller will assert)
* EXPECT that this is true, then return nullptr
* Change function to return profile via outparam, then ASSERT that dir_exists
here (ASSERT can only be used in a function that returns void)

(3 places)

[Alexander Yashkin, 2016/11/24 14:37:27]

Done

+
+  // Convert the passed |prefs| to base::DictionaryValue.
+  std::string replaced_prefs = prefs;
+  base::ReplaceSubstringsAfterOffset(&replaced_prefs, 0, "'", "\"");

[Peter Kasting, 2016/11/23 18:49:13]

Nit: Seems like you wouldn't need this if you inited the string with " inside to
begin with.  See comment below about using raw string literals.

+  std::unique_ptr<base::Value> prefs_value =
+      base::JSONReader::Read(replaced_prefs);
+  base::DictionaryValue* prefs_dict = nullptr;
+  prefs_value->GetAsDictionary(&prefs_dict);
+  DCHECK(prefs_dict);
+
+  // Calculate hashes of the requested settings.
+  for (const std::string& name : names_to_update_hashes) {
+    const base::Value* value = nullptr;
+    prefs_dict->Get(name, &value);
+    DCHECK(value);
+    prefs_dict->SetString("protection.macs." + name,
+                          hash_calculator_.Calculate(name, value));
+  }
+
+  // Write the result preferences to file.
+  JSONFileValueSerializer json_serializer(prefs_path);
+  if (!json_serializer.Serialize(*prefs_dict))
+    return nullptr;
+
+  // Finally create a profile.
+  Profile* profile = Profile::CreateProfile(profile_path, NULL,
+                                            Profile::CREATE_MODE_SYNCHRONOUS);
+  if (!profile)
+    return nullptr;
+  profile_manager->RegisterTestingProfile(profile, false, false);
+  return profile;

[Peter Kasting, 2016/11/23 18:49:13]

Nit: Simpler:

  if (profile)
    profile_manager->RegisterTestingProfile(profile, false, false);
  return profile;

[Alexander Yashkin, 2016/11/24 14:37:27]

Done

+}
+
+// Check that preferences that can influence default search provider choice
+// are protected.
+IN_PROC_BROWSER_TEST_F(TURLServicePrefsProtectBrowsertest,
+                       ChangeDefaultSearchProvider) {
+  struct TestCase {
+    const std::string pref_to_protect;
+    const std::vector<std::string> names_to_update_hashes;
+    const std::vector<base::string16> expected_keywords;
+    const std::vector<base::string16> not_expected_keywords;
+  };
+
+  constexpr const char* default_search_provider_data =

[Peter Kasting, 2016/11/23 18:49:13]

Nit: I suspect raw string literals would make these declarations easier to read.

[Alexander Yashkin, 2016/11/24 14:37:27]

Could not find raw literals example in sources.
Tried to indent to be more readable.

[Peter Kasting, 2016/11/24 19:39:48]

See https://codereview.chromium.org/2470643002 for a sample conversion.

Also not sure why you converted constexpr const -> const, as the two don't
actually mean the same thing in this case (and arguably the const is the more
important of the two).

+      "{"
+      "'default_search_provider_data' : {"
+      "'template_url_data' : {"
+      "'keyword' : 'my_search',"
+      "'short_name' : 'My Search',"
+      "'url' : '{google:baseURL}search?q=my+{searchTerms}'"
+      "}"
+      "}"
+      "}";
+  constexpr const char* search_provider_overrides =
+      "{"
+      "'search_provider_overrides' : ["
+      "{"
+      "'keyword' : 'my_search',"
+      "'name' : 'My Search',"
+      "'search_url' : '{google:baseURL}search?q=my+{searchTerms}',"
+      "'encoding' : 'utf-8',"
+      "'favicon_url' : 'http://www.google.com/favicon.ico',"
+      "'id' : 1"
+      "}, {"
+      "'keyword' : 'my_search2',"
+      "'name' : 'My Search2',"
+      "'search_url' : '{google:baseURL}search?q="
+      "my+{searchTerms}+2',"
+      "'encoding' : 'utf-8',"
+      "'favicon_url' : 'http://www.google.com/favicon.ico',"
+      "'id' : 2"
+      "}"
+      "]"
+      "}";
+  constexpr const char* default_search_provider =
+      "{"
+      "'default_search_provider' : {"
+      "'keyword' : 'my_search',"
+      "'name' : 'My Search',"
+      "'search_url' : '{google:baseURL}search?q=my+{searchTerms}'"
+      "}"
+      "}";
+
+  // Google is default search, so google keywod will be set for cases where
+  // protection check fail.

[Peter Kasting, 2016/11/23 18:49:13]

Nit: It might be nice to avoid this hardcoding by getting the "default" default
search provider keyword at runtime.

If you don't do this, this comment has a typo and is slightly nongrammatical.

[Alexander Yashkin, 2016/11/24 14:37:27]

Changed to get default provider from current browser test browser profile.

+  const base::string16 google_keyword(base::ASCIIToUTF16("google.com"));

[Peter Kasting, 2016/11/23 18:49:13]

Nit: Prefer = to () to init "simple" objects like strings (see
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...
).

[Alexander Yashkin, 2016/11/24 14:37:27]

Done

+  const base::string16 my_search(base::ASCIIToUTF16("my_search"));
+  const base::string16 my_search2(base::ASCIIToUTF16("my_search2"));
+
+  const TestCase test_cases[] = {
+      {default_search_provider_data,
+       {"default_search_provider_data.template_url_data"},
+       {my_search},
+       {}},
+      {default_search_provider_data, {}, {google_keyword}, {my_search}},
+      {search_provider_overrides,
+       {"search_provider_overrides"},
+       {my_search, my_search2},
+       {}},
+      {search_provider_overrides,
+       {},
+       {google_keyword},
+       {my_search, my_search2}},
+      {default_search_provider,
+       {"default_search_provider.keyword", "default_search_provider.name",
+        "default_search_provider.search_url"},
+       {my_search},
+       {}},
+      {default_search_provider, {}, {google_keyword}, {my_search}},
+  };
+
+  for (size_t i = 0; i != arraysize(test_cases); ++i) {
+    const TestCase& test_case = test_cases[i];
+    // Create profile with pref to protect and update hashes for
+    // protected prefs.
+    Profile* profile = CreateProfileWithPrefs(test_case.pref_to_protect,
+                                              test_case.names_to_update_hashes);
+    ASSERT_TRUE(profile);
+
+    TemplateURLService* template_url_service =
+        TemplateURLServiceFactory::GetForProfile(profile);
+    search_test_utils::WaitForTemplateURLServiceToLoad(template_url_service);
+
+    TemplateURL* dse = template_url_service->GetDefaultSearchProvider();
+    ASSERT_TRUE(dse) << "Test case i=" << i;
+
+    DCHECK(!test_case.expected_keywords.empty());

[Peter Kasting, 2016/11/23 18:49:13]

Nit: Avoid [D]CHECK in test code, use ASSERT instead if necessary

[Alexander Yashkin, 2016/11/24 14:37:27]

Done.

+    EXPECT_EQ(test_case.expected_keywords[0], dse->keyword()) << "i=" << i;
+
+    for (const base::string16& keyword : test_case.expected_keywords) {
+      EXPECT_TRUE(template_url_service->GetTemplateURLForKeyword(keyword))
+          << "keyword=" << keyword << ", i=" << i;
+    }
+
+    for (const base::string16& keyword : test_case.not_expected_keywords) {
+      EXPECT_FALSE(template_url_service->GetTemplateURLForKeyword(keyword))
+          << "keyword=" << keyword << ", i=" << i;
+    }
+  }
+}