Inherit popup permission to incognito mode

This changes the popup permission to be inherited to incognito profiles.
While other content settings are not inherited due to security or privacy
concerns, inheriting sites allowed to show popups is save.

Bug: 762349
Change-Id: Ia31f6cea46edf8c705302ee4a414a40a8321756e
Reviewed-on: https://chromium-review.googlesource.com/672547Reviewed-by: Raymes Khoury <raymes@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515921}

chrome/browser/content_settings/host_content_settings_map_unittest.cc

@@ -848,8 +848,8 @@ TEST_F(HostContentSettingsMapTest, IncognitoInheritInitialAllow) {
 }
 
 TEST_F(HostContentSettingsMapTest, IncognitoInheritPopups) {
-  // The popup setting has an initial value of BLOCK, so  popup doesn't inherit
-  // settings to incognito
+  // The popup setting has an initial value of BLOCK, but it is allowed
+  // to inherit ALLOW settings because it doesn't provide access to user data.
   TestingProfile profile;
   Profile* otr_profile = profile.GetOffTheRecordProfile();
   HostContentSettingsMap* host_content_settings_map =
@@ -866,7 +866,7 @@ TEST_F(HostContentSettingsMapTest, IncognitoInheritPopups) {
             otr_map->GetContentSetting(
                 host, host, CONTENT_SETTINGS_TYPE_POPUPS, std::string()));
 
-  // Changing content settings on the main map should not affect the
+  // Changing content settings on the main map should affect the
   // incognito map.
   host_content_settings_map->SetContentSettingDefaultScope(
       host, GURL(), CONTENT_SETTINGS_TYPE_POPUPS, std::string(),
@@ -874,9 +874,9 @@ TEST_F(HostContentSettingsMapTest, IncognitoInheritPopups) {
   EXPECT_EQ(CONTENT_SETTING_ALLOW,
             host_content_settings_map->GetContentSetting(
                 host, host, CONTENT_SETTINGS_TYPE_POPUPS, std::string()));
-  EXPECT_EQ(CONTENT_SETTING_BLOCK,
-            otr_map->GetContentSetting(
-                host, host, CONTENT_SETTINGS_TYPE_POPUPS, std::string()));
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            otr_map->GetContentSetting(host, host, CONTENT_SETTINGS_TYPE_POPUPS,
+                                       std::string()));
 
   // Changing content settings on the incognito map should NOT affect the
   // main map.

components/content_settings/core/browser/content_settings_info.h

@@ -20,14 +20,15 @@ class ContentSettingsInfo {
  public:
   enum IncognitoBehavior {
     // Content setting will be inherited from regular to incognito profiles
-    // as usual.
-    // TODO(dullweber): Remove as soon as INHERIT_IF_LESS_PERMISSIVE was tested.
+    // as usual. This should only be used for features that don't allow access
+    // to user data e.g. popup blocker or features that are allowed by default.
     INHERIT_IN_INCOGNITO,
 
     // Content settings can be inherited if the setting is less permissive
     // than the initial default value of the content setting. Example: A setting
     // with an initial value of ASK will be inherited if it is set to BLOCK or
-    // ASK but ALLOW will become ASK in incognito mode.
+    // ASK but ALLOW will become ASK in incognito mode. This should be used for
+    // all settings that allow access to user data, e.g. geolocation.
     INHERIT_IF_LESS_PERMISSIVE
   };
 

components/content_settings/core/browser/content_settings_registry.cc

@@ -125,7 +125,7 @@ void ContentSettingsRegistry::Init() {
                          CONTENT_SETTING_SESSION_ONLY),
            WebsiteSettingsInfo::REQUESTING_DOMAIN_ONLY_SCOPE,
            WebsiteSettingsRegistry::ALL_PLATFORMS,
-           ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE);
+           ContentSettingsInfo::INHERIT_IN_INCOGNITO);
 
   Register(CONTENT_SETTINGS_TYPE_IMAGES, "images", CONTENT_SETTING_ALLOW,
            WebsiteSettingsInfo::SYNCABLE,
@@ -134,7 +134,7 @@ void ContentSettingsRegistry::Init() {
            ValidSettings(CONTENT_SETTING_ALLOW, CONTENT_SETTING_BLOCK),
            WebsiteSettingsInfo::TOP_LEVEL_ORIGIN_ONLY_SCOPE,
            WebsiteSettingsRegistry::DESKTOP,
-           ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE);
+           ContentSettingsInfo::INHERIT_IN_INCOGNITO);
 
   Register(CONTENT_SETTINGS_TYPE_JAVASCRIPT, "javascript",
            CONTENT_SETTING_ALLOW, WebsiteSettingsInfo::SYNCABLE,
@@ -144,7 +144,7 @@ void ContentSettingsRegistry::Init() {
            WebsiteSettingsInfo::TOP_LEVEL_ORIGIN_ONLY_SCOPE,
            WebsiteSettingsRegistry::DESKTOP |
                WebsiteSettingsRegistry::PLATFORM_ANDROID,
-           ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE);
+           ContentSettingsInfo::INHERIT_IN_INCOGNITO);
 
   Register(CONTENT_SETTINGS_TYPE_PLUGINS, "plugins",
            CONTENT_SETTING_DETECT_IMPORTANT_CONTENT,
@@ -164,7 +164,7 @@ void ContentSettingsRegistry::Init() {
            ValidSettings(CONTENT_SETTING_ALLOW, CONTENT_SETTING_BLOCK),
            WebsiteSettingsInfo::TOP_LEVEL_ORIGIN_ONLY_SCOPE,
            WebsiteSettingsRegistry::ALL_PLATFORMS,
-           ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE);
+           ContentSettingsInfo::INHERIT_IN_INCOGNITO);
 
   Register(CONTENT_SETTINGS_TYPE_GEOLOCATION, "geolocation",
            CONTENT_SETTING_ASK, WebsiteSettingsInfo::UNSYNCABLE,
@@ -226,7 +226,7 @@ void ContentSettingsRegistry::Init() {
            WebsiteSettingsInfo::TOP_LEVEL_ORIGIN_ONLY_SCOPE,
            WebsiteSettingsRegistry::DESKTOP |
                WebsiteSettingsRegistry::PLATFORM_ANDROID,
-           ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE);
+           ContentSettingsInfo::INHERIT_IN_INCOGNITO);
 
   Register(CONTENT_SETTINGS_TYPE_MIDI_SYSEX, "midi-sysex", CONTENT_SETTING_ASK,
            WebsiteSettingsInfo::SYNCABLE, WhitelistedSchemes(),
@@ -255,7 +255,7 @@ void ContentSettingsRegistry::Init() {
            WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE,
            WebsiteSettingsRegistry::DESKTOP |
                WebsiteSettingsRegistry::PLATFORM_ANDROID,
-           ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE);
+           ContentSettingsInfo::INHERIT_IN_INCOGNITO);
 
   Register(CONTENT_SETTINGS_TYPE_BACKGROUND_SYNC, "background-sync",
            CONTENT_SETTING_ALLOW, WebsiteSettingsInfo::UNSYNCABLE,
@@ -264,7 +264,7 @@ void ContentSettingsRegistry::Init() {
            WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE,
            WebsiteSettingsRegistry::DESKTOP |
                WebsiteSettingsRegistry::PLATFORM_ANDROID,
-           ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE);
+           ContentSettingsInfo::INHERIT_IN_INCOGNITO);
 
   Register(CONTENT_SETTINGS_TYPE_AUTOPLAY, "autoplay", CONTENT_SETTING_ALLOW,
            WebsiteSettingsInfo::UNSYNCABLE, WhitelistedSchemes(),
@@ -272,7 +272,7 @@ void ContentSettingsRegistry::Init() {
            WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE,
            WebsiteSettingsRegistry::DESKTOP |
                WebsiteSettingsRegistry::PLATFORM_ANDROID,
-           ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE);
+           ContentSettingsInfo::INHERIT_IN_INCOGNITO);
 
   Register(CONTENT_SETTINGS_TYPE_SOUND, "sound", CONTENT_SETTING_ALLOW,
            WebsiteSettingsInfo::UNSYNCABLE, WhitelistedSchemes(),
@@ -280,7 +280,7 @@ void ContentSettingsRegistry::Init() {
            WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE,
            WebsiteSettingsRegistry::DESKTOP |
                WebsiteSettingsRegistry::PLATFORM_ANDROID,
-           ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE);
+           ContentSettingsInfo::INHERIT_IN_INCOGNITO);
 
   Register(CONTENT_SETTINGS_TYPE_ADS, "subresource-filter",
            CONTENT_SETTING_BLOCK, WebsiteSettingsInfo::UNSYNCABLE,
@@ -289,7 +289,7 @@ void ContentSettingsRegistry::Init() {
            WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE,
            WebsiteSettingsRegistry::DESKTOP |
                WebsiteSettingsRegistry::PLATFORM_ANDROID,
-           ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE);
+           ContentSettingsInfo::INHERIT_IN_INCOGNITO);
 
   // Content settings that aren't used to store any data. TODO(raymes): use a
   // different mechanism rather than content settings to represent these.
@@ -334,7 +334,7 @@ void ContentSettingsRegistry::Init() {
            WebsiteSettingsInfo::REQUESTING_ORIGIN_ONLY_SCOPE,
            WebsiteSettingsRegistry::DESKTOP |
                WebsiteSettingsRegistry::PLATFORM_ANDROID,
-           ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE);
+           ContentSettingsInfo::INHERIT_IN_INCOGNITO);
 }
 
 void ContentSettingsRegistry::Register(

components/content_settings/core/browser/content_settings_registry_unittest.cc

@@ -72,7 +72,7 @@ TEST_F(ContentSettingsRegistryTest, Properties) {
   // Check the other properties are populated correctly.
   EXPECT_TRUE(info->IsSettingValid(CONTENT_SETTING_SESSION_ONLY));
   EXPECT_FALSE(info->IsSettingValid(CONTENT_SETTING_ASK));
-  EXPECT_EQ(ContentSettingsInfo::INHERIT_IF_LESS_PERMISSIVE,
+  EXPECT_EQ(ContentSettingsInfo::INHERIT_IN_INCOGNITO,
             info->incognito_behavior());
 
   // Check the WebsiteSettingsInfo is populated correctly.
@@ -125,6 +125,41 @@ TEST_F(ContentSettingsRegistryTest, Iteration) {
   EXPECT_TRUE(cookies_found);
 }
 
+// Settings that control access to user data should not be inherited.
+// Check that only safe settings are inherited in incognito.
+TEST_F(ContentSettingsRegistryTest, Inheritance) {
+  // These settings are safe to inherit in incognito mode because they only
+  // disable features like popup blocking, download blocking or ad blocking.
+  // They do not allow access to user data.
+  const ContentSettingsType whitelist[] = {
+      CONTENT_SETTINGS_TYPE_POPUPS,               //
+      CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS,  //
+      CONTENT_SETTINGS_TYPE_ADS,                  //
+      CONTENT_SETTINGS_TYPE_DURABLE_STORAGE,
+  };
+
+  for (const ContentSettingsInfo* info : *registry()) {
+    SCOPED_TRACE("Content setting: " + info->website_settings_info()->name());
+    // TODO(crbug.com/781756): Check IsSettingValid() because "protocol-handler"
+    // and "mixed-script" don't have a proper initial default value.
+    if (info->IsSettingValid(CONTENT_SETTING_ALLOW) &&
+        info->GetInitialDefaultSetting() == CONTENT_SETTING_ALLOW) {
+      // ALLOW-by-default settings are not affected by incognito_behavior, so
+      // they should be marked as INHERIT_IN_INCOGNITO.
+      EXPECT_EQ(info->incognito_behavior(),
+                ContentSettingsInfo::INHERIT_IN_INCOGNITO);
+      continue;
+    }
+    ContentSettingsType type = info->website_settings_info()->type();
+    if (info->incognito_behavior() ==
+            ContentSettingsInfo::INHERIT_IN_INCOGNITO &&
+        std::find(std::begin(whitelist), std::end(whitelist), type) ==
+            std::end(whitelist)) {
+      FAIL() << "Content setting not whitelisted.";
+    }
+  }
+}
+
 TEST_F(ContentSettingsRegistryTest, IsDefaultSettingValid) {
   const ContentSettingsInfo* info =
       registry()->Get(CONTENT_SETTINGS_TYPE_COOKIES);