tab capture: Change the permissions for tabs.captureVisibleTab().

extensions/common/permissions/permissions_data.cc

Index: extensions/common/permissions/permissions_data.cc
diff --git a/extensions/common/permissions/permissions_data.cc b/extensions/common/permissions/permissions_data.cc
index 786d34df9c342447e2ab60ebf4fa3f03265e4ab4..6755384354b1384793e31b72bb5d167427b83255 100644
--- a/extensions/common/permissions/permissions_data.cc
+++ b/extensions/common/permissions/permissions_data.cc
@@ -537,27 +537,25 @@ bool PermissionsData::CanExecuteScriptEverywhere(const Extension* extension) {
 
 // static
 bool PermissionsData::CanCaptureVisiblePage(const Extension* extension,
-                                            const GURL& page_url,
                                             int tab_id,
                                             std::string* error) {
-  if (tab_id >= 0) {
-    scoped_refptr<const PermissionSet> tab_permissions =
-        GetTabSpecificPermissions(extension, tab_id);
-    if (tab_permissions.get() &&
-        tab_permissions->explicit_hosts().MatchesSecurityOrigin(page_url)) {
-      return true;
-    }
+  scoped_refptr<const PermissionSet> tab_permissions =
+      GetActivePermissions(extension);
+  URLPattern all_urls(URLPattern::SCHEME_ALL);
+  all_urls.SetMatchAllURLs(true);

[not at google - send to devlin, 2014/01/16 17:48:26]

you can construct this in a single line with

URLPattern all_urls(URLPattern::SCHEME_ALL, URLPattern::kAllUrlsPattern);

since kAllUrlsPattern is guaranteed to parse.

[sadrul, 2014/01/20 13:53:37]

Done.

+  if (tab_permissions.get() &&

[not at google - send to devlin, 2014/01/16 17:48:26]

I think there will always be active permissions, so you don't need to check
this?

[sadrul, 2014/01/20 13:53:37]

Done.

[not at google - send to devlin, 2014/01/21 21:44:21]

I think I may have misread your code actually :\

[sadrul, 2014/01/23 20:11:14]

Restored the null check for |tab_permissions|

+      tab_permissions->explicit_hosts().ContainsPattern(all_urls)) {
+    return true;
   }
 
-  if (HasHostPermission(extension, page_url) ||
-      page_url.GetOrigin() == extension->url()) {
+  if (tab_id >= 0 &&
+      HasAPIPermission(extension, APIPermission::kActiveTab) &&
+      GetTabSpecificPermissions(extension, tab_id)) {

[not at google - send to devlin, 2014/01/16 17:48:26]

I think the old check was what you want? the
tab_permissions->explicit_hosts().MatchesSecurityOrigin(page_url)?

[sadrul, 2014/01/20 13:53:37]

If the extension has the |activeTab| permission, and already been granted that
permission, does it still need to do MatchesSecurityOrigin()? I removed the
|page_url| parameter from CanCaptureVisiblePage() based on the reasoning that
this check would be superfluous. Let me know if I am mistaken about this.

[not at google - send to devlin, 2014/01/21 21:44:21]

I think you're right; the checks are actually effectively equivalent since
- the only way to get tab specific origin permissions is via activeTab
- the origin is cleared each navigation.

as you say, removing the URL check from CaptureVisibleTab is kind of nice in
that it now rightly implies that it no longer matters.

Having the check be "does this extension have any tab specific permissions"
seems too flaky to me though. What if we have some kind of tab specific
permissions at some point which aren't triggered by a user action? This would
break then.

I see two options. First, easy; check the kTabs permission here, i.e., identical
to before except instead of MatchesSecurityOrigin use HasAPIPermission(kTabs). I
think that would work. Second, not as easy, introduce another permission like
kCaptureVisibleTabForTab like what the tabCapture API uses. That seems
unnecessary, so, the first one?

[sadrul, 2014/01/23 20:11:14]

Yep, sounds good. Done.

     return true;
   }
 
-  if (error) {
-    *error = ErrorUtils::FormatErrorMessage(errors::kCannotAccessPage,
-                                            page_url.spec());
-  }
+  if (error)
+    *error = errors::kAllURLOrActiveTabNeeded;

[not at google - send to devlin, 2014/01/16 17:48:26]

hopefully developers won't be confused... "but I have the activeTab
permission!". maybe this error message needs to be improved slightly to explain
that activeTab also needs to have been granted (or have 2 separate error
messages, one when neither is present, and one when activeTab is present but not
granted).

[sadrul, 2014/01/20 13:53:37]

Done. (please let me know if you have suggestion about better error messages)

   return false;
 }