Take 2 at implementing activeTab.

chrome/browser/extensions/active_tab_unittest.cc

Index: chrome/browser/extensions/active_tab_unittest.cc
diff --git a/chrome/browser/extensions/active_tab_unittest.cc b/chrome/browser/extensions/active_tab_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f6317f42faa6a70dcb0c2f70ffdb1469882d3661
--- /dev/null
+++ b/chrome/browser/extensions/active_tab_unittest.cc
@@ -0,0 +1,447 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <string>
+
+#include "base/compiler_specific.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/values.h"
+#include "chrome/browser/extensions/active_tab_permission_manager.h"
+#include "chrome/browser/extensions/extension_tab_helper.h"
+#include "chrome/browser/ui/tab_contents/tab_contents.h"
+#include "chrome/browser/ui/tab_contents/test_tab_contents.h"
+#include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/extensions/extension.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/page_transition_types.h"
+#include "content/public/test/test_browser_thread.h"
+
+using base::DictionaryValue;
+using base::ListValue;
+using content::BrowserThread;
+using content::NavigationController;
+
+namespace extensions {
+namespace {
+
+class ActiveTabTest : public TabContentsTestHarness {
+ public:
+  ActiveTabTest() : ui_thread_(BrowserThread::UI, MessageLoop::current()) {
+    {
+      scoped_ptr<ListValue> permissions(new ListValue());
+        permissions->Append(Value::CreateStringValue("activeTab"));

[Aaron Boodman, 2012/06/08 05:31:30]

indent -= 2;

[not at google - send to devlin, 2012/06/12 20:40:51]

Aww I like it like that.

+      DictionaryValue manifest;
+        manifest.SetString("name", "Extension with activeTab.");
+        manifest.SetString("version", "1.0.0");
+        manifest.SetInteger("manifest_version", 2);
+        manifest.Set("permissions", permissions.release());
+      std::string error;
+      extension_ = Extension::Create(
+          FilePath(),

[Aaron Boodman, 2012/06/08 05:31:30]

Nit: these params can be start on the same line as Create(, and you can put
multiple on one line.

+          Extension::EXTERNAL_PREF,

[Aaron Boodman, 2012/06/08 05:31:30]

Is there any particular reason for this location? Otherwise I would just use
INTERNAL since it is 'standard'. Using other than standard makes it seem
important somehow.

[not at google - send to devlin, 2012/06/12 20:40:51]

Oh ok. INTERNAL it is.

+          manifest,
+          0,

[Aaron Boodman, 2012/06/08 05:31:30]

Document or constify magic number. Same with string below.

[not at google - send to devlin, 2012/06/12 20:40:51]

Done.

+          "extension",
+          &error);
+      CHECK_EQ("", error);
+    }
+    {

[Aaron Boodman, 2012/06/08 05:31:30]

These blocks are a little unusual. Consider moving these initializations into
helper functions. You might be able to share a little code, too.

[not at google - send to devlin, 2012/06/12 20:40:51]

Done.

+      scoped_ptr<ListValue> permissions(new ListValue());
+        permissions->Append(Value::CreateStringValue("activeTab"));

[Aaron Boodman, 2012/06/08 05:31:30]

indent -= 2;

[not at google - send to devlin, 2012/06/12 20:40:51]

Done.

+      DictionaryValue manifest;
+        manifest.SetString("name", "Another extension with activeTab.");
+        manifest.SetString("version", "1.0.0");
+        manifest.SetInteger("manifest_version", 2);
+        manifest.Set("permissions", permissions.release());
+      std::string error;
+      another_extension_ = Extension::Create(
+          FilePath(),
+          Extension::EXTERNAL_PREF,
+          manifest,
+          0,
+          "another extension",
+          &error);
+      CHECK_EQ("", error);
+    }
+    {
+      scoped_ptr<ListValue> permissions(new ListValue());
+      DictionaryValue manifest;
+        manifest.SetString("name", "Extension without activeTab.");
+        manifest.SetString("version", "1.0.0");
+        manifest.SetInteger("manifest_version", 2);
+        manifest.Set("permissions", permissions.release());
+      std::string error;
+      extension_without_active_tab_ = Extension::Create(
+          FilePath(),
+          Extension::EXTERNAL_PREF,
+          manifest,
+          0,
+          "extension without activeTab",
+          &error);
+      CHECK_EQ("", error);
+    }
+  }
+
+ protected:
+  const Extension* extension() {
+    return extension_.get();

[Aaron Boodman, 2012/06/08 05:31:30]

Can you just make the members protected? The getters seem overkill when
everything is used within this one file.

[not at google - send to devlin, 2012/06/12 20:40:51]

It's so I could type () instead of .get() everywhere. If I just make these
methods accept scoped_refptrs though, won't need either.

+  }
+
+  const Extension* another_extension() {
+    return another_extension_.get();
+  }
+
+  const Extension* extension_without_active_tab() {
+    return extension_without_active_tab_.get();
+  }
+
+  int tab_id() {

[Aaron Boodman, 2012/06/08 05:31:30]

Nit: GetTabId().

[not at google - send to devlin, 2012/06/12 20:40:51]

Done.

+    return tab_contents()->extension_tab_helper()->GetTabId();
+  }
+
+  ActiveTabPermissionManager* manager() {

[Aaron Boodman, 2012/06/08 05:31:30]

'manager' is a little generic. Maybe active_tab_manager, or just
active_tab_permission_manager.

[not at google - send to devlin, 2012/06/12 20:40:51]

Done.

+    return tab_contents()->extension_tab_helper()->
+                           active_tab_permission_manager();
+  }
+
+  bool Allowed(const Extension* extension, const GURL& url) {

[Aaron Boodman, 2012/06/08 05:31:30]

IsAllowed

[not at google - send to devlin, 2012/06/12 20:40:51]

Done.

+    return Allowed(extension, url, tab_id());
+  }
+
+  bool Allowed(const Extension* extension, const GURL& url, int tab_id) {
+    return (extension->CanExecuteScriptOnPage(url, tab_id, NULL, NULL) &&
+            extension->CanCaptureVisiblePage(url, tab_id, NULL));
+  }
+
+  bool Blocked(const Extension* extension, const GURL& url) {

[Aaron Boodman, 2012/06/08 05:31:30]

IsBlocked

[not at google - send to devlin, 2012/06/12 20:40:51]

Done.

+    return Blocked(extension, url, tab_id());
+  }
+
+  bool Blocked(const Extension* extension, const GURL& url, int tab_id) {
+    return (!extension->CanExecuteScriptOnPage(url, tab_id, NULL, NULL) &&
+            !extension->CanCaptureVisiblePage(url, tab_id, NULL));
+  }
+
+  // Fakes loading a new frame on the page using the WebContentsObserver
+  // interface.
+  // TODO(kalman): if somebody can tell me a way to do this from the
+  // TabContentsTestHarness (or any other test harness) then pray tell.
+  void AddFrame(const GURL& url) {
+    manager()->DidCommitProvisionalLoadForFrame(
+        0,      // frame_id
+        false,  // is_main_frame
+        url,
+        content::PAGE_TRANSITION_AUTO_SUBFRAME,
+        NULL);  // render_view_host
+  }
+
+ private:
+  // An extension with the activeTab permission.
+  scoped_refptr<const Extension> extension_;
+
+  // Another extension with activeTab (for good measure).
+  scoped_refptr<const Extension> another_extension_;
+
+  // An extension without the activeTab permission.
+  scoped_refptr<const Extension> extension_without_active_tab_;
+
+  content::TestBrowserThread ui_thread_;
+};
+
+TEST_F(ActiveTabTest, GrantToSinglePage) {
+  GURL google("http://www.google.com");
+  NavigateAndCommit(google);
+
+  // No access unless it's been granted.
+  EXPECT_TRUE(Blocked(extension(), google));
+  EXPECT_TRUE(Blocked(another_extension(), google));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), google));
+
+  manager()->MaybeGrant(extension());
+  manager()->MaybeGrant(extension_without_active_tab());
+
+  // Granted to extension() and extension_without_active_tab(), but the latter
+  // doesn't have the activeTab permission so not granted.
+  EXPECT_TRUE(Allowed(extension(), google));
+  EXPECT_TRUE(Blocked(another_extension(), google));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), google));
+
+  // Other subdomains shouldn't be given access.
+  GURL mail_google("http://mail.google.com");
+  EXPECT_TRUE(Blocked(extension(), mail_google));
+  EXPECT_TRUE(Blocked(another_extension(), google));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), google));
+
+  // Reloading the page should clear the active permissions.
+  Reload();
+
+  EXPECT_TRUE(Blocked(extension(), google));
+  EXPECT_TRUE(Blocked(another_extension(), google));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), google));
+
+  // But they should still be able to be granted again.
+  manager()->MaybeGrant(extension());
+
+  EXPECT_TRUE(Allowed(extension(), google));
+  EXPECT_TRUE(Blocked(another_extension(), google));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), google));
+
+  // And grant a few more times redundantly for good measure.
+  manager()->MaybeGrant(extension());
+  manager()->MaybeGrant(extension());
+  manager()->MaybeGrant(another_extension());
+  manager()->MaybeGrant(another_extension());
+  manager()->MaybeGrant(another_extension());
+  manager()->MaybeGrant(extension());
+  manager()->MaybeGrant(extension());
+  manager()->MaybeGrant(another_extension());
+  manager()->MaybeGrant(another_extension());
+
+  EXPECT_TRUE(Allowed(extension(), google));
+  EXPECT_TRUE(Allowed(another_extension(), google));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), google));
+
+  // Navigating to a new URL should clear the active permissions.
+  GURL chromium("http://www.chromium.org");
+  NavigateAndCommit(chromium);
+
+  EXPECT_TRUE(Blocked(extension(), google));
+  EXPECT_TRUE(Blocked(another_extension(), google));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), google));
+
+  EXPECT_TRUE(Blocked(extension(), chromium));
+  EXPECT_TRUE(Blocked(another_extension(), chromium));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), chromium));
+
+  // Should be able to grant to multiple extensions at the same time (if they
+  // have the activeTab permission, of course).
+  manager()->MaybeGrant(extension());
+  manager()->MaybeGrant(another_extension());
+  manager()->MaybeGrant(extension_without_active_tab());
+
+  EXPECT_TRUE(Blocked(extension(), google));
+  EXPECT_TRUE(Blocked(another_extension(), google));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), google));
+
+  EXPECT_TRUE(Allowed(extension(), chromium));
+  EXPECT_TRUE(Allowed(another_extension(), chromium));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), chromium));
+
+  // Should be able to go back to URLs that were previously cleared.
+  NavigateAndCommit(google);
+
+  manager()->MaybeGrant(extension());
+  manager()->MaybeGrant(another_extension());
+  manager()->MaybeGrant(extension_without_active_tab());
+
+  EXPECT_TRUE(Allowed(extension(), google));
+  EXPECT_TRUE(Allowed(another_extension(), google));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), google));
+
+  EXPECT_TRUE(Blocked(extension(), chromium));
+  EXPECT_TRUE(Blocked(another_extension(), chromium));
+  EXPECT_TRUE(Blocked(extension_without_active_tab(), chromium));
+};
+
+TEST_F(ActiveTabTest, GrantToMultiplePages) {
+  GURL google("http://www.google.com");
+  NavigateAndCommit(google);
+
+  manager()->MaybeGrant(extension());
+
+  // Adding a frame after access was granted shouldn't give it access.
+  GURL chromium("http://www.chromium.org");
+  AddFrame(chromium);
+
+  EXPECT_TRUE(Allowed(extension(), google));
+  EXPECT_TRUE(Blocked(extension(), chromium));
+
+  // Granting access to another extension should give it access to both the
+  // main and sub-frames, but still not to the first extension.
+  manager()->MaybeGrant(another_extension());
+
+  EXPECT_TRUE(Allowed(extension(), google));
+  EXPECT_TRUE(Blocked(extension(), chromium));
+  EXPECT_TRUE(Allowed(another_extension(), google));
+  EXPECT_TRUE(Allowed(another_extension(), chromium));
+
+  // Granting access to the first extension should now give it access to the
+  // frame.
+  manager()->MaybeGrant(extension());
+
+  EXPECT_TRUE(Allowed(extension(), google));
+  EXPECT_TRUE(Allowed(extension(), chromium));
+  EXPECT_TRUE(Allowed(another_extension(), google));
+  EXPECT_TRUE(Allowed(another_extension(), chromium));
+
+  // Reloading should clear all access.
+  Reload();
+
+  EXPECT_TRUE(Blocked(extension(), google));
+  EXPECT_TRUE(Blocked(extension(), chromium));
+  EXPECT_TRUE(Blocked(another_extension(), google));
+  EXPECT_TRUE(Blocked(another_extension(), chromium));
+
+  // And after granting, no access to the frames that were there.
+  manager()->MaybeGrant(extension());
+
+  EXPECT_TRUE(Allowed(extension(), google));
+  EXPECT_TRUE(Blocked(extension(), chromium));
+  EXPECT_TRUE(Blocked(another_extension(), google));
+  EXPECT_TRUE(Blocked(another_extension(), chromium));
+
+  // Having lots of frames on the same page should behave as expected.
+  GURL chromium_index("http://www.chromium.org/index.html");
+  GURL chromium_about("http://www.chromium.org/about.html");
+  GURL chromium_blank("http://www.chromium.org/blank.html");
+  GURL gmail("http://www.gmail.com");
+  GURL mail_google("http://mail.google.com");
+  GURL plus_google("http://plus.google.com");
+  GURL codereview_appspot("http://codereview.appspot.com");
+  GURL omahaproxy_appspot("http://omahaproxy.appspot.com");
+
+  AddFrame(chromium_index);
+  AddFrame(chromium_about);
+  AddFrame(gmail);
+  AddFrame(mail_google);
+
+  EXPECT_TRUE(Blocked(extension(), chromium_index));
+  EXPECT_TRUE(Blocked(extension(), chromium_about));
+  EXPECT_TRUE(Blocked(extension(), chromium_blank));
+  EXPECT_TRUE(Blocked(extension(), gmail));
+  EXPECT_TRUE(Blocked(extension(), mail_google));
+  EXPECT_TRUE(Blocked(extension(), plus_google));
+  EXPECT_TRUE(Blocked(extension(), codereview_appspot));
+  EXPECT_TRUE(Blocked(extension(), omahaproxy_appspot));
+
+  EXPECT_TRUE(Blocked(another_extension(), chromium_index));
+  EXPECT_TRUE(Blocked(another_extension(), chromium_about));
+  EXPECT_TRUE(Blocked(another_extension(), chromium_blank));
+  EXPECT_TRUE(Blocked(another_extension(), gmail));
+  EXPECT_TRUE(Blocked(another_extension(), mail_google));
+  EXPECT_TRUE(Blocked(another_extension(), plus_google));
+  EXPECT_TRUE(Blocked(another_extension(), codereview_appspot));
+  EXPECT_TRUE(Blocked(another_extension(), omahaproxy_appspot));
+
+  manager()->MaybeGrant(extension());
+
+  AddFrame(chromium_blank);
+  AddFrame(plus_google);
+  AddFrame(codereview_appspot);
+
+  EXPECT_TRUE(Allowed(extension(), chromium_index));
+  EXPECT_TRUE(Allowed(extension(), chromium_about));
+  EXPECT_TRUE(Allowed(extension(), chromium_blank));

[Aaron Boodman, 2012/06/08 05:31:30]

Maybe comment that this works because the comparison is by origin?

[not at google - send to devlin, 2012/06/12 20:40:51]

Done.

+  EXPECT_TRUE(Allowed(extension(), gmail));
+  EXPECT_TRUE(Allowed(extension(), mail_google));
+  EXPECT_TRUE(Blocked(extension(), plus_google));
+  EXPECT_TRUE(Blocked(extension(), codereview_appspot));
+  EXPECT_TRUE(Blocked(extension(), omahaproxy_appspot));
+
+  EXPECT_TRUE(Blocked(another_extension(), chromium_index));
+  EXPECT_TRUE(Blocked(another_extension(), chromium_about));
+  EXPECT_TRUE(Blocked(another_extension(), chromium_blank));
+  EXPECT_TRUE(Blocked(another_extension(), gmail));
+  EXPECT_TRUE(Blocked(another_extension(), mail_google));
+  EXPECT_TRUE(Blocked(another_extension(), plus_google));
+  EXPECT_TRUE(Blocked(another_extension(), codereview_appspot));
+  EXPECT_TRUE(Blocked(another_extension(), omahaproxy_appspot));
+
+  manager()->MaybeGrant(another_extension());
+
+  AddFrame(omahaproxy_appspot);
+
+  EXPECT_TRUE(Allowed(extension(), chromium_index));
+  EXPECT_TRUE(Allowed(extension(), chromium_about));
+  EXPECT_TRUE(Allowed(extension(), chromium_blank));
+  EXPECT_TRUE(Allowed(extension(), gmail));
+  EXPECT_TRUE(Allowed(extension(), mail_google));
+  EXPECT_TRUE(Blocked(extension(), plus_google));
+  EXPECT_TRUE(Blocked(extension(), codereview_appspot));
+  EXPECT_TRUE(Blocked(extension(), omahaproxy_appspot));
+
+  EXPECT_TRUE(Allowed(another_extension(), chromium_index));
+  EXPECT_TRUE(Allowed(another_extension(), chromium_about));
+  EXPECT_TRUE(Allowed(another_extension(), chromium_blank));
+  EXPECT_TRUE(Allowed(another_extension(), gmail));
+  EXPECT_TRUE(Allowed(another_extension(), mail_google));
+  EXPECT_TRUE(Allowed(another_extension(), plus_google));
+  EXPECT_TRUE(Allowed(another_extension(), codereview_appspot));
+  EXPECT_TRUE(Blocked(another_extension(), omahaproxy_appspot));
+
+  manager()->MaybeGrant(extension());
+
+  EXPECT_TRUE(Allowed(extension(), chromium_index));
+  EXPECT_TRUE(Allowed(extension(), chromium_about));
+  EXPECT_TRUE(Allowed(extension(), chromium_blank));
+  EXPECT_TRUE(Allowed(extension(), gmail));
+  EXPECT_TRUE(Allowed(extension(), mail_google));
+  EXPECT_TRUE(Allowed(extension(), plus_google));
+  EXPECT_TRUE(Allowed(extension(), codereview_appspot));
+  EXPECT_TRUE(Allowed(extension(), omahaproxy_appspot));
+
+  EXPECT_TRUE(Allowed(another_extension(), chromium_index));
+  EXPECT_TRUE(Allowed(another_extension(), chromium_about));
+  EXPECT_TRUE(Allowed(another_extension(), chromium_blank));
+  EXPECT_TRUE(Allowed(another_extension(), gmail));
+  EXPECT_TRUE(Allowed(another_extension(), mail_google));
+  EXPECT_TRUE(Allowed(another_extension(), plus_google));
+  EXPECT_TRUE(Allowed(another_extension(), codereview_appspot));
+  EXPECT_TRUE(Blocked(another_extension(), omahaproxy_appspot));
+}
+
+TEST_F(ActiveTabTest, Uninstalling) {
+  // Some semi-arbitrary setup.
+  GURL google("http://www.google.com");
+  NavigateAndCommit(google);
+
+  GURL chromium("http://www.chromium.org");
+  AddFrame(chromium);
+
+  manager()->MaybeGrant(extension());
+
+  GURL gmail("http://www.gmail.com");
+  AddFrame(gmail);
+
+  EXPECT_TRUE(Allowed(extension(), google));
+  EXPECT_TRUE(Allowed(extension(), chromium));
+  EXPECT_TRUE(Blocked(extension(), gmail));
+
+  // Uninstalling the extension should clear its tab permissions.
+  UnloadedExtensionInfo details(
+      extension(),
+      extension_misc::UNLOAD_REASON_DISABLE);
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_EXTENSION_UNLOADED,
+      content::Source<Profile>(tab_contents()->profile()),
+      content::Details<UnloadedExtensionInfo>(&details));
+
+  EXPECT_TRUE(Blocked(extension(), google));
+  EXPECT_TRUE(Blocked(extension(), chromium));
+  EXPECT_TRUE(Blocked(extension(), gmail));
+
+  // Granting the extension again should give them back.
+  manager()->MaybeGrant(extension());
+
+  EXPECT_TRUE(Allowed(extension(), google));
+  EXPECT_TRUE(Allowed(extension(), chromium));
+  EXPECT_TRUE(Allowed(extension(), gmail));
+}
+
+TEST_F(ActiveTabTest, OnlyActiveTab) {
+  GURL google("http://www.google.com");
+  NavigateAndCommit(google);
+
+  manager()->MaybeGrant(extension());
+
+  EXPECT_TRUE(Allowed(extension(), google, tab_id()));
+  EXPECT_TRUE(Blocked(extension(), google, tab_id() + 1));
+}

[Aaron Boodman, 2012/06/08 05:31:30]

Very nice tests.

[not at google - send to devlin, 2012/06/12 20:40:51]

Thank you.

+
+}  // namespace
+}  // namespace extensions