ConfigurationPolicyPrefStore refactoring to surface error messages.

chrome/browser/policy/configuration_policy_handler.cc

+// Copyright (c) 2011 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 "chrome/browser/policy/configuration_policy_handler.h"
+
+#include <sstream>
+
+#include "base/file_path.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "chrome/browser/download/download_util.h"
+#include "chrome/browser/policy/configuration_policy_pref_store.h"
+#include "chrome/browser/policy/policy_path_parser.h"
+#include "chrome/browser/prefs/proxy_config_dictionary.h"
+#include "chrome/browser/prefs/proxy_prefs.h"
+#include "chrome/browser/search_engines/search_terms_data.h"
+#include "chrome/browser/search_engines/template_url.h"
+#include "chrome/common/pref_names.h"
+
+namespace policy {
+
+// CheckPolicyValueType
+CheckPolicyValueType::CheckPolicyValueType(
+    ConfigurationPolicyType policy,
+    Value::Type value_type,
+    const char* pref_path) : policy_type_(policy),

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

please break before the :

I don't think the style guide disallows what you did here, but I've never seen a
multi-line initializer list in the code base that was indented like this.

[simo, 2011/09/22 11:43:26]

Done.

+                             value_type_(value_type),
+                             pref_path_(pref_path) {
+}
+
+CheckPolicyValueType::~CheckPolicyValueType() {
+}
+
+ConfigurationPolicyType CheckPolicyValueType::policy_type() const {
+  return policy_type_;
+}
+
+const char* CheckPolicyValueType::pref_path() const {
+  return pref_path_;
+}
+
+bool CheckPolicyValueType::CheckPolicySettings(PolicyMap* policies,
+                                               PolicyErrorMap* errors) {
+  const Value* value = policies->Get(policy_type_);
+  if (value && value_type_ != value->GetType()) {
+    std::string message = "Expected " + ValueTypeToString(value_type_);

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

Ah, so what about i18n? These errors show up in the UI, so we should be able to
translate them.

[simo, 2011/09/22 11:43:26]

Done.

+    errors->AddError(policy_type_, Value::CreateStringValue(message));
+    LOG(WARNING) << "Mismatch in provided and expected policy value for policy "
+                 <<  pref_path_ << ". expected = " << value_type_
+                 << ", actual = " << value->GetType();
+    return false;
+  }
+  return true;
+}
+
+std::string CheckPolicyValueType::ValueTypeToString(Value::Type type) {
+  static const char* strings[] = { "null", "boolean",
+                                   "integer", "double",
+                                   "string", "binary",
+                                   "dictionary", "list" };

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

each on one line please.

[simo, 2011/09/22 11:43:26]

Done.

+  DCHECK(static_cast<size_t>(type) < arraysize(strings));
+  return std::string(strings[type]);
+}
+
+// SimplePolicyHandler
+SimplePolicyHandler::SimplePolicyHandler(
+    ConfigurationPolicyType policy,
+    Value::Type value_type,
+    const char* pref_path)
+    : CheckPolicyValueType(policy, value_type, pref_path) {
+}
+
+SimplePolicyHandler::~SimplePolicyHandler() {
+}
+
+void SimplePolicyHandler::ApplyPolicySettings(PolicyMap* policies,
+                                              PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  const Value* value = policies->Get(policy_type());
+  if (value && CheckPolicySettings(policies, &errors))

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

This three-line pattern repeats a lot. Have you thought about implementing
ApplyPolicySettings in CheckPolicyValueType, calling a new
ApplyPolicyValue(const Value* value) function that subclasses of
CheckPolicyValueType can override?

[simo, 2011/09/22 11:43:26]

Done.

+    prefs->SetValue(pref_path(), value->DeepCopy());
+}
+
+// SyncPolicyHandler
+SyncPolicyHandler::SyncPolicyHandler()
+    : CheckPolicyValueType(kPolicySyncDisabled,
+                           Value::TYPE_BOOLEAN,
+                           prefs::kSyncManaged) {
+}
+
+SyncPolicyHandler::~SyncPolicyHandler() {
+}
+
+void SyncPolicyHandler::ApplyPolicySettings(PolicyMap* policies,
+                                            PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  const Value* value = policies->Get(policy_type());
+  if (value && CheckPolicySettings(policies, &errors)) {
+    bool disable_sync;
+    value->GetAsBoolean(&disable_sync);

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

you should check the return value of GetAsBoolean as well.

[simo, 2011/09/22 11:43:26]

Is it enough if I do this in a DCHECK? The call to CheckPolicySettings should
check that the value has the right type.

+    if (disable_sync)
+      prefs->SetValue(prefs::kSyncManaged, value->DeepCopy());
+  }
+}
+
+// AutofillPolicyHandler
+AutofillPolicyHandler::AutofillPolicyHandler()
+    : CheckPolicyValueType(kPolicyAutoFillEnabled,
+                           Value::TYPE_BOOLEAN,
+                           prefs::kAutofillEnabled) {
+}
+
+AutofillPolicyHandler::~AutofillPolicyHandler() {
+}
+
+void AutofillPolicyHandler::ApplyPolicySettings(PolicyMap* policies,
+                                                PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  const Value* value = policies->Get(policy_type());
+  if (value && CheckPolicySettings(policies, &errors)) {
+    bool auto_fill_enabled;
+    value->GetAsBoolean(&auto_fill_enabled);

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

check return value.

[simo, 2011/09/22 11:43:26]

Done.

+    if (!auto_fill_enabled) {
+      prefs->SetValue(prefs::kAutofillEnabled,
+                      Value::CreateBooleanValue(false));
+    }
+  }
+}
+
+// DownloadDirPolicyHandler
+DownloadDirPolicyHandler::DownloadDirPolicyHandler()
+    : CheckPolicyValueType(kPolicyDownloadDirectory,
+                           Value::TYPE_STRING,
+                           prefs::kDownloadDefaultDirectory) {
+}
+
+DownloadDirPolicyHandler::~DownloadDirPolicyHandler() {
+}
+
+bool DownloadDirPolicyHandler::CheckPolicySettings(PolicyMap* policies,
+                                                   PolicyErrorMap* errors) {
+  // This policy is ignored on ChromeOS because the download path there is fixed
+  // and can not be configured by the user.
+#if !defined(OS_CHROMEOS)
+  return CheckPolicyValueType::CheckPolicySettings(policies, errors);
+#endif  // defined(OS_CHROMEOS)
+  return true;
+}
+
+void DownloadDirPolicyHandler::ApplyPolicySettings(PolicyMap* policies,
+                                                   PrefValueMap* prefs) {
+#if !defined(OS_CHROMEOS)
+  PolicyErrorMap errors;
+  const Value* value = policies->Get(policy_type());
+  if (value && CheckPolicySettings(policies, &errors)) {
+    FilePath::StringType string_value;
+    value->GetAsString(&string_value);

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

check return value.

[simo, 2011/09/22 11:43:26]

Done.

+    FilePath::StringType expanded_value =
+        policy::path_parser::ExpandPathVariables(string_value);
+    // Leaving the policy empty would revert to the default download location
+    // else we would point in an undefined location. We do this after the
+    // path expansion because it might lead to an empty string(e.g. for "\"\"").
+    if (expanded_value.empty())
+      expanded_value = download_util::GetDefaultDownloadDirectory().value();
+    prefs->SetValue(prefs::kDownloadDefaultDirectory,
+                    Value::CreateStringValue(expanded_value));
+    prefs->SetValue(prefs::kPromptForDownload,
+                    Value::CreateBooleanValue(false));
+#endif  // !defined(OS_CHROMEOS)

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

this preprocessor guard is misplaced, should move one line down.

[simo, 2011/09/22 11:43:26]

Done.

+  }
+}
+
+// DiskCacheDirPolicyHandler
+DiskCacheDirPolicyHandler::DiskCacheDirPolicyHandler()
+    : CheckPolicyValueType(kPolicyDiskCacheDir,
+                           Value::TYPE_STRING,
+                           prefs::kDiskCacheDir) {
+}
+
+DiskCacheDirPolicyHandler::~DiskCacheDirPolicyHandler() {
+}
+
+void DiskCacheDirPolicyHandler::ApplyPolicySettings(PolicyMap* policies,
+                                                    PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  const Value* value = policies->Get(policy_type());
+  if (value && CheckPolicySettings(policies, &errors)) {
+    FilePath::StringType string_value;
+    value->GetAsString(&string_value);

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

check return value.

[simo, 2011/09/22 11:43:26]

Done.

+    FilePath::StringType expanded_value =
+        policy::path_parser::ExpandPathVariables(string_value);
+    prefs->SetValue(prefs::kDiskCacheDir,
+                    Value::CreateStringValue(expanded_value));
+  }
+}
+
+// FileSelectionDialogsHandler
+FileSelectionDialogsHandler::FileSelectionDialogsHandler()
+    : CheckPolicyValueType(kPolicyAllowFileSelectionDialogs,
+                           Value::TYPE_BOOLEAN,
+                           prefs::kAllowFileSelectionDialogs) {
+}
+
+FileSelectionDialogsHandler::~FileSelectionDialogsHandler() {
+}
+
+void FileSelectionDialogsHandler::ApplyPolicySettings(PolicyMap* policies,
+                                                      PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  const Value* value = policies->Get(policy_type());
+  if (value && CheckPolicySettings(policies, &errors)) {
+    prefs->SetValue(prefs::kAllowFileSelectionDialogs, value->DeepCopy());
+    // If file-selection dialogs are not allowed we forbid the user to be
+    // prompted for the download location, since this would end up in an Infobar
+    // explaining that file-selection dialogs are forbidden anyways.
+    bool allow_file_selection_dialogs = true;
+    value->GetAsBoolean(&allow_file_selection_dialogs);
+    if (!allow_file_selection_dialogs) {
+      prefs->SetValue(prefs::kPromptForDownload,
+                      Value::CreateBooleanValue(false));
+    }
+  }
+}
+
+// BookmarksPolicyHandler
+BookmarksPolicyHandler::BookmarksPolicyHandler()
+    : CheckPolicyValueType(kPolicyBookmarkBarEnabled,
+                           Value::TYPE_BOOLEAN,
+                           prefs::kEnableBookmarkBar) {
+}
+
+BookmarksPolicyHandler::~BookmarksPolicyHandler() {
+}
+
+void BookmarksPolicyHandler::ApplyPolicySettings(PolicyMap* policies,
+                                                 PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  const Value* value = policies->Get(policy_type());
+  if (value && CheckPolicySettings(policies, &errors)) {
+    prefs->SetValue(prefs::kEnableBookmarkBar, value->DeepCopy());
+    // kShowBookmarkBar is not managed directly by a policy, but when
+    // kEnableBookmarkBar is managed, kShowBookmarkBar should be false so that
+    // the bookmarks bar either is completely disabled or only shows on the NTP.
+    // This also disables the checkbox for this preference in the prefs UI.
+    prefs->SetValue(prefs::kShowBookmarkBar, Value::CreateBooleanValue(false));
+  }
+}
+
+// IncognitoModePolicyHandler
+IncognitoModePolicyHandler::IncognitoModePolicyHandler() {
+}
+
+IncognitoModePolicyHandler::~IncognitoModePolicyHandler() {
+}
+
+bool IncognitoModePolicyHandler::CheckPolicySettings(PolicyMap* policies,
+                                                     PolicyErrorMap* errors) {
+  int int_value = IncognitoModePrefs::ENABLED;
+  std::string message;
+  const Value* availability = policies->Get(kPolicyIncognitoModeAvailability);
+  const Value* deprecated_enabled = policies->Get(kPolicyIncognitoEnabled);
+  if (availability) {
+    if (availability->GetAsInteger(&int_value)) {
+      if (!IncognitoModePrefs::IntToAvailability(int_value,
+                                                 &availability_enum_value_)) {
+        message = "IncognitoModeAvailability policy value is out of "
+                  "range " + int_value;

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

i18n

[simo, 2011/09/22 11:43:26]

Done.

+        errors->AddError(kPolicyIncognitoModeAvailability,
+                        Value::CreateStringValue(message));
+        LOG(WARNING) << message;
+        return false;
+      }
+    } else {
+      message = "IncognitoModeAvailability policy value could not be parsed";

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

i18n

[simo, 2011/09/22 11:43:26]

Done.

+      errors->AddError(kPolicyIncognitoModeAvailability,
+                     Value::CreateStringValue(message));
+      LOG(WARNING) << message;
+      return false;
+    }
+  } else {
+    // If kPolicyIncognitoModeAvailability is not specified, check the obsolete
+    // kPolicyIncognitoEnabled.

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

could move the deprecated_enabled lookup from line 262 to here.

[simo, 2011/09/22 11:43:26]

Done.

+    if (deprecated_enabled &&
+        !deprecated_enabled->IsType(Value::TYPE_BOOLEAN)) {
+      message = "IncognitoEnabled policy value could not be parsed";
+      errors->AddError(kPolicyIncognitoEnabled,

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

i18n

[simo, 2011/09/22 11:43:26]

Done.

+                     Value::CreateStringValue(message));
+      LOG(WARNING) << message;
+      return false;
+    }
+  }
+  return true;
+}
+
+void IncognitoModePolicyHandler::ApplyPolicySettings(PolicyMap* policies,
+                                                     PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  const Value* availability = policies->Get(kPolicyIncognitoModeAvailability);
+  const Value* deprecated_enabled = policies->Get(kPolicyIncognitoEnabled);
+  if (CheckPolicySettings(policies, &errors)) {
+    if (availability) {
+      prefs->SetValue(prefs::kIncognitoModeAvailability,
+                      Value::CreateIntegerValue(availability_enum_value_));

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

Ah, keeping state in the handler prevents us from reusing them. Can't we just
have an internal function that checks and extracts the value which could then
called by both ApplyPolicySettings() and CheckPolicySettings()?

[simo, 2011/09/22 11:43:26]

Ah yes, true. I think adding an method isn't really going to help here though
because the different conversions have to be done separately in
CheckPolicySettings() to generate different error messages. I will just repeat
the conversions in ApplyPolicySettings() but with DCHECKs.

+    } else if (deprecated_enabled) {
+      bool enabled = true;
+      deprecated_enabled->GetAsBoolean(&enabled);
+      prefs->SetInteger(prefs::kIncognitoModeAvailability,
+                        enabled ? IncognitoModePrefs::ENABLED :
+                                  IncognitoModePrefs::DISABLED);
+    }
+  }
+}
+
+namespace {
+
+// Implementation of SearchTermsData just for validation.
+class SearchTermsDataForValidation : public SearchTermsData {
+ public:
+  SearchTermsDataForValidation() {}
+
+  // Implementation of SearchTermsData.
+  virtual std::string GoogleBaseURLValue() const {
+    return "http://www.google.com/";
+  }
+  virtual std::string GetApplicationLocale() const {
+    return "en";
+  }
+#if defined(OS_WIN) && defined(GOOGLE_CHROME_BUILD)
+  virtual string16 GetRlzParameterValue() const {
+    return string16();
+  }
+#endif
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SearchTermsDataForValidation);
+};
+
+}  // namespace
+
+// DefaultSearchEncodingsPolicyHandler
+DefaultSearchEncodingsPolicyHandler::DefaultSearchEncodingsPolicyHandler()
+    : CheckPolicyValueType(kPolicyDefaultSearchProviderEncodings,
+                           Value::TYPE_LIST,
+                           prefs::kDefaultSearchProviderEncodings) {
+}
+
+DefaultSearchEncodingsPolicyHandler::~DefaultSearchEncodingsPolicyHandler() {
+}
+
+void DefaultSearchEncodingsPolicyHandler::ApplyPolicySettings(
+    PolicyMap* policies, PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  const Value* value = policies->Get(policy_type());
+  if (value && CheckPolicySettings(policies, &errors)) {
+    const ListValue* list;
+    value->GetAsList(&list);
+    ListValue::const_iterator iter(list->begin());
+    ListValue::const_iterator end(list->end());
+    std::string encodings;
+    for (; iter != end; ++iter) {
+      std::string s;
+      if ((*iter)->GetAsString(&s)) {
+        if (!encodings.empty())
+          encodings.push_back(';');
+        encodings.append(s);
+      } else {
+        NOTREACHED();
+      }
+    }
+    prefs->SetValue(prefs::kDefaultSearchProviderEncodings,
+                    Value::CreateStringValue(encodings));
+  }
+}
+
+// DefaultSearchPolicyHandler
+DefaultSearchPolicyHandler::DefaultSearchPolicyHandler() {
+  handlers_.push_back(new SimplePolicyHandler(
+                              kPolicyDefaultSearchProviderEnabled,
+                              Value::TYPE_BOOLEAN,
+                              prefs::kDefaultSearchProviderEnabled));
+  handlers_.push_back(new SimplePolicyHandler(
+                              kPolicyDefaultSearchProviderName,
+                              Value::TYPE_STRING,
+                              prefs::kDefaultSearchProviderName));
+  handlers_.push_back(new SimplePolicyHandler(
+                              kPolicyDefaultSearchProviderKeyword,
+                              Value::TYPE_STRING,
+                              prefs::kDefaultSearchProviderKeyword));
+  handlers_.push_back(new SimplePolicyHandler(
+                              kPolicyDefaultSearchProviderSearchURL,
+                              Value::TYPE_STRING,
+                              prefs::kDefaultSearchProviderSearchURL));
+  handlers_.push_back(new SimplePolicyHandler(
+                              kPolicyDefaultSearchProviderSuggestURL,
+                              Value::TYPE_STRING,
+                              prefs::kDefaultSearchProviderSuggestURL));
+  handlers_.push_back(new SimplePolicyHandler(
+                              kPolicyDefaultSearchProviderInstantURL,
+                              Value::TYPE_STRING,
+                              prefs::kDefaultSearchProviderInstantURL));
+  handlers_.push_back(new SimplePolicyHandler(
+                              kPolicyDefaultSearchProviderIconURL,
+                              Value::TYPE_STRING,
+                              prefs::kDefaultSearchProviderIconURL));
+  handlers_.push_back(new SimplePolicyHandler(
+                              kPolicyDefaultSearchProviderIconURL,
+                              Value::TYPE_STRING,
+                              prefs::kDefaultSearchProviderIconURL));
+  handlers_.push_back(new DefaultSearchEncodingsPolicyHandler());
+}
+
+DefaultSearchPolicyHandler::~DefaultSearchPolicyHandler() {
+  STLDeleteContainerPointers(handlers_.begin(), handlers_.end());
+  handlers_.clear();
+}
+
+bool DefaultSearchPolicyHandler::CheckPolicySettings(PolicyMap* policies,
+                                                     PolicyErrorMap* errors) {
+  if (!CheckIndividualPolicies(policies, errors))
+    return false;
+
+  if (DefaultSearchProviderIsDisabled(policies)) {
+    std::string message = "This policy will be ignored because default search "
+                          "is disabled by policy.";

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

i18n

[simo, 2011/09/22 11:43:26]

Done.

+    errors->AddError(kPolicyDefaultSearchProviderName,
+                     Value::CreateStringValue(message));
+    errors->AddError(kPolicyDefaultSearchProviderSearchURL,
+                     Value::CreateStringValue(message));
+    errors->AddError(kPolicyDefaultSearchProviderSuggestURL,
+                     Value::CreateStringValue(message));
+    errors->AddError(kPolicyDefaultSearchProviderIconURL,
+                     Value::CreateStringValue(message));
+    errors->AddError(kPolicyDefaultSearchProviderEncodings,
+                     Value::CreateStringValue(message));
+    errors->AddError(kPolicyDefaultSearchProviderKeyword,
+                     Value::CreateStringValue(message));
+    errors->AddError(kPolicyDefaultSearchProviderInstantURL,
+                     Value::CreateStringValue(message));
+    return false;
+  }
+
+  const Value* search_url =
+      policies->Get(kPolicyDefaultSearchProviderSearchURL);
+  if (!search_url) {
+    std::string message = "Search URL must be specified.";
+    errors->AddError(kPolicyDefaultSearchProviderSearchURL,
+                     Value::CreateStringValue(message));
+    return false;
+  }
+  if (DefaultSearchURLIsPresentAndValid(policies)) {
+    std::string message = "Invalid search URL.";
+    errors->AddError(kPolicyDefaultSearchProviderSearchURL,
+                     Value::CreateStringValue(message));
+    return false;
+  }
+  return true;
+}
+
+bool DefaultSearchPolicyHandler::CheckIndividualPolicies(
+    PolicyMap* policies, PolicyErrorMap* errors) {
+  HandlerList::const_iterator handler = handlers_.begin();
+  for ( ; handler != handlers_.end(); ++handler) {
+    if (!(*handler)->CheckPolicySettings(policies, errors))
+      return false;
+  }
+  return true;
+}
+
+bool DefaultSearchPolicyHandler::DefaultSearchProviderIsDisabled(
+    PolicyMap* policies) {
+  const Value* provider_enabled =
+      policies->Get(kPolicyDefaultSearchProviderEnabled);
+  bool enabled = true;
+  return provider_enabled &&
+         provider_enabled->GetAsBoolean(&enabled) &&
+         !enabled;
+}
+
+bool DefaultSearchPolicyHandler::DefaultSearchURLIsPresentAndValid(
+    PolicyMap* policies) {
+  const Value* search_url =
+      policies->Get(kPolicyDefaultSearchProviderSearchURL);
+  std::string search_url_string;
+  if (search_url) {
+    std::string search_url_string;
+    search_url->GetAsString(&search_url_string);
+    SearchTermsDataForValidation search_terms_data;
+    const TemplateURLRef search_url_ref(search_url_string, 0, 0);
+    // It must support replacement (which implies it is valid).
+    return search_url_ref.SupportsReplacementUsingTermsData(search_terms_data);
+  }
+
+  return false;
+}
+
+void DefaultSearchPolicyHandler::EnsureStringPrefExists(PrefValueMap* prefs,
+    const std::string& path) {
+  std::string value;
+  if (!prefs->GetString(path, &value))
+    prefs->SetString(path, value);
+}
+
+void DefaultSearchPolicyHandler::ClearDefaultSearchPreferences(
+    PrefValueMap* prefs) {
+  prefs->RemoveValue(prefs::kDefaultSearchProviderEnabled);
+  prefs->RemoveValue(prefs::kDefaultSearchProviderName);
+  prefs->RemoveValue(prefs::kDefaultSearchProviderKeyword);
+  prefs->RemoveValue(prefs::kDefaultSearchProviderSearchURL);
+  prefs->RemoveValue(prefs::kDefaultSearchProviderSuggestURL);
+  prefs->RemoveValue(prefs::kDefaultSearchProviderInstantURL);
+  prefs->RemoveValue(prefs::kDefaultSearchProviderIconURL);
+  prefs->RemoveValue(prefs::kDefaultSearchProviderEncodings);
+}
+
+void DefaultSearchPolicyHandler::ApplyPolicySettings(PolicyMap* policies,
+                                                     PrefValueMap* prefs) {

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

Hm, should we run CheckPolicySettings() here?

[simo, 2011/09/22 11:43:26]

Hm well, the CheckPolicySettings() for each of the handlers in |handlers| will
be run when ApplyPolicySettings is called on them. Running CheckPolicySettings()
here doesn't really help because depending on the reason the checks fail, I
might still have to do some work in ApplyPolicySettings(). That's why I made
separate methods like DefaultSearchURLIsPresentAndValid() and
DefaultSearchProviderIsDisabled(). 

+  if (DefaultSearchProviderIsDisabled(policies)) {
+    // If default search is disabled, we ignore the other fields.
+    prefs->SetString(prefs::kDefaultSearchProviderName, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderSearchURL, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderSuggestURL, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderIconURL, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderEncodings, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderKeyword, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderInstantURL, std::string());
+    return;
+  }
+
+  const Value* search_url =
+      policies->Get(kPolicyDefaultSearchProviderSearchURL);
+  // The search URL is required.
+  if (!search_url) {
+    return;
+  }
+
+  // Apply all default search policies.
+  PolicyErrorMap errors;
+  HandlerList::const_iterator handler = handlers_.begin();
+  for ( ; handler != handlers_.end(); ++handler) {
+    (*handler)->ApplyPolicySettings(policies, prefs);
+  }
+
+  // The other entries are optional.  Just make sure that they are all
+  // specified via policy, so that we don't use regular prefs.
+  if (DefaultSearchURLIsPresentAndValid(policies)) {
+    EnsureStringPrefExists(prefs, prefs::kDefaultSearchProviderSuggestURL);
+    EnsureStringPrefExists(prefs, prefs::kDefaultSearchProviderIconURL);
+    EnsureStringPrefExists(prefs, prefs::kDefaultSearchProviderEncodings);
+    EnsureStringPrefExists(prefs, prefs::kDefaultSearchProviderKeyword);
+    EnsureStringPrefExists(prefs, prefs::kDefaultSearchProviderInstantURL);
+
+    // For the name, default to the host if not specified.
+    std::string name;
+    if (!prefs->GetString(prefs::kDefaultSearchProviderName, &name) ||
+        name.empty()) {
+      const Value* search_url =
+          policies->Get(kPolicyDefaultSearchProviderSearchURL);
+      std::string search_url_string;
+      search_url->GetAsString(&search_url_string);
+      prefs->SetString(prefs::kDefaultSearchProviderName,
+                       GURL(search_url_string).host());
+    }
+
+    // And clear the IDs since these are not specified via policy.
+    prefs->SetString(prefs::kDefaultSearchProviderID, std::string());
+    prefs->SetString(prefs::kDefaultSearchProviderPrepopulateID,
+                     std::string());
+    return;
+  }
+
+  // Required entries are not there.  Remove any related entries.

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

It might be nicer to just update a private PrefValueMap here and then copy stuff
over in the end.

[simo, 2011/09/22 11:43:26]

Done.

+  ClearDefaultSearchPreferences(prefs);
+}
+
+// ProxyPolicyHandler
+ProxyPolicyHandler::ProxyPolicyHandler() {
+}
+
+ProxyPolicyHandler::~ProxyPolicyHandler() {
+}
+
+bool ProxyPolicyHandler::CheckPolicySettings(PolicyMap* policies,

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

wow > 200 lines function! Can you please break it up into logical parts?

[simo, 2011/09/22 11:43:26]

Done.

+                                             PolicyErrorMap* errors) {
+  const Value* mode = policies->Get(kPolicyProxyMode);
+  const Value* server = policies->Get(kPolicyProxyServer);
+  const Value* server_mode = policies->Get(kPolicyProxyServerMode);
+  const Value* pac_url = policies->Get(kPolicyProxyPacUrl);
+  const Value* bypass_list = policies->Get(kPolicyProxyBypassList);
+  std::string message;
+  if ((server || pac_url || bypass_list) && !(mode || server_mode)) {
+    message = "A centrally-administered policy defines proxy setting details "
+              "without setting a proxy mode.";
+    errors->AddError(kPolicyProxyMode, Value::CreateStringValue(message));
+    LOG(WARNING) << message;

[Mattias Nissler (ping if slow), 2011/09/20 13:12:25]

It seems like we want to log the errors produced by CheckPolicySettings() when
we run ApplyPolicySettings() in any case, so is there a way to move the LOG
statements to a more central place?

[simo, 2011/09/22 11:43:26]

Done.

+    return false;
+  }
+
+  // If there's a server mode, convert it into a mode.
+  bool ignoring_proxy_mode = mode && server_mode;
+  std::string mode_value;
+  if (mode) {
+    if (server_mode) {
+      message = "Both ProxyMode and ProxyServerMode policies defined, ignoring "
+                "ProxyMode.";
+      errors->AddError(kPolicyProxyMode, Value::CreateStringValue(message));
+      LOG(WARNING) << message;
+    }
+    std::string mode_value;
+    if (!mode->GetAsString(&mode_value)) {
+      message = "Invalid ProxyMode value.";
+      errors->AddError(kPolicyProxyMode, Value::CreateStringValue(message));
+      LOG(WARNING) << message;
+      return false;
+    }
+
+    ProxyPrefs::ProxyMode mode;
+    if (!ProxyPrefs::StringToProxyMode(mode_value, &mode)) {
+      message = "ProxyMode value isn't recognized";
+      errors->AddError(kPolicyProxyMode, Value::CreateStringValue(message));
+      LOG(WARNING) << "A centrally-administered policy specifies a value for "
+                      "the ProxyMode policy that isn't recognized.";
+      return false;
+    }
+
+    if (mode == ProxyPrefs::MODE_PAC_SCRIPT && !pac_url) {
+      message = "Missing PAC script URL.";
+      errors->AddError(kPolicyProxyPacUrl, Value::CreateStringValue(message));
+      LOG(WARNING) << "A centrally-administered policy specifies to use a PAC "
+                      "script, but doesn't supply the PAC script URL.";
+      return false;
+    } else if (mode == ProxyPrefs::MODE_FIXED_SERVERS && !server) {
+      message = "Missing server address.";
+      errors->AddError(kPolicyProxyServer, Value::CreateStringValue(message));
+      LOG(WARNING) << "A centrally-administered policy specifies to use a "
+                      "fixed server, but doesn't supply the server address.";
+      return false;
+    }
+  } else if (server_mode) {
+    int server_mode_value;
+    if (!server_mode->GetAsInteger(&server_mode_value)) {
+      message = "Invalid ProxyServerMode value.";
+      errors->AddError(kPolicyProxyServerMode,
+                       Value::CreateStringValue(message));
+      LOG(WARNING) << message;
+      return false;
+    }
+
+    switch (server_mode_value) {
+      case kPolicyNoProxyServerMode:
+        mode_value = ProxyPrefs::kDirectProxyModeName;
+        break;
+      case kPolicyAutoDetectProxyServerMode:
+        mode_value = ProxyPrefs::kAutoDetectProxyModeName;
+        break;
+      case kPolicyManuallyConfiguredProxyServerMode:
+        if (server && pac_url) {
+          message  = "Both fixed proxy servers and a .pac url are specified.";
+          errors->AddError(
+              kPolicyProxyServer, Value::CreateStringValue(message));
+          errors->AddError(
+              kPolicyProxyPacUrl, Value::CreateStringValue(message));
+          LOG(WARNING) << "A centrally-administered policy dictates that both "
+                          "fixed proxy servers and a .pac url should be used "
+                          "for proxy configuration.";
+          return false;
+        }
+        if (!server && !pac_url) {
+          message = "Neither fixed proxy servers nor a .pac url are specified.";
+          errors->AddError(
+              kPolicyProxyServer, Value::CreateStringValue(message));
+          errors->AddError(
+              kPolicyProxyPacUrl, Value::CreateStringValue(message));
+          LOG(WARNING) << "A centrally-administered policy dictates that "
+                          "the proxy settings should use either fixed proxy "
+                          "servers or a .pac url, but specifies neither.";
+          return false;
+        }
+        if (pac_url)
+          mode_value = ProxyPrefs::kPacScriptProxyModeName;
+        else
+          mode_value = ProxyPrefs::kFixedServersProxyModeName;
+        break;
+      case kPolicyUseSystemProxyServerMode:
+        mode_value = ProxyPrefs::kSystemProxyModeName;
+        break;
+      default:
+        // TODO(simo) map server_mode_value to string.
+        message = "Invalid proxy mode " + server_mode_value;
+        errors->AddError(kPolicyProxyServer, Value::CreateStringValue(message));
+        LOG(WARNING) << message;
+        return false;
+    }
+  }
+
+  // If neither ProxyMode nor ProxyServerMode are specified, mode_value will be
+  // empty and the proxy shouldn't be configured at all.
+  if (mode_value.empty())
+    return true;
+
+  if (mode_value == ProxyPrefs::kDirectProxyModeName) {
+    if (server || pac_url || bypass_list) {
+      message = "Use of a proxy is disabled but an explicit proxy "
+                "configuration is specified.";
+      if (server)
+        errors->AddError(kPolicyProxyServer, Value::CreateStringValue(message));
+      if (bypass_list)
+        errors->AddError(
+            kPolicyProxyBypassList, Value::CreateStringValue(message));
+      if (pac_url)
+        errors->AddError(
+            kPolicyProxyPacUrl, Value::CreateStringValue(message));
+
+      LOG(WARNING) << "A centrally-administered policy disables the use of a "
+                      "proxy but also specifies an explicit proxy "
+                      "configuration.";
+      return false;
+    }
+  } else if (mode_value == ProxyPrefs::kAutoDetectProxyModeName) {
+    if (server || bypass_list || pac_url) {
+      message = "Proxy is set to auto configured.";
+      if (server)
+        errors->AddError(kPolicyProxyServer, Value::CreateStringValue(message));
+      if (bypass_list)
+        errors->AddError(
+            kPolicyProxyBypassList, Value::CreateStringValue(message));
+      if (pac_url)
+        errors->AddError(
+            kPolicyProxyPacUrl, Value::CreateStringValue(message));
+
+      LOG(WARNING) << "A centrally-administered policy dictates that a proxy "
+                      "shall be auto configured but specifies fixed proxy "
+                      "servers, a by-pass list or a .pac script URL.";
+      return false;
+    }
+  } else if (mode_value == ProxyPrefs::kPacScriptProxyModeName) {
+    if (server|| bypass_list) {
+      message = "Proxy configuration is set to use a .pac script URL.";
+      if (server)
+        errors->AddError(kPolicyProxyServer, Value::CreateStringValue(message));
+      if (bypass_list)
+        errors->AddError(
+            kPolicyProxyBypassList, Value::CreateStringValue(message));
+      LOG(WARNING) << "A centrally-administered policy dictates that a .pac "
+                      ".pac script URL should be used for proxy configuration "
+                      "configuration but also specifies policies only for "
+                      "fixed proxy servers.";
+      return false;
+    }
+  } else if (mode_value == ProxyPrefs::kFixedServersProxyModeName) {
+    if (pac_url) {
+      message = "Proxy is set to use fixed proxy servers";
+      errors->AddError(
+            kPolicyProxyPacUrl, Value::CreateStringValue(message));
+      LOG(WARNING) << "A centrally-administered policy dictates that fixed "
+                      "fixed proxy servers should be used but also specifies "
+                      "a .pac script URL.";
+      return false;
+    }
+  } else if (mode_value == ProxyPrefs::kSystemProxyModeName) {
+    if (server || pac_url || bypass_list) {
+      message = "System proxy settings are set to be used but an explicit proxy"
+                "configuration is also specified.";
+      if (server)
+        errors->AddError(kPolicyProxyServer, Value::CreateStringValue(message));
+      if (bypass_list)
+        errors->AddError(
+            kPolicyProxyBypassList, Value::CreateStringValue(message));
+      if (pac_url)
+        errors->AddError(
+            kPolicyProxyPacUrl, Value::CreateStringValue(message));
+
+      LOG(WARNING) << "A centrally-administered policy dictates that the "
+                      "system proxy settings should be used but also "
+                      "specifies an explicit proxy configuration.";
+      return false;
+    }
+  } else {
+    // TODO (simo) map to string.
+    message = "Invalid proxy mode " + mode_value;
+    if (ignoring_proxy_mode)
+      errors->AddError(kPolicyProxyServerMode,
+                       Value::CreateStringValue(message));
+    else
+      errors->AddError(kPolicyProxyMode, Value::CreateStringValue(message));
+
+    LOG(WARNING) << message;
+    return false;
+  }
+  return true;
+}
+
+void ProxyPolicyHandler::ApplyPolicySettings(PolicyMap* policies,
+                                             PrefValueMap* prefs) {
+  PolicyErrorMap errors;
+  if (!CheckPolicySettings(policies, &errors))
+    return;
+
+  const Value* mode = policies->Get(kPolicyProxyMode);
+  const Value* server = policies->Get(kPolicyProxyServer);
+  const Value* server_mode = policies->Get(kPolicyProxyServerMode);
+  const Value* pac_url = policies->Get(kPolicyProxyPacUrl);
+  const Value* bypass_list = policies->Get(kPolicyProxyBypassList);
+
+  ProxyPrefs::ProxyMode proxy_mode;
+  if (mode) {
+    std::string string_mode;
+    mode->GetAsString(&string_mode);
+    ProxyPrefs::StringToProxyMode(string_mode, &proxy_mode);
+  } else if (server_mode) {
+    int int_mode = 0;
+    server_mode->GetAsInteger(&int_mode);
+    switch (int_mode) {
+      case kPolicyNoProxyServerMode:
+        proxy_mode = ProxyPrefs::MODE_DIRECT;
+        break;
+      case kPolicyAutoDetectProxyServerMode:
+        proxy_mode = ProxyPrefs::MODE_AUTO_DETECT;
+        break;
+      case kPolicyManuallyConfiguredProxyServerMode:
+        proxy_mode = ProxyPrefs::MODE_FIXED_SERVERS;
+        if (pac_url)
+          proxy_mode = ProxyPrefs::MODE_PAC_SCRIPT;
+        break;
+      case kPolicyUseSystemProxyServerMode:
+        proxy_mode = ProxyPrefs::MODE_SYSTEM;
+        break;
+      default:
+        proxy_mode = ProxyPrefs::MODE_DIRECT;
+        NOTREACHED();
+    }
+  } else {
+    return;
+  }
+  switch (proxy_mode) {
+    case ProxyPrefs::MODE_DIRECT:
+      prefs->SetValue(prefs::kProxy, ProxyConfigDictionary::CreateDirect());
+      break;
+    case ProxyPrefs::MODE_AUTO_DETECT:
+      prefs->SetValue(prefs::kProxy, ProxyConfigDictionary::CreateAutoDetect());
+      break;
+    case ProxyPrefs::MODE_PAC_SCRIPT: {
+      std::string pac_url_string;
+      pac_url->GetAsString(&pac_url_string);
+      prefs->SetValue(prefs::kProxy,
+          ProxyConfigDictionary::CreatePacScript(pac_url_string, false));
+      break;
+    }
+    case ProxyPrefs::MODE_FIXED_SERVERS: {
+      std::string proxy_server;
+      server->GetAsString(&proxy_server);
+      std::string bypass_list_string;
+      if (bypass_list)
+        bypass_list->GetAsString(&bypass_list_string);
+      prefs->SetValue(prefs::kProxy,
+                      ProxyConfigDictionary::CreateFixedServers(
+                          proxy_server, bypass_list_string));
+      break;
+    }
+    case ProxyPrefs::MODE_SYSTEM:
+      prefs->SetValue(prefs::kProxy,
+                      ProxyConfigDictionary::CreateSystem());
+      break;
+    case ProxyPrefs::kModeCount:
+      NOTREACHED();
+  }
+}
+
+}  // namespace policy