ChromeOS: implement per-user time zone preferences.

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

 // Copyright 2015 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/extensions/api/settings_private/prefs_util.h"
 
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/extensions/chrome_extension_function.h"
 #include "chrome/browser/extensions/settings_api_helpers.h"
@@ skipping 21 matching lines @@
 #include "extensions/browser/management_policy.h"
 #include "extensions/common/extension.h"
 
 #if defined(OS_CHROMEOS)
 #include "ash/public/cpp/ash_pref_names.h"  // nogncheck
 #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 "chrome/browser/chromeos/system/timezone_util.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "ui/chromeos/events/pref_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
+  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
 
 namespace extensions {
 
 namespace settings_private = api::settings_private;
 
 PrefsUtil::PrefsUtil(Profile* profile) : profile_(profile) {}
 
@@ skipping 213 matching lines @@
       settings_private::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)[proxy_config::prefs::kUseSharedProxies] =
       settings_private::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)[::prefs::kWakeOnWifiDarkConnect] =
       settings_private::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)[::chromeos::kSignedDataRoamingEnabled] =
       settings_private::PrefType::PREF_TYPE_BOOLEAN;
 
   // 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;
 
   // Ash settings.
   (*s_whitelist)[::prefs::kEnableStylusTools] =
       settings_private::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)[::prefs::kLaunchPaletteOnEjectEvent] =
       settings_private::PrefType::PREF_TYPE_BOOLEAN;
   (*s_whitelist)[::prefs::kNoteTakingAppEnabledOnLockScreen] =
       settings_private::PrefType::PREF_TYPE_BOOLEAN;
@@ skipping 147 matching lines @@
     pref = pref_service->FindPreference(name);
     if (!pref)
       return nullptr;
     pref_object.reset(new settings_private::PrefObject());
     pref_object->key = pref->name();
     pref_object->type = GetType(name, pref->GetType());
     pref_object->value.reset(pref->GetValue()->DeepCopy());
   }
 
 #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;
     pref_object->enforcement =
         settings_private::Enforcement::ENFORCEMENT_ENFORCED;
     pref_object->controlled_by_name.reset(
         new std::string(user_manager::UserManager::Get()
                             ->GetPrimaryUser()
                             ->GetAccountId()
                             .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()) {
     pref_object->controlled_by =
         settings_private::ControlledBy::CONTROLLED_BY_USER_POLICY;
     pref_object->enforcement =
         settings_private::Enforcement::ENFORCEMENT_ENFORCED;
     return pref_object;
   }
 
@@ skipping 168 matching lines @@
 bool PrefsUtil::IsPrefTypeURL(const std::string& pref_name) {
   return GetWhitelistedPrefType(pref_name) ==
          settings_private::PrefType::PREF_TYPE_URL;
 }
 
 #if defined(OS_CHROMEOS)
 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;
   }
   return false;
 }
 
 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;
 }
 #endif
 
 bool PrefsUtil::IsPrefSupervisorControlled(const std::string& pref_name) {
   if (pref_name != prefs::kBrowserGuestModeEnabled &&
       pref_name != prefs::kBrowserAddPersonEnabled) {
     return false;
   }
   return profile_->IsSupervised();
 }
 
 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)
     return profile_pref->IsUserModifiable();
 
   const PrefService::Preference* local_state_pref =
       g_browser_process->local_state()->FindPreference(pref_name);
   if (local_state_pref)
     return local_state_pref->IsUserModifiable();
 
@@ skipping 67 matching lines @@
       ExtensionPrefValueMapFactory::GetForBrowserContext(profile_)
           ->GetExtensionControllingPref(pref_object.key);
   if (extension_id.empty())
     return nullptr;
 
   return ExtensionRegistry::Get(profile_)->GetExtensionById(
       extension_id, ExtensionRegistry::ENABLED);
 }
 
 }  // namespace extensions