ChromeOS: implement per-user time zone preferences.

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 2c9b357f4048f77fc9ed8b31f0c5b27f04735d2a..0ce2dd59b1eda0a117da68ae2eafc1e7c6607bca 100644
--- a/chrome/browser/extensions/api/settings_private/prefs_util.cc
+++ b/chrome/browser/extensions/api/settings_private/prefs_util.cc
@@ -39,6 +39,7 @@
 #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 "chrome/browser/chromeos/system/timezone_util.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "ui/chromeos/events/pref_names.h"
 #endif
@@ -49,13 +50,25 @@ namespace {
 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
+  if (!chromeos::system::PerUserTimezoneEnabled()) {
+    // 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.

[stevenjb, 2017/05/30 16:47:18]

This comment is duplicated below, remove it here.

[Alexander Alekseev, 2017/07/06 06:30:29]

This comment is correct only if per-user time zone is not enabled.
It would be more correct to put the second comment inside else{}block, but I
haven't seen anything like that in chromium code ;)

The idea behind the current "if(!chromeos::system::PerUserTimezoneEnabled())"
checks implementation is to make it more clear that it is backward compatible.
So I am trying to keep old code in-place and wrap it with checks.

+  }
+  // All Cros settings are considered privileged and are either policy
   // controlled or owner controlled.
   return true;
 }
+
+bool IsCrosSettingReadOnly(const std::string& pref_name) {
+  if (chromeos::system::PerUserTimezoneEnabled()) {
+    // System timezone is never directly changable by user.
+    return pref_name == chromeos::kSystemTimezone;
+  }
+  return false;
+}
 #endif
 
 }  // namespace
@@ -289,7 +302,9 @@ const PrefsUtil::TypedPrefMap& PrefsUtil::GetWhitelistedKeys() {
 
   // Timezone settings.
   (*s_whitelist)[chromeos::kSystemTimezone] =
-      settings_private::PrefType::PREF_TYPE_BOOLEAN;
+      settings_private::PrefType::PREF_TYPE_STRING;
+  (*s_whitelist)[prefs::kUserTimezone] =
+      settings_private::PrefType::PREF_TYPE_STRING;
   (*s_whitelist)[::prefs::kResolveTimezoneByGeolocation] =
       settings_private::PrefType::PREF_TYPE_BOOLEAN;
 
@@ -457,6 +472,21 @@ std::unique_ptr<settings_private::PrefObject> PrefsUtil::GetPref(
   }
 
 #if defined(OS_CHROMEOS)
+  // We first check for enterprise-managed, then for primary-user managed.
+  // Otherwise in multiprofile mode enterprise preference for the secondary
+  // user will appear primary-user-controlled, which looks strange, because
+  // primary user preference will be disabled with "enterprise controlled"
+  // status.

[michaelpg, 2017/05/30 22:21:23]

This makes sense, but can you run it by Tom? Is this CL the right place to
change this UI?

I could also go with the opposite: if the primary user has a user policy but the
secondary user does not, the secondary user's settings are overridden because
the primary user's state is being applied (e.g., signed in with @google.com as
primary and @gmail.com as secondary). Is it better to give the user the root
reason (some policy somewhere) or the most immediate reason (go look at Primary
user's settings if you want to know more)?

[Alexander Alekseev, 2017/07/06 06:30:28]

This is a good point, but if current user doesn't have policy for this value, it
won't be enterprise managed (thus this check will be false).
So this doesn't affect user policies.

I did this for system preferences, which are device-policy managed.

+  if (IsPrefEnterpriseManaged(name)) {
+    // Enterprise managed prefs are treated the same as device policy restricted
+    // prefs in the UI.
+    pref_object->controlled_by =
+        settings_private::ControlledBy::CONTROLLED_BY_DEVICE_POLICY;
+    pref_object->enforcement =
+        settings_private::Enforcement::ENFORCEMENT_ENFORCED;
+    return pref_object;
+  }
+
   if (IsPrefPrimaryUserControlled(name)) {
     pref_object->controlled_by =
         settings_private::ControlledBy::CONTROLLED_BY_PRIMARY_USER;
@@ -469,16 +499,6 @@ std::unique_ptr<settings_private::PrefObject> PrefsUtil::GetPref(
                             .GetUserEmail()));
     return pref_object;
   }
