Observe prefs changes to display allowlist warnings

- Observe the allowlist state changes and Enhance Safe Browsing (ESB)
  prefs changes in the private developer api to display or hide the ESB
  warning in 'chrome://extensions' for extensions not included in the
  ESB CRX allowlist.
- Also includes a refactoring to encapsulate the extension allowlist
  prefs in `ExtensionAllowlist`.

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

chrome/browser/extensions/api/developer_private/developer_private_api.cc

@@ -32,6 +32,7 @@
 #include "chrome/browser/extensions/error_console/error_console_factory.h"
 #include "chrome/browser/extensions/extension_commands_global_registry.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system_factory.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/install_verifier.h"
@@ -330,6 +331,7 @@ void BrowserContextKeyedAPIFactory<
   DependsOn(ExtensionManagementFactory::GetInstance());
   DependsOn(CommandService::GetFactoryInstance());
   DependsOn(EventRouterFactory::GetInstance());
+  DependsOn(ExtensionSystemFactory::GetInstance());
 }
 
 // static
@@ -354,6 +356,8 @@ DeveloperPrivateEventRouter::DeveloperPrivateEventRouter(Profile* profile)
   extension_management_observation_.Observe(
       ExtensionManagementFactory::GetForBrowserContext(profile));
   command_service_observation_.Observe(CommandService::Get(profile));
+  extension_allowlist_observer_.Observe(
+      ExtensionSystem::Get(profile)->extension_service()->allowlist());
   pref_change_registrar_.Init(profile->GetPrefs());
   // The unretained is safe, since the PrefChangeRegistrar unregisters the
   // callback on destruction.
@@ -493,6 +497,12 @@ void DeveloperPrivateEventRouter::OnExtensionRuntimePermissionsChanged(
                             extension_id);
 }
 
