Allow adding disable reasons to blocklisted extensions

Blocklisted extensions may have additional disable reasons added such as
when they are added to an additional safe browsing blocklist. Allow
extensions that are already in the blocklisted set to have their disable
reasons updated.

This also allows us to remove a one-off condition in ExtensionPrefs that
side-stepped this condition.

Bug: 1073570
Change-Id: I300f6b3dde6617c56ee0dd8d92c6cdc0bebe58b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622452
Commit-Queue: Jeff Cyr <jeffcyr@google.com>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842728}

chrome/browser/extensions/extension_service.h

@@ -732,6 +732,8 @@ class ExtensionService : public ExtensionServiceInterface,
                            ManagementPolicyProhibitsEnableOnInstalled);
   FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
                            BlockAndUnblockBlocklistedExtension);
+  FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
+                           CanAddDisableReasonToBlocklistedExtension);
   FRIEND_TEST_ALL_PREFIXES(::BlocklistedExtensionSyncServiceTest,
                            SyncBlocklistedExtension);
   friend class ::BlocklistedExtensionSyncServiceTest;

chrome/browser/extensions/extension_service_unittest.cc

@@ -4838,6 +4838,56 @@ TEST_F(ExtensionServiceTest, NoEnableRemotelyDisabledExtension) {
   EXPECT_FALSE(prefs->IsExtensionBlocklisted(good_crx));
 }
 
