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);
+};
+
+// Passing empty strings to the |hash_calculator_| would be insecure for real
+// usage, but is sufficient in tests to verify the protection functionality.
+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;
+  };
+
+  const char* default_search_provider_data =

[gab, 2016/11/25 14:24:41]

nit: char[] instead of char*
nit: prefer "static" for local const (on phone and don't have chromium-dev
reference but it avoids a copy on MSVC)
nit: I think constexpr char[] is even better than static const char[] these days

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

Please find the reference; I've been explicitly asking people not to use static
for local consts in general because it can make generated code worse.
constexpr was the original method here, and I asked why it was removed, and
thought it would be put back.  Disappointed to see it wasn't, with no response
to my question.

[gab, 2016/11/28 16:12:20]

According to our tests from 3 years ago, static was required for MSVC to
generate copy-less code:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pF7nAmUQ2uw/wrNhX...

[Peter Kasting, 2016/11/28 20:20:44]

Fascinating!  I forgot about this thread.  So, looks like this applies mainly to
compound types (which makes more sense), so technically, it's not really needed
if this stays as char* instead of being converted back to char[].

I was curious whether the story has changed with MSVC 2015 + constexpr.  I would
hope the two of those together eliminate the need for "static".  However, in
some local testing, it looks to me as if it has not, at least for the test code
in your email; no use of constexpr I could find made "char[]" as good as the
static version thereof.

+      R"({
+          'default_search_provider_data' :
+           {
+            'template_url_data' :
+            {
+              'keyword' : 'my_search',
+              'short_name' : 'My Search',
+              'url' : '{google:baseURL}search?q=my+{searchTerms}'
+            }
+          }
+        })";
+  const char* search_provider_overrides =
+      R"({
+          '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
+          }
+          ]
+        })";
+  const char* default_search_provider =
+      R"({
+          'default_search_provider' :
+          {
+            'keyword' : 'my_search',
+            'name' : 'My Search',
+            'search_url' : '{google:baseURL}search?q=my+{searchTerms}'
+          }
+        })";
+
+  // In cases below where the protection check fails, the default search
+  // provider's keyword will be unchanged. Get that keyword from an unmodified
+  // profile.
+  TemplateURLService* turl_service =
+      TemplateURLServiceFactory::GetForProfile(browser()->profile());
+  ASSERT_TRUE(turl_service);
+  search_test_utils::WaitForTemplateURLServiceToLoad(turl_service);
+  const base::string16 default_keyword =
+      turl_service->GetDefaultSearchProvider()->keyword();
+  const base::string16 my_search = base::ASCIIToUTF16("my_search");

[gab, 2016/11/25 14:24:41]

L"my_search" instead of ASCIIToUTF16

and below

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

Doesn't work.  Can't assign a wide string to a string16 on non-Windows.

[gab, 2016/11/28 16:12:20]

Duh, right, that's unfortunate. 

Beyond this CL, but this makes me think that the string conversion methods
should perhaps be constexpr so that the compiler can do this work ahead of time
when possible?

Also, file_path.h provides the FILE_PATH_LITERAL macro to avoid exactly this,
maybe that macro needs to go beyond FilePaths?

+  const base::string16 my_search2 = base::ASCIIToUTF16("my_search2");
+
+  const TestCase test_cases[] = {
+      {default_search_provider_data,

[gab, 2016/11/25 14:24:41]

Add a comment above each test case mentioning what it's testing

+       {"default_search_provider_data.template_url_data"},
+       {my_search},
+       std::vector<base::string16>()},
+      {default_search_provider_data, std::vector<std::string>(),
+       {default_keyword}, {my_search}},
+      {search_provider_overrides,
+       {"search_provider_overrides"},
+       {my_search, my_search2},
+       std::vector<base::string16>()},
+      {search_provider_overrides,
+       std::vector<std::string>(),
+       {default_keyword},
+       {my_search, my_search2}},
+      {default_search_provider,
+       {"default_search_provider.keyword", "default_search_provider.name",
+        "default_search_provider.search_url"},
+       {my_search},
+       std::vector<base::string16>()},
+      {default_search_provider, std::vector<std::string>(),
+       {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;

[gab, 2016/11/25 14:24:41]

Can replace all of these logs by SCOPED_TRACE(i) at to of loop

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

Good call, I forgot about that.

+
+    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;
+    }
+  }
+}