Click To Script: Prompt user to refresh page when toggling access through context menu.

Currently when a user grants access to the current page using the extension
action context menu, we run the blocked actions. But the extension may have
blocked actions which can't be run at this stage. Prompt the user to refresh the
page in this case, and grant permissions once the user accepts the refresh
bubble.

BUG=876791

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

chrome/browser/extensions/extension_action_runner.cc

@@ -19,6 +19,7 @@
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/permissions_updater.h"
+#include "chrome/browser/extensions/scripting_permissions_modifier.h"
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
@@ -41,6 +42,7 @@
 #include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "ipc/ipc_message_macros.h"
+#include "url/origin.h"
 
 namespace extensions {
 
@@ -93,7 +95,11 @@ ExtensionAction::ShowAction ExtensionActionRunner::RunAction(
   if (grant_tab_permissions) {
     int blocked = GetBlockedActions(extension);
     if ((blocked & kRefreshRequiredActionsMask) != 0) {
-      ShowBlockedActionBubble(extension);
+      ShowBlockedActionBubble(
+          extension,
+          base::Bind(
+              &ExtensionActionRunner::OnBlockedActionBubbleForRunActionClosed,
+              weak_factory_.GetWeakPtr(), extension->id()));
       return ExtensionAction::ACTION_NONE;
     }
     TabHelper::FromWebContents(web_contents())
@@ -125,24 +131,35 @@ ExtensionAction::ShowAction ExtensionActionRunner::RunAction(
   return ExtensionAction::ACTION_NONE;
 }
 
-void ExtensionActionRunner::RunBlockedActions(const Extension* extension) {
-  DCHECK(base::ContainsKey(pending_scripts_, extension->id()) ||
-         web_request_blocked_.count(extension->id()) != 0);
+void ExtensionActionRunner::HandlePageAccessModified(const Extension* extension,
+                                                     PageAccess current_access,
+                                                     PageAccess new_access) {
+  DCHECK_NE(current_access, new_access);
 
-  // Clicking to run the extension counts as granting it permission to run on
-  // the given tab.
-  // The extension may already have active tab at this point, but granting
-  // it twice is essentially a no-op.
-  TabHelper::FromWebContents(web_contents())
-      ->active_tab_permission_granter()
-      ->GrantIfRequested(extension);
+  // If we are restricting page access, just change permissions.
+  if (new_access == PageAccess::RUN_ON_CLICK) {
+    UpdatePageAccessSettings(extension, current_access, new_access);
+    return;
+  }
 
-  RunPendingScriptsForExtension(extension);
-  web_request_blocked_.erase(extension->id());
+  int blocked_actions = GetBlockedActions(extension);
+
+  // Refresh the page if there are pending actions which mandate a refresh.
+  if (blocked_actions & kRefreshRequiredActionsMask) {
+    // TODO(devlin): The bubble text should make it clear that permissions are
+    // granted only after the user accepts the refresh.
+    ShowBlockedActionBubble(
+        extension, base::Bind(&ExtensionActionRunner::
+                                  OnBlockedActionBubbleForPageAccessGrantClosed,
+                              weak_factory_.GetWeakPtr(), extension->id(),
+                              web_contents()->GetLastCommittedURL(),
+                              current_access, new_access));
+    return;
+  }
 
-  // The extension ran, so we need to tell the ExtensionActionAPI that we no
-  // longer want to act.
-  NotifyChange(extension);
+  UpdatePageAccessSettings(extension, current_access, new_access);
+  if (blocked_actions)
+    RunBlockedActions(extension);
 }
 
 void ExtensionActionRunner::OnActiveTabPermissionGranted(
@@ -349,14 +366,13 @@ void ExtensionActionRunner::LogUMA() const {
 }
 
 void ExtensionActionRunner::ShowBlockedActionBubble(
-    const Extension* extension) {
+    const Extension* extension,
+    const base::Callback<void(ToolbarActionsBarBubbleDelegate::CloseAction)>&
+        callback) {
   Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
   ToolbarActionsBar* toolbar_actions_bar =
       browser ? browser->window()->GetToolbarActionsBar() : nullptr;
   if (toolbar_actions_bar) {
-    auto callback =
-        base::Bind(&ExtensionActionRunner::OnBlockedActionBubbleClosed,
-                   weak_factory_.GetWeakPtr(), extension->id());
     if (default_bubble_close_action_for_testing_) {
       base::ThreadTaskRunnerHandle::Get()->PostTask(
           FROM_HERE,
@@ -369,7 +385,7 @@ void ExtensionActionRunner::ShowBlockedActionBubble(
   }
 }
 
-void ExtensionActionRunner::OnBlockedActionBubbleClosed(
+void ExtensionActionRunner::OnBlockedActionBubbleForRunActionClosed(
     const std::string& extension_id,
     ToolbarActionsBarBubbleDelegate::CloseAction action) {
   // If the user agreed to refresh the page, do so.
@@ -392,6 +408,82 @@ void ExtensionActionRunner::OnBlockedActionBubbleClosed(
   }
 }
 
+void ExtensionActionRunner::OnBlockedActionBubbleForPageAccessGrantClosed(
+    const std::string& extension_id,
+    const GURL& page_url,
+    PageAccess current_access,
+    PageAccess new_access,
+    ToolbarActionsBarBubbleDelegate::CloseAction action) {
+  DCHECK(new_access == PageAccess::RUN_ON_SITE ||
+         new_access == PageAccess::RUN_ON_ALL_SITES);
+  DCHECK_EQ(PageAccess::RUN_ON_CLICK, current_access);
+
+  // Don't change permissions if the user chose to not refresh the page.
+  if (action != ToolbarActionsBarBubbleDelegate::CLOSE_EXECUTE)
+    return;
+
+  // If the web contents have navigated to a different origin, do nothing.
+  if (!url::IsSameOriginWith(page_url, web_contents()->GetLastCommittedURL()))
+    return;
+
+  const Extension* extension = ExtensionRegistry::Get(browser_context_)
+                                   ->enabled_extensions()
+                                   .GetByID(extension_id);
+  if (!extension)
+    return;
+
+  UpdatePageAccessSettings(extension, current_access, new_access);
+  web_contents()->GetController().Reload(content::ReloadType::NORMAL, false);
+}
+
+void ExtensionActionRunner::UpdatePageAccessSettings(const Extension* extension,
+                                                     PageAccess current_access,
+                                                     PageAccess new_access) {
+  DCHECK_NE(current_access, new_access);
+
+  const GURL& url = web_contents()->GetLastCommittedURL();
+  ScriptingPermissionsModifier modifier(browser_context_, extension);
+  DCHECK(modifier.CanAffectExtension());
+
+  switch (new_access) {
+    case PageAccess::RUN_ON_CLICK:
+      if (current_access == PageAccess::RUN_ON_ALL_SITES)
+        modifier.SetWithholdHostPermissions(true);
+      if (modifier.HasGrantedHostPermission(url))
+        modifier.RemoveGrantedHostPermission(url);
+      break;
+    case PageAccess::RUN_ON_SITE:
+      if (current_access == PageAccess::RUN_ON_ALL_SITES)
+        modifier.SetWithholdHostPermissions(true);
+      if (!modifier.HasGrantedHostPermission(url))
+        modifier.GrantHostPermission(url);
+      break;
+    case PageAccess::RUN_ON_ALL_SITES:
+      modifier.SetWithholdHostPermissions(false);
+      break;
+  }
+}
+
+void ExtensionActionRunner::RunBlockedActions(const Extension* extension) {
+  DCHECK(base::ContainsKey(pending_scripts_, extension->id()) ||
+         web_request_blocked_.count(extension->id()) != 0);
+
+  // Clicking to run the extension counts as granting it permission to run on
+  // the given tab.
+  // The extension may already have active tab at this point, but granting
+  // it twice is essentially a no-op.
+  TabHelper::FromWebContents(web_contents())
+      ->active_tab_permission_granter()
+      ->GrantIfRequested(extension);
+
+  RunPendingScriptsForExtension(extension);
+  web_request_blocked_.erase(extension->id());
+
+  // The extension ran, so we need to tell the ExtensionActionAPI that we no
+  // longer want to act.
+  NotifyChange(extension);
+}
+
 bool ExtensionActionRunner::OnMessageReceived(
     const IPC::Message& message,
     content::RenderFrameHost* render_frame_host) {

chrome/browser/extensions/extension_action_runner.h

@@ -43,6 +43,12 @@ class ExtensionRegistry;
 class ExtensionActionRunner : public content::WebContentsObserver,
                               public ExtensionRegistryObserver {
  public:
+  enum class PageAccess {
+    RUN_ON_CLICK,
+    RUN_ON_SITE,
+    RUN_ON_ALL_SITES,
+  };
+
   class TestObserver {
    public:
     virtual void OnBlockedActionAdded() = 0;
@@ -64,9 +70,11 @@ class ExtensionActionRunner : public content::WebContentsObserver,
   ExtensionAction::ShowAction RunAction(const Extension* extension,
                                         bool grant_tab_permissions);
 
-  // Runs any actions that were blocked for the given |extension|. As a
-  // requirement, this will grant activeTab permission to the extension.
-  void RunBlockedActions(const Extension* extension);
+  // Notifies the ExtensionActionRunner that the page access for |extension| has
+  // changed.
+  void HandlePageAccessModified(const Extension* extension,
+                                PageAccess current_access,
+                                PageAccess new_access);
 
   // Notifies the ExtensionActionRunner that an extension has been granted
   // active tab permissions. This will run any pending injections for that
@@ -164,14 +172,38 @@ class ExtensionActionRunner : public content::WebContentsObserver,
   void LogUMA() const;
 
   // Shows the bubble to prompt the user to refresh the page to run the blocked
-  // actions for the given |extension|.
-  void ShowBlockedActionBubble(const Extension* extension);
+  // actions for the given |extension|. |callback| is invoked when the bubble is
+  // closed.
+  void ShowBlockedActionBubble(
+      const Extension* extension,
+      const base::Callback<void(ToolbarActionsBarBubbleDelegate::CloseAction)>&
+          callback);
+
+  // Called when the blocked actions bubble invoked to run the extension action
+  // is closed.
+  void OnBlockedActionBubbleForRunActionClosed(
+      const std::string& extension_id,
+      ToolbarActionsBarBubbleDelegate::CloseAction action);
 
-  // Called when the blocked actions bubble is closed.
-  void OnBlockedActionBubbleClosed(
+  // Called when the blocked actions bubble invoked for the page access grant is
+  // closed.
+  void OnBlockedActionBubbleForPageAccessGrantClosed(
       const std::string& extension_id,
+      const GURL& page_url,
+      PageAccess current_access,
+      PageAccess new_access,
       ToolbarActionsBarBubbleDelegate::CloseAction action);
 
+  // Handles permission changes necessary for page access modification of the
+  // |extension|.
+  void UpdatePageAccessSettings(const Extension* extension,
+                                PageAccess current_access,
+                                PageAccess new_access);
+
+  // Runs any actions that were blocked for the given |extension|. As a
+  // requirement, this will grant activeTab permission to the extension.
+  void RunBlockedActions(const Extension* extension);
+
   // content::WebContentsObserver implementation.
   bool OnMessageReceived(const IPC::Message& message,
                          content::RenderFrameHost* render_frame_host) override;

chrome/browser/extensions/extension_context_menu_model.cc

@@ -424,36 +424,25 @@ void ExtensionContextMenuModel::HandlePageAccessCommand(
   if (command_id == current_access)
     return;
 
-  const GURL& url = web_contents->GetLastCommittedURL();
-  ScriptingPermissionsModifier modifier(profile_, extension);
-  DCHECK(modifier.CanAffectExtension());
+  auto convert_page_access = [](int command_id) {
     switch (command_id) {
       case PAGE_ACCESS_RUN_ON_CLICK:
-      if (current_access == PAGE_ACCESS_RUN_ON_ALL_SITES)
-        modifier.SetWithholdHostPermissions(true);
-      if (modifier.HasGrantedHostPermission(url))
-        modifier.RemoveGrantedHostPermission(url);
-      break;
+        return ExtensionActionRunner::PageAccess::RUN_ON_CLICK;
       case PAGE_ACCESS_RUN_ON_SITE:
-      if (current_access == PAGE_ACCESS_RUN_ON_ALL_SITES)
-        modifier.SetWithholdHostPermissions(true);
-      if (!modifier.HasGrantedHostPermission(url))
-        modifier.GrantHostPermission(url);
-      break;
+        return ExtensionActionRunner::PageAccess::RUN_ON_SITE;
       case PAGE_ACCESS_RUN_ON_ALL_SITES:
-      modifier.SetWithholdHostPermissions(false);
-      break;
-    default:
-      NOTREACHED();
+        return ExtensionActionRunner::PageAccess::RUN_ON_ALL_SITES;
     }
+    NOTREACHED();
+    return ExtensionActionRunner::PageAccess::RUN_ON_CLICK;
+  };
 
-  if (command_id == PAGE_ACCESS_RUN_ON_SITE ||
-      command_id == PAGE_ACCESS_RUN_ON_ALL_SITES) {
   ExtensionActionRunner* runner =
       ExtensionActionRunner::GetForWebContents(web_contents);
-    if (runner && runner->WantsToRun(extension))
-      runner->RunBlockedActions(extension);
-  }
+  if (runner)
+    runner->HandlePageAccessModified(extension,
+                                     convert_page_access(current_access),
+                                     convert_page_access(command_id));
 }
 
 content::WebContents* ExtensionContextMenuModel::GetActiveWebContents() const {

chrome/browser/ui/extensions/extension_action_view_controller_unittest.cc

@@ -84,9 +84,20 @@ TEST_P(ToolbarActionsBarUnitTest, ExtensionActionBlockedActions) {
       extensions_features::kRuntimeHostPermissions);
 
   scoped_refptr<const extensions::Extension> browser_action_ext =
-      CreateAndAddExtension(
-          "browser action",
-          extensions::ExtensionBuilder::ActionType::BROWSER_ACTION);
+      extensions::ExtensionBuilder("browser action")
+          .SetAction(extensions::ExtensionBuilder::ActionType::BROWSER_ACTION)
+          .SetLocation(extensions::Manifest::INTERNAL)
+          .AddPermission("https://www.google.com/*")
+          .Build();
+
+  extensions::ExtensionService* service =
+      extensions::ExtensionSystem::Get(profile())->extension_service();
+  service->GrantPermissions(browser_action_ext.get());
+  service->AddExtension(browser_action_ext.get());
+  extensions::ScriptingPermissionsModifier permissions_modifier_browser_ext(
+      profile(), browser_action_ext);
+  permissions_modifier_browser_ext.SetWithholdHostPermissions(true);
+
   ASSERT_EQ(1u, toolbar_actions_bar()->GetIconCount());
   AddTab(browser(), GURL("https://www.google.com/"));
 
@@ -117,7 +128,7 @@ TEST_P(ToolbarActionsBarUnitTest, ExtensionActionBlockedActions) {
   EXPECT_FALSE(image_source->paint_page_action_decoration());
   EXPECT_TRUE(image_source->paint_blocked_actions_decoration());
 
-  action_runner->RunBlockedActions(browser_action_ext.get());
+  action_runner->RunForTesting(browser_action_ext.get());
   image_source =
       browser_action->GetIconImageSourceForTesting(web_contents, size);
   EXPECT_FALSE(image_source->grayscale());
@@ -125,8 +136,17 @@ TEST_P(ToolbarActionsBarUnitTest, ExtensionActionBlockedActions) {
   EXPECT_FALSE(image_source->paint_blocked_actions_decoration());
 
   scoped_refptr<const extensions::Extension> page_action_ext =
-      CreateAndAddExtension(
-          "page action", extensions::ExtensionBuilder::ActionType::PAGE_ACTION);
+      extensions::ExtensionBuilder("page action")
+          .SetAction(extensions::ExtensionBuilder::ActionType::PAGE_ACTION)
+          .SetLocation(extensions::Manifest::INTERNAL)
+          .AddPermission("https://www.google.com/*")
+          .Build();
+  service->GrantPermissions(page_action_ext.get());
+  service->AddExtension(page_action_ext.get());
+  extensions::ScriptingPermissionsModifier permissions_modifier_page_action(
+      profile(), page_action_ext);
+  permissions_modifier_page_action.SetWithholdHostPermissions(true);
+
   ASSERT_EQ(2u, toolbar_actions_bar()->GetIconCount());
   ExtensionActionViewController* page_action =
       static_cast<ExtensionActionViewController*>(
@@ -167,9 +187,9 @@ TEST_P(ToolbarActionsBarUnitTest, ExtensionActionBlockedActions) {
                            web_contents, false);
   toolbar_model()->SetVisibleIconCount(2u);
 
-  action_runner->RunBlockedActions(page_action_ext.get());
+  action_runner->RunForTesting(page_action_ext.get());
   image_source = page_action->GetIconImageSourceForTesting(web_contents, size);
-  EXPECT_TRUE(image_source->grayscale());
+  EXPECT_FALSE(image_source->grayscale());
   EXPECT_FALSE(image_source->paint_page_action_decoration());
   EXPECT_FALSE(image_source->paint_blocked_actions_decoration());
 }