Intorduce 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 289 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) {
+      lower_priority_proxy_settings_overridden_(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 20 matching lines @@
           << "mismatch in provided and expected policy value for preferences"
           << map[current].preference_path << ". expected = "
           << map[current].value_type << ", actual = "<< value->GetType();
       prefs_->Set(map[current].preference_path, value);
       return true;
     }
   }
   return false;
 }
 
+bool ConfigurationPolicyPrefStore::NonNullValueExists(const std::string& path) {
+  Value* value = NULL;
+  if (!prefs_->Get(path, &value))
+    return false;
+  return value != NULL && !value->IsType(Value::TYPE_NULL);
+}
+
 bool ConfigurationPolicyPrefStore::ApplyProxyPolicy(
     ConfigurationPolicyType policy,
     Value* value) {
-  bool result = false;
-  bool warn_about_proxy_disable_config = false;
-  bool warn_about_proxy_system_config = false;
+
+  // Indicates if the specified proxy server mode conflicts with other proxy
+  // preferences.
+  bool conflict = false;
+  // If |conflict| is true, then this variable specifies the selected proxy
+  // server mode that lead to the conflict.

[Mattias Nissler (ping if slow), 2010/12/20 13:34:03]

s/lead/led/

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

Gone.

+  ProxyPrefs::ProxyServerMode conflicting_mode = ProxyPrefs::MANUAL;
+
+  std::string preference_path;

[Mattias Nissler (ping if slow), 2010/12/20 13:34:03]

Could use a comment saying what this is used for.

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

Gone.

 
   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) {
+      (match_entry || policy == kPolicyProxyServerMode)) {
+    for (size_t current = 0; current < arraysize(kProxyPolicyMap); ++current) {
       // We use values of TYPE_NULL to mark preferences for which
       // READ_USE_DEFAULT should be returned by GetValue().
-      prefs_->Set(*i, Value::CreateNullValue());
+      prefs_->Set(kProxyPolicyMap[current].preference_path,
+                  Value::CreateNullValue());
     }
+    prefs_->Set(prefs::kProxyServerMode, Value::CreateNullValue());
     lower_priority_proxy_settings_overridden_ = true;
   }
 
-  // Translate the proxy policy into preferences.
   if (policy == kPolicyProxyServerMode) {
+    preference_path = prefs::kProxyServerMode;
+
+    // Don't claim ownership of the policy if we find it invalid.
     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 (!value->GetAsInteger(&int_value))
+      return false;
+    ProxyPrefs::ProxyServerMode mode;
+    if (!ProxyPrefs::IntToProxyMode(int_value, &mode))
+      return false;
 
-      if (int_value != kPolicyUseSystemProxyMode) {
-        prefs_->Set(prefs::kNoProxyServer,
-                    Value::CreateBooleanValue(proxy_disabled_));
-        prefs_->Set(prefs::kProxyAutoDetect,
-                    Value::CreateBooleanValue(proxy_auto_detect));
+    // Determine if the applied proxy policy settings conflict and issue
+    // a corresponding warning if they do.
+    if (mode == ProxyPrefs::DISABLED || mode == ProxyPrefs::SYSTEM) {
+      for (size_t current = 0; current < arraysize(kProxyPolicyMap);
+           ++current) {
+        if (NonNullValueExists(kProxyPolicyMap[current].preference_path)) {
+          conflict = true;
+          conflicting_mode = mode;
+        }
       }
     }
   } else if (match_entry) {
+    // Any proxy policy other than kPolicyProxyServerMode.
+
+    preference_path = match_entry->preference_path;
+
     // 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;
+    int int_mode = 0;
+    if (prefs_->GetInteger(prefs::kProxyServerMode, &int_mode)) {
+      if (ProxyPrefs::IntToProxyMode(int_mode, &conflicting_mode)) {
+        if (conflicting_mode == ProxyPrefs::DISABLED ||
+            conflicting_mode == ProxyPrefs::SYSTEM) {
+          conflict = 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;
+  } else {
+    return false;
   }
 
-  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.";
+  // We accept |value| as a policy and take ownership of it.
+  if (conflict) {
+    delete value;
+
+    if (conflicting_mode == ProxyPrefs::DISABLED) {
+      LOG(WARNING) << "A centrally-administered policy disables the use of"
+                   << " a proxy but also specifies an explicit proxy"
+                   << " configuration.";
+    } else if (conflicting_mode == ProxyPrefs::SYSTEM) {
+      LOG(WARNING) << "A centrally-administered policy dictates that the"
+                   << " system proxy settings should be used but also specifies"
+                   << " an explicit proxy configuration.";
+    } else {
+      NOTREACHED() << "Unexpected reason for a proxy policy conflict.";
+    }

[Mattias Nissler (ping if slow), 2010/12/20 13:34:03]

Hm, this whole logic for detecting we have a conflict is really cumbersome and
hard to understand. Maybe a two-step approach would be better? I'm thinking
along the lines of applying whatever we get fed in Apply() and then making the
sanity checks and printing the warning after Provide() is done? This would make
it obvious that the code behaves consistently no matter what order we Apply()
policies in.

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

I thought the same and implemented a Completed() method that is called by
Provide() at the end.

Done.

+  } else {
+    prefs_->Set(preference_path, value);
   }
 
-  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 the policy was a proxy policy, cleanup |value|.
-  if (result && value)
-    delete value;
-  return result;
+  return true;
 }
 
 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