Create withheld permissions

extensions/common/permissions/permissions_data.h

Index: extensions/common/permissions/permissions_data.h
diff --git a/extensions/common/permissions/permissions_data.h b/extensions/common/permissions/permissions_data.h
index 9470d586ba8320bfbe1bdddbaf3febbc27f81755..ad6b331d0f9813e790559a22c1bc057856cc4f0b 100644
--- a/extensions/common/permissions/permissions_data.h
+++ b/extensions/common/permissions/permissions_data.h
@@ -35,6 +35,14 @@ class UserScript;
 // straight.
 class PermissionsData {
  public:
+  // The possible types of access for a given frame.
+  enum AccessType {
+    DENY_ACCESS,    // The extension is not allowed to access the given page.
+    ALLOW_ACCESS,   // The extension is allowed to access the given page.
+    REQUEST_ACCESS  // The browser must determine if the extension can access

[not at google - send to devlin, 2014/06/27 23:24:34]

the concept of "request access" isn't really the concern of permissions data
(and this goes for the methods like CanRunCOntentScriptWIthUserConsent). it
should just deal with granted vs withheld permissions. like the enum should be
"yes", "no", or "no but it's been withheld". or maybe more mathematically,
perhaps:

enum AccessType {
  kAccessAllowed,
  kAccessDenied,
  kAccessWithheld
};

and then let callers interpret that.

[Devlin, 2014/06/30 17:06:10]

Done (though sadly we use SCREAMING_STYLE instead of kConstantStyle for enums
:()

+                    // the given page.
+  };
+
   // Delegate class to allow different contexts (e.g. browser vs renderer) to
   // have control over policy decisions.
   class PolicyDelegate {
@@ -78,6 +86,16 @@ class PermissionsData {
 
   // Sets the runtime permissions of the given |extension| to |permissions|.
   void SetActivePermissions(const PermissionSet* active) const;
+  // Initializes the withheld/active permissions from |permissions|.

[not at google - send to devlin, 2014/06/27 23:24:34]

blank line above here

[Devlin, 2014/06/30 17:06:10]

Done.

+  void InitializePermissions(const PermissionSet* permissions,

[not at google - send to devlin, 2014/06/27 23:24:34]

it's odd to call this InitializePermissions when it's not actually being used on
initialization (PermissionsUpdater uses it). ideally you'd just not expose
anything like this and have the |permissions_data| of Extension replaced by a
new permission set. in lieu of that... call this something else.
ImportFromExtension maybe. Reset. along those lines.

[Devlin, 2014/06/30 17:06:10]

I don't feel really strongly about the name on this... but it's called from
PermissionsUpdater::InitializeActivePermissions(), which is called when an
extension's permissions are initialized... so isn't the name fairly accurate? 
It's used for initialization, and should be used for initialization _only_
(since it does all the withholding stuff).

[Devlin, 2014/06/30 20:28:58]

Per offline chat, this has basically been moved to PermissionsUpdater.

+                             const Extension* extension) const;
+
+  // Sets the runtime permissions of the given |extension| to |active| and
+  // |withheld|.
+  // This should only be used as a means of "copying" permissions data, e.g.
+  // to duplicate the information in the renderer.

[not at google - send to devlin, 2014/06/27 23:24:34]

I don't see this method being used that way, like it's used in
PermissionsUpdater::SetActivePermissions.

I find the name a bit confusing compared to InitializePermissions.

could both of these be replaced by a set_permissions_data method on Extension?

[Devlin, 2014/06/30 17:06:10]

You're right - I didn't update the comment after the changes to
PermissionsUpdater.  Done.
And we couldn't really use a set_permissions_data()... see below on immutable
permissions datas.

+  void SetPermissions(const PermissionSet* active,
+                      const PermissionSet* withheld) const;
 
   // Updates the tab-specific permissions of |tab_id| to include those from
   // |permissions|.
@@ -132,6 +150,23 @@ class PermissionsData {
   // display at install time as strings.
   std::vector<base::string16> GetPermissionMessageDetailsStrings() const;
 
+  // The following three functions pertain to (possibly withheld) all-hosts
+  // permissions. Since all-hosts can take many different forms, we have
+  // these functions for them. Any additional withheld permissions should
+  // simply be checked, granted, and withheld via a generic
+  // HasWithheldPermission(), GrantWithheldPermission(), and
+  // WithholdPermission().
+
+  // Returns true if the extension has requested all-hosts permissions (or
+  // something close to it), but has had it withheld.
+  bool HasWithheldAllHosts() const;

[not at google - send to devlin, 2014/06/30 14:37:03]

Why specifically all hosts? The way I was imagining this was more abstract.

- See comment in ActiveTabPermissionGranter for how that could be implemented
without using this.

- The logic that deals with actually withholding permissions (initialization,
preferences) can just rebuild the permissions data each time based on the state
of the world. That is, rather than the permissions logic being

    if (allowed)
      extension->permissions_data()->GrantWithheldAllHosts();
    else
      extension->permissions_data()->WithholdAllHosts();

it could be

    // Putting this on Extension seems like the easiest way to share state,
    // particularly since this will need to work on the renderer eventually,
    // not to mention on startup.
    extension->SetWithholdingPermissions(allowed);
    // and I mentioned elsewhere that I'd rather PermissionsData be
    // immutable and Extension just rebuild it itself, if possible.
    extension->permissions_data()->Rebuild();

[Devlin, 2014/06/30 17:06:10]

See comment in ActiveTabPermissionGranter for why we need this. ;)

I think there are a few things wrong with this.
- We've deliberately tried to _remove_ permissions logic from the Extension
class.
- Simply saying "set withholding permissions" isn't scalable.  We don't want to
have to choose between "withhold none" and "withhold all withholdable".
- PermissionsData is by nature not immutable.  It'd be weird enough having to
rebuild it each time we grant an optional permission, but having to rebuild it
for every active tab?

[not at google - send to devlin, 2014/07/01 00:28:35]

good points. moving this to PermissionsUpdater has made this more or less moot,
right?

[Devlin, 2014/07/01 16:27:05]

Yep.

+
+  // Grants any withheld all-hosts (or all-hosts-like) permissions.
+  void GrantWithheldAllHosts() const;
+
+  // Revokes any requests all-hosts (or all-hosts-like) permissions.
+  void WithholdAllHosts() const;
+
   // Returns true if the |extension| has permission to access and interact with
   // the specified page, in order to do things like inject scripts or modify
   // the content.
@@ -143,6 +178,15 @@ class PermissionsData {
                      int tab_id,
                      int process_id,
                      std::string* error) const;
+  // Like CanAccessPage, but also takes withheld permissions into account.
+  // TODO(rdevlin.cronin) We shouldn't have two functions, but not all callers
+  // know how to wait for permission.
+  AccessType CanAccessPageWithUserConsent(const Extension* extension,

[not at google - send to devlin, 2014/06/27 23:24:34]

in the spirit of the comment at the top of the enum, this name shouldn't imply a
boolean since it's no longer a boolean. instead GetPageAccess.

[Devlin, 2014/06/30 17:06:10]

Shorter to write, yay.

+                                          const GURL& document_url,
+                                          const GURL& top_document_url,
+                                          int tab_id,
+                                          int process_id,
+                                          std::string* error) const;
 
   // Returns true if the |extension| has permission to inject a content script
   // on the page.
@@ -156,6 +200,17 @@ class PermissionsData {
                                  int tab_id,
                                  int process_id,
                                  std::string* error) const;
+  // Like CanRunContentScriptOnPage, but also takes withheld permissions into
+  // account.
+  // TODO(rdevlin.cronin) We shouldn't have two functions, but not all callers
+  // know how to wait for permission.
+  AccessType CanRunContentScriptOnPageWithUserConsent(

[not at google - send to devlin, 2014/06/27 23:24:34]

likewise, GetContentScriptAccess

[Devlin, 2014/06/30 17:06:10]

Done.

+      const Extension* extension,
+      const GURL& document_url,
+      const GURL& top_document_url,
+      int tab_id,
+      int process_id,
+      std::string* error) const;
 
   // Returns true if extension is allowed to obtain the contents of a page as
   // an image. Since a page may contain sensitive information, this is
@@ -163,19 +218,16 @@ class PermissionsData {
   // page itself.
   bool CanCaptureVisiblePage(int tab_id, std::string* error) const;
 
-  // Returns true if the user should be alerted that the |extension| is running
-  // a script. If |tab_id| and |url| are included, this also considers tab-
-  // specific permissions.
-  bool RequiresActionForScriptExecution(const Extension* extension) const;
-  bool RequiresActionForScriptExecution(const Extension* extension,
-                                        int tab_id,
-                                        const GURL& url) const;
-
   scoped_refptr<const PermissionSet> active_permissions() const {
     base::AutoLock auto_lock(runtime_lock_);
     return active_permissions_unsafe_;
   }
 
+  scoped_refptr<const PermissionSet> withheld_permissions() const {
+    base::AutoLock auto_lock(runtime_lock_);
+    return withheld_permissions_unsafe_;
+  }
+
 #if defined(UNIT_TEST)
   scoped_refptr<const PermissionSet> GetTabSpecificPermissionsForTesting(
       int tab_id) const {
@@ -198,16 +250,17 @@ class PermissionsData {
   bool HasTabSpecificPermissionToExecuteScript(int tab_id,
                                                const GURL& url) const;
 
-  // Returns true if the extension is permitted to run on the given page,
+  // Returns whether or not the extension is permitted to run on the given page,
   // checking against |permitted_url_patterns| in addition to blocking special
   // sites (like the webstore or chrome:// urls).
-  bool CanRunOnPage(const Extension* extension,
-                    const GURL& document_url,
-                    const GURL& top_document_url,
-                    int tab_id,
-                    int process_id,
-                    const URLPatternSet& permitted_url_patterns,
-                    std::string* error) const;
+  AccessType CanRunOnPage(const Extension* extension,
+                          const GURL& document_url,
+                          const GURL& top_document_url,
+                          int tab_id,
+                          int process_id,
+                          const URLPatternSet& permitted_url_patterns,
+                          const URLPatternSet& withheld_url_patterns,
+                          std::string* error) const;
 
   // The associated extension's id.
   std::string extension_id_;
@@ -224,6 +277,13 @@ class PermissionsData {
   // active_permissions() accessor.
   mutable scoped_refptr<const PermissionSet> active_permissions_unsafe_;
 
+  // The permissions the extension requested, but was not granted due because
+  // they are too powerful. This includes things like all_hosts.
+  // Unsafe indicates that we must lock anytime this is directly accessed.
+  // Unless you need to change |withheld_permissions_unsafe_|, use the (safe)
+  // withheld_permissions() accessor.
+  mutable scoped_refptr<const PermissionSet> withheld_permissions_unsafe_;
+
   mutable TabPermissionsMap tab_specific_permissions_;
 
   DISALLOW_COPY_AND_ASSIGN(PermissionsData);