Introduce a separate preference for 'proxy server mode'

chrome/browser/policy/configuration_policy_pref_store.cc

Index: chrome/browser/policy/configuration_policy_pref_store.cc
diff --git a/chrome/browser/policy/configuration_policy_pref_store.cc b/chrome/browser/policy/configuration_policy_pref_store.cc
index c777e35c701e0c93c9514f79d2ecff12add852a6..debb32792f730c1eba4cd7ec2fe4ca7b9f8f7de8 100644
--- a/chrome/browser/policy/configuration_policy_pref_store.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store.cc
@@ -12,6 +12,7 @@
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
+#include "chrome/browser/prefs/proxy_prefs.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/policy/configuration_policy_provider.h"
 #if defined(OS_WIN)
@@ -242,7 +243,7 @@ ConfigurationPolicyPrefStore::GetChromePolicyDefinitionList() {
       key::kDefaultSearchProviderIconURL },
     { kPolicyDefaultSearchProviderEncodings, Value::TYPE_STRING,
       key::kDefaultSearchProviderEncodings },
-    { kPolicyProxyServerMode, Value::TYPE_INTEGER, key::kProxyServerMode },
+    { kPolicyProxyMode, Value::TYPE_INTEGER, key::kProxyMode },
     { kPolicyProxyServer, Value::TYPE_STRING, key::kProxyServer },
     { kPolicyProxyPacUrl, Value::TYPE_STRING, key::kProxyPacUrl },
     { kPolicyProxyBypassList, Value::TYPE_STRING, key::kProxyBypassList },
@@ -321,10 +322,7 @@ ConfigurationPolicyPrefStore::ConfigurationPolicyPrefStore(
     ConfigurationPolicyProvider* provider)
     : provider_(provider),
       prefs_(new DictionaryValue()),
