Make enterprise-disabled DSE behavior slightly safer

This changes the code such that when the DSE has been disabled by
enterprise policy and is then re-enabled, we revert the DSE setting to
BLOCK. We only do this if the origin that is becoming the DSE wasn't
previously granted permission.

Bug: 799534
Change-Id: I9a061262e6e2886ff04ffa97e487ff0eb200d892
Reviewed-on: https://chromium-review.googlesource.com/812588
Commit-Queue: Raymes Khoury <raymes@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523614}

chrome/browser/android/search_permissions/search_permissions_service.cc

@@ -142,6 +142,7 @@ void SearchPermissionsService::Factory::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterDictionaryPref(prefs::kDSEGeolocationSettingDeprecated);
   registry->RegisterDictionaryPref(prefs::kDSEPermissionsSettings);
+  registry->RegisterBooleanPref(prefs::kDSEWasDisabledByPolicy, false);
 }
 
 SearchPermissionsService::SearchPermissionsService(Profile* profile)
@@ -320,11 +321,11 @@ void SearchPermissionsService::InitializeSettingsIfNeeded() {
   GURL dse_origin = delegate_->GetDSEOrigin().GetURL();
 
   // This can happen in tests or if the DSE is disabled by policy. If that's
-  // the case, we restore the old settings and erase the pref. This means that
-  // things will be re-initialized to defaults when the DSE is no longer under
-  // enterprise policy.
+  // the case, we restore the old settings and erase the pref.
   if (!dse_origin.is_valid()) {
     if (pref_service_->HasPrefPath(prefs::kDSEPermissionsSettings)) {
+      pref_service_->SetBoolean(prefs::kDSEWasDisabledByPolicy, true);
+
       PrefValue pref = GetDSEPref();
       GURL old_dse_origin(pref.dse_origin);
       RestoreOldSettingAndReturnPrevious(old_dse_origin,
@@ -342,11 +343,19 @@ void SearchPermissionsService::InitializeSettingsIfNeeded() {
     return;
   }
 
+  // If we get to here, the DSE is not disabled by enterprise policy. If it was
+  // previously enterprise controlled, we initialize the setting to BLOCK since
+  // we don't know what the user's setting was previously.
+  bool was_enterprise_controlled =
+      pref_service_->GetBoolean(prefs::kDSEWasDisabledByPolicy);
+  pref_service_->ClearPref(prefs::kDSEWasDisabledByPolicy);
+
   // Initialize the pref for geolocation if it hasn't been initialized yet.
   if (!pref_service_->HasPrefPath(prefs::kDSEPermissionsSettings)) {
     ContentSetting geolocation_setting_to_restore =
         GetContentSetting(dse_origin, CONTENT_SETTINGS_TYPE_GEOLOCATION);
     ContentSetting dse_geolocation_setting = geolocation_setting_to_restore;
+
     bool reset_disclosure = true;
     // Migrate the old geolocation pref if it exists.
     if (pref_service_->HasPrefPath(prefs::kDSEGeolocationSettingDeprecated)) {
@@ -372,7 +381,9 @@ void SearchPermissionsService::InitializeSettingsIfNeeded() {
     } else if (dse_geolocation_setting == CONTENT_SETTING_ASK) {
       // If the user hasn't explicitly allowed or blocked geolocation for the
       // DSE, initialize it to allowed.
-      dse_geolocation_setting = CONTENT_SETTING_ALLOW;
+      dse_geolocation_setting = was_enterprise_controlled
+                                    ? CONTENT_SETTING_BLOCK
+                                    : CONTENT_SETTING_ALLOW;
     }
 
     // Update the content setting with the auto-grants for the DSE.
@@ -399,8 +410,11 @@ void SearchPermissionsService::InitializeSettingsIfNeeded() {
     ContentSetting dse_notifications_setting = notifications_setting_to_restore;
     // If the user hasn't explicitly allowed or blocked notifications for the
     // DSE, initialize it to allowed.
-    if (dse_notifications_setting == CONTENT_SETTING_ASK)
-      dse_notifications_setting = CONTENT_SETTING_ALLOW;
+    if (dse_notifications_setting == CONTENT_SETTING_ASK) {
+      dse_notifications_setting = was_enterprise_controlled
+                                      ? CONTENT_SETTING_BLOCK
+                                      : CONTENT_SETTING_ALLOW;
+    }
 
     // Update the content setting with the auto-grants for the DSE.
     SetContentSetting(dse_origin, CONTENT_SETTINGS_TYPE_NOTIFICATIONS,

chrome/browser/android/search_permissions/search_permissions_service_unittest.cc

@@ -152,8 +152,10 @@ class SearchPermissionsServiceTest : public testing::Test {
   // |clear_pref| is true, then it simulates the first time the service is ever
   // created.
   void ReinitializeService(bool clear_pref) {
-    if (clear_pref)
+    if (clear_pref) {
       profile()->GetPrefs()->ClearPref(prefs::kDSEPermissionsSettings);
+      profile()->GetPrefs()->ClearPref(prefs::kDSEWasDisabledByPolicy);
+    }
 
     GetService()->OnDSEChanged();
   }
@@ -532,13 +534,14 @@ TEST_F(SearchPermissionsServiceTest, DSEChangedButDisabled) {
       CONTENT_SETTING_ASK,
       GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS));
 
-  // Now disable enterprise policy. We revert to the default ALLOW behavior.
+  // Now disable enterprise policy. The settings will be BLOCK because we don't
+  // know what the user's previous DSE setting was.
   test_delegate()->ChangeDSEOrigin(kGoogleAusURL);
   EXPECT_EQ(
       CONTENT_SETTING_BLOCK,
       GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_GEOLOCATION));
   EXPECT_EQ(
-      CONTENT_SETTING_ALLOW,
+      CONTENT_SETTING_BLOCK,
       GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS));
 }
 
@@ -581,12 +584,16 @@ TEST_F(SearchPermissionsServiceTest, DSEInitializedButDisabled) {
   EXPECT_FALSE(
       profile()->GetPrefs()->HasPrefPath(prefs::kDSEPermissionsSettings));
 
-  // Re-enabling the DSE origin should revert to the default ALLOW behavior.
+  // Re-enabling the DSE origin should set the permissions to BLOCK for safety,
+  // except for notifications where the user had manually granted permission.
+  SetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                    CONTENT_SETTING_ALLOW);
+
   test_delegate()->set_dse_origin(kGoogleAusURL);
   ReinitializeService(/*clear_pref=*/false);
 
   EXPECT_EQ(
-      CONTENT_SETTING_ALLOW,
+      CONTENT_SETTING_BLOCK,
       GetContentSetting(kGoogleAusURL, CONTENT_SETTINGS_TYPE_GEOLOCATION));
   EXPECT_EQ(
       CONTENT_SETTING_ALLOW,

chrome/common/pref_names.cc

@@ -2351,6 +2351,10 @@ const char kDSEGeolocationSettingDeprecated[] = "dse_geolocation_setting";
 // that they can be restored if the DSE is ever changed.
 const char kDSEPermissionsSettings[] = "dse_permissions_settings";
 
+// A boolean indicating whether the DSE was previously disabled by enterprise
+// policy.
+const char kDSEWasDisabledByPolicy[] = "dse_was_disabled_by_policy";
+
 // A dictionary of manifest URLs of Web Share Targets to a dictionary containing
 // attributes of its share_target field found in its manifest. Each key in the
 // dictionary is the name of the attribute, and the value is the corresponding

chrome/common/pref_names.h

@@ -842,6 +842,7 @@ extern const char kSearchGeolocationPostDisclosureMetricsRecorded[];
 extern const char kDSEGeolocationSettingDeprecated[];
 
 extern const char kDSEPermissionsSettings[];
+extern const char kDSEWasDisabledByPolicy[];
 
 extern const char kWebShareVisitedTargets[];