[Extensions Click-to-Script] More extensions page data tweaks

Tweak the data sent to the extensions page slightly more. Move all
runtime host permissions data into a separate field on the
developerPrivate.Permissions struct, and unconditionally populate all
fields (including specific granted hosts) if the feature is enabled.
This simplifies some of the code we need to pass to the
RuntimeHostPermissions element, since we can now just pass
developerPrivate.RuntimeHostPermissions, and paves the way for adding
a separate toggle-able implementation for extensions that request access
to specific sites.

No observable UI or behavior change is expected in this CL.

Update and add tests for the same.

Bug: 891803

Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: Id95d3b0a77d8621db811a28fcdcfd705d60ed0f6
Reviewed-on: https://chromium-review.googlesource.com/c/1295271
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605046}

chrome/browser/extensions/api/developer_private/extension_info_generator.cc

@@ -14,6 +14,7 @@
 #include "base/callback_helpers.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
+#include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
@@ -225,17 +226,17 @@ void ConstructCommands(CommandService* command_service,
 
 // Creates and returns a SpecificSiteControls object for the given
 // |granted_permissions| and |withheld_permissions|.
-std::unique_ptr<developer::SpecificSiteControls> GetSpecificSiteControls(
-    const PermissionSet& runtime_granted_permissions,
+std::vector<developer::SiteControl> GetSpecificSiteControls(
+    const PermissionSet& granted_permissions,
     const PermissionSet& withheld_permissions) {
-  auto controls = std::make_unique<developer::SpecificSiteControls>();
-  constexpr bool kIncludeApiPermissions = false;
-  controls->has_all_hosts =
-      withheld_permissions.ShouldWarnAllHosts(kIncludeApiPermissions) ||
-      runtime_granted_permissions.ShouldWarnAllHosts(kIncludeApiPermissions);
+  std::vector<developer::SiteControl> controls;
 
+  // NOTE(devlin): This is similar, but not identical, to our host collapsing
+  // for permission warnings. The primary difference is that this will not
+  // collapse permissions for sites with separate TLDs; i.e., google.com and
+  // google.net will remain distinct entities in this list.
   auto get_distinct_hosts = [](const URLPatternSet& patterns) {
-    std::set<std::string> distinct_hosts;
+    std::vector<URLPattern> pathless_hosts;
     for (URLPattern pattern : patterns) {
       // We only allow addition/removal of full hosts (since from a
       // permissions point of view, path is irrelevant). We always make the
@@ -244,27 +245,59 @@ std::unique_ptr<developer::SpecificSiteControls> GetSpecificSiteControls(
       // TODO(devlin): Investigate, and possibly change the optional
       // permissions API.
       pattern.SetPath("/*");
-      distinct_hosts.insert(pattern.GetAsString());
+      pathless_hosts.push_back(std::move(pattern));
+    }
+
+    // Iterate over the list of hosts and add any that aren't entirely contained
+    // by another pattern. This is pretty inefficient, but the list of hosts
+    // should be reasonably small.
+    std::vector<const URLPattern*> distinct_hosts;
+    for (const URLPattern& host : pathless_hosts) {
+      // If the host is fully contained within the set, we don't add it again.
+      bool consumed_by_other = false;
+      for (const URLPattern* added_host : distinct_hosts) {
+        if (added_host->Contains(host)) {
+          consumed_by_other = true;
+          break;
+        }
+      }
+      if (consumed_by_other)
+        continue;
+
+      // Otherwise, add the host. This might mean we get to prune some hosts
+      // from |distinct_hosts|.
+      base::EraseIf(distinct_hosts, [host](const URLPattern* other_host) {
+        return host.Contains(*other_host);
+      });
+
+      distinct_hosts.push_back(&host);
     }
-    return distinct_hosts;
+
+    std::vector<std::string> distinct_host_strings;
+    distinct_host_strings.reserve(distinct_hosts.size());
+    for (const URLPattern* host : distinct_hosts)
+      distinct_host_strings.push_back(host->GetAsString());
+
+    return distinct_host_strings;
   };
 
-  std::set<std::string> distinct_granted =
-      get_distinct_hosts(runtime_granted_permissions.effective_hosts());
-  std::set<std::string> distinct_withheld =
+  std::vector<std::string> distinct_granted =
+      get_distinct_hosts(granted_permissions.effective_hosts());
+  std::vector<std::string> distinct_withheld =
       get_distinct_hosts(withheld_permissions.effective_hosts());
-  controls->hosts.reserve(distinct_granted.size() + distinct_withheld.size());
+  controls.reserve(distinct_granted.size() + distinct_withheld.size());
+
   for (auto& host : distinct_granted) {
     developer::SiteControl host_control;
     host_control.host = std::move(host);
     host_control.granted = true;
-    controls->hosts.push_back(std::move(host_control));
+    controls.push_back(std::move(host_control));
   }
   for (auto& host : distinct_withheld) {
     developer::SiteControl host_control;
     host_control.host = std::move(host);
     host_control.granted = false;
-    controls->hosts.push_back(std::move(host_control));
+    controls.push_back(std::move(host_control));
   }
 
   return controls;
@@ -316,27 +349,40 @@ void AddPermissionsInfo(content::BrowserContext* browser_context,
                                             extension.GetType()));
   permissions->simple_permissions = get_permission_messages(api_messages);
 
+  auto runtime_host_permissions =
+      std::make_unique<developer::RuntimeHostPermissions>();
+
+  ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(browser_context);
+  // "Effective" granted permissions are stored in different prefs, based on
+  // whether host permissions are withheld.
+  // TODO(devlin): Create a common helper method to retrieve granted prefs based
+  // on whether host permissions are withheld?
+  std::unique_ptr<const PermissionSet> granted_permissions;
   // Add the host access data, including the mode and any runtime-granted
   // hosts.
   if (!permissions_modifier.HasWithheldHostPermissions()) {
-    permissions->host_access = developer::HOST_ACCESS_ON_ALL_SITES;
+    granted_permissions =
+        extension_prefs->GetGrantedPermissions(extension.id());
+    runtime_host_permissions->host_access = developer::HOST_ACCESS_ON_ALL_SITES;
   } else {
-    ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(browser_context);
-    std::unique_ptr<const PermissionSet> runtime_granted_permissions =
+    granted_permissions =
         extension_prefs->GetRuntimeGrantedPermissions(extension.id());
-    if (runtime_granted_permissions->effective_hosts().is_empty()) {
-      // TODO(devlin): This isn't quite right - it's possible the user just
-      // selected "on specific sites" from the dropdown, and hasn't yet added
-      // any sites. We'll need to handle this.
-      // https://crbug.com/844128.
-      permissions->host_access = developer::HOST_ACCESS_ON_CLICK;
-    } else {
-      permissions->host_access = developer::HOST_ACCESS_ON_SPECIFIC_SITES;
-      permissions->specific_site_controls = GetSpecificSiteControls(
-          *runtime_granted_permissions,
-          extension.permissions_data()->withheld_permissions());
-    }
+    runtime_host_permissions->host_access =
+        granted_permissions->effective_hosts().is_empty()
+            ? developer::HOST_ACCESS_ON_CLICK
+            : developer::HOST_ACCESS_ON_SPECIFIC_SITES;
   }
+
+  runtime_host_permissions->hosts = GetSpecificSiteControls(
+      *granted_permissions,
+      extension.permissions_data()->withheld_permissions());
+  constexpr bool kIncludeApiPermissions = false;
+  runtime_host_permissions->has_all_hosts =
+      extension.permissions_data()->withheld_permissions().ShouldWarnAllHosts(
+          kIncludeApiPermissions) ||
+      granted_permissions->ShouldWarnAllHosts(kIncludeApiPermissions);
+
+  permissions->runtime_host_permissions = std::move(runtime_host_permissions);
 }
 
 }  // namespace

chrome/browser/resources/md_extensions/detail_view.html

@@ -329,9 +329,9 @@
                 </li>
               </template>
             </ul>
-            <template is="dom-if" if="[[showRuntimeHostPermissions_(data.*)]]">
+            <template is="dom-if" if="[[hasRuntimeHostPermissions_(data.*)]]">
               <extensions-runtime-host-permissions
-                  permissions="[[data.permissions]]"
+                  permissions="[[data.permissions.runtimeHostPermissions]]"
                   delegate="[[delegate]]"
                   item-id="[[data.id]]">
               </extensions-runtime-host-permissions>

chrome/browser/resources/md_extensions/detail_view.js

@@ -292,15 +292,15 @@ cr.define('extensions', function() {
      */
     hasPermissions_: function() {
       return this.data.permissions.simplePermissions.length > 0 ||
-          !!this.data.permissions.hostAccess;
+          this.hasRuntimeHostPermissions_();
     },
 
     /**
      * @return {boolean}
      * @private
      */
-    showRuntimeHostPermissions_: function() {
-      return !!this.data.permissions.hostAccess;
+    hasRuntimeHostPermissions_: function() {
+      return !!this.data.permissions.runtimeHostPermissions;
     },
   });
 

chrome/browser/resources/md_extensions/runtime_host_permissions.html

@@ -81,7 +81,7 @@
       </div>
       <ul id="hosts">
         <template is="dom-repeat"
-            items="[[getRuntimeHosts_(permissions.specificSiteControls)]]">
+            items="[[getRuntimeHosts_(permissions.hosts)]]">
           <li>
             <div>[[item]]</div>
             <paper-icon-button-light class="icon-more-vert">

chrome/browser/resources/md_extensions/runtime_host_permissions.js

@@ -11,7 +11,7 @@ cr.define('extensions', function() {
     properties: {
       /**
        * The underlying permissions data.
-       * @type {chrome.developerPrivate.Permissions}
+       * @type {chrome.developerPrivate.RuntimeHostPermissions}
        */
       permissions: Object,
 
@@ -101,7 +101,8 @@ cr.define('extensions', function() {
           /** @type {chrome.developerPrivate.HostAccess} */ (select.value);
 
       if (access == chrome.developerPrivate.HostAccess.ON_SPECIFIC_SITES &&
-          !this.permissions.specificSiteControls) {
+          this.permissions.hostAccess !=
+              chrome.developerPrivate.HostAccess.ON_SPECIFIC_SITES) {
         // If the user is transitioning to the "on specific sites" option, show
         // the "add host" dialog. This serves two purposes:
         // - The user is prompted to add a host immediately, since otherwise
@@ -110,7 +111,7 @@ cr.define('extensions', function() {
         //   specific sites" is by checking if there are any specific sites.
         //   This ensures there will be at least one, so that the host access
         //   is properly calculated.
-        this.oldHostAccess_ = assert(this.permissions.hostAccess);
+        this.oldHostAccess_ = this.permissions.hostAccess;
         this.doShowHostDialog_(select, null);
       } else {
         this.delegate.setItemHostAccess(this.itemId, access);
@@ -122,8 +123,7 @@ cr.define('extensions', function() {
      * @private
      */
     showSpecificSites_: function() {
-      return this.permissions &&
-          this.permissions.hostAccess ==
+      return this.permissions.hostAccess ==
           chrome.developerPrivate.HostAccess.ON_SPECIFIC_SITES;
     },
 
@@ -133,14 +133,13 @@ cr.define('extensions', function() {
      * @private
      */
     getRuntimeHosts_: function() {
-      if (!this.permissions.specificSiteControls)
+      if (!this.permissions.hosts)
         return [];
 
       // Only show granted hosts in the list.
       // TODO(devlin): For extensions that request a finite set of hosts,
       // display them in a toggle list. https://crbug.com/891803.
-      return this.permissions.specificSiteControls.hosts
-          .filter(control => control.granted)
+      return this.permissions.hosts.filter(control => control.granted)
           .map(control => control.host)
           .sort();
     },

chrome/common/extensions/api/developer_private.idl

@@ -201,10 +201,13 @@ namespace developerPrivate {
     boolean granted;
   };
 
-  dictionary SpecificSiteControls {
+  dictionary RuntimeHostPermissions {
     // True if |hosts| contains an all hosts like pattern.
     boolean hasAllHosts;
 
+    // The current HostAccess setting for the extension.
+    HostAccess hostAccess;
+
     // The site controls for all granted and requested patterns.
     SiteControl[] hosts;
   };
@@ -213,12 +216,8 @@ namespace developerPrivate {
     Permission[] simplePermissions;
 
     // Only populated for extensions that can be affected by the runtime host
-    // permissions feature.The current host access.
-    HostAccess? hostAccess;
-
-    // Only populated for extensions affected by the runtime host
-    // permissions feature and |hostAccess| is equal to ON_SPECIFIC_SITES.
-    SpecificSiteControls? specificSiteControls;
+    // permissions feature.
+    RuntimeHostPermissions? runtimeHostPermissions;
   };
 
   dictionary ExtensionInfo {

chrome/test/data/webui/extensions/detail_view_test.js

@@ -167,9 +167,15 @@ cr.define('extension_detail_view_tests', function() {
 
       // Adding any runtime host permissions should result in the runtime host
       // controls becoming visible.
-      item.set(
-          'data.permissions.hostAccess',
-          chrome.developerPrivate.HostAccess.ON_CLICK);
+      const allSitesPermissions = {
+        simplePermissions: [],
+        runtimeHostPermissions: {
+          hosts: [{granted: false, host: '<all_urls>'}],
+          hasAllHosts: true,
+          hostAccess: chrome.developerPrivate.HostAccess.ON_CLICK,
+        },
+      };
+      item.set('data.permissions', allSitesPermissions);
       Polymer.dom.flush();
       expectTrue(testIsVisible('extensions-runtime-host-permissions'));
     });

chrome/test/data/webui/extensions/runtime_host_permissions_test.js

@@ -25,8 +25,9 @@ suite('RuntimeHostPermissions', function() {
 
   test('permissions display', function() {
     const permissions = {
-      simplePermissions: ['permission 1', 'permission 2'],
       hostAccess: HostAccess.ON_CLICK,
+      hasAllHosts: true,
+      hosts: [{granted: false, host: 'https://*/*'}],
     };
 
     element.set('permissions', permissions);
@@ -51,13 +52,10 @@ suite('RuntimeHostPermissions', function() {
     // Setting the mode to on specific sites should display the runtime hosts
     // list.
     element.set('permissions.hostAccess', HostAccess.ON_SPECIFIC_SITES);
-    element.set('permissions.specificSiteControls', {
-      hasAllHosts: false,
-      hosts: [
-        {host: 'https://example.com', granted: true},
-        {host: 'https://chromium.org', granted: true}
-      ],
-    });
+    element.set('permissions.hosts', [
+      {host: 'https://example.com', granted: true},
+      {host: 'https://chromium.org', granted: true}
+    ]);
     Polymer.dom.flush();
     expectEquals(HostAccess.ON_SPECIFIC_SITES, selectHostAccess.value);
     expectTrue(testIsVisible('#hosts'));
@@ -67,8 +65,9 @@ suite('RuntimeHostPermissions', function() {
 
   test('permissions selection', function() {
     const permissions = {
-      simplePermissions: ['permission 1', 'permission 2'],
       hostAccess: HostAccess.ON_CLICK,
+      hasAllHosts: true,
+      hosts: [{granted: false, host: 'https://*.com/*'}],
     };
 
     element.set('permissions', permissions);
@@ -101,6 +100,8 @@ suite('RuntimeHostPermissions', function() {
   test('on select sites cancel', function() {
     const permissions = {
       hostAccess: HostAccess.ON_CLICK,
+      hasAllHosts: true,
+      hosts: [{granted: false, host: 'https://*/*'}],
     };
 
     element.permissions = permissions;
@@ -131,8 +132,9 @@ suite('RuntimeHostPermissions', function() {
 
   test('on select sites accept', function() {
     const permissions = {
-      simplePermissions: ['permission 1', 'permission 2'],
       hostAccess: HostAccess.ON_CLICK,
+      hasAllHosts: true,
+      hosts: [{granted: false, host: 'https://*/*'}],
     };
 
     element.set('permissions', permissions);
@@ -169,15 +171,13 @@ suite('RuntimeHostPermissions', function() {
 
   test('clicking add host triggers dialog', function() {
     const permissions = {
-      simplePermissions: [],
       hostAccess: HostAccess.ON_SPECIFIC_SITES,
-      specificSiteControls: {
-        hasAllHosts: false,
-        hosts: [
-          {host: 'https://www.example.com/*', granted: true},
-          {host: 'https://*.google.com', granted: false}
-        ],
-      },
+      hasAllHosts: true,
+      hosts: [
+        {host: 'https://www.example.com/*', granted: true},
+        {host: 'https://*.google.com', granted: false},
+        {host: '*://*.com/*', granted: false},
+      ],
     };
 
     element.set('permissions', permissions);
@@ -198,15 +198,13 @@ suite('RuntimeHostPermissions', function() {
 
   test('removing runtime host permissions', function() {
     const permissions = {
-      simplePermissions: [],
       hostAccess: HostAccess.ON_SPECIFIC_SITES,
-      specificSiteControls: {
-        hasAllHosts: false,
-        hosts: [
-          {host: 'https://example.com', granted: true},
-          {host: 'https://chromium.org', granted: true}
-        ],
-      },
+      hasAllHosts: true,
+      hosts: [
+        {host: 'https://example.com', granted: true},
+        {host: 'https://chromium.org', granted: true},
+        {host: '*://*.com/*', granted: false},
+      ],
     };
     element.set('permissions', permissions);
     Polymer.dom.flush();
@@ -231,15 +229,13 @@ suite('RuntimeHostPermissions', function() {
 
   test('clicking edit host triggers dialog', function() {
     const permissions = {
-      simplePermissions: [],
       hostAccess: HostAccess.ON_SPECIFIC_SITES,
-      specificSiteControls: {
-        hasAllHosts: false,
-        hosts: [
-          {host: 'https://example.com', granted: true},
-          {host: 'https://chromium.org', granted: true}
-        ],
-      },
+      hasAllHosts: true,
+      hosts: [
+        {host: 'https://example.com', granted: true},
+        {host: 'https://chromium.org', granted: true},
+        {host: '*://*.com/*', granted: false},
+      ],
     };
     element.set('permissions', permissions);
     Polymer.dom.flush();

third_party/closure_compiler/externs/developer_private.js

@@ -279,16 +279,16 @@ chrome.developerPrivate.SiteControl;
 /**
  * @typedef {{
  *   hasAllHosts: boolean,
+ *   hostAccess: !chrome.developerPrivate.HostAccess,
  *   hosts: !Array<!chrome.developerPrivate.SiteControl>
  * }}
  */
-chrome.developerPrivate.SpecificSiteControls;
+chrome.developerPrivate.RuntimeHostPermissions;
 
 /**
  * @typedef {{
  *   simplePermissions: !Array<!chrome.developerPrivate.Permission>,
- *   hostAccess: (!chrome.developerPrivate.HostAccess|undefined),
- *   specificSiteControls: (!chrome.developerPrivate.SpecificSiteControls|undefined)
+ *   runtimeHostPermissions: (!chrome.developerPrivate.RuntimeHostPermissions|undefined)
  * }}
  */
 chrome.developerPrivate.Permissions;