[Extensions] Add code to migrate to a new withholding pref key

This change adds a new pref key to represent if permission withholding
is enabled on an extension and code to migrate all existing extension
prefs to use the new key if it doesn't exist for an extension on
startup. It also changes the logic for setting the pref on
installation to only be set for extensions which can have permissions
withheld from them.

Bug: 984069
Change-Id: Ibbde3a7bdb0000e71f3d18f451ac9c8792714330
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1911479
Commit-Queue: Tim Judkins <tjudkins@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721823}

chrome/browser/extensions/extension_prefs_unittest.cc

@@ -1107,4 +1107,77 @@ class ExtensionPrefsObsoletePrefRemoval : public ExtensionPrefsTest {
 
 TEST_F(ExtensionPrefsObsoletePrefRemoval, ExtensionPrefsObsoletePrefRemoval) {}
 
+using ExtensionPrefsWithholdingTest = testing::Test;
+
+// Tests the migration from the old withholding pref key to the new one.
+TEST_F(ExtensionPrefsWithholdingTest, OldWithholdingPrefMigration) {
+  constexpr char kOldPrefKey[] = "extension_can_script_all_urls";
+  constexpr char kNewPrefKey[] = "withholding_permissions";
+
+  content::BrowserTaskEnvironment task_environment_;
+  TestExtensionPrefs prefs(base::ThreadTaskRunnerHandle::Get());
+
+  std::string previous_false_id = prefs.AddExtensionAndReturnId("Old false");
+  std::string previous_true_id = prefs.AddExtensionAndReturnId("Old true");
+  std::string previous_empty_id = prefs.AddExtensionAndReturnId("Old empty");
+  std::string force_installed_id =
+      prefs
+          .AddExtensionWithLocation("Force installed",
+                                    Manifest::EXTERNAL_POLICY)
+          ->id();
+
+  // We need to explicitly remove the default value for the new pref as it is
+  // added on install by default.
+  prefs.prefs()->UpdateExtensionPref(previous_false_id, kNewPrefKey, nullptr);
+  prefs.prefs()->UpdateExtensionPref(previous_true_id, kNewPrefKey, nullptr);
+  prefs.prefs()->UpdateExtensionPref(previous_empty_id, kNewPrefKey, nullptr);
+
+  prefs.prefs()->UpdateExtensionPref(previous_false_id, kOldPrefKey,
+                                     std::make_unique<base::Value>(false));
+  prefs.prefs()->UpdateExtensionPref(previous_true_id, kOldPrefKey,
+                                     std::make_unique<base::Value>(true));
+
+  // First make sure that all prefs start out as we expect them to be.
+  bool bool_value = false;
+  EXPECT_TRUE(prefs.prefs()->ReadPrefAsBoolean(previous_false_id, kOldPrefKey,
+                                               &bool_value));
+  EXPECT_FALSE(bool_value);
+
+  EXPECT_TRUE(prefs.prefs()->ReadPrefAsBoolean(previous_true_id, kOldPrefKey,
+                                               &bool_value));
+  EXPECT_TRUE(bool_value);
+
+  EXPECT_FALSE(prefs.prefs()->ReadPrefAsBoolean(previous_empty_id, kOldPrefKey,
+                                                &bool_value));
+  EXPECT_FALSE(prefs.prefs()->ReadPrefAsBoolean(force_installed_id, kOldPrefKey,
+                                                &bool_value));
+
+  EXPECT_FALSE(prefs.prefs()->ReadPrefAsBoolean(previous_false_id, kNewPrefKey,
+                                                &bool_value));
+  EXPECT_FALSE(prefs.prefs()->ReadPrefAsBoolean(previous_true_id, kNewPrefKey,
+                                                &bool_value));
+  EXPECT_FALSE(prefs.prefs()->ReadPrefAsBoolean(previous_empty_id, kNewPrefKey,
+                                                &bool_value));
+  EXPECT_FALSE(prefs.prefs()->ReadPrefAsBoolean(force_installed_id, kNewPrefKey,
+                                                &bool_value));
+
+  // Now we reload the prefs and verify the migration happens.
+  prefs.RecreateExtensionPrefs();
+
+  EXPECT_TRUE(prefs.prefs()->ReadPrefAsBoolean(previous_false_id, kNewPrefKey,
+                                               &bool_value));
+  EXPECT_TRUE(bool_value);
+
+  EXPECT_TRUE(prefs.prefs()->ReadPrefAsBoolean(previous_true_id, kNewPrefKey,
+                                               &bool_value));
+  EXPECT_FALSE(bool_value);
+
+  EXPECT_TRUE(prefs.prefs()->ReadPrefAsBoolean(previous_empty_id, kNewPrefKey,
+                                               &bool_value));
+  EXPECT_FALSE(bool_value);
+
+  EXPECT_FALSE(prefs.prefs()->ReadPrefAsBoolean(force_installed_id, kNewPrefKey,
+                                                &bool_value));
+}
+
 }  // namespace extensions

