Have the Debugger extension api check that it has access to the tab

chrome/browser/extensions/api/debugger/debugger_apitest.cc

Index: chrome/browser/extensions/api/debugger/debugger_apitest.cc
diff --git a/chrome/browser/extensions/api/debugger/debugger_apitest.cc b/chrome/browser/extensions/api/debugger/debugger_apitest.cc
index 878135a222aad3d6525d1533a9eb39c260ee3783..8a904d87884266b90c34d6aaf80f832df4bbe857 100644
--- a/chrome/browser/extensions/api/debugger/debugger_apitest.cc
+++ b/chrome/browser/extensions/api/debugger/debugger_apitest.cc
@@ -2,10 +2,145 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>
+
 #include "base/command_line.h"
+#include "base/memory/ref_counted.h"
+#include "base/path_service.h"
+#include "base/strings/stringprintf.h"
+#include "chrome/browser/extensions/api/debugger/debugger_api.h"
+#include "chrome/browser/extensions/api/debugger/debugger_api_constants.h"
 #include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/extensions/extension_function_test_utils.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "extensions/browser/extension_function.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/switches.h"
+#include "extensions/common/value_builder.h"
+
+namespace extensions {
+
+class DebuggerApiTest : public ExtensionApiTest {
+ protected:
+  virtual ~DebuggerApiTest() {}
+
+  virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE;
+  virtual void SetUpOnMainThread() OVERRIDE;
+
+  // Run the attach function. If |expected_error| is not empty, then the
+  // function should fail with the error. Otherwise, the function is expected
+  // to succeed.
+  testing::AssertionResult RunAttachFunction(const std::string& url,
+                                             const std::string& expected_error);
+
+  const Extension* extension() const { return extension_; }
+  base::CommandLine* command_line() { return command_line_; }

[meacer, 2014/06/24 18:15:54]

command_line() const {..}?

[Devlin, 2014/06/24 18:45:40]

Sure.  Didn't originally because it _does_ modify the command line, but the test
object is untouched, I suppose.

+
+ private:
+  // The command-line for the test process, preserved in order to modify
+  // mid-test.
+  base::CommandLine* command_line_;
+
+  // A basic extension with the debugger permission.
+  scoped_refptr<const Extension> extension_;
+};
+
+void DebuggerApiTest::SetUpCommandLine(base::CommandLine* command_line) {
+  ExtensionApiTest::SetUpCommandLine(command_line);
+  // We need to hold onto |command_line| in order to modify it during the test.
+  command_line_ = command_line;
+}
+
+void DebuggerApiTest::SetUpOnMainThread() {
+  ExtensionApiTest::SetUpOnMainThread();
+  extension_ =
+      ExtensionBuilder().SetManifest(
+          DictionaryBuilder().Set("name", "debugger")
+                             .Set("version", "0.1")
+                             .Set("manifest_version", 2)
+                             .Set("permissions",
+                                  ListBuilder().Append("debugger"))).Build();
+}
+
+testing::AssertionResult DebuggerApiTest::RunAttachFunction(
+    const std::string& url, const std::string& expected_error) {

[meacer, 2014/06/24 18:15:54]

nit: Could pass |url| as a GURL.

[Devlin, 2014/06/24 18:45:40]

Yeah, just made the test code look more cluttered to me.  But I don't feel
strongly. Done.

+  ui_test_utils::NavigateToURL(browser(), GURL(url));
+  content::WebContents* web_contents =
+    browser()->tab_strip_model()->GetActiveWebContents();

[meacer, 2014/06/24 18:15:54]

Two more spaces

[Devlin, 2014/06/24 18:45:40]

Done.

+  int tab_id = SessionID::IdForTab(web_contents);
+  scoped_refptr<DebuggerAttachFunction> attach_function =
+      new DebuggerAttachFunction();
+  attach_function->set_extension(extension_);
+  std::string args = base::StringPrintf("[{\"tabId\": %d}, \"1.1\"]", tab_id);
+
+  if (!expected_error.empty()) {
+    std::string actual_error =
+        extension_function_test_utils::RunFunctionAndReturnError(
+            attach_function, args, browser());
+    if (actual_error != expected_error) {
+      return testing::AssertionFailure() << "Did not get correct error: "
+          << "expected: " << expected_error << ", found: " << actual_error;
+    }
+  } else {
+    if (!RunFunction(attach_function,
+                     args,
+                     browser(),
+                     extension_function_test_utils::NONE)) {
+      return testing::AssertionFailure() << "Could not run function: "
+          << attach_function->GetError();
+    }
+
+    // Clean up and detach.
+    scoped_refptr<DebuggerDetachFunction> detach_function =
+        new DebuggerDetachFunction();
+    detach_function->set_extension(extension_);
+    if (!RunFunction(detach_function,
+                     base::StringPrintf("[{\"tabId\": %d}]", tab_id),
+                     browser(),
+                     extension_function_test_utils::NONE)) {
+      return testing::AssertionFailure() << "Could not detach: "
+          << detach_function->GetError();
+    }
+  }
+

[meacer, 2014/06/24 18:15:54]

nit: remove empty line

[Devlin, 2014/06/24 18:45:40]

Done.

+  return testing::AssertionSuccess();
+}
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, Debugger) {
   ASSERT_TRUE(RunExtensionTest("debugger")) << message_;
 }
+
+IN_PROC_BROWSER_TEST_F(DebuggerApiTest,
+                       DebuggerNotAllowedOnOtherExtensionPages) {
+  // Load another arbitrary extension with an associated resource (popup.html).
+  base::FilePath path;
+  ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &path));
+  path = path.AppendASCII("extensions").AppendASCII("good_unpacked");
+  const Extension* another_extension = LoadExtension(path);
+  ASSERT_TRUE(another_extension);
+
+  std::string other_ext_url =
+      base::StringPrintf("chrome-extension://%s/popup.html",
+                         another_extension->id().c_str());
+
+  // This extension should not be able to access another extension.
+  EXPECT_TRUE(RunAttachFunction(

[meacer, 2014/06/24 18:15:54]

nit: It's somewhat unintuitive to get EXPECT_TRUE from all test cases when they
different opposite things. Maybe rename to CanAttachFunction and test for true
and false?

[Devlin, 2014/06/24 18:45:40]

It makes the code messier.  If we just test for true and false, we can't
necessarily also test for the error, unless we, e.g., populate an out parameter
and compare it there.  Then we also have to pass in a bool for if we expect an
error.  Even then, it gets somewhat tricky - what if we can attach it but fail
to detach it?  Do we return true (because we could attach) or false (because we
failed)?  If it would make it more clear, I could move all the logic into the
main test body, or make two functions (one for run successfully, one for fail
and expect error) but I personally find the former a bit harder to read and the
latter results in more code.

Preferences?

[meacer, 2014/06/24 18:57:02]

Thanks, makes sense. Sounds good as it is.

+      other_ext_url, debugger_api_constants::kAttachToOtherExtensionError));
+
+  // This extension *should* be able to debug itself.
+  EXPECT_TRUE(RunAttachFunction(
+                  base::StringPrintf("chrome-extension://%s/foo.html",
+                                     extension()->id().c_str()),
+                  std::string()));
+
+  // Append extensions on chrome urls switch. The extension should now be able
+  // to debug any extension.
+  command_line()->AppendSwitch(switches::kExtensionsOnChromeURLs);
+  EXPECT_TRUE(RunAttachFunction(other_ext_url, std::string()));
+}
+
+}  // namespace extensions