Introduce a separate preference for 'proxy server mode'

chrome/browser/policy/configuration_policy_pref_store.cc

 // Copyright (c) 2010 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_pref_store.h"
 
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/string16.h"
 #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)
 #include "chrome/browser/policy/configuration_policy_provider_win.h"
 #elif defined(OS_MACOSX)
 #include "chrome/browser/policy/configuration_policy_provider_mac.h"
 #elif defined(OS_POSIX)
 #include "chrome/browser/policy/config_dir_policy_provider.h"
 #endif
 #include "chrome/browser/policy/device_management_policy_provider.h"
@@ skipping 210 matching lines @@
     { kPolicyDefaultSearchProviderKeyword, Value::TYPE_STRING,
       key::kDefaultSearchProviderKeyword },
     { kPolicyDefaultSearchProviderSearchURL, Value::TYPE_STRING,
       key::kDefaultSearchProviderSearchURL },
     { kPolicyDefaultSearchProviderSuggestURL, Value::TYPE_STRING,
       key::kDefaultSearchProviderSuggestURL },
     { kPolicyDefaultSearchProviderIconURL, Value::TYPE_STRING,
       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 },
     { kPolicyAlternateErrorPagesEnabled, Value::TYPE_BOOLEAN,
       key::kAlternateErrorPagesEnabled },
     { kPolicySearchSuggestEnabled, Value::TYPE_BOOLEAN,
       key::kSearchSuggestEnabled },
     { kPolicyDnsPrefetchingEnabled, Value::TYPE_BOOLEAN,
       key::kDnsPrefetchingEnabled },
     { kPolicyDisableSpdy, Value::TYPE_BOOLEAN, key::kDisableSpdy },
@@ skipping 58 matching lines @@
     entries,
     entries + arraysize(entries),
   };
   return &policy_list;
 }
 
 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();
 }
 
 ConfigurationPolicyPrefStore::~ConfigurationPolicyPrefStore() {}
 
 PrefStore::ReadResult ConfigurationPolicyPrefStore::GetValue(
     const std::string& key,
     Value** value) const {
@@ skipping 59 matching lines @@
   return new ConfigurationPolicyPrefStore(provider);
 }
 
 // static
 ConfigurationPolicyPrefStore*
 ConfigurationPolicyPrefStore::CreateRecommendedPolicyPrefStore() {
   return new ConfigurationPolicyPrefStore(
       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,
     const PolicyToPreferenceMapEntry* map,
     int table_size) const {
   for (int i = 0; i < table_size; ++i) {
     if (map[i].policy_type == policy)
       return map + i;
   }
   return NULL;
@@ skipping 23 matching lines @@
       prefs_->Set(map[current].preference_path, value);
       return true;
     }
   }
   return false;
 }
 
 bool ConfigurationPolicyPrefStore::ApplyProxyPolicy(
     ConfigurationPolicyType policy,
     Value* value) {
-  bool result = false;
-  bool warn_about_proxy_disable_config = false;
-  bool warn_about_proxy_system_config = false;
+  // 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);
 
-  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;
+  if (error_in_proxy_settings_) {
+    delete value;
+    return true;
   }
 
-  // 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));
-      }
-    }
-  } 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;
-    }
-    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;
-    }
-    result = 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;
   }
 
-  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.";
+  (*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 (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.";
+  if (!mode)
+    return true;
+
+  int mode_value;
+  if (!proxy_mode_container_->GetAsInteger(&mode_value)) {
+    LOG(WARNING) << "Invalid proxy mode value.";
+    return false;
   }
 
-  // If the policy was a proxy policy, cleanup |value|.
-  if (result && value)
-    delete value;
-  return result;
+  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;
+    }
+    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;
+    }
+    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;
+    }
+    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;
+}
+
+void ConfigurationPolicyPrefStore::ApplyProxySettings() {
+  if (proxy_mode_container_.get() == NULL ||
+      proxy_mode_container_->IsType(Value::TYPE_NULL))
+    return;
+
+  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 (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(
     ConfigurationPolicyType policy, Value* value) {
   if (policy == kPolicySyncDisabled) {
     bool disable_sync;
     if (value->GetAsBoolean(&disable_sync) && disable_sync)
       prefs_->Set(prefs::kSyncManaged, value);
     else
       delete value;
@@ skipping 87 matching lines @@
                         std::string());
       return;
     }
   }
   // Required entries are not there.  Remove any related entries.
   RemovePreferencesOfMap(kDefaultSearchPolicyMap,
                          arraysize(kDefaultSearchPolicyMap));
 }
 
 }  // namespace policy