chrome/browser/extensions/scripting_permissions_modifier.cc

@@ -10,6 +10,7 @@
 #include "content/public/common/url_constants.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_util.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest_handlers/permissions_parser.h"
 #include "extensions/common/permissions/permission_set.h"
@@ -20,21 +21,6 @@ namespace extensions {
 
 namespace {
 
-// Returns true if Chrome can potentially withhold permissions from the
-// extension.
-bool CanWithholdFromExtension(const Extension& extension) {
-  // Some extensions must retain privilege to all requested host permissions.
-  // Specifically, extensions that don't show up in chrome:extensions (where
-  // withheld permissions couldn't be granted), extensions that are part of
-  // chrome or corporate policy, and extensions that are whitelisted to script
-  // everywhere must always have permission to run on a page.
-  return extension.ShouldDisplayInExtensionSettings() &&
-         !Manifest::IsPolicyLocation(extension.location()) &&
-         !Manifest::IsComponentLocation(extension.location()) &&
-         !PermissionsData::CanExecuteScriptEverywhere(extension.id(),
-                                                      extension.location());
-}
-
 // Iterates over |requested_permissions| and returns a permission set of any
 // permissions that should be  granted. These include any non-host
 // permissions or host permissions that are present in
@@ -87,7 +73,7 @@ std::unique_ptr<const PermissionSet> PartitionHostPermissions(
 // by the runtime host permissions experiment.
 bool ShouldConsiderExtension(const Extension& extension) {
   // Certain extensions are always exempt from having permissions withheld.
-  if (!CanWithholdFromExtension(extension))
+  if (!util::CanWithholdPermissionsFromExtension(extension))
     return false;
 
   return true;
@@ -186,7 +172,7 @@ void ScriptingPermissionsModifier::SetWithholdHostPermissions(
 
   // Set the pref first, so that listeners for permission changes get the proper
   // value if they query HasWithheldHostPermissions().
-  extension_prefs_->SetShouldWithholdPermissions(extension_->id(),
+  extension_prefs_->SetWithholdingPermissions(extension_->id(),
                                               should_withhold);
 
   if (should_withhold)
@@ -198,7 +184,7 @@ void ScriptingPermissionsModifier::SetWithholdHostPermissions(
 bool ScriptingPermissionsModifier::HasWithheldHostPermissions() const {
   DCHECK(CanAffectExtension());
 
-  return extension_prefs_->GetShouldWithholdPermissions(extension_->id());
+  return extension_prefs_->GetWithholdingPermissions(extension_->id());
 }
 
 bool ScriptingPermissionsModifier::CanAffectExtension() const {
@@ -367,8 +353,7 @@ ScriptingPermissionsModifier::WithholdPermissionsIfNecessary(
   if (extension.creation_flags() & Extension::WITHHOLD_PERMISSIONS) {
     should_withhold = true;
   } else {
-    should_withhold =
-        extension_prefs.GetShouldWithholdPermissions(extension.id());
+    should_withhold = extension_prefs.GetWithholdingPermissions(extension.id());
   }
 
   if (!should_withhold)

chrome/browser/extensions/test_extension_prefs.cc

@@ -138,11 +138,7 @@ void TestExtensionPrefs::RecreateExtensionPrefs() {
 
 scoped_refptr<Extension> TestExtensionPrefs::AddExtension(
     const std::string& name) {
-  base::DictionaryValue dictionary;
-  dictionary.SetString(manifest_keys::kName, name);
-  dictionary.SetString(manifest_keys::kVersion, "0.1");
-  dictionary.SetInteger(manifest_keys::kManifestVersion, 2);
-  return AddExtensionWithManifest(dictionary, Manifest::INTERNAL);
+  return AddExtensionWithLocation(name, Manifest::INTERNAL);
 }
 
 scoped_refptr<Extension> TestExtensionPrefs::AddApp(const std::string& name) {
@@ -155,6 +151,16 @@ scoped_refptr<Extension> TestExtensionPrefs::AddApp(const std::string& name) {
 
 }
 
+scoped_refptr<Extension> TestExtensionPrefs::AddExtensionWithLocation(
+    const std::string& name,
+    Manifest::Location location) {
+  base::DictionaryValue dictionary;
+  dictionary.SetString(manifest_keys::kName, name);
+  dictionary.SetString(manifest_keys::kVersion, "0.1");
+  dictionary.SetInteger(manifest_keys::kManifestVersion, 2);
+  return AddExtensionWithManifest(dictionary, location);
+}
+
 scoped_refptr<Extension> TestExtensionPrefs::AddExtensionWithManifest(
     const base::DictionaryValue& manifest, Manifest::Location location) {
   return AddExtensionWithManifestAndFlags(manifest, location,

chrome/browser/extensions/test_extension_prefs.h

@@ -66,6 +66,11 @@ class TestExtensionPrefs {
   // As above, but the extension is an app.
   scoped_refptr<Extension> AddApp(const std::string& name);
 
+  // Similar to AddExtension, but with a specified location.
+  scoped_refptr<Extension> AddExtensionWithLocation(
+      const std::string& name,
+      Manifest::Location location);
+
   // Similar to AddExtension, but takes a dictionary with manifest values.
   scoped_refptr<Extension> AddExtensionWithManifest(
       const base::DictionaryValue& manifest,

extensions/browser/extension_prefs.cc

@@ -29,6 +29,7 @@
 #include "extensions/browser/extension_prefs_factory.h"
 #include "extensions/browser/extension_prefs_observer.h"
 #include "extensions/browser/extension_system.h"
+#include "extensions/browser/extension_util.h"
 #include "extensions/browser/install_flag.h"
 #include "extensions/browser/pref_names.h"
 #include "extensions/common/constants.h"
@@ -135,12 +136,18 @@ constexpr const char kPrefAllowFileAccess[] = "newAllowFileAccess";
 constexpr const char kPrefActivePermissions[] = "active_permissions";
 constexpr const char kPrefGrantedPermissions[] = "granted_permissions";
 
-// A preference indicating if an extension should be granted all the requested
-// host permissions without requiring explicit runtime permission from the user.
-// The preference name is different for legacy reasons.
+// Pref that was previously used to indicate if host permissions should be
+// withheld. Due to the confusing name and the need to logically invert it when
+// being used, we transitioned to use kPrefWithholdingPermissions
+// instead.
 const char kGrantExtensionAllHostPermissions[] =
     "extension_can_script_all_urls";
 
+// A preference indicating if requested host permissions are being withheld from
+// the extension, requiring them to be granted through the permissions API or
+// runtime host permissions.
+const char kPrefWithholdingPermissions[] = "withholding_permissions";
+
 // The set of permissions that were granted at runtime, rather than at install
 // time. This includes permissions granted through the permissions API and
 // runtime host permissions.
@@ -1031,36 +1038,28 @@ void ExtensionPrefs::SetActivePermissions(const std::string& extension_id,
       extension_id, kPrefActivePermissions, permissions);
 }
 
-void ExtensionPrefs::SetShouldWithholdPermissions(
-    const ExtensionId& extension_id,
+void ExtensionPrefs::SetWithholdingPermissions(const ExtensionId& extension_id,
                                                bool should_withhold) {
-  // NOTE: For legacy reasons, the preference stores whether the extension was
-  // allowed access to all its host permissions, rather than if Chrome should
-  // withhold permissions. Invert the boolean for backwards compatibility.
-  bool permissions_allowed = !should_withhold;
-  UpdateExtensionPref(extension_id, kGrantExtensionAllHostPermissions,
-                      std::make_unique<base::Value>(permissions_allowed));
+  UpdateExtensionPref(extension_id, kPrefWithholdingPermissions,
+                      std::make_unique<base::Value>(should_withhold));
 }
 
-bool ExtensionPrefs::GetShouldWithholdPermissions(
+bool ExtensionPrefs::GetWithholdingPermissions(
     const ExtensionId& extension_id) const {
   bool permissions_allowed = false;
-  if (ReadPrefAsBoolean(extension_id, kGrantExtensionAllHostPermissions,
+  if (ReadPrefAsBoolean(extension_id, kPrefWithholdingPermissions,
                         &permissions_allowed)) {
-    // NOTE: For legacy reasons, the preference stores whether the extension was
-    // allowed access to all its host permissions, rather than if Chrome should
-    // withhold permissions. Invert the boolean for backwards compatibility.
-    return !permissions_allowed;
+    return permissions_allowed;
   }
 
   // If no pref was found, we use the default.
   return kDefaultWithholdingBehavior;
 }
 
-bool ExtensionPrefs::HasShouldWithholdPermissionsSetting(
+bool ExtensionPrefs::HasWithholdingPermissionsSetting(
     const ExtensionId& extension_id) const {
   const base::DictionaryValue* ext = GetExtensionPref(extension_id);
-  return ext && ext->HasKey(kGrantExtensionAllHostPermissions);
+  return ext && ext->HasKey(kPrefWithholdingPermissions);
 }
 
 std::unique_ptr<const PermissionSet>
@@ -1914,6 +1913,8 @@ ExtensionPrefs::ExtensionPrefs(
   }
 
   InitPrefStore();
+
+  MigrateToNewWithholdingPref();
 }
 
 AppSorting* ExtensionPrefs::app_sorting() const {
@@ -2024,17 +2025,17 @@ void ExtensionPrefs::PopulateExtensionInfoPrefs(
   if (dnr_ruleset_checksum)
     extension_dict->SetInteger(kPrefDNRRulesetChecksum, *dnr_ruleset_checksum);
 
+  if (util::CanWithholdPermissionsFromExtension(*extension)) {
     // If the withhold permission creation flag is present it takes precedence
     // over any previous stored value.
     if (extension->creation_flags() & Extension::WITHHOLD_PERMISSIONS) {
-    extension_dict->SetBoolean(kGrantExtensionAllHostPermissions, false);
-  } else if (!HasShouldWithholdPermissionsSetting(extension->id())) {
+      extension_dict->SetBoolean(kPrefWithholdingPermissions, true);
+    } else if (!HasWithholdingPermissionsSetting(extension->id())) {
       // If no withholding creation flag was specified and there is no value
       // stored already, we set the default value.
-    // NOTE: For legacy reasons the value is inverted here as the pref itself
-    // stores if the extension was allowed access to all its host permissions.
-    extension_dict->SetBoolean(kGrantExtensionAllHostPermissions,
-                               !kDefaultWithholdingBehavior);
+      extension_dict->SetBoolean(kPrefWithholdingPermissions,
+                                 kDefaultWithholdingBehavior);
+    }
   }
 
   base::FilePath::StringType path = MakePathRelative(install_directory_,
@@ -2198,4 +2199,47 @@ void ExtensionPrefs::MigrateObsoleteExtensionPrefs() {
   }
 }
 
+void ExtensionPrefs::MigrateToNewWithholdingPref() {
+  std::unique_ptr<ExtensionsInfo> extensions_info(GetInstalledExtensionsInfo());
+
+  for (const auto& info : *extensions_info) {
+    const ExtensionId& extension_id = info->extension_id;
+    // The manifest may be null in some cases, such as unpacked extensions
+    // retrieved from the Preference file.
+    if (!info->extension_manifest)
+      continue;
+
+    // If the new key is present in the prefs already, we don't need to check
+    // further.
+    bool value = false;
+    if (ReadPrefAsBoolean(extension_id, kPrefWithholdingPermissions, &value)) {
+      continue;
+    }
+
+    // We only want to migrate extensions we can actually withhold permissions
+    // from.
+    Manifest::Type type =
+        Manifest::GetTypeFromManifestValue(*info->extension_manifest);
+    Manifest::Location location = info->extension_location;
+    if (!util::CanWithholdPermissionsFromExtension(extension_id, type,
+                                                   location))
+      continue;
+
+    bool old_pref_value = false;
+    // If there was an old preference set, use the same (conceptual) value.
+    // Otherwise, use the default setting.
+    bool new_pref_value = kDefaultWithholdingBehavior;
+    if (ReadPrefAsBoolean(extension_id, kGrantExtensionAllHostPermissions,
+                          &old_pref_value)) {
+      // We invert the value as the previous pref stored if the extension was
+      // granted all the requested permissions, whereas the new pref stores if
+      // requested permissions are currently being withheld.
+      new_pref_value = !old_pref_value;
+    }
+
+    UpdateExtensionPref(extension_id, kPrefWithholdingPermissions,
+                        std::make_unique<base::Value>(new_pref_value));
+  }
+}
+
 }  // namespace extensions

extensions/browser/extension_prefs.h

@@ -397,9 +397,9 @@ class ExtensionPrefs : public KeyedService {
   // Sets/Gets the value indicating if an extension should be granted all the
   // requested host permissions without requiring explicit runtime-granted
   // permissions from the user.
-  void SetShouldWithholdPermissions(const ExtensionId& extension_id,
+  void SetWithholdingPermissions(const ExtensionId& extension_id,
                                  bool should_withhold);
-  bool GetShouldWithholdPermissions(const ExtensionId& extension_id) const;
+  bool GetWithholdingPermissions(const ExtensionId& extension_id) const;
 
   // Returns the set of runtime-granted permissions. These are permissions that
   // the user explicitly approved at runtime, rather than install time (such
@@ -615,6 +615,11 @@ class ExtensionPrefs : public KeyedService {
   // extension's dictionary, which is keyed on the extension ID.
   void MigrateObsoleteExtensionPrefs();
 
+  // Updates an extension to use the new withholding pref key if it doesn't have
+  // it yet, removing the old key in the process.
+  // TODO(tjudkins): Remove this and the obsolete key in M83.
+  void MigrateToNewWithholdingPref();
+
   // When called before the ExtensionService is created, alerts that are
   // normally suppressed in first run will still trigger.
   static void SetRunAlertsInFirstRunForTest();
@@ -782,8 +787,7 @@ class ExtensionPrefs : public KeyedService {
 
   // Returns true if the prefs have any permission withholding setting stored
   // for a given extension.
-  bool HasShouldWithholdPermissionsSetting(
-      const ExtensionId& extension_id) const;
+  bool HasWithholdingPermissionsSetting(const ExtensionId& extension_id) const;
 
   content::BrowserContext* browser_context_;
 

extensions/browser/extension_util.cc

@@ -8,12 +8,14 @@
 #include "content/public/browser/site_instance.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension.h"
 #include "extensions/common/features/behavior_feature.h"
 #include "extensions/common/features/feature.h"
 #include "extensions/common/features/feature_provider.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/manifest_handlers/incognito_info.h"
 #include "extensions/common/manifest_handlers/shared_module_info.h"
+#include "extensions/common/permissions/permissions_data.h"
 
 namespace extensions {
 namespace util {
@@ -128,5 +130,24 @@ bool MapUrlToLocalFilePath(const ExtensionSet* extensions,
   return true;
 }
 
+bool CanWithholdPermissionsFromExtension(const Extension& extension) {
+  return CanWithholdPermissionsFromExtension(
+      extension.id(), extension.GetType(), extension.location());
+}
+
+bool CanWithholdPermissionsFromExtension(const ExtensionId& extension_id,
+                                         Manifest::Type type,
+                                         Manifest::Location location) {
+  // Some extensions must retain privilege to all requested host permissions.
+  // Specifically, extensions that don't show up in chrome:extensions (where
+  // withheld permissions couldn't be granted), extensions that are part of
+  // chrome or corporate policy, and extensions that are whitelisted to script
+  // everywhere must always have permission to run on a page.
+  return Extension::ShouldDisplayInExtensionSettings(type, location) &&
+         !Manifest::IsPolicyLocation(location) &&
+         !Manifest::IsComponentLocation(location) &&
+         !PermissionsData::CanExecuteScriptEverywhere(extension_id, location);
+}
+
 }  // namespace util
 }  // namespace extensions

extensions/browser/extension_util.h

@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "extensions/common/manifest.h"
 #include "url/gurl.h"
 
 namespace base {
@@ -60,6 +61,12 @@ bool MapUrlToLocalFilePath(const ExtensionSet* extensions,
                            bool use_blocking_api,
                            base::FilePath* file_path);
 
+// Returns true if the browser can potentially withhold permissions from the
+// extension.
+bool CanWithholdPermissionsFromExtension(const Extension& extension);
+bool CanWithholdPermissionsFromExtension(const std::string& extension_id,
+                                         const Manifest::Type type,
+                                         const Manifest::Location location);
 }  // namespace util
 }  // namespace extensions
 

extensions/common/extension.cc

@@ -326,6 +326,33 @@ GURL Extension::GetBaseURLFromExtensionId(const std::string& extension_id) {
                             url::kStandardSchemeSeparator, extension_id}));
 }
 
+// static
+bool Extension::ShouldDisplayInExtensionSettings(Manifest::Type type,
+                                                 Manifest::Location location) {
+  // Don't show for themes since the settings UI isn't really useful for them.
+  if (type == Manifest::TYPE_THEME)
+    return false;
+
+  // Hide component extensions because they are only extensions as an
+  // implementation detail of Chrome.
+  if (Manifest::IsComponentLocation(location) &&
+      !base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kShowComponentExtensionOptions)) {
+    return false;
+  }
+
+  // Unless they are unpacked, never show hosted apps. Note: We intentionally
+  // show packaged apps and platform apps because there are some pieces of
+  // functionality that are only available in chrome://extensions/ but which
+  // are needed for packaged and platform apps. For example, inspecting
+  // background pages. See http://crbug.com/116134.
+  if (!Manifest::IsUnpackedLocation(location) &&
+      type == Manifest::TYPE_HOSTED_APP)
+    return false;
+
+  return true;
+}
+
 bool Extension::OverlapsWithOrigin(const GURL& origin) const {
   if (url() == origin)
     return true;
@@ -361,27 +388,7 @@ bool Extension::ShouldDisplayInNewTabPage() const {
 }
 
 bool Extension::ShouldDisplayInExtensionSettings() const {
-  // Don't show for themes since the settings UI isn't really useful for them.
-  if (is_theme())
-    return false;
-
-  // Hide component extensions because they are only extensions as an
-  // implementation detail of Chrome.
-  if (extensions::Manifest::IsComponentLocation(location()) &&
-      !base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kShowComponentExtensionOptions)) {
-    return false;
-  }
-
-  // Unless they are unpacked, never show hosted apps. Note: We intentionally
-  // show packaged apps and platform apps because there are some pieces of
-  // functionality that are only available in chrome://extensions/ but which
-  // are needed for packaged and platform apps. For example, inspecting
-  // background pages. See http://crbug.com/116134.
-  if (!Manifest::IsUnpackedLocation(location()) && is_hosted_app())
-    return false;
-
-  return true;
+  return Extension::ShouldDisplayInExtensionSettings(GetType(), location());
 }
 
 bool Extension::ShouldExposeViaManagementAPI() const {

extensions/common/extension.h

@@ -220,6 +220,11 @@ class Extension : public base::RefCountedThreadSafe<Extension> {
   // Returns the base extension url for a given |extension_id|.
   static GURL GetBaseURLFromExtensionId(const ExtensionId& extension_id);
 
+  // Returns true if the extension should be displayed in the extension
+  // settings page (i.e. chrome://extensions).
+  static bool ShouldDisplayInExtensionSettings(Manifest::Type type,
+                                               Manifest::Location location);
+
   // Returns true if this extension or app includes areas within |origin|.
   bool OverlapsWithOrigin(const GURL& origin) const;