Make ArcBackupRestoreEnabled and ArcLocationServiceEnabled static

Change the implementation of ArcBackupRestoreEnabled and
ArcLocationServiceEnabled to disallow policy changes after the initial
setup of ARC.

BUG=b:74220823
TEST=Opt in to ARC, ensure settings still applied.

Change-Id: I695cfaf0335faa6eefe96f63e5a5ce41ed9880a2
Reviewed-on: https://chromium-review.googlesource.com/959503
Commit-Queue: Josh Horwich <jhorwich@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Reviewed-by: Luis Hector Chavez <lhchavez@chromium.org>
Reviewed-by: Bartosz Fabianowski <bartfab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548774}

chrome/browser/chromeos/arc/intent_helper/arc_settings_service.cc

@@ -150,10 +150,6 @@ class ArcSettingsServiceImpl
   // Android boot after AppInstance is ready and send it to Android.
   // TODO(crbug.com/762553): Sync settings at proper time.
   void SyncAppTimeSettings();
-  // Determine whether a particular setting needs to be synced to Android.
-  // Keep these lines ordered lexicographically.
-  bool ShouldSyncBackupEnabled() const;
-  bool ShouldSyncLocationServiceEnabled() const;
   // Send particular settings to Android.
   // Keep these lines ordered lexicographically.
   void SyncAccessibilityLargeMouseCursorEnabled() const;
