[Extensions Click-to-Script] Add observer for runtime permission changes

Add a method to ExtensionPrefsObserver to monitor for runtime permission
changes, and listen for these changes in the chrome://extensions page.
This solves an issue where adding a new runtime host permission wouldn't
be reflected immediately in the chrome://extensions page if it wasn't a
permission directly requested by the extension, because there wouldn't
be an active permissions change.

Also reorder setting the "withhold permissions" pref to happen before
adding the withheld permissions so that when the observers are notified,
the bit is properly set.

Bug: 878939

Change-Id: I98c70ecd951a6ebb3d3372fec797f3657215a2b1
Reviewed-on: https://chromium-review.googlesource.com/1196107
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589081}

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

@@ -493,6 +493,12 @@ void DeveloperPrivateEventRouter::OnExtensionDisableReasonsChanged(
   BroadcastItemStateChanged(developer::EVENT_TYPE_PREFS_CHANGED, extension_id);
 }
 
+void DeveloperPrivateEventRouter::OnExtensionRuntimePermissionsChanged(
+    const std::string& extension_id) {
+  BroadcastItemStateChanged(developer::EVENT_TYPE_PERMISSIONS_CHANGED,
+                            extension_id);
+}
+
 void DeveloperPrivateEventRouter::OnExtensionManagementSettingsChanged() {
   std::unique_ptr<base::ListValue> args(new base::ListValue());
   args->Append(CreateProfileInfo(profile_)->ToValue());

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

@@ -110,6 +110,8 @@ class DeveloperPrivateEventRouter : public ExtensionRegistryObserver,
   // ExtensionPrefsObserver:
   void OnExtensionDisableReasonsChanged(const std::string& extension_id,
                                         int disable_reasons) override;
+  void OnExtensionRuntimePermissionsChanged(
+      const std::string& extension_id) override;
 
   // ExtensionManagement::Observer:
   void OnExtensionManagementSettingsChanged() override;

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

@@ -96,6 +96,34 @@ bool HasPrefsPermission(bool (*has_pref)(const std::string&,
   return has_pref(id, context);
 }
 
+bool WasPermissionsUpdatedEventDispatched(
+    const TestEventRouterObserver& observer,
+    const ExtensionId& extension_id) {
+  const std::string kEventName =
+      api::developer_private::OnItemStateChanged::kEventName;
+  const auto& event_map = observer.events();
+  auto iter = event_map.find(kEventName);
+  if (iter == event_map.end())
+    return false;
+
+  const Event& event = *iter->second;
+  CHECK(event.event_args);
+  CHECK_GE(1u, event.event_args->GetList().size());
+  std::unique_ptr<api::developer_private::EventData> event_data =
+      api::developer_private::EventData::FromValue(
+          event.event_args->GetList()[0]);
+  if (!event_data)
+    return false;
+
+  if (event_data->item_id != extension_id ||
+      event_data->event_type !=
+          api::developer_private::EVENT_TYPE_PERMISSIONS_CHANGED) {
+    return false;
+  }
+
+  return true;
+}
+
 }  // namespace
 
 class DeveloperPrivateApiUnitTest : public ExtensionServiceTestWithInstall {
@@ -1605,11 +1633,65 @@ TEST_F(DeveloperPrivateApiUnitTest,
             *extension_prefs->GetRuntimeGrantedPermissions(extension->id()));
 }
 
+TEST_F(DeveloperPrivateApiUnitTest,
+       UpdateHostAccess_UnrequestedHostsDispatchUpdateEvents) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      extensions_features::kRuntimeHostPermissions);
+
+  scoped_refptr<const Extension> extension =
+      ExtensionBuilder("test").AddPermission("http://google.com/*").Build();
+  service()->AddExtension(extension.get());
+  ScriptingPermissionsModifier modifier(profile(), extension.get());
+  modifier.SetWithholdHostPermissions(true);
+
+  // 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);
+
+  TestEventRouterObserver test_observer(event_router);
+  EXPECT_FALSE(
+      WasPermissionsUpdatedEventDispatched(test_observer, extension->id()));
+
+  URLPatternSet hosts({URLPattern(Extension::kValidHostPermissionSchemes,
+                                  "https://example.com/*")});
+  PermissionSet permissions(APIPermissionSet(), ManifestPermissionSet(), hosts,
+                            hosts);
+  PermissionsUpdater(profile()).GrantRuntimePermissions(*extension,
+                                                        permissions);
+  // 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()));
+
+  test_observer.ClearEvents();
+
+  PermissionsUpdater(profile()).RevokeRuntimePermissions(*extension,
+                                                         permissions);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(
+      WasPermissionsUpdatedEventDispatched(test_observer, extension->id()));
+}
+
 TEST_F(DeveloperPrivateApiUnitTest, ExtensionUpdatedEventOnPermissionsChange) {
+  // 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;
@@ -1622,31 +1704,8 @@ TEST_F(DeveloperPrivateApiUnitTest, ExtensionUpdatedEventOnPermissionsChange) {
           .Build();
 
   TestEventRouterObserver test_observer(event_router);
-  auto dispatched_updated_event = [&test_observer, kEventName,
-                                   &dummy_extension]() {
-    const auto& event_map = test_observer.events();
-    auto iter = event_map.find(kEventName);
-    if (iter == event_map.end())
-      return false;
-
-    const Event& event = *iter->second;
-    CHECK(event.event_args);
-    CHECK_GE(1u, event.event_args->GetList().size());
-    std::unique_ptr<api::developer_private::EventData> event_data =
-        api::developer_private::EventData::FromValue(
-            event.event_args->GetList()[0]);
-    if (!event_data)
-      return false;
-
-    if (event_data->item_id != dummy_extension->id() ||
-        event_data->event_type !=
-            api::developer_private::EVENT_TYPE_PERMISSIONS_CHANGED) {
-      return false;
-    }
-
-    return true;
-  };
-  EXPECT_FALSE(dispatched_updated_event());
+  EXPECT_FALSE(WasPermissionsUpdatedEventDispatched(test_observer,
+                                                    dummy_extension->id()));
 
   APIPermissionSet apis;
   apis.insert(APIPermission::kTab);
@@ -1654,15 +1713,19 @@ TEST_F(DeveloperPrivateApiUnitTest, ExtensionUpdatedEventOnPermissionsChange) {
                             URLPatternSet());
   PermissionsUpdater(profile()).GrantOptionalPermissions(*dummy_extension,
                                                          permissions);
+  // 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(dispatched_updated_event());
+  EXPECT_TRUE(WasPermissionsUpdatedEventDispatched(test_observer,
+                                                   dummy_extension->id()));
 
   test_observer.ClearEvents();
 
   PermissionsUpdater(profile()).RevokeOptionalPermissions(
       *dummy_extension, permissions, PermissionsUpdater::REMOVE_HARD);
   base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(dispatched_updated_event());
+  EXPECT_TRUE(WasPermissionsUpdatedEventDispatched(test_observer,
+                                                   dummy_extension->id()));
 }
 
 class DeveloperPrivateZipInstallerUnitTest

chrome/browser/extensions/scripting_permissions_modifier.cc

@@ -150,13 +150,15 @@ void ScriptingPermissionsModifier::SetWithholdHostPermissions(
   if (HasWithheldHostPermissions() == should_withhold)
     return;
 
+  // Set the pref first, so that listeners for permission changes get the proper
+  // value if they query HasWithheldHostPermissions().
+  SetWithholdPermissionsPrefValue(extension_prefs_, extension_->id(),
+                                  should_withhold);
+
   if (should_withhold)
     WithholdHostPermissions();
   else
     GrantWithheldHostPermissions();
-
-  SetWithholdPermissionsPrefValue(extension_prefs_, extension_->id(),
-                                  should_withhold);
 }
 
 bool ScriptingPermissionsModifier::HasWithheldHostPermissions() const {

extensions/browser/extension_prefs.cc

@@ -1000,6 +1000,8 @@ void ExtensionPrefs::AddRuntimeGrantedPermissions(
     const PermissionSet& permissions) {
   AddToPrefPermissionSet(extension_id, permissions,
                          kPrefRuntimeGrantedPermissions);
+  for (auto& observer : observer_list_)
+    observer.OnExtensionRuntimePermissionsChanged(extension_id);
 }
 
 void ExtensionPrefs::RemoveRuntimeGrantedPermissions(
@@ -1007,6 +1009,8 @@ void ExtensionPrefs::RemoveRuntimeGrantedPermissions(
     const PermissionSet& permissions) {
   RemoveFromPrefPermissionSet(extension_id, permissions,
                               kPrefRuntimeGrantedPermissions);
+  for (auto& observer : observer_list_)
+    observer.OnExtensionRuntimePermissionsChanged(extension_id);
 }
 
 void ExtensionPrefs::SetExtensionRunning(const std::string& extension_id,

extensions/browser/extension_prefs_observer.h

@@ -39,6 +39,14 @@ class ExtensionPrefsObserver {
   // events might not match up.
   virtual void OnExtensionStateChanged(const std::string& extension_id,
                                        bool state) {}
+
+  // Called when the runtime permissions for an extension are changed.
+  // TODO(devlin): This is a bit out of place here, and may be better suited on
+  // a general "extension permissions" observer, if/when we have one. See
+  // discussion at
+  // https://chromium-review.googlesource.com/c/chromium/src/+/1196107/3/chrome/browser/extensions/runtime_permissions_observer.h#26.
+  virtual void OnExtensionRuntimePermissionsChanged(
+      const std::string& extension_id) {}
 };
 
 }  // namespace extensions

extensions/browser/test_event_router_observer.h

@@ -23,8 +23,8 @@ class TestEventRouterObserver : public EventRouter::TestObserver {
   // Clears all recorded events.
   void ClearEvents();
 
-  const EventMap& events() { return events_; }
-  const EventMap& dispatched_events() { return dispatched_events_; }
+  const EventMap& events() const { return events_; }
+  const EventMap& dispatched_events() const { return dispatched_events_; }
 
  private:
   // EventRouter::TestObserver: