Add module-level permissions to extensions.

Add module-level permissions to extensions.

This first pass is fairly simple. If a permission is not specified in the
manifest, the corresponding module will not be exposed to script. For example,
without specifying the "tabs" permission, chrome.tabs and chrome.windows will
not be available.

BUG=12140
TEST=no

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

[Matt Perry, 2009-08-06 01:32:53]

rafaelw: extension_process_bindings.js
aa: everything else

[rafaelw, 2009-08-06 15:11:37]

http://codereview.chromium.org/164039/diff/1/13
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/164039/diff/1/13#newcode155
Line 155: // TODO(rafaelw): Handle synchronous functions.
can you remove this TODO while you're here. this is actually done.

http://codereview.chromium.org/164039/diff/1/13#newcode156
Line 156: // TOOD(rafaelw): Consider providing some convenient override points
Likewise remove this TODO

http://codereview.chromium.org/164039/diff/1/13#newcode221
Line 221: // Page action events send (pageActionId, {tabId, tabUrl}).
So does this mean that pageAction events don't require permissions? or that if
there are any, the extension by definition already has permission?

[Aaron Boodman, 2009-08-06 19:25:05]

http://codereview.chromium.org/164039/diff/1/3
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/164039/diff/1/3#newcode392
Line 392: render_view_host->Send(new ViewMsg_Extension_SetPermissions(
We did used to create ExtensionFunctionDispatcher around this time. It was
moved, and this broke other things. See:

http://code.google.com/p/chromium/issues/detail?id=18240

We really need to fix that bug. It made page actions not work :(

http://codereview.chromium.org/164039/diff/1/5
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/164039/diff/1/5#newcode54
Line 54: // Given a permission string (like "tabs"), return the corresponding
enum value.
Wouldn't it be easier to just have all the permissions be strings? We could use
constant strings for the api permissions. I have a hard time believing a string
compare for every API call is really going to matter in the scheme of things.

http://codereview.chromium.org/164039/diff/1/6
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/164039/diff/1/6#newcode192
Line 192: // TODO(mpcomplete): fix naming.
I actually like the names "api permissions" vs "host permissions". Why don't we
go with that throughout?

http://codereview.chromium.org/164039/diff/1/13
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/164039/diff/1/13#newcode162
Line 162: var apiPermissions = GetPermissions(extensionId);
I'm really uncomfortable enforcing this in the renderer. We should move
enforcement to the browser. We already have a mechanism to return errors for
method calls that we can easily use. For events, we don't have one. Maybe we
should add one, or have it be a TODO or something.

http://codereview.chromium.org/164039/diff/1/13#newcode168
Line 168: if (apiPermissions[permissionFromNamespace(apiDef.namespace)] ==
false)
If you want to explicitly check for false you need three equals signs. It seems
like as is, this won't do what the comment says it does.

http://codereview.chromium.org/164039/diff/1/17
File chrome/test/data/extensions/uitest/roundtrip_api_call/manifest.json
(right):

http://codereview.chromium.org/164039/diff/1/17#newcode6
Line 6: "permissions": ["tabs"]
Can we have this be a non-breaking change for now? Something that we can easily
make complete with a few line change.

[Matt Perry, 2009-08-06 23:58:47]

http://codereview.chromium.org/164039/diff/1/3
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/164039/diff/1/3#newcode392
Line 392: render_view_host->Send(new ViewMsg_Extension_SetPermissions(
On 2009/08/06 19:25:05, Aaron Boodman wrote:
> We did used to create ExtensionFunctionDispatcher around this time. It was
> moved, and this broke other things. See:
> 
> http://code.google.com/p/chromium/issues/detail?id=18240
> 
> We really need to fix that bug. It made page actions not work :(

Would it be as simple as adding efd_.reset(new EFD) back here?  We'd reset again
when we got DidNavigate, but at least that would cause PageActions to get sent.

http://codereview.chromium.org/164039/diff/1/5
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/164039/diff/1/5#newcode54
Line 54: // Given a permission string (like "tabs"), return the corresponding
enum value.
On 2009/08/06 19:25:05, Aaron Boodman wrote:
> Wouldn't it be easier to just have all the permissions be strings? We could
use
> constant strings for the api permissions. I have a hard time believing a
string
> compare for every API call is really going to matter in the scheme of things.

Done.

http://codereview.chromium.org/164039/diff/1/6
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/164039/diff/1/6#newcode192
Line 192: // TODO(mpcomplete): fix naming.
On 2009/08/06 19:25:05, Aaron Boodman wrote:
> I actually like the names "api permissions" vs "host permissions". Why don't
we
> go with that throughout?

Done.

http://codereview.chromium.org/164039/diff/1/13
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/164039/diff/1/13#newcode155
Line 155: // TODO(rafaelw): Handle synchronous functions.
On 2009/08/06 15:11:37, rafaelw wrote:
> can you remove this TODO while you're here. this is actually done.

Done.

http://codereview.chromium.org/164039/diff/1/13#newcode156
Line 156: // TOOD(rafaelw): Consider providing some convenient override points
On 2009/08/06 15:11:37, rafaelw wrote:
> Likewise remove this TODO

Done.

http://codereview.chromium.org/164039/diff/1/13#newcode162
Line 162: var apiPermissions = GetPermissions(extensionId);
On 2009/08/06 19:25:05, Aaron Boodman wrote:
> I'm really uncomfortable enforcing this in the renderer. We should move
> enforcement to the browser. We already have a mechanism to return errors for
> method calls that we can easily use. For events, we don't have one. Maybe we
> should add one, or have it be a TODO or something.

Are you worried about a compromised extension process? I'm not sure enforcement
in the browser buys us anything security-wise.

We have to do enforcement in the renderer anyway, because of the case where
multiple extensions share a process.

http://codereview.chromium.org/164039/diff/1/13#newcode168
Line 168: if (apiPermissions[permissionFromNamespace(apiDef.namespace)] ==
false)
On 2009/08/06 19:25:05, Aaron Boodman wrote:
> If you want to explicitly check for false you need three equals signs. It
seems
> like as is, this won't do what the comment says it does. 

My goal is to distinguish between true, false, and undefined. This seems to do
what I want in that regard, but so does ===, so I'll use that.

http://codereview.chromium.org/164039/diff/1/13#newcode221
Line 221: // Page action events send (pageActionId, {tabId, tabUrl}).
On 2009/08/06 15:11:37, rafaelw wrote:
> So does this mean that pageAction events don't require permissions? or that if
> there are any, the extension by definition already has permission?

I've only added tabs and bookmarks as permissions for now.

http://codereview.chromium.org/164039/diff/1/17
File chrome/test/data/extensions/uitest/roundtrip_api_call/manifest.json
(right):

http://codereview.chromium.org/164039/diff/1/17#newcode6
Line 6: "permissions": ["tabs"]
On 2009/08/06 19:25:05, Aaron Boodman wrote:
> Can we have this be a non-breaking change for now? Something that we can
easily
> make complete with a few line change.

Done.

[Aaron Boodman, 2009-08-07 00:31:21]

lgtm, with nits. neat

http://codereview.chromium.org/164039/diff/1023/1025
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/164039/diff/1023/1025#newcode392
Line 392: render_view_host->Send(new ViewMsg_Extension_SetPermissions(
I guess creating an EFD here is a good solution. Can you do that and add a TODO
noting the duplication and to look for a better place to hook in.

http://codereview.chromium.org/164039/diff/1023/1033
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/164039/diff/1023/1033#newcode165
Line 165: std::string extension_id =
*v8::String::Utf8Value(args[0]->ToString());
Why did you make this change?

http://codereview.chromium.org/164039/diff/1023/1033#newcode205
Line 205: if (!ExtensionProcessBindings::CurrentContextHasPermission(name)) {
braces not needed

http://codereview.chromium.org/164039/diff/1023/1033#newcode318
Line 318: #if EXTENSION_TIME_TO_BREAK_API
nice

[Matt Perry, 2009-08-07 00:51:45]

all done

http://codereview.chromium.org/164039/diff/1023/1033
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/164039/diff/1023/1033#newcode165
Line 165: std::string extension_id =
*v8::String::Utf8Value(args[0]->ToString());
On 2009/08/07 00:31:21, Aaron Boodman wrote:
> Why did you make this change?

Thanks for bringing this up - it requires a JS change that I accidentally undid.

ExtensionIdFromCurrentContext might not work during chromeHidden.onLoad
dispatch, which is when this is called.  This is because the page load may not
have been committed, so the URL we check might be for the previous page. So
instead I'm plumbing the extension_id through from earlier in the code where we
get it reliably.

http://codereview.chromium.org/164039/diff/1023/1033#newcode205
Line 205: if (!ExtensionProcessBindings::CurrentContextHasPermission(name)) {
On 2009/08/07 00:31:21, Aaron Boodman wrote:
> braces not needed

Done.