[DevTools] Check DeveloperToolsPolicy on WebContents navigation

We used to only check the policy on attaching, but we should
also check it when navigating, similar to other session restrictions.

Bug: 880476
Change-Id: I8cbfe149d67eadbc2f723652d140f2cd0925c44c
Reviewed-on: https://chromium-review.googlesource.com/1211948
Commit-Queue: Pavel Feldman <pfeldman@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589383}

chrome/browser/devtools/chrome_devtools_manager_delegate.cc

@@ -26,6 +26,7 @@
 #include "components/guest_view/browser/guest_view_base.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/extension_registry.h"
@@ -129,11 +130,12 @@ std::string ChromeDevToolsManagerDelegate::GetTargetTitle(
   return extension_name;
 }
 
-bool ChromeDevToolsManagerDelegate::AllowInspectingWebContents(
-    content::WebContents* web_contents) {
-  return AllowInspection(
-      Profile::FromBrowserContext(web_contents->GetBrowserContext()),
-      web_contents);
+bool ChromeDevToolsManagerDelegate::AllowInspectingRenderFrameHost(
+    content::RenderFrameHost* rfh) {
+  Profile* profile =
+      Profile::FromBrowserContext(rfh->GetProcess()->GetBrowserContext());
+  return AllowInspection(profile, extensions::ProcessManager::Get(profile)
+                                      ->GetExtensionForRenderFrameHost(rfh));
 }
 
 // static

chrome/browser/devtools/chrome_devtools_manager_delegate.h

@@ -65,7 +65,7 @@ class ChromeDevToolsManagerDelegate : public content::DevToolsManagerDelegate {
   void DisposeBrowserContext(content::BrowserContext*,
                              DisposeCallback callback) override;
 
-  bool AllowInspectingWebContents(content::WebContents* web_contents) override;
+  bool AllowInspectingRenderFrameHost(content::RenderFrameHost* rfh) override;
   void ClientAttached(content::DevToolsAgentHost* agent_host,
                       content::DevToolsAgentHostClient* client) override;
   void ClientDetached(content::DevToolsAgentHost* agent_host,

chrome/browser/devtools/devtools_sanity_browsertest.cc

@@ -1898,9 +1898,8 @@ class DevToolsSanityExtensionTest : public extensions::ExtensionBrowserTest {
   // Installs an extensions, emulating that it has been force-installed by
   // policy.
   // Contains assertions - callers should wrap calls of this method in
-  // |ASSERT_NO_FATAL_FAILURE|. Fills |*out_web_contents| with a |WebContents|
-  // that belongs to the force-installed extension.
-  void ForceInstallExtension(content::WebContents** out_web_contents) {
+  // |ASSERT_NO_FATAL_FAILURE|.
+  void ForceInstallExtension(std::string* extension_id) {
     base::FilePath crx_path;
     base::PathService::Get(chrome::DIR_TEST_DATA, &crx_path);
     crx_path = crx_path.AppendASCII("devtools")
@@ -1909,8 +1908,15 @@ class DevToolsSanityExtensionTest : public extensions::ExtensionBrowserTest {
     const Extension* extension = InstallExtension(
         crx_path, 1, extensions::Manifest::EXTERNAL_POLICY_DOWNLOAD);
     ASSERT_TRUE(extension);
+    *extension_id = extension->id();
+  }
 
-    GURL url("chrome-extension://" + extension->id() + "/options.html");
+  // Same as above, but also fills |*out_web_contents| with a |WebContents|
+  // that has been navigated to the force-installed extension.
+  void ForceInstallExtensionAndOpen(content::WebContents** out_web_contents) {
+    std::string extension_id;
+    ForceInstallExtension(&extension_id);
+    GURL url("chrome-extension://" + extension_id + "/options.html");
     ui_test_utils::NavigateToURL(browser(), url);
     content::WebContents* web_contents =
         browser()->tab_strip_model()->GetWebContentsAt(0);
@@ -1926,10 +1932,35 @@ IN_PROC_BROWSER_TEST_F(DevToolsSanityExtensionTest,
                            kDisallowedForForceInstalledExtensions));
 
   content::WebContents* web_contents = nullptr;
-  ASSERT_NO_FATAL_FAILURE(ForceInstallExtension(&web_contents));
+  ASSERT_NO_FATAL_FAILURE(ForceInstallExtensionAndOpen(&web_contents));
+
+  DevToolsWindow::OpenDevToolsWindow(web_contents);
+  auto agent_host = content::DevToolsAgentHost::GetOrCreateFor(web_contents);
+  ASSERT_FALSE(DevToolsWindow::FindDevToolsWindow(agent_host.get()));
+}
+
+IN_PROC_BROWSER_TEST_F(
+    DevToolsSanityExtensionTest,
+    PolicyDisallowedForForceInstalledExtensionsAfterNavigation) {
+  browser()->profile()->GetPrefs()->SetInteger(
+      prefs::kDevToolsAvailability,
+      static_cast<int>(policy::DeveloperToolsPolicyHandler::Availability::
+                           kDisallowedForForceInstalledExtensions));
 
+  std::string extension_id;
+  ASSERT_NO_FATAL_FAILURE(ForceInstallExtension(&extension_id));
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetWebContentsAt(0);
+
+  // It's possible to open DevTools for about:blank.
+  ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
   DevToolsWindow::OpenDevToolsWindow(web_contents);
   auto agent_host = content::DevToolsAgentHost::GetOrCreateFor(web_contents);
+  ASSERT_TRUE(DevToolsWindow::FindDevToolsWindow(agent_host.get()));
+
+  // Navigating to extension page should close DevTools.
+  ui_test_utils::NavigateToURL(
+      browser(), GURL("chrome-extension://" + extension_id + "/options.html"));
   ASSERT_FALSE(DevToolsWindow::FindDevToolsWindow(agent_host.get()));
 }
 
@@ -1957,7 +1988,7 @@ IN_PROC_BROWSER_TEST_F(DevToolsAllowedByCommandLineSwitch,
                            kDisallowedForForceInstalledExtensions));
 
   content::WebContents* web_contents = nullptr;
-  ASSERT_NO_FATAL_FAILURE(ForceInstallExtension(&web_contents));
+  ASSERT_NO_FATAL_FAILURE(ForceInstallExtensionAndOpen(&web_contents));
 
   DevToolsWindow::OpenDevToolsWindow(web_contents);
   auto agent_host = content::DevToolsAgentHost::GetOrCreateFor(web_contents);

content/browser/devtools/render_frame_devtools_agent_host.cc

@@ -427,14 +427,7 @@ WebContents* RenderFrameDevToolsAgentHost::GetWebContents() {
 
 bool RenderFrameDevToolsAgentHost::AttachSession(DevToolsSession* session,
                                                  TargetRegistry* registry) {
-  DevToolsManager* manager = DevToolsManager::GetInstance();
-  if (manager->delegate() && web_contents()) {
-    if (!manager->delegate()->AllowInspectingWebContents(web_contents()))
-      return false;
-  }
-  const bool is_webui =
-      frame_host_ && (frame_host_->web_ui() || frame_host_->pending_web_ui());
-  if (!session->client()->MayAttachToRenderer(frame_host_, is_webui))
+  if (!ShouldAllowSession(session, frame_host_))
     return false;
 
   session->SetRenderer(frame_host_ ? frame_host_->GetProcess()->GetID()
@@ -599,14 +592,10 @@ void RenderFrameDevToolsAgentHost::UpdateFrameHost(
   agent_ptr_.reset();
 
   std::vector<DevToolsSession*> restricted_sessions;
-  const bool is_webui =
-      frame_host && (frame_host->web_ui() || frame_host->pending_web_ui());
-
   for (DevToolsSession* session : sessions()) {
-    if (!session->client()->MayAttachToRenderer(frame_host, is_webui))
+    if (!ShouldAllowSession(session, frame_host))
       restricted_sessions.push_back(session);
   }
-
   if (!restricted_sessions.empty())
     ForceDetachRestrictedSessions(restricted_sessions);
 
@@ -959,4 +948,19 @@ bool RenderFrameDevToolsAgentHost::IsChildFrame() {
   return frame_tree_node_ && frame_tree_node_->parent();
 }
 
+bool RenderFrameDevToolsAgentHost::ShouldAllowSession(
+    DevToolsSession* session,
+    RenderFrameHostImpl* frame_host) {
+  DevToolsManager* manager = DevToolsManager::GetInstance();
+  if (manager->delegate() && frame_host) {
+    if (!manager->delegate()->AllowInspectingRenderFrameHost(frame_host))
+      return false;
+  }
+  const bool is_webui =
+      frame_host && (frame_host->web_ui() || frame_host->pending_web_ui());
+  if (!session->client()->MayAttachToRenderer(frame_host, is_webui))
+    return false;
+  return true;
+}
+
 }  // namespace content

content/browser/devtools/render_frame_devtools_agent_host.h

@@ -190,6 +190,9 @@ class CONTENT_EXPORT RenderFrameDevToolsAgentHost
   void RevokePolicy();
   void SetFrameTreeNode(FrameTreeNode* frame_tree_node);
 
+  bool ShouldAllowSession(DevToolsSession* session,
+                          RenderFrameHostImpl* frame_host);
+
 #if defined(OS_ANDROID)
   device::mojom::WakeLock* GetWakeLock();
 #endif

content/public/browser/devtools_manager_delegate.cc

@@ -23,7 +23,8 @@ std::string DevToolsManagerDelegate::GetTargetDescription(WebContents* wc) {
   return std::string();
 }
 
-bool DevToolsManagerDelegate::AllowInspectingWebContents(WebContents* wc) {
+bool DevToolsManagerDelegate::AllowInspectingRenderFrameHost(
+    RenderFrameHost* rfh) {
   return true;
 }
 

content/public/browser/devtools_manager_delegate.h

@@ -20,6 +20,7 @@ class DictionaryValue;
 namespace content {
 
 class DevToolsAgentHostClient;
+class RenderFrameHost;
 class WebContents;
 
 class CONTENT_EXPORT DevToolsManagerDelegate {
@@ -36,8 +37,8 @@ class CONTENT_EXPORT DevToolsManagerDelegate {
   // Returns DevToolsAgentHost title to use for given |web_contents| target.
   virtual std::string GetTargetDescription(WebContents* web_contents);
 
-  // Returns whether embedder allows to inspect given |web_contents|.
-  virtual bool AllowInspectingWebContents(WebContents* web_contents);
+  // Returns whether embedder allows to inspect given |rfh|.
+  virtual bool AllowInspectingRenderFrameHost(RenderFrameHost* rfh);
 
   // Returns all targets embedder would like to report as debuggable
   // remotely.