-
-  if (IsPrefEnterpriseManaged(name)) {
-    // Enterprise managed prefs are treated the same as device policy restricted
-    // prefs in the UI.
-    pref_object->controlled_by =
-        settings_private::ControlledBy::CONTROLLED_BY_DEVICE_POLICY;
-    pref_object->enforcement =
-        settings_private::Enforcement::ENFORCEMENT_ENFORCED;
-    return pref_object;
-  }
 #endif
 
   if (pref && pref->IsManaged()) {
@@ -667,13 +687,30 @@ bool PrefsUtil::IsPrefEnterpriseManaged(const std::string& pref_name) {
   if (IsPrivilegedCrosSetting(pref_name)) {
     policy::BrowserPolicyConnectorChromeOS* connector =
         g_browser_process->platform_part()->browser_policy_connector_chromeos();
-    if (connector->IsEnterpriseManaged())
-      return true;
+    return connector->IsEnterpriseManaged();
+  } else {

[stevenjb, 2017/05/30 16:47:18]

No else after return

[Alexander Alekseev, 2017/07/06 06:30:28]

Done.

+    if (!chromeos::system::PerUserTimezoneEnabled())
+      return false;
+
+    policy::BrowserPolicyConnectorChromeOS* connector =
+        g_browser_process->platform_part()->browser_policy_connector_chromeos();
+    if (!connector->IsEnterpriseManaged())
+      return false;
+
+    if (pref_name == prefs::kUserTimezone ||
+        pref_name == prefs::kResolveTimezoneByGeolocation) {
+      return chromeos::system::IsTimezonePrefManaged(pref_name);
+    }
   }
   return false;
 }
 
 bool PrefsUtil::IsPrefOwnerControlled(const std::string& pref_name) {
+  // chromeos::kSystemTimezone is global display-only preference and
+  // it should appear as disabled, but not owned.

[michaelpg, 2017/05/30 22:21:23]

Isn't this idea (and code) already built into IsPrivilegedCrosSetting? It looks
like the line 718 would be triggered anyway for kSystemTimezone since the check
at line 714 would be false.

[Alexander Alekseev, 2017/07/06 06:30:28]

IsPrivilegedCrosSetting() depends on whether per-user time zone is enabled.

+  if (pref_name == chromeos::kSystemTimezone)
+    return false;
+
   if (IsPrivilegedCrosSetting(pref_name)) {
     if (!chromeos::ProfileHelper::IsOwnerProfile(profile_))
       return true;
@@ -682,13 +719,19 @@ bool PrefsUtil::IsPrefOwnerControlled(const std::string& pref_name) {
 }
 
 bool PrefsUtil::IsPrefPrimaryUserControlled(const std::string& pref_name) {
-  if (pref_name == prefs::kWakeOnWifiDarkConnect) {
+  // chromeos::kSystemTimezone is read-only, but for the non-primary users
+  // it should have "primary user controlled" attribute.
+  if (pref_name == prefs::kWakeOnWifiDarkConnect ||
+      pref_name == prefs::kResolveTimezoneByGeolocation ||
+      pref_name == prefs::kUserTimezone ||
+      pref_name == chromeos::kSystemTimezone) {
     user_manager::UserManager* user_manager = user_manager::UserManager::Get();
     const user_manager::User* user =
         chromeos::ProfileHelper::Get()->GetUserByProfile(profile_);
-    if (user &&
-        user->GetAccountId() != user_manager->GetPrimaryUser()->GetAccountId())
+    if (user && user->GetAccountId() !=
+                    user_manager->GetPrimaryUser()->GetAccountId()) {
       return true;
+    }
   }
   return false;
 }
@@ -703,6 +746,11 @@ bool PrefsUtil::IsPrefSupervisorControlled(const std::string& pref_name) {
 }
 
 bool PrefsUtil::IsPrefUserModifiable(const std::string& pref_name) {
+#if defined(OS_CHROMEOS)
+  if (IsCrosSettingReadOnly(pref_name))
+    return false;
+#endif
+
   const PrefService::Preference* profile_pref =
       profile_->GetPrefs()->FindPreference(pref_name);
   if (profile_pref)