Re-enable extensions disabled due to permission increase if they have all permissions
BUG=616474
Review-Url: https://codereview.chromium.org/2019423007
Cr-Commit-Position: refs/heads/master@{#399876}
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 271ca2e..e4c4a36c9 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -1439,10 +1439,10 @@
}
bool is_extension_upgrade = false;
- bool is_extension_installed = false;
+ bool is_extension_loaded = false;
const Extension* old = GetInstalledExtension(extension->id());
if (old) {
- is_extension_installed = true;
+ is_extension_loaded = true;
int version_compare_result =
extension->version()->CompareTo(*(old->version()));
is_extension_upgrade = version_compare_result > 0;
@@ -1466,9 +1466,9 @@
// Check if the extension's privileges have changed and mark the
// extension disabled if necessary.
- CheckPermissionsIncrease(extension, is_extension_installed);
+ CheckPermissionsIncrease(extension, is_extension_loaded);
- if (is_extension_installed && !reloading) {
+ if (is_extension_loaded && !reloading) {
// To upgrade an extension in place, unload the old one and then load the
// new one. ReloadExtension disables the extension, which is sufficient.
UnloadExtension(extension->id(), UnloadedExtensionInfo::REASON_UPDATE);
@@ -1548,7 +1548,7 @@
}
void ExtensionService::CheckPermissionsIncrease(const Extension* extension,
- bool is_extension_installed) {
+ bool is_extension_loaded) {
extensions::PermissionsUpdater(profile_).InitializePermissions(extension);
// We keep track of all permissions the user has granted each extension.
@@ -1611,14 +1611,13 @@
GrantPermissions(extension);
}
- if (is_extension_installed) {
- // If the extension was already disabled, suppress any alerts for becoming
- // disabled on permissions increase.
- bool previously_disabled =
- extension_prefs_->IsExtensionDisabled(extension->id());
+ bool previously_disabled =
+ extension_prefs_->IsExtensionDisabled(extension->id());
+ // TODO(treib): Is the |is_extension_loaded| check needed here?
+ if (is_extension_loaded && previously_disabled) {
// Legacy disabled extensions do not have a disable reason. Infer that it
// was likely disabled by the user.
- if (previously_disabled && disable_reasons == Extension::DISABLE_NONE)
+ if (disable_reasons == Extension::DISABLE_NONE)
disable_reasons |= Extension::DISABLE_USER_ACTION;
// Extensions that came to us disabled from sync need a similar inference,
@@ -1626,8 +1625,7 @@
// TODO(treib,devlin): Since M48, DISABLE_UNKNOWN_FROM_SYNC isn't used
// anymore; this code is still here to migrate any existing old state.
// Remove it after some grace period.
- if (previously_disabled &&
- (disable_reasons & Extension::DEPRECATED_DISABLE_UNKNOWN_FROM_SYNC)) {
+ if (disable_reasons & Extension::DEPRECATED_DISABLE_UNKNOWN_FROM_SYNC) {
// Remove the DISABLE_UNKNOWN_FROM_SYNC reason.
disable_reasons &= ~Extension::DEPRECATED_DISABLE_UNKNOWN_FROM_SYNC;
extension_prefs_->RemoveDisableReason(
@@ -1638,6 +1636,22 @@
}
}
+ // If the extension is disabled due to a permissions increase, but does in
+ // fact have all permissions, remove that disable reason.
+ // TODO(devlin): This was added to fix crbug.com/616474, but it's unclear
+ // if this behavior should stay forever.
+ if (disable_reasons & Extension::DISABLE_PERMISSIONS_INCREASE) {
+ bool reset_permissions_increase = false;
+ if (!is_privilege_increase) {
+ reset_permissions_increase = true;
+ disable_reasons &= ~Extension::DISABLE_PERMISSIONS_INCREASE;
+ extension_prefs_->RemoveDisableReason(
+ extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE);
+ }
+ UMA_HISTOGRAM_BOOLEAN("Extensions.ResetPermissionsIncrease",
+ reset_permissions_increase);
+ }
+
// Extension has changed permissions significantly. Disable it. A
// notification should be sent by the caller. If the extension is already
// disabled because it was installed remotely, don't add another disable
@@ -1664,7 +1678,10 @@
}
#endif
}
- if (disable_reasons != Extension::DISABLE_NONE)
+
+ if (disable_reasons == Extension::DISABLE_NONE)
+ extension_prefs_->SetExtensionEnabled(extension->id());
+ else
extension_prefs_->SetExtensionDisabled(extension->id(), disable_reasons);
}
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index eb16786..556cc4f 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -537,7 +537,7 @@
// Disables the extension if the privilege level has increased
// (e.g., due to an upgrade).
void CheckPermissionsIncrease(const extensions::Extension* extension,
- bool is_extension_installed);
+ bool is_extension_loaded);
// Helper that updates the active extension list used for crash reporting.
void UpdateActiveExtensionsInCrashReporter();
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 52f7a7d..5849ecab 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -1429,12 +1429,14 @@
const base::FilePath path2 = base_path.AppendASCII("update_2");
const base::FilePath path3 = base_path.AppendASCII("update_3");
const base::FilePath path4 = base_path.AppendASCII("update_4");
+ const base::FilePath path5 = base_path.AppendASCII("update_5");
ASSERT_TRUE(base::PathExists(pem_path));
ASSERT_TRUE(base::PathExists(path1));
ASSERT_TRUE(base::PathExists(path2));
ASSERT_TRUE(base::PathExists(path3));
ASSERT_TRUE(base::PathExists(path4));
+ ASSERT_TRUE(base::PathExists(path5));
ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
@@ -1505,6 +1507,169 @@
}
}
+TEST_F(ExtensionServiceTest, ReenableWithAllPermissionsGranted) {
+ InitializeEmptyExtensionService();
+ const base::FilePath base_path = data_dir().AppendASCII("permissions");
+
+ const base::FilePath pem_path = base_path.AppendASCII("update.pem");
+ const base::FilePath path1 = base_path.AppendASCII("update_1");
+ const base::FilePath path4 = base_path.AppendASCII("update_4");
+ const base::FilePath path5 = base_path.AppendASCII("update_5");
+
+ ASSERT_TRUE(base::PathExists(pem_path));
+ ASSERT_TRUE(base::PathExists(path1));
+ ASSERT_TRUE(base::PathExists(path4));
+ ASSERT_TRUE(base::PathExists(path5));
+
+ ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+
+ // Install version 1, which has the kHistory permission.
+ const Extension* extension = PackAndInstallCRX(path1, pem_path, INSTALL_NEW);
+ const std::string id = extension->id();
+
+ EXPECT_EQ(0u, GetErrors().size());
+ ASSERT_TRUE(registry()->enabled_extensions().Contains(id));
+
+ // Update to version 4 that adds the kNotifications permission, which has a
+ // message and hence is considered a permission increase. The extension
+ // should get disabled due to a permissions increase.
+ PackCRXAndUpdateExtension(id, path4, pem_path, DISABLED);
+ extension = service()->GetInstalledExtension(id);
+ ASSERT_TRUE(extension);
+ EXPECT_TRUE(registry()->disabled_extensions().Contains(id));
+ EXPECT_TRUE(
+ prefs->HasDisableReason(id, Extension::DISABLE_PERMISSIONS_INCREASE));
+
+ // Update to version 5 that removes the kNotifications permission again.
+ // The extension should get re-enabled.
+ PackCRXAndUpdateExtension(id, path5, pem_path, ENABLED);
+ extension = service()->GetInstalledExtension(id);
+ ASSERT_TRUE(extension);
+ EXPECT_TRUE(registry()->enabled_extensions().Contains(id));
+}
+
+TEST_F(ExtensionServiceTest, ReenableWithAllPermissionsGrantedOnStartup) {
+ InitializeEmptyExtensionService();
+ const base::FilePath base_path = data_dir().AppendASCII("permissions");
+
+ const base::FilePath pem_path = base_path.AppendASCII("update.pem");
+ const base::FilePath path1 = base_path.AppendASCII("update_1");
+
+ ASSERT_TRUE(base::PathExists(pem_path));
+ ASSERT_TRUE(base::PathExists(path1));
+
+ // Install an extension which has the kHistory permission.
+ const Extension* extension = PackAndInstallCRX(path1, pem_path, INSTALL_NEW);
+ const std::string id = extension->id();
+
+ EXPECT_EQ(0u, GetErrors().size());
+ ASSERT_TRUE(registry()->enabled_extensions().Contains(id));
+
+ ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+
+ // Disable the extension due to a supposed permission increase, but retain its
+ // granted permissions.
+ service()->DisableExtension(id, Extension::DISABLE_PERMISSIONS_INCREASE);
+ EXPECT_TRUE(registry()->disabled_extensions().Contains(id));
+ EXPECT_TRUE(
+ prefs->HasDisableReason(id, Extension::DISABLE_PERMISSIONS_INCREASE));
+
+ // Simulate a Chrome restart. Since the extension has all required
+ // permissions, it should get re-enabled.
+ service()->ReloadExtensionsForTest();
+ EXPECT_TRUE(registry()->enabled_extensions().Contains(id));
+ EXPECT_FALSE(
+ prefs->HasDisableReason(id, Extension::DISABLE_PERMISSIONS_INCREASE));
+}
+
+TEST_F(ExtensionServiceTest,
+ DontReenableWithAllPermissionsGrantedButOtherReason) {
+ InitializeEmptyExtensionService();
+ const base::FilePath base_path = data_dir().AppendASCII("permissions");
+
+ const base::FilePath pem_path = base_path.AppendASCII("update.pem");
+ const base::FilePath path1 = base_path.AppendASCII("update_1");
+ const base::FilePath path4 = base_path.AppendASCII("update_4");
+ const base::FilePath path5 = base_path.AppendASCII("update_5");
+
+ ASSERT_TRUE(base::PathExists(pem_path));
+ ASSERT_TRUE(base::PathExists(path1));
+ ASSERT_TRUE(base::PathExists(path4));
+ ASSERT_TRUE(base::PathExists(path5));
+
+ ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+
+ // Install version 1, which has the kHistory permission.
+ const Extension* extension = PackAndInstallCRX(path1, pem_path, INSTALL_NEW);
+ const std::string id = extension->id();
+
+ EXPECT_EQ(0u, GetErrors().size());
+ ASSERT_TRUE(registry()->enabled_extensions().Contains(id));
+
+ // Disable the extension.
+ service()->DisableExtension(id, Extension::DISABLE_USER_ACTION);
+ EXPECT_TRUE(registry()->disabled_extensions().Contains(id));
+ EXPECT_TRUE(prefs->HasDisableReason(id, Extension::DISABLE_USER_ACTION));
+
+ // Update to version 4 that adds the kNotifications permission, which has a
+ // message and hence is considered a permission increase. The extension
+ // should get disabled due to a permissions increase.
+ PackCRXAndUpdateExtension(id, path4, pem_path, DISABLED);
+ extension = service()->GetInstalledExtension(id);
+ ASSERT_TRUE(extension);
+ EXPECT_TRUE(registry()->disabled_extensions().Contains(id));
+ EXPECT_TRUE(
+ prefs->HasDisableReason(id, Extension::DISABLE_PERMISSIONS_INCREASE));
+ // The USER_ACTION reason should also still be there.
+ EXPECT_TRUE(prefs->HasDisableReason(id, Extension::DISABLE_USER_ACTION));
+
+ // Update to version 5 that removes the kNotifications permission again.
+ // The PERMISSIONS_INCREASE should be removed, but the extension should stay
+ // disabled since USER_ACTION is still there.
+ PackCRXAndUpdateExtension(id, path5, pem_path, DISABLED);
+ extension = service()->GetInstalledExtension(id);
+ ASSERT_TRUE(extension);
+ EXPECT_TRUE(registry()->disabled_extensions().Contains(id));
+ EXPECT_EQ(Extension::DISABLE_USER_ACTION, prefs->GetDisableReasons(id));
+}
+
+TEST_F(ExtensionServiceTest,
+ DontReenableWithAllPermissionsGrantedOnStartupButOtherReason) {
+ InitializeEmptyExtensionService();
+ const base::FilePath base_path = data_dir().AppendASCII("permissions");
+
+ const base::FilePath pem_path = base_path.AppendASCII("update.pem");
+ const base::FilePath path1 = base_path.AppendASCII("update_1");
+
+ ASSERT_TRUE(base::PathExists(pem_path));
+ ASSERT_TRUE(base::PathExists(path1));
+
+ // Install an extension which has the kHistory permission.
+ const Extension* extension = PackAndInstallCRX(path1, pem_path, INSTALL_NEW);
+ const std::string id = extension->id();
+
+ EXPECT_EQ(0u, GetErrors().size());
+ ASSERT_TRUE(registry()->enabled_extensions().Contains(id));
+
+ ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+
+ // Disable the extension due to a supposed permission increase, but retain its
+ // granted permissions.
+ service()->DisableExtension(
+ id,
+ Extension::DISABLE_PERMISSIONS_INCREASE | Extension::DISABLE_USER_ACTION);
+ EXPECT_TRUE(registry()->disabled_extensions().Contains(id));
+ EXPECT_TRUE(
+ prefs->HasDisableReason(id, Extension::DISABLE_PERMISSIONS_INCREASE));
+
+ // Simulate a Chrome restart. Since the extension has all required
+ // permissions, the DISABLE_PERMISSIONS_INCREASE should get removed, but it
+ // should stay disabled due to the remaining DISABLE_USER_ACTION reason.
+ service()->ReloadExtensionsForTest();
+ EXPECT_TRUE(registry()->disabled_extensions().Contains(id));
+ EXPECT_EQ(Extension::DISABLE_USER_ACTION, prefs->GetDisableReasons(id));
+}
+
#if !defined(OS_CHROMEOS)
// This tests that the granted permissions preferences are correctly set for
// default apps.