+TEST_F(ExtensionServiceTest, CanAddDisableReasonToBlocklistedExtension) {
+  InitializeGoodInstalledExtensionService();
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+  TestBlocklist blocklist;
+
+  blocklist.Attach(service()->blocklist_);
+  service()->Init();
+
+  blocklist.SetBlocklistState(good0, BLOCKLISTED_MALWARE, true);
+  blocklist.SetBlocklistState(good1, BLOCKLISTED_MALWARE, true);
+  content::RunAllTasksUntilIdle();
+  EXPECT_TRUE(prefs->IsExtensionBlocklisted(good0));
+  EXPECT_TRUE(prefs->IsExtensionBlocklisted(good1));
+
+  // Test that a disable reason can be added to a blocklisted extension.
+  prefs->AddDisableReason(good0, disable_reason::DISABLE_REMOTELY_FOR_MALWARE);
+  EXPECT_TRUE(prefs->HasDisableReason(
+      good0, disable_reason::DISABLE_REMOTELY_FOR_MALWARE));
+
+  // Test that a blocklisted extension can be disabled.
+  service()->DisableExtension(good1, disable_reason::DISABLE_BLOCKED_BY_POLICY);
+  EXPECT_TRUE(prefs->HasDisableReason(
+      good1, disable_reason::DISABLE_BLOCKED_BY_POLICY));
+  EXPECT_TRUE(prefs->IsExtensionBlocklisted(good1));
+  // Even though the extension was disabled with a new disable reason, it should
+  // remain in the blocklisted set (which can't be re-enabled by the user).
+  EXPECT_TRUE(registry()->blocklisted_extensions().Contains(good1));
+  // Since the extension is blocklisted, it should not be in the disabled set.
+  EXPECT_FALSE(registry()->disabled_extensions().Contains(good1));
+
+  // Extensions should remain in the appropriate sets after being reloaded (as
+  // in a profile restart).
+  service()->ReloadExtensionsForTest();
+  EXPECT_TRUE(prefs->HasDisableReason(
+      good1, disable_reason::DISABLE_BLOCKED_BY_POLICY));
+  EXPECT_TRUE(prefs->IsExtensionBlocklisted(good1));
+  EXPECT_TRUE(registry()->blocklisted_extensions().Contains(good1));
+  EXPECT_FALSE(registry()->disabled_extensions().Contains(good1));
+
+  // Test that the extension is disabled when unblocklisted.
+  blocklist.SetBlocklistState(good1, NOT_BLOCKLISTED, true);
+  content::RunAllTasksUntilIdle();
+  EXPECT_FALSE(prefs->IsExtensionBlocklisted(good1));
+  EXPECT_TRUE(prefs->IsExtensionDisabled(good1));
+  EXPECT_FALSE(registry()->blocklisted_extensions().Contains(good1));
+  EXPECT_TRUE(registry()->disabled_extensions().Contains(good1));
+  EXPECT_TRUE(prefs->HasDisableReason(
+      good1, disable_reason::DISABLE_BLOCKED_BY_POLICY));
+}
+
 TEST_F(ExtensionServiceTest, TerminateExtension) {
   InitializeEmptyExtensionService();
 

chrome/browser/extensions/updater/extension_updater_unittest.cc

@@ -56,6 +56,7 @@
 #include "content/public/browser/notification_source.h"
 #include "content/public/test/browser_task_environment.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/browser/blocklist_state.h"
 #include "extensions/browser/disable_reason.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
@@ -2685,6 +2686,8 @@ TEST_F(ExtensionUpdaterTest, TestUpdatingRemotelyDisabledExtensions) {
   ASSERT_EQ(1u, enabled_extensions.size());
   ASSERT_EQ(2u, blocklisted_extensions.size());
   const std::string& remotely_blocklisted_id = blocklisted_extensions[0]->id();
+  service.extension_prefs()->SetExtensionBlocklistState(remotely_blocklisted_id,
+                                                        BLOCKLISTED_MALWARE);
   service.extension_prefs()->AddDisableReason(
       remotely_blocklisted_id, disable_reason::DISABLE_REMOTELY_FOR_MALWARE);
 

extensions/browser/extension_prefs.cc

@@ -964,17 +964,13 @@ bool ExtensionPrefs::HasDisableReason(
 void ExtensionPrefs::AddDisableReason(
     const std::string& extension_id,
     disable_reason::DisableReason disable_reason) {
-  // TODO(https://crbug.com/1073570): Extensions can be blocklisted but in
-  // enabled state. This checks the kPrefState which is the state of the
-  // extension.
-  DCHECK(!DoesExtensionHaveState(extension_id, Extension::ENABLED) ||
-         disable_reason == disable_reason::DISABLE_REMOTELY_FOR_MALWARE);
-  ModifyDisableReasons(extension_id, disable_reason, DISABLE_REASON_ADD);
+  AddDisableReasons(extension_id, disable_reason);
 }
 
 void ExtensionPrefs::AddDisableReasons(const std::string& extension_id,
                                        int disable_reasons) {
-  DCHECK(!DoesExtensionHaveState(extension_id, Extension::ENABLED));
+  DCHECK(!DoesExtensionHaveState(extension_id, Extension::ENABLED) ||
+         IsExtensionBlocklisted(extension_id));
   ModifyDisableReasons(extension_id, disable_reasons, DISABLE_REASON_ADD);
 }
 

extensions/browser/extension_registrar.cc

@@ -228,17 +228,6 @@ void ExtensionRegistrar::DisableExtension(const ExtensionId& extension_id,
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_NE(disable_reason::DISABLE_NONE, disable_reasons);
 
-  if (extension_prefs_->IsExtensionBlocklisted(extension_id))
-    return;
-
-  // The extension may have been disabled already. Just add the disable reasons.
-  // TODO(michaelpg): Move this after the policy check, below, to ensure that
-  // disable reasons disallowed by policy are not added here.
-  if (!IsExtensionEnabled(extension_id)) {
-    extension_prefs_->AddDisableReasons(extension_id, disable_reasons);
-    return;
-  }
-
   scoped_refptr<const Extension> extension =
       registry_->GetExtensionById(extension_id, ExtensionRegistry::EVERYTHING);
 
@@ -262,6 +251,12 @@ void ExtensionRegistrar::DisableExtension(const ExtensionId& extension_id,
       return;
   }
 
+  // The extension may have been disabled already. Just add the disable reasons.
+  if (!IsExtensionEnabled(extension_id)) {
+    extension_prefs_->AddDisableReasons(extension_id, disable_reasons);
+    return;
+  }
+
   extension_prefs_->SetExtensionDisabled(extension_id, disable_reasons);
 
   int include_mask =