Make ActiveTabPermissionManager also grant the tabs permission.

Make ActiveTabPermissionManager also grant the tabs permission for
that tab; only respected for tabs.executeScript and tabs.insertCSS.

This involved converting tabs.executeScript/insertCSS to use JSON
schema compiler.


BUG=137643
TBR=ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149631

[not at google - send to devlin, 2012-07-25 07:03:46]

https://chromiumcodereview.appspot.com/10815028/diff/6001/chrome/browser/exte...
File chrome/browser/extensions/extension_function.h (right):

https://chromiumcodereview.appspot.com/10815028/diff/6001/chrome/browser/exte...
chrome/browser/extensions/extension_function.h:47: #ifdef NDEBUG
Pretty please I want to make this change. It's really annoying when chrome
crashes when it tries to send the response in debug mode, rather than crashing
at the moment it should have failed.

https://chromiumcodereview.appspot.com/10815028/diff/6001/chrome/common/exten...
File chrome/common/extensions/extension_unittest.cc (right):

https://chromiumcodereview.appspot.com/10815028/diff/6001/chrome/common/exten...
chrome/common/extensions/extension_unittest.cc:877:
EXPECT_FALSE(extension->GetTabSpecificPermissions(0).get());
I just did enough throughout here to make the tests pass, so that I can run a
tryjob and head home for the day, happy.

I need to test the Update stuff properly, and the new HasAPIPermissionForTab
test, and possibly more. We shall see.

But this patch is big enough such that I think it can do with a round of
reviews.

[not at google - send to devlin, 2012-07-25 08:24:33]

Looks like it's failing some of the browser tests. Possibly premature to have
sent this out for review.

[not at google - send to devlin, 2012-07-31 08:57:11]

Hey Aaron, PTAL. This is ready now.

[Aaron Boodman, 2012-07-31 14:53:43]

http://codereview.chromium.org/10815028/diff/17016/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/10815028/diff/17016/chrome/browser/extensions/...
chrome/browser/extensions/extension_function.h:86: // Returns true if the
function permission to run.
^has

http://codereview.chromium.org/10815028/diff/17016/chrome/browser/extensions/...
chrome/browser/extensions/extension_function.h:93: virtual bool HasPermission();
Neat.

http://codereview.chromium.org/10815028/diff/17016/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/10815028/diff/17016/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_dispatcher.cc:220: if
(!function->HasPermission()) {
Bummer about the duplication. Why can't this go at the end of
CreateExtensionFunction()?

http://codereview.chromium.org/10815028/diff/17016/chrome/renderer/extensions...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10815028/diff/17016/chrome/renderer/extensions...
chrome/renderer/extensions/extension_dispatcher.cc:1071: bool
skip_permission_check = (function_name == "tabs.executeScript") ||
Wait, then why are we sending the active tab permissions into the renderers in
the first place?

[not at google - send to devlin, 2012-07-31 15:09:05]

working on the background page thing before I upload the next patch set.

http://codereview.chromium.org/10815028/diff/17016/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function.h (right):

http://codereview.chromium.org/10815028/diff/17016/chrome/browser/extensions/...
chrome/browser/extensions/extension_function.h:86: // Returns true if the
function permission to run.
On 2012/07/31 14:53:43, Aaron Boodman wrote:
> ^has

Done.

http://codereview.chromium.org/10815028/diff/17016/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/10815028/diff/17016/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_dispatcher.cc:220: if
(!function->HasPermission()) {
On 2012/07/31 14:53:43, Aaron Boodman wrote:
> Bummer about the duplication. Why can't this go at the end of
> CreateExtensionFunction()?

Because of the extra initialization that this needs to do after calling
CreateExtensionFunction (ditto the other time that it's done). Most notably, the
profile.

http://codereview.chromium.org/10815028/diff/17016/chrome/renderer/extensions...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10815028/diff/17016/chrome/renderer/extensions...
chrome/renderer/extensions/extension_dispatcher.cc:1071: bool
skip_permission_check = (function_name == "tabs.executeScript") ||
On 2012/07/31 14:53:43, Aaron Boodman wrote:
> Wait, then why are we sending the active tab permissions into the renderers in
> the first place?

The renderer does a check in UserScriptScheduler against the host permissions,
as the last thing that checks permissions before injecting the script (the
browser also does, but acknowledges race conditions with the page changing in
the 

That involves sending the active tab permissions to the renderer that has the
permissions (nothing is sent to the background page).

But for this to work (avoiding whitelisting these functions), we'd need to send
the active tab stuff for every renderer to the background page. Come to think of
it, that's not so unreasonable. More IPC code but whatever.

[not at google - send to devlin, 2012-08-01 13:57:04]

http://codereview.chromium.org/10815028/diff/17016/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/10815028/diff/17016/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_dispatcher.cc:220: if
(!function->HasPermission()) {
On 2012/07/31 15:09:05, kalman wrote:
> On 2012/07/31 14:53:43, Aaron Boodman wrote:
> > Bummer about the duplication. Why can't this go at the end of
> > CreateExtensionFunction()?
> 
> Because of the extra initialization that this needs to do after calling
> CreateExtensionFunction (ditto the other time that it's done). Most notably,
the
> profile. 

Turns out I was wrong about this. Done.

http://codereview.chromium.org/10815028/diff/17016/chrome/renderer/extensions...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10815028/diff/17016/chrome/renderer/extensions...
chrome/renderer/extensions/extension_dispatcher.cc:1071: bool
skip_permission_check = (function_name == "tabs.executeScript") ||
On 2012/07/31 15:09:05, kalman wrote:
> On 2012/07/31 14:53:43, Aaron Boodman wrote:
> > Wait, then why are we sending the active tab permissions into the renderers
in
> > the first place?
> 
> The renderer does a check in UserScriptScheduler against the host permissions,
> as the last thing that checks permissions before injecting the script (the
> browser also does, but acknowledges race conditions with the page changing in
> the 
> 
> That involves sending the active tab permissions to the renderer that has the
> permissions (nothing is sent to the background page).
> 
> But for this to work (avoiding whitelisting these functions), we'd need to
send
> the active tab stuff for every renderer to the background page. Come to think
of
> it, that's not so unreasonable. More IPC code but whatever.

I remember again why I did this. Oops. Comment updated.

[not at google - send to devlin, 2012-08-02 12:23:47]

Added an API test, PTAL.

Had to mess with ExtensionFunctionDispatcher a bit more, since the API test
found a bug. Heh.

https://chromiumcodereview.appspot.com/10815028/diff/18017/chrome/browser/ext...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10815028/diff/18017/chrome/browser/ext...
chrome/browser/extensions/extension_function_dispatcher.cc:283:
function_ui->SetRenderViewHost(render_view_host);
I needed to make this change because the render_view_host is needed to call
GetCurrentBrowser().

It was either thread through that redundant argument, or do the permissions
checking outside of CreateExtensionFunction.

[Aaron Boodman, 2012-08-02 12:55:04]

LGTM

[commit-bot: I haz the power, 2012-08-02 12:58:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10815028/18017

[commit-bot: I haz the power, 2012-08-02 12:58:20]

Presubmit check for 10815028-18017 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.9s to calculate.

[commit-bot: I haz the power, 2012-08-02 14:03:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10815028/18017

[commit-bot: I haz the power, 2012-08-02 14:05:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10815028/18020

[commit-bot: I haz the power, 2012-08-02 15:32:18]

Change committed as 149631