+void DeveloperPrivateEventRouter::OnExtensionAllowlistWarningStateChanged(
+    const std::string& extension_id,
+    bool show_warning) {
+  BroadcastItemStateChanged(developer::EVENT_TYPE_PREFS_CHANGED, extension_id);
+}
+
 void DeveloperPrivateEventRouter::OnExtensionManagementSettingsChanged() {
   std::unique_ptr<base::ListValue> args(new base::ListValue());
   args->Append(DeveloperPrivateAPI::CreateProfileInfo(profile_)->ToValue());

chrome/browser/extensions/api/developer_private/developer_private_api.h

@@ -15,6 +15,7 @@
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/api/developer_private/entry_picker.h"
 #include "chrome/browser/extensions/error_console/error_console.h"
+#include "chrome/browser/extensions/extension_allowlist.h"
 #include "chrome/browser/extensions/extension_management.h"
 #include "chrome/browser/extensions/extension_uninstall_dialog.h"
 #include "chrome/browser/extensions/load_error_reporter.h"
@@ -67,6 +68,7 @@ class DeveloperPrivateEventRouter : public ExtensionRegistryObserver,
                                     public AppWindowRegistry::Observer,
                                     public CommandService::Observer,
                                     public ExtensionPrefsObserver,
+                                    public ExtensionAllowlist::Observer,
                                     public ExtensionManagement::Observer,
                                     public WarningService::Observer,
                                     public content::NotificationObserver {
@@ -122,6 +124,10 @@ class DeveloperPrivateEventRouter : public ExtensionRegistryObserver,
   void OnExtensionRuntimePermissionsChanged(
       const std::string& extension_id) override;
 
+  // ExtensionAllowlist::Observer
+  void OnExtensionAllowlistWarningStateChanged(const std::string& extension_id,
+                                               bool show_warning) override;
+
   // ExtensionManagement::Observer:
   void OnExtensionManagementSettingsChanged() override;
 
@@ -134,7 +140,7 @@ class DeveloperPrivateEventRouter : public ExtensionRegistryObserver,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
-  // Handles a profile preferance change.
+  // Handles a profile preference change.
   void OnProfilePrefChanged();
 
   // Broadcasts an event to all listeners.
@@ -162,6 +168,8 @@ class DeveloperPrivateEventRouter : public ExtensionRegistryObserver,
       extension_management_observation_{this};
   base::ScopedObservation<CommandService, CommandService::Observer>
       command_service_observation_{this};
+  base::ScopedObservation<ExtensionAllowlist, ExtensionAllowlist::Observer>
+      extension_allowlist_observer_{this};
 
   Profile* profile_;
 

chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc

@@ -31,6 +31,7 @@
 #include "chrome/test/base/testing_profile.h"
 #include "components/crx_file/id_util.h"
 #include "components/policy/core/common/mock_configuration_policy_provider.h"
+#include "components/safe_browsing/core/common/safe_browsing_prefs.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/web_contents_tester.h"
@@ -50,6 +51,7 @@
 #include "extensions/browser/test_extension_registry_observer.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
+#include "extensions/common/extension_features.h"
 #include "extensions/common/extension_id.h"
 #include "extensions/common/extension_set.h"
 #include "extensions/common/manifest_constants.h"
@@ -84,9 +86,10 @@ bool HasPrefsPermission(bool (*has_pref)(const std::string&,
   return has_pref(id, context);
 }
 
-bool WasPermissionsUpdatedEventDispatched(
+bool WasItemChangedEventDispatched(
     const TestEventRouterObserver& observer,
-    const ExtensionId& extension_id) {
+    const ExtensionId& extension_id,
+    const api::developer_private::EventType event_type) {
   const std::string kEventName =
       api::developer_private::OnItemStateChanged::kEventName;
   const auto& event_map = observer.events();
@@ -104,8 +107,7 @@ bool WasPermissionsUpdatedEventDispatched(
     return false;
 
   if (event_data->item_id != extension_id ||
-      event_data->event_type !=
-          api::developer_private::EVENT_TYPE_PERMISSIONS_CHANGED) {
+      event_data->event_type != event_type) {
     return false;
   }
 
@@ -1686,8 +1688,9 @@ TEST_F(DeveloperPrivateApiUnitTest,
   event_router->AddEventListener(kEventName, process, listener_id);
 
   TestEventRouterObserver test_observer(event_router);
-  EXPECT_FALSE(
-      WasPermissionsUpdatedEventDispatched(test_observer, extension->id()));
+  EXPECT_FALSE(WasItemChangedEventDispatched(
+      test_observer, extension->id(),
+      api::developer_private::EVENT_TYPE_PERMISSIONS_CHANGED));
 
   URLPatternSet hosts({URLPattern(Extension::kValidHostPermissionSchemes,
                                   "https://example.com/*")});
@@ -1699,16 +1702,18 @@ TEST_F(DeveloperPrivateApiUnitTest,
   // The event router fetches icons from a blocking thread when sending the
   // update event; allow it to finish before verifying the event was dispatched.
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(
-      WasPermissionsUpdatedEventDispatched(test_observer, extension->id()));
+  EXPECT_TRUE(WasItemChangedEventDispatched(
+      test_observer, extension->id(),
+      api::developer_private::EVENT_TYPE_PERMISSIONS_CHANGED));
 
   test_observer.ClearEvents();
 
   permissions_test_util::RevokeRuntimePermissionsAndWaitForCompletion(
       profile(), *extension, permissions);
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(
-      WasPermissionsUpdatedEventDispatched(test_observer, extension->id()));
+  EXPECT_TRUE(WasItemChangedEventDispatched(
+      test_observer, extension->id(),
+      api::developer_private::EVENT_TYPE_PERMISSIONS_CHANGED));
 }
 
 TEST_F(DeveloperPrivateApiUnitTest, ExtensionUpdatedEventOnPermissionsChange) {
@@ -1732,8 +1737,9 @@ TEST_F(DeveloperPrivateApiUnitTest, ExtensionUpdatedEventOnPermissionsChange) {
           .Build();
 
   TestEventRouterObserver test_observer(event_router);
-  EXPECT_FALSE(WasPermissionsUpdatedEventDispatched(test_observer,
-                                                    dummy_extension->id()));
+  EXPECT_FALSE(WasItemChangedEventDispatched(
+      test_observer, dummy_extension->id(),
+      api::developer_private::EVENT_TYPE_PERMISSIONS_CHANGED));
 
   APIPermissionSet apis;
   apis.insert(extensions::mojom::APIPermissionID::kTab);
@@ -1745,17 +1751,19 @@ TEST_F(DeveloperPrivateApiUnitTest, ExtensionUpdatedEventOnPermissionsChange) {
   // The event router fetches icons from a blocking thread when sending the
   // update event; allow it to finish before verifying the event was dispatched.
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(WasPermissionsUpdatedEventDispatched(test_observer,
-                                                   dummy_extension->id()));
+  EXPECT_TRUE(WasItemChangedEventDispatched(
+      test_observer, dummy_extension->id(),
+      api::developer_private::EVENT_TYPE_PERMISSIONS_CHANGED));
 
   test_observer.ClearEvents();
 
   permissions_test_util::RevokeOptionalPermissionsAndWaitForCompletion(
       profile(), *dummy_extension, permissions,
       PermissionsUpdater::REMOVE_HARD);
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(WasPermissionsUpdatedEventDispatched(test_observer,
-                                                   dummy_extension->id()));
+  EXPECT_TRUE(WasItemChangedEventDispatched(
+      test_observer, dummy_extension->id(),
+      api::developer_private::EVENT_TYPE_PERMISSIONS_CHANGED));
 }
 
 TEST_F(DeveloperPrivateApiUnitTest, InstallDroppedFileZip) {
@@ -1781,6 +1789,72 @@ TEST_F(DeveloperPrivateApiUnitTest, InstallDroppedFileZip) {
   EXPECT_EQ("Simple Empty Extension", extension->name());
 }
 
+class DeveloperPrivateApiAllowlistUnitTest
+    : public DeveloperPrivateApiUnitTest {
+ public:
+  DeveloperPrivateApiAllowlistUnitTest() {
+    feature_list_.InitAndEnableFeature(
+        extensions_features::kEnforceSafeBrowsingExtensionAllowlist);
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+TEST_F(DeveloperPrivateApiAllowlistUnitTest,
+       ExtensionUpdatedEventOnAllowlistWarningChange) {
+  // We need to call DeveloperPrivateAPI::Get() in order to instantiate the
+  // keyed service, since it's not created by default in unit tests.
+  DeveloperPrivateAPI::Get(profile());
+  const ExtensionId listener_id = crx_file::id_util::GenerateId("listener");
+  EventRouter* event_router = EventRouter::Get(profile());
+
+  // The DeveloperPrivateEventRouter will only dispatch events if there's at
+  // least one listener to dispatch to. Create one.
+  content::RenderProcessHost* process = nullptr;
+  const char* kEventName =
+      api::developer_private::OnItemStateChanged::kEventName;
+  event_router->AddEventListener(kEventName, process, listener_id);
+
+  scoped_refptr<const Extension> dummy_extension = LoadSimpleExtension();
+  base::RunLoop().RunUntilIdle();
+
+  TestEventRouterObserver test_observer(event_router);
+  EXPECT_FALSE(WasItemChangedEventDispatched(
+      test_observer, dummy_extension->id(),
+      api::developer_private::EVENT_TYPE_PREFS_CHANGED));
+
+  safe_browsing::SetSafeBrowsingState(profile()->GetPrefs(),
+                                      safe_browsing::ENHANCED_PROTECTION);
+
+  base::RunLoop().RunUntilIdle();
+  // The warning state should not have changed since the allowlist state is not
+  // set yet.
+  EXPECT_FALSE(WasItemChangedEventDispatched(
+      test_observer, dummy_extension->id(),
+      api::developer_private::EVENT_TYPE_PREFS_CHANGED));
+
+  service()->allowlist()->SetExtensionAllowlistState(dummy_extension->id(),
+                                                     ALLOWLIST_NOT_ALLOWLISTED);
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(WasItemChangedEventDispatched(
+      test_observer, dummy_extension->id(),
+      api::developer_private::EVENT_TYPE_PREFS_CHANGED));
+
+  test_observer.ClearEvents();
+
+  safe_browsing::SetSafeBrowsingState(profile()->GetPrefs(),
+                                      safe_browsing::STANDARD_PROTECTION);
+
+  base::RunLoop().RunUntilIdle();
+  // The warning is now hidden because the profile is no longer Enhanced
+  // Protection.
+  EXPECT_TRUE(WasItemChangedEventDispatched(
+      test_observer, dummy_extension->id(),
+      api::developer_private::EVENT_TYPE_PREFS_CHANGED));
+}
+
 class DeveloperPrivateApiSupervisedUserUnitTest
     : public DeveloperPrivateApiUnitTest {
  public:

chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc

@@ -669,12 +669,13 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebstorePrivateApiAllowlistEnforcementTest,
   ASSERT_TRUE(
       RunInstallTest("safebrowsing_not_allowlisted.html", "extension.crx"));
 
-  ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(browser()->profile());
   EXPECT_EQ(ALLOWLIST_NOT_ALLOWLISTED,
-            extension_prefs->GetExtensionAllowlistState(kExtensionId));
+            extension_service()->allowlist()->GetExtensionAllowlistState(
+                kExtensionId));
   EXPECT_EQ(
       ALLOWLIST_ACKNOWLEDGE_ENABLED_BY_USER,
-      extension_prefs->GetExtensionAllowlistAcknowledgeState(kExtensionId));
+      extension_service()->allowlist()->GetExtensionAllowlistAcknowledgeState(
+          kExtensionId));
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionWebstorePrivateApiAllowlistEnforcementTest,
@@ -683,9 +684,9 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebstorePrivateApiAllowlistEnforcementTest,
                                       safe_browsing::ENHANCED_PROTECTION);
   ASSERT_TRUE(RunInstallTest("safebrowsing_allowlisted.html", "extension.crx"));
 
-  ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(browser()->profile());
   EXPECT_EQ(ALLOWLIST_UNDEFINED,
-            extension_prefs->GetExtensionAllowlistState(kExtensionId));
+            extension_service()->allowlist()->GetExtensionAllowlistState(
+                kExtensionId));
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionWebstorePrivateApiAllowlistEnforcementTest,
@@ -695,9 +696,9 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebstorePrivateApiAllowlistEnforcementTest,
   ASSERT_TRUE(
       RunInstallTest("safebrowsing_not_allowlisted.html", "extension.crx"));
 
-  ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(browser()->profile());
   EXPECT_EQ(ALLOWLIST_UNDEFINED,
-            extension_prefs->GetExtensionAllowlistState(kExtensionId));
+            extension_service()->allowlist()->GetExtensionAllowlistState(
+                kExtensionId));
 }
 
 }  // namespace extensions