Add cr_policy_indicator for settings controls

chrome/browser/extensions/api/settings_private/prefs_util.cc

Index: chrome/browser/extensions/api/settings_private/prefs_util.cc
diff --git a/chrome/browser/extensions/api/settings_private/prefs_util.cc b/chrome/browser/extensions/api/settings_private/prefs_util.cc
index 20547c74d729559be19c60c927ca6ef075c4a53e..f6263f1b593f360b7b3065c90af1681913e1324a 100644
--- a/chrome/browser/extensions/api/settings_private/prefs_util.cc
+++ b/chrome/browser/extensions/api/settings_private/prefs_util.cc
@@ -2,9 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/extensions/api/settings_private/prefs_util.h"
+
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/extensions/api/settings_private/prefs_util.h"
 #include "chrome/browser/extensions/chrome_extension_function.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
@@ -13,18 +14,36 @@
 #if defined(OS_CHROMEOS)
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service_chromeos_factory.h"
+#include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chromeos/settings/cros_settings_names.h"
+#endif
+
+namespace {
+
+#if defined(OS_CHROMEOS)
+bool IsPrivilegedCrosSetting(const std::string& pref_name) {
+  if (!chromeos::CrosSettings::IsCrosSettings(pref_name))
+    return false;
+  // kSystemTimezone should be changeable by all users.
+  if (pref_name == chromeos::kSystemTimezone)
+    return false;
+  // All other Cros settings are considered privileged and are either policy
+  // controlled or owner controlled.
+  return true;
+}
 #endif
 
+}  // namespace
+
 namespace extensions {
 
-namespace settings_private = api::settings_private;
+namespace settings_api = api::settings_private;

[Dan Beam, 2015/08/28 00:20:22]

can you do this in a separate CL?

[stevenjb, 2015/08/28 23:18:07]

I'll undo it. I know the churn sucks. It helped align some stuff, but in
hindsight is was a bad call.

[Dan Beam, 2015/08/29 00:09:08]

churn is not bad.  big diffs are bad.  because they make it hard to review

 
-PrefsUtil::PrefsUtil(Profile* profile) : profile_(profile) {
-}
+PrefsUtil::PrefsUtil(Profile* profile) : profile_(profile) {}
 
-PrefsUtil::~PrefsUtil() {
-}
+PrefsUtil::~PrefsUtil() {}
 
 #if defined(OS_CHROMEOS)
 using CrosSettings = chromeos::CrosSettings;
@@ -36,96 +55,95 @@ const PrefsUtil::TypedPrefMap& PrefsUtil::GetWhitelistedKeys() {
     return *s_whitelist;
   s_whitelist = new PrefsUtil::TypedPrefMap();
   (*s_whitelist)["alternate_error_pages.enabled"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["bookmark_bar.show_on_all_tabs"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["browser.show_home_button"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["download.default_directory"] =
-      settings_private::PrefType::PREF_TYPE_STRING;
+      settings_api::PrefType::PREF_TYPE_STRING;
   (*s_whitelist)["download.prompt_for_download"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["enable_do_not_track"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
-  (*s_whitelist)["homepage"] = settings_private::PrefType::PREF_TYPE_URL;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
+  (*s_whitelist)["homepage"] = settings_api::PrefType::PREF_TYPE_URL;
   (*s_whitelist)["net.network_prediction_options"] =
-      settings_private::PrefType::PREF_TYPE_NUMBER;
+      settings_api::PrefType::PREF_TYPE_NUMBER;
   (*s_whitelist)["safebrowsing.enabled"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["safebrowsing.extended_reporting_enabled"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["search.suggest_enabled"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["spellcheck.use_spelling_service"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
 
 #if defined(OS_CHROMEOS)
   (*s_whitelist)["cros.accounts.allowBWSI"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["cros.accounts.supervisedUsersEnabled"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["cros.accounts.showUserNamesOnSignIn"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["cros.accounts.allowGuest"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["cros.accounts.users"] =
-      settings_private::PrefType::PREF_TYPE_LIST;
+      settings_api::PrefType::PREF_TYPE_LIST;
   (*s_whitelist)["settings.accessibility"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["settings.a11y.autoclick"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["settings.a11y.autoclick_delay_ms"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["settings.a11y.enable_menu"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["settings.a11y.high_contrast_enabled"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["settings.a11y.large_cursor_enabled"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["settings.a11y.screen_magnifier"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["settings.a11y.sticky_keys_enabled"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["settings.a11y.virtual_keyboard"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["settings.clock.use_24hour_clock"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["settings.touchpad.enable_tap_dragging"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["cros.metrics.reportingEnabled"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["cros.device.attestation_for_content_protection_enabled"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)["settings.internet.wake_on_wifi_ssid"] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
 #endif
 
   return *s_whitelist;
 }
 
-api::settings_private::PrefType PrefsUtil::GetType(const std::string& name,
-                                                   base::Value::Type type) {
+settings_api::PrefType PrefsUtil::GetType(const std::string& name,
+                                          base::Value::Type type) {
   switch (type) {
     case base::Value::Type::TYPE_BOOLEAN:
-      return api::settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      return settings_api::PrefType::PREF_TYPE_BOOLEAN;
     case base::Value::Type::TYPE_INTEGER:
     case base::Value::Type::TYPE_DOUBLE:
-      return api::settings_private::PrefType::PREF_TYPE_NUMBER;
+      return settings_api::PrefType::PREF_TYPE_NUMBER;
     case base::Value::Type::TYPE_STRING:
-      return IsPrefTypeURL(name)
-                 ? api::settings_private::PrefType::PREF_TYPE_URL
-                 : api::settings_private::PrefType::PREF_TYPE_STRING;
+      return IsPrefTypeURL(name) ? settings_api::PrefType::PREF_TYPE_URL
+                                 : settings_api::PrefType::PREF_TYPE_STRING;
     case base::Value::Type::TYPE_LIST:
-      return api::settings_private::PrefType::PREF_TYPE_LIST;
+      return settings_api::PrefType::PREF_TYPE_LIST;
     default:
-      return api::settings_private::PrefType::PREF_TYPE_NONE;
+      return settings_api::PrefType::PREF_TYPE_NONE;
   }
 }
 
-scoped_ptr<api::settings_private::PrefObject> PrefsUtil::GetCrosSettingsPref(
+scoped_ptr<settings_api::PrefObject> PrefsUtil::GetCrosSettingsPref(
     const std::string& name) {
-  scoped_ptr<api::settings_private::PrefObject> pref_object(
-      new api::settings_private::PrefObject());
+  scoped_ptr<settings_api::PrefObject> pref_object(
+      new settings_api::PrefObject());
 
 #if defined(OS_CHROMEOS)
   const base::Value* value = CrosSettings::Get()->GetPref(name);

[michaelpg, 2015/08/27 23:40:01]

GetPref doesn't check the whitelist, so |value| could be null if the settings
page uses a bad pref key, leading to a segfault. Do we care? (for non-CrOS
prefs, GetPref returns a nullptr for an invalid key.)

[stevenjb, 2015/08/28 23:18:07]

We probably do care, I'll fix that while I'm in here.

@@ -137,41 +155,65 @@ scoped_ptr<api::settings_private::PrefObject> PrefsUtil::GetCrosSettingsPref(
   return pref_object.Pass();
 }
 
-scoped_ptr<api::settings_private::PrefObject> PrefsUtil::GetPref(
+scoped_ptr<settings_api::PrefObject> PrefsUtil::GetPref(
     const std::string& name) {
-  if (IsCrosSetting(name))
-    return GetCrosSettingsPref(name);
+  const PrefService::Preference* pref = nullptr;
+  scoped_ptr<settings_api::PrefObject> pref_object;
+  if (IsCrosSetting(name)) {
+    pref_object = GetCrosSettingsPref(name);
+  } else {
+    PrefService* pref_service = FindServiceForPref(name);
+    pref = pref_service->FindPreference(name);
+    if (!pref)
+      return nullptr;
+    pref_object.reset(new settings_api::PrefObject());
+    pref_object->key = pref->name();
+    pref_object->type = GetType(name, pref->GetType());
+    pref_object->value.reset(pref->GetValue()->DeepCopy());
+  }
 
-  PrefService* pref_service = FindServiceForPref(name);
-  const PrefService::Preference* pref = pref_service->FindPreference(name);
-  if (!pref)
-    return nullptr;
-
-  scoped_ptr<api::settings_private::PrefObject> pref_object(
-      new api::settings_private::PrefObject());
-  pref_object->key = pref->name();
-  pref_object->type = GetType(name, pref->GetType());
-  pref_object->value.reset(pref->GetValue()->DeepCopy());
-
-  if (pref->IsManaged()) {
-    if (pref->IsManagedByCustodian()) {
-      pref_object->policy_source =
-          api::settings_private::PolicySource::POLICY_SOURCE_DEVICE;
+  settings_api::PolicySource source =
+      settings_api::PolicySource::POLICY_SOURCE_NONE;
+  settings_api::PolicyEnforcement enforcement =
+      settings_api::PolicyEnforcement::POLICY_ENFORCEMENT_NONE;
+
+  if (IsPrefEnterpriseManaged(name)) {
+    // Enterprise managed prefs are treated the same as device policy restricted
+    // prefs in the UI.
+    source = settings_api::PolicySource::POLICY_SOURCE_DEVICE;
+    enforcement = settings_api::PolicyEnforcement::POLICY_ENFORCEMENT_ENFORCED;
+  } else if (IsPrefOwnerControlled(name)) {
+    source = settings_api::PolicySource::POLICY_SOURCE_OWNER;
+    enforcement = settings_api::PolicyEnforcement::POLICY_ENFORCEMENT_ENFORCED;
+  } else if (IsPrefSupervisorControlled(name)) {
+    // We treat supervised prefs as 'OWNER' because on Chrome OS they are
+    // also restricted to the owner, and on non Chrome OS we hide these controls

[michaelpg, 2015/08/27 23:40:01]

nit: "non-Chrome OS"

[stevenjb, 2015/08/28 23:18:07]

really? :P

+    // so do not need to distinguish them.
+    source = settings_api::PolicySource::POLICY_SOURCE_OWNER;
+    enforcement = settings_api::PolicyEnforcement::POLICY_ENFORCEMENT_ENFORCED;
+  } else if (pref && pref->IsManaged()) {
+    if (pref->IsManagedByCustodian())

[michaelpg, 2015/08/27 23:40:01]

I'm confused about the difference between "managed by custodian" and
"supervisor-controlled".

And when can a pref be managed (which implies admin policy) and also managed by
custodian?

[stevenjb, 2015/08/28 23:18:07]

I'm a bit confused too. I didn't change this logic, but I'm not sure where oren
got this from.

The comment says:

IsManaged() says:
"Returns true if the Preference is managed, i.e. set by an admin policy.
 Since managed prefs have the highest priority, this also indicates
 whether the pref is actually being controlled by the policy setting."

IsManagedByCustondian() says:
"Returns true if the Preference is controlled by the custodian of the
 supervised user. Since a supervised user is not expected to have an admin
 policy, this is the controlling pref if set."

I don't see any other references to IsManagedByCustondian(); maybe it was in the
original code and pulled? I'm going to eliminate that logic. In practice we
don't currently differentiate between 'USER' and "DEVICE' anyway.

+      source = settings_api::PolicySource::POLICY_SOURCE_DEVICE;
+    else
+      source = settings_api::PolicySource::POLICY_SOURCE_USER;

[michaelpg, 2015/08/27 23:40:01]

When is a preference "managed", but not mandated by enterprise policy or a
supervisor?

[stevenjb, 2015/08/28 23:18:07]

I believe that desktop chrome allows for managed preferences, but I'm not sure
how that works.

+    if (pref->IsRecommended()) {

[michaelpg, 2015/08/27 23:40:01]

Another point of confusion :-) How can a pref be in the managed store and the
recommended store at the same time? Seems to me if it's in the managed store,
PrefValueStore::ControllingPrefStoreForPref will always return MANAGED_STORE, so
PrefValueStore::PrefValueFromRecommendedStore will be false.

[stevenjb, 2015/08/28 23:18:07]

Wow, the existing logic is just... wrong. Looking at the original source and
matching the logic to it.

+      enforcement =
+          settings_api::PolicyEnforcement::POLICY_ENFORCEMENT_RECOMMENDED;
+      pref_object->recommended_value.reset(
+          pref->GetRecommendedValue()->DeepCopy());
     } else {
-      pref_object->policy_source =
-          api::settings_private::PolicySource::POLICY_SOURCE_USER;
+      enforcement =
+          settings_api::PolicyEnforcement::POLICY_ENFORCEMENT_ENFORCED;
     }
-    pref_object->policy_enforcement =
-        pref->IsRecommended() ? api::settings_private::PolicyEnforcement::
-                                    POLICY_ENFORCEMENT_RECOMMENDED
-                              : api::settings_private::PolicyEnforcement::
-                                    POLICY_ENFORCEMENT_ENFORCED;
-  } else if (!IsPrefUserModifiable(name)) {
-    pref_object->policy_source =
-        api::settings_private::PolicySource::POLICY_SOURCE_USER;
-    pref_object->policy_enforcement =
-        api::settings_private::PolicyEnforcement::POLICY_ENFORCEMENT_ENFORCED;
+  } else if (IsPrefPrimaryUserControlled(name)) {
+    source = settings_api::PolicySource::POLICY_SOURCE_PRIMARY_USER;
+    enforcement = settings_api::PolicyEnforcement::POLICY_ENFORCEMENT_ENFORCED;
+  } else if (pref && !pref->IsUserModifiable()) {
+    source = settings_api::PolicySource::POLICY_SOURCE_USER;

[michaelpg, 2015/08/27 23:40:01]

What is a user policy? In this case the pref is controlled by an extension or
the command-line store, right?

[stevenjb, 2015/08/28 23:18:07]

Yeah, we appear to be conflating things here. The original code sets a
'disabled' property; I am going to add a 'readOnly' property instead.

+    enforcement = settings_api::PolicyEnforcement::POLICY_ENFORCEMENT_ENFORCED;
   }
+  pref_object->policy_source = source;
+  pref_object->policy_enforcement = enforcement;
 
   return pref_object.Pass();
 }
@@ -186,8 +228,7 @@ bool PrefsUtil::SetPref(const std::string& pref_name,
   if (!IsPrefUserModifiable(pref_name))
     return false;
 
-  const PrefService::Preference* pref =
-      pref_service->FindPreference(pref_name);
+  const PrefService::Preference* pref = pref_service->FindPreference(pref_name);
   if (!pref)
     return false;
 
@@ -241,7 +282,7 @@ bool PrefsUtil::SetCrosSettingsPref(const std::string& pref_name,
   if (service && service->HandlesSetting(pref_name))
     return service->Set(pref_name, *value);
 
-  chromeos::CrosSettings::Get()->Set(pref_name, *value);
+  CrosSettings::Get()->Set(pref_name, *value);
   return true;
 #else
   return false;
@@ -260,7 +301,7 @@ bool PrefsUtil::AppendToListCrosSetting(const std::string& pref_name,
     return service->AppendToList(pref_name, value);
   }
 
-  chromeos::CrosSettings::Get()->AppendToList(pref_name, &value);
+  CrosSettings::Get()->AppendToList(pref_name, &value);
   return true;
 #else
   return false;
@@ -279,7 +320,7 @@ bool PrefsUtil::RemoveFromListCrosSetting(const std::string& pref_name,
     return service->RemoveFromList(pref_name, value);
   }
 
-  chromeos::CrosSettings::Get()->RemoveFromList(pref_name, &value);
+  CrosSettings::Get()->RemoveFromList(pref_name, &value);
   return true;
 #else
   return false;
@@ -287,29 +328,69 @@ bool PrefsUtil::RemoveFromListCrosSetting(const std::string& pref_name,
 }
 
 bool PrefsUtil::IsPrefTypeURL(const std::string& pref_name) {
-  settings_private::PrefType pref_type =
-      settings_private::PrefType::PREF_TYPE_NONE;
+  settings_api::PrefType pref_type = settings_api::PrefType::PREF_TYPE_NONE;
 
   const TypedPrefMap keys = GetWhitelistedKeys();
   const auto& iter = keys.find(pref_name);
   if (iter != keys.end())
     pref_type = iter->second;
 
-  return pref_type == settings_private::PrefType::PREF_TYPE_URL;
+  return pref_type == settings_api::PrefType::PREF_TYPE_URL;
 }
 
-bool PrefsUtil::IsPrefUserModifiable(const std::string& pref_name) {
-  if (pref_name != prefs::kBrowserGuestModeEnabled &&
-      pref_name != prefs::kBrowserAddPersonEnabled) {
-    return true;
+bool PrefsUtil::IsPrefEnterpriseManaged(const std::string& pref_name) {
+#if defined(OS_CHROMEOS)
+  if (IsPrivilegedCrosSetting(pref_name)) {
+    policy::BrowserPolicyConnectorChromeOS* connector =
+        g_browser_process->platform_part()->browser_policy_connector_chromeos();
+    if (connector->IsEnterpriseManaged())
+      return true;
+  }
+#endif
+  return false;
+}
+
+bool PrefsUtil::IsPrefOwnerControlled(const std::string& pref_name) {
+#if defined(OS_CHROMEOS)
+  if (IsPrivilegedCrosSetting(pref_name)) {
+    if (!chromeos::ProfileHelper::IsOwnerProfile(profile_))
+      return true;
+  }
+#endif
+  return false;
+}
+
+bool PrefsUtil::IsPrefSupervisorControlled(const std::string& pref_name) {

[michaelpg, 2015/08/27 23:40:01]

Does PrefService::IsManagedByCustodian result in different behavior?

[stevenjb, 2015/08/28 23:18:07]

This should also be 'readOnly' = true to match the current behavior.

+  if (pref_name == prefs::kBrowserGuestModeEnabled ||
+      pref_name == prefs::kBrowserAddPersonEnabled) {
+    if (profile_->IsSupervised())
+      return true;
   }
+  return false;
+}
+
+bool PrefsUtil::IsPrefPrimaryUserControlled(const std::string& pref_name) {
+  if (pref_name == prefs::kWakeOnWifiSsid) {

[michaelpg, 2015/08/27 23:40:01]

#if defined(OS_CHROMEOS)?

[stevenjb, 2015/08/28 23:18:07]

Done.

+    user_manager::UserManager* user_manager = user_manager::UserManager::Get();
+    const user_manager::User* user =
+        chromeos::ProfileHelper::Get()->GetUserByProfile(profile_);
+    if (user && user->email() != user_manager->GetPrimaryUser()->email())
+      return true;
+  }
+  return false;
+}
 
+bool PrefsUtil::IsPrefUserModifiable(const std::string& pref_name) {
   PrefService* pref_service = profile_->GetPrefs();
   const PrefService::Preference* pref =
       pref_service->FindPreference(pref_name.c_str());
-  if (!pref || !pref->IsUserModifiable() || profile_->IsSupervised())
+  if (!pref || !pref->IsUserModifiable())
     return false;
-
+  if (IsPrefEnterpriseManaged(pref_name) || IsPrefOwnerControlled(pref_name) ||
+      IsPrefSupervisorControlled(pref_name) ||

[michaelpg, 2015/08/27 23:40:01]

Why isn't it enough to check pref->IsUserModifiable()?

[Dan Beam, 2015/08/28 00:20:21]

+1

[stevenjb, 2015/08/28 23:18:07]

I think actually that should be sufficient. Eliminating the cruft.

+      IsPrefPrimaryUserControlled(pref_name)) {
+    return false;
+  }
   return true;
 }