Support "always allow" for runtime script execution

chrome/browser/extensions/active_script_controller.cc

Index: chrome/browser/extensions/active_script_controller.cc
diff --git a/chrome/browser/extensions/active_script_controller.cc b/chrome/browser/extensions/active_script_controller.cc
index 1b35b2872c9415706f9bd7585315856576cdc57b..e79e1a551779d72a52936c3dfc0843e9052306ad 100644
--- a/chrome/browser/extensions/active_script_controller.cc
+++ b/chrome/browser/extensions/active_script_controller.cc
@@ -13,19 +13,24 @@
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/extensions/location_bar_controller.h"
+#include "chrome/browser/extensions/permissions_updater.h"
 #include "chrome/browser/extensions/tab_helper.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
+#include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/extension_set.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest.h"
+#include "extensions/common/manifest_handlers/permissions_parser.h"
+#include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "ipc/ipc_message_macros.h"
 
@@ -100,6 +105,55 @@ void ActiveScriptController::OnAdInjectionDetected(
       ad_injectors.size() - num_preventable_ad_injectors);
 }
 
+void ActiveScriptController::AddPersistedPermission(
+    const Extension* extension) {
+  // Allow current tab to run injection.
+  OnClicked(extension);

[not at google - send to devlin, 2014/08/01 18:15:12]

it might be safer to do this at the end in case some logic turns around and
expects the new host permissions are in place?

[gpdavis, 2014/08/01 20:06:06]

Sure thing.

+
+  GURL url = web_contents()->GetVisibleURL();
+  URLPattern pattern(extensions::UserScript::ValidUserScriptSchemes());
+  pattern.SetScheme(url.scheme());
+  pattern.SetHost(url.host());
+  pattern.SetPath("/*");
+
+  extensions::URLPatternSet new_explicit_hosts;
+  extensions::URLPatternSet new_scriptable_hosts;
+
+  scoped_refptr<const PermissionSet> permissions(
+      PermissionsParser::GetRequiredPermissions(extension));

[not at google - send to devlin, 2014/08/01 18:15:11]

shouldn't this be querying the withheld permissions? there's no point granting
an extension permissions it already has.

scoped_refptr<const PermissionSet> withheld_permissions =
    extension->permissions_data()->withheld_permissions();

(note that withheld is a subset of required at the moment, so your code *works*,
but they won't always be.)

[gpdavis, 2014/08/01 20:06:06]

That seems reasonable.  I chose RequiredPermissions because I was under the
impression that this corresponded to the manifest permissions that don't change:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

I was essentially trying to find out if the host matches anything in either the
"permissions" key, or the "content_scripts" key (which is populated via
SetScriptableHosts before Finalize is called).  I suppose it does make more
sense to just check the withheld permissions considering that's how we got into
this section of code in the first place.  I'll do that.

+  if (permissions->explicit_hosts().MatchesURL(url))
+    new_explicit_hosts.AddPattern(pattern);
+  if (permissions->scriptable_hosts().MatchesURL(url))
+    new_scriptable_hosts.AddPattern(pattern);
+
+  scoped_refptr<extensions::PermissionSet> new_permissions =
+      new extensions::PermissionSet(extensions::APIPermissionSet(),
+                                    extensions::ManifestPermissionSet(),
+                                    new_explicit_hosts,
+                                    new_scriptable_hosts);
+
+  // Update active permissions for the session.
+  extensions::PermissionsUpdater updater(
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext()));
+  updater.AddPermissions(extension, new_permissions.get());
+

[not at google - send to devlin, 2014/08/01 18:15:11]

we should add UMA for this. I think a useful metric would be how many pages the
user has persisted permissions for (after this method executes of course), which
you can approximate using the size of the effective permission set:

UMA_HISTOGRAM_COUNTS_100(
    "Extensions.ActiveScriptController.PersistedPermissionCount",
    extensions()->permissions_data()->GetEffectiveHostPermissions().size());

[gpdavis, 2014/08/01 20:06:06]

Okay, sure.  Should I add both you and devlin as owners in histograms.xml?

[not at google - send to devlin, 2014/08/04 23:53:13]

no, histograms.xml has its own owners. git-cl upload should suggest a reviewer
for that.

[gpdavis, 2014/08/07 20:50:39]

Oh, no, I meant for the <owner></owner> tags in the file itself (see the
histograms.xml diff in the next patch set).

[not at google - send to devlin, 2014/08/08 14:31:28]

I thought I replied to this question. yes, <owner> tag.

[gpdavis, 2014/08/08 18:07:34]

I added this comment right before I added the one you responded to :)

+  // Update persisted permissions for extension.

[not at google - send to devlin, 2014/08/01 18:15:12]

PermissionsUpdater should already do this?

[gpdavis, 2014/08/01 20:06:06]

Update Persisted Permissions?

The updater adds to active permissions so that the extension has permission to
inject scripts right away.  Then we add to persisted permissions, which are
loaded into active permissions when we InitializePermissions at the start of a
new session:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Without the first update (active), we grant active tab permissions, but the
extension requests permission to inject on the same host in a different tab. 
It's not until the browser is restarted that the extension is always allowed to
inject on that host.  Without the second update (persisted), the extension needs
permission again for that host when the browser is restarted.

Hopefully this explanation makes sense without being too verbose, but I'd be
more than happy to clarify further.

[not at google - send to devlin, 2014/08/04 23:53:13]