-      lower_priority_proxy_settings_overridden_(false),
-      proxy_disabled_(false),
-      proxy_configuration_specified_(false),
-      use_system_proxy_(false) {
+      error_in_proxy_settings_(false) {
   if (!provider_->Provide(this))
     LOG(WARNING) << "Failed to get policy from provider.";
   FinalizeDefaultSearchPolicySettings();
@@ -404,17 +402,6 @@ ConfigurationPolicyPrefStore::CreateRecommendedPolicyPrefStore() {
       g_configuration_policy_provider_keeper.Get().recommended_provider());
 }
 
-// static
-void ConfigurationPolicyPrefStore::GetProxyPreferenceSet(
-    ProxyPreferenceSet* proxy_pref_set) {
-  proxy_pref_set->clear();
-  for (size_t current = 0; current < arraysize(kProxyPolicyMap); ++current) {
-    proxy_pref_set->insert(kProxyPolicyMap[current].preference_path);
-  }
-  proxy_pref_set->insert(prefs::kNoProxyServer);
-  proxy_pref_set->insert(prefs::kProxyAutoDetect);
-}
-
 const ConfigurationPolicyPrefStore::PolicyToPreferenceMapEntry*
 ConfigurationPolicyPrefStore::FindPolicyInMap(
     ConfigurationPolicyType policy,
@@ -458,108 +445,177 @@ bool ConfigurationPolicyPrefStore::ApplyPolicyFromMap(
 bool ConfigurationPolicyPrefStore::ApplyProxyPolicy(
     ConfigurationPolicyType policy,
     Value* value) {
-  bool result = false;
-  bool warn_about_proxy_disable_config = false;
-  bool warn_about_proxy_system_config = false;
-
-  const PolicyToPreferenceMapEntry* match_entry =
-      FindPolicyInMap(policy, kProxyPolicyMap, arraysize(kProxyPolicyMap));
-
-  // When the first proxy-related policy is applied, ALL proxy-related
-  // preferences that have been set by command-line switches, extensions,
-  // user preferences or any other mechanism are overridden. Otherwise
-  // it's possible for a user to interfere with proxy policy by setting
-  // proxy-related command-line switches or set proxy-related prefs in an
-  // extension that are related, but not identical, to the ones set through
-  // policy.
-  if (!lower_priority_proxy_settings_overridden_ &&
-      (match_entry ||
-       policy == kPolicyProxyServerMode)) {
-    ProxyPreferenceSet proxy_preference_set;
-    GetProxyPreferenceSet(&proxy_preference_set);
-    for (ProxyPreferenceSet::const_iterator i = proxy_preference_set.begin();
-         i != proxy_preference_set.end(); ++i) {
-      // We use values of TYPE_NULL to mark preferences for which
-      // READ_USE_DEFAULT should be returned by GetValue().
-      prefs_->Set(*i, Value::CreateNullValue());
-    }
-    lower_priority_proxy_settings_overridden_ = true;
+  // We only collect the values until we have sufficient information when
+  // Completed() is called to determine whether the presented values were
+  // correct and apply them in that case.

[Mattias Nissler (ping if slow), 2010/12/21 15:54:29]

Why can't we just write to the real dictionary instead of the containers and
sanitize everything later?

[battre (please use the other), 2010/12/21 20:14:09]

This would mean that we need some rollback mechanism. Besides this, we want an
atomic commit soon anyway (upcoming CL). There we want to replace the
configuration as one dictionary at once.

[Mattias Nissler (ping if slow), 2010/12/22 10:22:11]

The rollback mechanism is delete-all-proxy-prefs-from-the-map. This is what's
already done for default search providers in
FinalizeDefaultSearchProviderSettings() and I feel we should be consistent, no?

[battre, 2010/12/22 14:41:16]

Well, the point is that eventually we want a single
prefs_->Set("proxy", proxy_dictionary)-alike call to set all proxy settings
atomically. I feel that the current code is closer to this than a
delete-all-proxy-prefs-from-the-map.

[Mattias Nissler (ping if slow), 2010/12/22 15:26:15]

OK, but if you want to keep the evaluate-later approach, please use a
std::map<ConfigurationPolicyType, Value*> for caching the values instead of the
tedious switching here.

[battre, 2010/12/22 16:15:12]

Done.

+  scoped_ptr<Value>* target_container = NULL;
+  switch (policy) {
+  case kPolicyProxyMode:

[danno, 2010/12/21 16:16:38]

Indenting is not right here:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

[battre (please use the other), 2010/12/21 20:14:09]

Done.

+    target_container = &proxy_mode_container_;
+    break;
+  case kPolicyProxyServer:
+    target_container = &proxy_server_container_;
+    break;
+  case kPolicyProxyPacUrl:
+    target_container = &proxy_pac_url_container_;
+    break;
+  case kPolicyProxyBypassList:
+    target_container = &proxy_bypass_list_container_;
+    break;
+  default:
+    // We are not interested in this policy.
+    return false;
+  }
+  CHECK(target_container);
+
+  if (error_in_proxy_settings_) {
+    delete value;
+    return true;
+  }
+
+  if ((*target_container).get()) {
+    LOG(ERROR) << "Ignoring proxy policies because a policy was defined twice.";

[Mattias Nissler (ping if slow), 2010/12/21 15:54:29]

This is not what we want. The idea is that you can call Apply() from within
Provide() as often as you want. The state should only be examined and
error-checked once Provide() returns.

[battre (please use the other), 2010/12/21 20:14:09]

Done.

[Mattias Nissler (ping if slow), 2010/12/22 10:22:11]

Not done?

[battre, 2010/12/22 14:41:16]

Sorry. I deleted some other code related to this but forgot the main location.
Not it is really done. :-)

+    delete value;
+    return true;
+  }
+
+  (*target_container).reset(value);
+  return true;
+}
+
+void ConfigurationPolicyPrefStore::Completed() {

[Mattias Nissler (ping if slow), 2010/12/21 15:54:29]

Note that for default search, we just call FinalizeDefaultSearchSettings() after
the provide call. I think that's the better solution, since clients should not
need to worry about calling Completed() at the right times.

[danno, 2010/12/21 16:16:38]

I am not sure I agree. I like that there's a method the provider must call when
it's done providing to finish. FinalizeDefaultSearchPolicySettings should
actually be called from in here.

On 2010/12/21 15:54:29, Mattias Nissler wrote:

[Mattias Nissler (ping if slow), 2010/12/21 16:22:41]

My rationale for opposing the idea is this: We'd add a function to an interface
declaration for the sole benefit of one implementor, namely
ConfigurationPolicyPrefStore, even if this implementor technically doesn't
require the extended interface. Thus, we'd make life harder for clients for no
good reason. I don't like the sprinkling of Completed() over the code much,
either.

On 2010/12/21 16:16:38, danno wrote:

[battre (please use the other), 2010/12/21 20:14:09]

I checked the code again and agree with Mattias. His proposal is quite elegant
and ConfigurationPolicyPrefStore::FinalizeDefaultSearchPolicySettings does
pretty much the same thing.

Done.

+  if (CheckProxySettings())
+    ApplyProxySettings();
+  proxy_mode_container_.reset(NULL);
+  proxy_server_container_.reset(NULL);
+  proxy_pac_url_container_.reset(NULL);
+  proxy_bypass_list_container_.reset(NULL);
+}
+
+bool ConfigurationPolicyPrefStore::CheckProxySettings() {
+  if (error_in_proxy_settings_)
+    return false;
+
+  bool mode = proxy_mode_container_.get() &&
+              !proxy_mode_container_->IsType(Value::TYPE_NULL);
+  bool server = proxy_server_container_.get() &&
+                !proxy_server_container_->IsType(Value::TYPE_NULL);
+  bool pac_url = proxy_pac_url_container_.get() &&
+                 !proxy_pac_url_container_->IsType(Value::TYPE_NULL);
+  bool bypass_list = proxy_bypass_list_container_.get() &&
+                     !proxy_bypass_list_container_->IsType(Value::TYPE_NULL);
+
+  if ((server || pac_url || bypass_list) && !mode) {
+    LOG(WARNING) << "A centrally-administered policy defines proxy setting"
+                 << " details without setting a proxy mode.";
+    return false;
+  }
+
+  if (!mode)
+    return true;
+
+  int mode_value;
+  if (!proxy_mode_container_->GetAsInteger(&mode_value)) {
+    LOG(WARNING) << "Invalid proxy mode value.";
+    return false;
   }
 
-  // Translate the proxy policy into preferences.
-  if (policy == kPolicyProxyServerMode) {
-    int int_value;
-    bool proxy_auto_detect = false;
-    if (value->GetAsInteger(&int_value)) {
-      result = true;
-      switch (int_value) {
-        case kPolicyNoProxyServerMode:
-          if (!proxy_disabled_) {
-            if (proxy_configuration_specified_)
-              warn_about_proxy_disable_config = true;
-            proxy_disabled_ = true;
-          }
-          break;
-        case kPolicyAutoDetectProxyMode:
-          proxy_auto_detect = true;
-          break;
-        case kPolicyManuallyConfiguredProxyMode:
-          break;
-        case kPolicyUseSystemProxyMode:
-          if (!use_system_proxy_) {
-            if (proxy_configuration_specified_)
-              warn_about_proxy_system_config = true;
-            use_system_proxy_ = true;
-          }
-          break;
-        default:
-          // Not a valid policy, don't assume ownership of |value|
-          result = false;
-          break;
-      }
-
-      if (int_value != kPolicyUseSystemProxyMode) {
-        prefs_->Set(prefs::kNoProxyServer,
-                    Value::CreateBooleanValue(proxy_disabled_));
-        prefs_->Set(prefs::kProxyAutoDetect,
-                    Value::CreateBooleanValue(proxy_auto_detect));
-      }
+  switch (mode_value) {
+  case kPolicyNoProxyServerMode:
+    if (server || pac_url || bypass_list) {
+      LOG(WARNING) << "A centrally-administered policy disables the use of"
+                   << " a proxy but also specifies an explicit proxy"
+                   << " configuration.";
+      return false;
     }
-  } else if (match_entry) {
-    // Determine if the applied proxy policy settings conflict and issue
-    // a corresponding warning if they do.
-    if (!proxy_configuration_specified_) {
-      if (proxy_disabled_)
-        warn_about_proxy_disable_config = true;
-      if (use_system_proxy_)
-        warn_about_proxy_system_config = true;
-      proxy_configuration_specified_ = true;
+    break;
+  case kPolicyAutoDetectProxyMode:
+    if (server || bypass_list) {
+      LOG(WARNING) << "A centrally-administered policy dictates that a proxy"
+                   << " shall be auto configured but specifies fixed proxy"
+                   << " servers or a by-pass list.";
+      return false;
     }
-    if (!use_system_proxy_ && !proxy_disabled_) {
-      prefs_->Set(match_entry->preference_path, value);
-      // The ownership of value has been passed on to |prefs_|,
-      // don't clean it up later.
-      value = NULL;
+    break;
+  case kPolicyManuallyConfiguredProxyMode:
+    if (!server) {
+      LOG(WARNING) << "A centrally-administered policy dictates that the"
+                   << " system proxy settings should use fixed proxy servers"
+                   << " without specifying which ones.";
+      return false;
     }
-    result = true;
+    if (pac_url) {
+      LOG(WARNING) << "A centrally-administered policy dictates that the"
+                   << " system proxy settings should use fixed proxy servers"
+                   << " but also specifies a PAC script.";
+      return false;
+    }
+    break;
+  case kPolicyUseSystemProxyMode:
+    if (server || pac_url || bypass_list) {
+      LOG(WARNING) << "A centrally-administered policy dictates that the"
+                   << " system proxy settings should be used but also specifies"
+                   << " an explicit proxy configuration.";
+      return false;
+    }
+    break;
+  default:
+    LOG(WARNING) << "Invalid proxy mode " << mode_value;
+    return false;
   }
+  return true;
+}
 
-  if (warn_about_proxy_disable_config) {
-    LOG(WARNING) << "A centrally-administered policy disables the use of"
-                 << " a proxy but also specifies an explicit proxy"
-                 << " configuration.";
-  }
+void ConfigurationPolicyPrefStore::ApplyProxySettings() {
+  if (proxy_mode_container_.get() == NULL ||
+      proxy_mode_container_->IsType(Value::TYPE_NULL))
+    return;
 
-  if (warn_about_proxy_system_config) {
-    LOG(WARNING) << "A centrally-administered policy dictates that the"
-                 << " system proxy settings should be used but also specifies"
-                 << " an explicit proxy configuration.";
+  int int_mode;
+  CHECK(proxy_mode_container_->GetAsInteger(&int_mode));
+  ProxyPrefs::ProxyMode mode;
+  switch (int_mode) {
+  case kPolicyNoProxyServerMode:
+    mode = ProxyPrefs::DISABLED;
+    break;
+  case kPolicyAutoDetectProxyMode:
+    mode = ProxyPrefs::AUTO_DETECT;
+    if (proxy_pac_url_container_.get() &&
+        !proxy_pac_url_container_->IsType(Value::TYPE_NULL)) {
+      mode = ProxyPrefs::PAC_SCRIPT;
+    }
+    break;
+  case kPolicyManuallyConfiguredProxyMode:
+    mode = ProxyPrefs::FIXED_SERVERS;
+    break;
+  case kPolicyUseSystemProxyMode:
+    mode = ProxyPrefs::SYSTEM;
+    break;
+  default:
+    mode = ProxyPrefs::DISABLED;
+    NOTREACHED();
   }
+  prefs_->Set(prefs::kProxyMode, Value::CreateIntegerValue(mode));
 
-  // If the policy was a proxy policy, cleanup |value|.
-  if (result && value)
-    delete value;
-  return result;
+  if (proxy_server_container_.get() &&
+      !proxy_server_container_->IsType(Value::TYPE_NULL)) {
+    prefs_->Set(prefs::kProxyServer, proxy_server_container_.release());
+  } else {
+    prefs_->Set(prefs::kProxyServer, Value::CreateNullValue());
+  }
+  if (proxy_pac_url_container_.get() &&
+      !proxy_pac_url_container_->IsType(Value::TYPE_NULL)) {
+    prefs_->Set(prefs::kProxyPacUrl, proxy_pac_url_container_.release());
+  } else {
+    prefs_->Set(prefs::kProxyPacUrl, Value::CreateNullValue());
+  }
+  if (proxy_bypass_list_container_.get() &&
+      !proxy_bypass_list_container_->IsType(Value::TYPE_NULL)) {
+    prefs_->Set(prefs::kProxyBypassList,
+                proxy_bypass_list_container_.release());
+  } else {
+    prefs_->Set(prefs::kProxyBypassList, Value::CreateNullValue());
+  }
 }
 
 bool ConfigurationPolicyPrefStore::ApplySyncPolicy(