@@ -270,12 +266,6 @@ void ArcSettingsServiceImpl::OnPrefChanged(const std::string& pref_name) const {
     SyncSwitchAccessEnabled();
   } else if (pref_name == ash::prefs::kAccessibilityVirtualKeyboardEnabled) {
     SyncAccessibilityVirtualKeyboardEnabled();
-  } else if (pref_name == prefs::kArcBackupRestoreEnabled) {
-    if (ShouldSyncBackupEnabled())
-      SyncBackupEnabled();
-  } else if (pref_name == prefs::kArcLocationServiceEnabled) {
-    if (ShouldSyncLocationServiceEnabled())
-      SyncLocationServiceEnabled();
   } else if (pref_name == ::prefs::kApplicationLocale ||
              pref_name == ::prefs::kLanguagePreferredLanguages) {
     SyncLocale();
@@ -329,8 +319,6 @@ void ArcSettingsServiceImpl::StartObservingSettingsChanges() {
   AddPrefToObserve(ash::prefs::kAccessibilitySpokenFeedbackEnabled);
   AddPrefToObserve(ash::prefs::kAccessibilitySwitchAccessEnabled);
   AddPrefToObserve(ash::prefs::kAccessibilityVirtualKeyboardEnabled);
-  AddPrefToObserve(prefs::kArcBackupRestoreEnabled);
-  AddPrefToObserve(prefs::kArcLocationServiceEnabled);
   AddPrefToObserve(prefs::kSmsConnectEnabled);
   AddPrefToObserve(::prefs::kResolveTimezoneByGeolocationMethod);
   AddPrefToObserve(::prefs::kUse24HourClock);
@@ -341,6 +329,10 @@ void ArcSettingsServiceImpl::StartObservingSettingsChanges() {
   AddPrefToObserve(onc::prefs::kDeviceOpenNetworkConfiguration);
   AddPrefToObserve(onc::prefs::kOpenNetworkConfiguration);
 
+  // Note that some preferences, such as kArcBackupRestoreEnabled and
+  // kArcLocationServiceEnabled, are not dynamically updated after initial
+  // ARC setup and therefore are not observed here.
+
   reporting_consent_subscription_ = CrosSettings::Get()->AddSettingsObserver(
       chromeos::kStatsReportingPref,
       base::Bind(&ArcSettingsServiceImpl::SyncReportingConsent,
@@ -391,11 +383,6 @@ void ArcSettingsServiceImpl::SyncBootTimeSettings() const {
   SyncTimeZone();
   SyncTimeZoneByGeolocation();
   SyncUse24HourClock();
-
-  if (ShouldSyncBackupEnabled())
-    SyncBackupEnabled();
-  if (ShouldSyncLocationServiceEnabled())
-    SyncLocationServiceEnabled();
 }
 
 void ArcSettingsServiceImpl::SyncAppTimeSettings() {
@@ -410,22 +397,6 @@ void ArcSettingsServiceImpl::SyncAppTimeSettings() {
   AddPrefToObserve(::prefs::kLanguagePreferredLanguages);
 }
 
-bool ArcSettingsServiceImpl::ShouldSyncBackupEnabled() const {
-  // Always sync the managed setting. Also sync when the pref is unset, which
-  // normally happens once after the pref changes from the managed state to
-  // unmanaged.
-  return GetPrefs()->IsManagedPreference(prefs::kArcBackupRestoreEnabled) ||
-         !GetPrefs()->HasPrefPath(prefs::kArcBackupRestoreEnabled);
-}
-
-bool ArcSettingsServiceImpl::ShouldSyncLocationServiceEnabled() const {
-  // Always sync the managed setting. Also sync when the pref is unset, which
-  // normally happens once after the pref changes from the managed state to
-  // unmanaged.
-  return GetPrefs()->IsManagedPreference(prefs::kArcLocationServiceEnabled) ||
-         !GetPrefs()->HasPrefPath(prefs::kArcLocationServiceEnabled);
-}
-
 void ArcSettingsServiceImpl::SyncAccessibilityLargeMouseCursorEnabled() const {
   SendBoolPrefSettingsBroadcast(
       ash::prefs::kAccessibilityLargeCursorEnabled,
@@ -450,16 +421,6 @@ void ArcSettingsServiceImpl::SyncBackupEnabled() const {
     backup_settings->SetBackupEnabled(value->GetBool(),
                                       !pref->IsUserModifiable());
   }
-
-  if (GetPrefs()->IsManagedPreference(prefs::kArcBackupRestoreEnabled)) {
-    // Unset the user pref so that if the pref becomes unmanaged at some point,
-    // this change will be synced.
-    GetPrefs()->ClearPref(prefs::kArcBackupRestoreEnabled);
-  } else if (!GetPrefs()->HasPrefPath(prefs::kArcBackupRestoreEnabled)) {
-    // Set the pref value in order to prevent the subsequent syncing. The
-    // "false" value is a safe default from the legal/privacy perspective.
-    GetPrefs()->SetBoolean(prefs::kArcBackupRestoreEnabled, false);
-  }
 }
 
 void ArcSettingsServiceImpl::SyncFocusHighlightEnabled() const {
@@ -518,15 +479,6 @@ void ArcSettingsServiceImpl::SyncLocationServiceEnabled() const {
   SendBoolPrefSettingsBroadcast(
       prefs::kArcLocationServiceEnabled,
       "org.chromium.arc.intent_helper.SET_LOCATION_SERVICE_ENABLED");
-  if (GetPrefs()->IsManagedPreference(prefs::kArcLocationServiceEnabled)) {
-    // Unset the user pref so that if the pref becomes unmanaged at some point,
-    // this change will be synced.
-    GetPrefs()->ClearPref(prefs::kArcLocationServiceEnabled);
-  } else if (!GetPrefs()->HasPrefPath(prefs::kArcLocationServiceEnabled)) {
-    // Set the pref value in order to prevent the subsequent syncing. The
-    // "false" value is a safe default from the legal/privacy perspective.
-    GetPrefs()->SetBoolean(prefs::kArcLocationServiceEnabled, false);
-  }
 }
 
 void ArcSettingsServiceImpl::SyncProxySettings() const {

chrome/browser/chromeos/arc/intent_helper/arc_settings_service_browsertest.cc

@@ -179,20 +179,17 @@ constexpr char kWifi1Guid[] = "{wifi1_guid}";
 
 constexpr char kONCPacUrl[] = "http://domain.com/x";
 
-constexpr char kLocationServiceBroadcastAction[] =
-    "org.chromium.arc.intent_helper.SET_LOCATION_SERVICE_ENABLED";
 constexpr char kSetProxyBroadcastAction[] =
     "org.chromium.arc.intent_helper.SET_PROXY";
 
-// Returns the number of |broadcasts| having the |action| action, and checks
-// that all their extras match with |extras|.
-int CountBroadcasts(
+// Returns the number of |broadcasts| having the proxy action, and checks that
+// all their extras match with |extras|.
+int CountProxyBroadcasts(
     const std::vector<FakeIntentHelperInstance::Broadcast>& broadcasts,
-    const std::string& action,
     const base::DictionaryValue* extras) {
   int count = 0;
   for (const FakeIntentHelperInstance::Broadcast& broadcast : broadcasts) {
-    if (broadcast.action == action) {
+    if (broadcast.action == kSetProxyBroadcastAction) {
       EXPECT_TRUE(base::JSONReader::Read(broadcast.extras)->Equals(extras));
       count++;
     }
@@ -200,14 +197,6 @@ int CountBroadcasts(
   return count;
 }
 
-// Returns the number of |broadcasts| having the proxy action, and checks that
-// all their extras match with |extras|.
-int CountProxyBroadcasts(
-    const std::vector<FakeIntentHelperInstance::Broadcast>& broadcasts,
-    const base::DictionaryValue* proxy_settings) {
-  return CountBroadcasts(broadcasts, kSetProxyBroadcastAction, proxy_settings);
-}
-
 void RunUntilIdle() {
   DCHECK(base::MessageLoop::current());
   base::RunLoop loop;
@@ -362,13 +351,13 @@ IN_PROC_BROWSER_TEST_F(ArcSettingsServiceTest, BackupRestorePolicyTest) {
              nullptr);
   UpdatePolicy(policy);
 
-  // The pref is disabled and managed, and the corresponding sync method
-  // reflects the pref.
+  // The pref is disabled and managed, but the corresponding sync method does
+  // not reflect the pref as it is not dynamically applied.
   EXPECT_FALSE(prefs->GetBoolean(prefs::kArcBackupRestoreEnabled));
   EXPECT_TRUE(prefs->IsManagedPreference(prefs::kArcBackupRestoreEnabled));
-  EXPECT_TRUE(fake_backup_settings_instance_->set_backup_enabled_called());
+  EXPECT_FALSE(fake_backup_settings_instance_->set_backup_enabled_called());
   EXPECT_FALSE(fake_backup_settings_instance_->enabled());
-  EXPECT_TRUE(fake_backup_settings_instance_->managed());
+  EXPECT_FALSE(fake_backup_settings_instance_->managed());
 
   fake_backup_settings_instance_->ClearCallHistory();
 
@@ -379,13 +368,13 @@ IN_PROC_BROWSER_TEST_F(ArcSettingsServiceTest, BackupRestorePolicyTest) {
              nullptr);
   UpdatePolicy(policy);
 
-  // The pref is enabled and managed, and the corresponding sync method
-  // reflects the pref.
+  // The pref is enabled and managed, but the corresponding sync method does
+  // not reflect the pref as it is not dynamically applied.
   EXPECT_TRUE(prefs->GetBoolean(prefs::kArcBackupRestoreEnabled));
   EXPECT_TRUE(prefs->IsManagedPreference(prefs::kArcBackupRestoreEnabled));
-  EXPECT_TRUE(fake_backup_settings_instance_->set_backup_enabled_called());
-  EXPECT_TRUE(fake_backup_settings_instance_->enabled());
-  EXPECT_TRUE(fake_backup_settings_instance_->managed());
+  EXPECT_FALSE(fake_backup_settings_instance_->set_backup_enabled_called());
+  EXPECT_FALSE(fake_backup_settings_instance_->enabled());
+  EXPECT_FALSE(fake_backup_settings_instance_->managed());
 
   fake_backup_settings_instance_->ClearCallHistory();
 
@@ -393,11 +382,11 @@ IN_PROC_BROWSER_TEST_F(ArcSettingsServiceTest, BackupRestorePolicyTest) {
   policy.Erase(policy::key::kArcBackupRestoreEnabled);
   UpdatePolicy(policy);
 
-  // The pref is disabled and unmanaged, and the corresponding sync method
-  // reflects the pref.
-  EXPECT_FALSE(prefs->GetBoolean(prefs::kArcBackupRestoreEnabled));
+  // The pref is unmanaged, but the corresponding sync method does not reflect
+  // the pref as it is not dynamically applied.
+  EXPECT_TRUE(prefs->GetBoolean(prefs::kArcBackupRestoreEnabled));
   EXPECT_FALSE(prefs->IsManagedPreference(prefs::kArcBackupRestoreEnabled));
-  EXPECT_TRUE(fake_backup_settings_instance_->set_backup_enabled_called());
+  EXPECT_FALSE(fake_backup_settings_instance_->set_backup_enabled_called());
   EXPECT_FALSE(fake_backup_settings_instance_->enabled());
   EXPECT_FALSE(fake_backup_settings_instance_->managed());
 }
@@ -419,17 +408,11 @@ IN_PROC_BROWSER_TEST_F(ArcSettingsServiceTest, LocationServicePolicyTest) {
              nullptr);
   UpdatePolicy(policy);
 
-  // The pref is disabled and managed, and the corresponding broadcast is sent
-  // at least once.
+  // The pref is disabled and managed, but no broadcast is sent as the setting
+  // is not dynamically applied.
   EXPECT_FALSE(prefs->GetBoolean(prefs::kArcLocationServiceEnabled));
   EXPECT_TRUE(prefs->IsManagedPreference(prefs::kArcLocationServiceEnabled));
-  base::DictionaryValue expected_broadcast_extras;
-  expected_broadcast_extras.SetBoolean("enabled", false);
-  expected_broadcast_extras.SetBoolean("managed", true);
-  EXPECT_GE(CountBroadcasts(fake_intent_helper_instance_->broadcasts(),
-                            kLocationServiceBroadcastAction,
-                            &expected_broadcast_extras),
-            1);
+  EXPECT_EQ(0UL, fake_intent_helper_instance_->broadcasts().size());
 
   fake_intent_helper_instance_->clear_broadcasts();
 
@@ -440,15 +423,11 @@ IN_PROC_BROWSER_TEST_F(ArcSettingsServiceTest, LocationServicePolicyTest) {
              nullptr);
   UpdatePolicy(policy);
 
-  // The pref is enabled and managed, and the corresponding broadcast is sent at
-  // least once.
+  // The pref is enabled and managed, but no broadcast is sent as the setting
+  // is not dynamically applied.
   EXPECT_TRUE(prefs->GetBoolean(prefs::kArcLocationServiceEnabled));
   EXPECT_TRUE(prefs->IsManagedPreference(prefs::kArcLocationServiceEnabled));
-  expected_broadcast_extras.SetBoolean("enabled", true);
-  EXPECT_GE(CountBroadcasts(fake_intent_helper_instance_->broadcasts(),
-                            kLocationServiceBroadcastAction,
-                            &expected_broadcast_extras),
-            1);
+  EXPECT_EQ(0UL, fake_intent_helper_instance_->broadcasts().size());
 
   fake_intent_helper_instance_->clear_broadcasts();
 
@@ -456,16 +435,10 @@ IN_PROC_BROWSER_TEST_F(ArcSettingsServiceTest, LocationServicePolicyTest) {
   policy.Erase(policy::key::kArcLocationServiceEnabled);
   UpdatePolicy(policy);
 
-  // The pref is disabled and unmanaged, and the corresponding broadcast is
-  // sent.
-  EXPECT_FALSE(prefs->GetBoolean(prefs::kArcLocationServiceEnabled));
+  // The pref is unmanaged, but no broadcast is sent as the setting is not
+  // dynamically applied.
   EXPECT_FALSE(prefs->IsManagedPreference(prefs::kArcLocationServiceEnabled));
-  expected_broadcast_extras.SetBoolean("enabled", false);
-  expected_broadcast_extras.SetBoolean("managed", false);
-  EXPECT_EQ(CountBroadcasts(fake_intent_helper_instance_->broadcasts(),
-                            kLocationServiceBroadcastAction,
-                            &expected_broadcast_extras),
-            1);
+  EXPECT_EQ(0UL, fake_intent_helper_instance_->broadcasts().size());
 }
 
 IN_PROC_BROWSER_TEST_F(ArcSettingsServiceTest, ProxyModePolicyTest) {

components/policy/resources/policy_templates.json

@@ -9827,7 +9827,7 @@
       'schema': { 'type': 'boolean' },
       'supported_on': ['chrome_os:53-'],
       'features': {
-        'dynamic_refresh': True,
+        'dynamic_refresh': False,
         'per_profile': False,
       },
       'example_value': False,
@@ -9849,7 +9849,7 @@
       'schema': { 'type': 'boolean' },
       'supported_on': ['chrome_os:57-'],
       'features': {
-        'dynamic_refresh': True,
+        'dynamic_refresh': False,
         'per_profile': False,
       },
       'example_value': False,