I mean PermissionsUpdater::AddPermissions:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

I don't quite follow your explanation, sorry. It looks to me like in the code
above calling AddPermissions does persist the permissions:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext....

Perhaps answer this: how is this different from granting optional permissions?

(it looks like you might have to take into account withheld permissions in
GetBoundedActivePermissions:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...)

[gpdavis, 2014/08/07 20:50:39]

Ahh, I see what you're saying.  AddPermissions already adds to
granted_permissions.  But the problem is that this isn't consulted when the
permissions are initialized:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

So these aren't really persisted, because they're not loaded into active
permissions at the start of a new session.  But I understand what you're saying
now.  I'll upload a patch set with a solution that doesn't involve persisted
permissions, so refer to my comment in the new patch set and we'll see if it's
reasonable.

[not at google - send to devlin, 2014/08/07 22:03:45]

It looks like GetBoundedActivePermissions reads in the active permissions from
prefs. That's this:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

and given this "always run" should be saving to the active permissions in prefs,
it should be loaded.

am I understanding you correctly?

if this isn't working something is going wrong with this required vs withheld
permission stuff. check to see what the active permissions end up in prefs.
given an extension with

permissions: http://*/*
optional_permissions: https://*/*

Preferences should show

permissions: http://*/*
optional_permissions: https://*/*
active_permissions: []

because they got withheld.

then you should be able to go to http://example.com, see an always-run button,
click it, then see in Preferenes

permissions: http://*/*
optional_permissions: https://*/*
active_permissions: ["http://example.com/*"]

then reload and see it agian.

[gpdavis, 2014/08/07 23:23:36]

I believe this is the line that's causing problems:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

The host does indeed show up in the active permissions in preferences.  However,
I believe they're getting filtered out because GetBoundedActivePermissions
creates this intersection; http://example.com doesn't end up in the Bounded
Permissions because it's not a part of the manifest default or optional
permissions.

"//  a) active permissions must be a subset of optional + default permissions"

So what I was doing was going through and explicitly taking values from granted
permissions and passing them to SetPermissions as active permissions.

Does this make more sense?

If I hit the always-run button for a host and trace it, it ends up in the
active_permissions of Preferences, but after GetBoundedActivePermissions is
called in InitializePermissions
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...),
it no longer exists, and I believe the above reason is why; it gets shaved off
because it's not a part of the extension's required or optional permissions.

[not at google - send to devlin, 2014/08/08 00:24:29]

ah I see. so that condition needs to take into account withheld permissions as
well?

[gpdavis, 2014/08/08 00:51:43]

You mean as a part of the bound, meaning that active permissions are a part of
the bounded active permissions if they're a subset of required, optional, and
withheld?

The problem is that if we have a withheld permission that implies all hosts,
we're gonna get a script injection request for a particular site
(http://example.com).  Upon granting, http://example.com will be added to active
permissions.  Then this won't work, because the specific host won't be part of
the subset, as http://example.com isn't found in the withheld permissions; the
permission that implies all hosts is.

This is why I made the separate method to add non-all-host permissions from
granted to active.

[not at google - send to devlin, 2014/08/08 14:31:28]

ah I see the confusion here, on both our parts. in most cases host permissions
are aware that http://example.com is within <all_urls>. If you were to do
(taking {...} as URLPatternSet notation):

{http://example.com/*, https://google.com/*} U {http://*/*}

you *should* get

{http://*/*, https://google.com/*}

not

{http://example.com/*, https://google.com/*, http://*/*}

likewise intersection would be

{http://example.com/*} not {}

and this *should* be carried through to the permission set level. but it doesn't
look like it is:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
-->
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

i.e. it uses STLSetIntersection not URLPatternSet intersection.

now I *thought* it had been fixed, and my memory was almost correct, except that
it didn't actually get committed:

https://codereview.chromium.org/181043006/

so... hmm! that's problematic. I'll follow up on that today.

[gpdavis, 2014/08/08 18:07:34]

Ah!  That does make a lot of sense.  I wonder why that patch never went
through...

So I guess with that fixed, this will be simple.

+  extensions::ExtensionPrefs* prefs =
+      extensions::ExtensionPrefs::Get(web_contents()->GetBrowserContext());
+  prefs->ClearPersistedPermissions(extension->id());
+  prefs->AddPersistedPermission(extension->id(), new_permissions.get());
+}
+
+bool ActiveScriptController::HasActiveScriptAction(
+    const Extension* extension) {
+  if (!enabled_ || pending_requests_.count(extension->id()) == 0)

[not at google - send to devlin, 2014/08/01 18:15:12]

why this second check? again while it seems like checking it won't be a bug, but
the |active_script_actions_| check below is a more idiomatic check.

this whole method can be

return enabled_ && active_script_actions_.count(extension->id()) > 0;

[gpdavis, 2014/08/01 20:06:06]

Ah, I see.  That makes sense.  I guess I was going for a preliminary check with
this line, but I think I misinterpreted it, because it does make a lot more
sense to just check active_script_actions_.

+    return false;  // No action for this extension.
+
+  ActiveScriptMap::iterator existing =
+      active_script_actions_.find(extension->id());
+  return existing != active_script_actions_.end();
+}
+
 ExtensionAction* ActiveScriptController::GetActionForExtension(
     const Extension* extension) {
   if (!enabled_ || pending_requests_.count(extension->id()) == 0)