Added browser test for TemplateUrlService protected prefs

chrome/browser/search_engines/template_url_service_prefs_protect_browsertest.cc

+// 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/browser/ui/browser.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"
+
+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_;
+  DISALLOW_COPY_AND_ASSIGN(TURLServicePrefsProtectBrowsertest);

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

Nit: Frequently we put a blank line above this

[Alexander Yashkin, 2016/11/25 09:33:34]

Done

+};
+
+// Seed and device_id passed to hash_calculator_ constructor are empty strings
+// in tests.

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

Nit: This comment kinda restates the code.  There's no reason we couldn't use
non-empty strings too, it's just that we don't need anything special.  I might
say something like "Passing empty strings to the |hash_calculator_| would be
insecure for real usage, but is sufficient in tests to verify the protection
functionality."

[Alexander Yashkin, 2016/11/25 09:33:34]

What I really meant is that protected prefs subsystem uses empty strings in
tests, 
so that we have to use same values if we want calculate correct hash.
Changed to your variant.

[Peter Kasting, 2016/11/26 06:12:40]

Wait, where is that code that you're referring to?  If we have to match
something else's behavior, we need to say that more clearly here.  My comment
doesn't say that because I didn't know that was the case.

+TURLServicePrefsProtectBrowsertest::TURLServicePrefsProtectBrowsertest()
+    : hash_calculator_(std::string(), std::string()) {}
+
+// 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());
+  if (!dir_exists)
+    return nullptr;
+
+  // Convert the passed |prefs| to base::DictionaryValue.
+  std::string replaced_prefs = prefs;
+  base::ReplaceSubstringsAfterOffset(&replaced_prefs, 0, "'", "\"");
+  std::unique_ptr<base::Value> prefs_value =
+      base::JSONReader::Read(replaced_prefs);
+  base::DictionaryValue* prefs_dict = nullptr;
+  prefs_value->GetAsDictionary(&prefs_dict);
+  if (!prefs_dict)
+    return nullptr;
+
+  // 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);
+    if (!value)
+      return nullptr;
+    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)
+    profile_manager->RegisterTestingProfile(profile, false, false);
+  return profile;
+}
+
+// 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 char* default_search_provider_data =
+      "{"
+      " 'default_search_provider_data' :"
+      " {"
+      "   'template_url_data' :"
+      "   {"
+      "     'keyword' : 'my_search',"
+      "     'short_name' : 'My Search',"
+      "     'url' : '{google:baseURL}search?q=my+{searchTerms}'"
+      "   }"
+      " }"
+      "}";
+  constexpr 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 char* default_search_provider =
+      "{"
+      " 'default_search_provider' :"
+      " {"
+      "   'keyword' : 'my_search',"
+      "   'name' : 'My Search',"
+      "   'search_url' : '{google:baseURL}search?q=my+{searchTerms}'"
+      " }"
+      "}";
+
+  TemplateURLService* turl_service =
+    TemplateURLServiceFactory::GetForProfile(browser()->profile());
+  ASSERT_TRUE(turl_service);
+  search_test_utils::WaitForTemplateURLServiceToLoad(turl_service);
+  // Get keyword for default search provider. It will be set for cases where
+  // protection check fail.

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

Nit: This comment probably belongs above this whole block instead of in the
middle, since the code all seems related.  If not, then probably a blank line
above the comment would be good.

The comment has some grammar issues.  What about: "In cases below where the
protection check fails, the default search provider's keyword will be unchanged.
 Get that keyword from an unmodified profile."

[Alexander Yashkin, 2016/11/25 09:33:34]

Thanks, changed to your variant.

+  const base::string16 default_keyword = turl_service->
+      GetDefaultSearchProvider()->keyword();

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

Nit: Prefer to wrap after '=' where there's already whitespace instead of
breaking after "->".  (git cl format would probably have changed this)

[Alexander Yashkin, 2016/11/25 09:33:34]

Fixed, thanks.

+
+  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, {}, {default_keyword}, {my_search}},
+      {search_provider_overrides,
+       {"search_provider_overrides"},
+       {my_search, my_search2},
+       {}},
+      {search_provider_overrides,
+       {},
+       {default_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, {}, {default_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);
+
+    turl_service = TemplateURLServiceFactory::GetForProfile(profile);
+    search_test_utils::WaitForTemplateURLServiceToLoad(turl_service);
+
+    TemplateURL* dse = turl_service->GetDefaultSearchProvider();
+    ASSERT_TRUE(dse) << "Test case i=" << i;
+
+    ASSERT_FALSE(test_case.expected_keywords.empty());
+    EXPECT_EQ(test_case.expected_keywords[0], dse->keyword()) << "i=" << i;
+
+    for (const base::string16& keyword : test_case.expected_keywords) {
+      EXPECT_TRUE(turl_service->GetTemplateURLForKeyword(keyword))
+          << "keyword=" << keyword << ", i=" << i;
+    }
+
+    for (const base::string16& keyword : test_case.not_expected_keywords) {
+      EXPECT_FALSE(turl_service->GetTemplateURLForKeyword(keyword))
+          << "keyword=" << keyword << ", i=" << i;
+    }
+  }
+}