Add an experimental permissions API for extensions.

Re-land the experimental permissions API for extensions.

The permissions API lets extensions specify optional permissions in their manifest that they can request at run-time. It currently supports API permissions through a white-list. Host permissions will come later. This also fixes some clang errors from the previous attempt.

BUG=48119, 70466, 84507
TEST=*Extension*

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

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

[jstritar, 2011-07-19 17:07:27]

Hi Mihai, 

I'm thinking of making this experimental before landing it (though its not
experimental right now). I'm also planning on adding a few more tests in the
next day or two, but I think it's ready for an initial review at least.

Thanks
--Jon

[jstritar, 2011-07-19 17:10:53]

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
chrome/common/extensions/extension.h:705: mutable RuntimeData runtime_data_;
I kept this in Extension to simplify these changes, since there are lots of
calls to the Extension methods that access permissions. We could pull this out
of the Extension in a follow up CL if necessary though.

http://codereview.chromium.org/7432006/diff/2002/chrome/renderer/resources/re...
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/renderer/resources/re...
chrome/renderer/resources/renderer_extension_bindings.js:332: "permissions",
I plan on making chrome.permissions accessible by web apps, but it isn't right
now.

[jstritar, 2011-07-19 17:15:13]

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:1151: "description": "Requests
access to the specified permissions.",
Mention the optional_permissions field of the manifest here and in the manifest
section of documentation.

[jstritar, 2011-07-20 19:41:25]

+atwilson to review the background_mode_manager changes

[Mihai Parparita -not on Chrome, 2011-07-20 22:03:42]

Here's what I have so far (have about 1/3 of the files left to go, but I'm
heading out for the day).

http://codereview.chromium.org/7432006/diff/2002/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:3667: <message
name="IDS_EXTENSION_PERMISSIONS_PROMPT_TITLE" desc="Titlebar of the extenxion or
app permissions prompt window">
Typo (extenxion).

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_permissions_api.cc (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:54: bool
UnpackPermissionSet(DictionaryValue* value,
Seems like CreatePermissionsValue and UnpackPermissionSet should have
symmetrical names, since they do ExtensionPermissionSet <-> Value conversion.
PackPermissionsToValue and UnpackPermissionsFromValue?

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:111: if
(extension->location() == Extension::INTERNAL)
Not sure I understand the location check. Don't we want to update the granted
permissions for all extension types?

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:137: if (!permissions ||
permissions->IsEmpty())
Any reason why this check is here, and not earlier in NotifyPermissionsUpdated
(presumably you don't want to send notifications on empty changes either)? Also,
the argument name should probably be changed_permissions, to make it clearer
that it's just a delta.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:315: // of the granted
permissions set.
Would it make sense to filter out the requested permissions if some are already
granted, so that the prompt only has newly requested ones?

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_permissions_api.h (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.h:30: // and sends the
relevant messages and notifications.
Add a comment that it's assumed the user has already been prompted (if
necessary) for the extra permissions?

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.h:41: const
ExtensionPermissionSet& white_list() const { return *white_list_; }
"whitelist" (no dash/underscore) seems to be the preferred spelling in Chromium.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.h:76: virtual bool
RunImpl();
Add OVERRIDE to all the RunImpls.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.h:98: // FOR TESTS ONLY to
by pass the confirmation UI.
"bypass" is one word.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_permissions_api_constants.cc (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api_constants.cc:9: const char
kApisKey[] = "apis";
Not sure "apis" is the best name, since it's not always a 1:1 mapping between
APIs and permissions (e.g. chrome.tabs and chrome.windows are both controlled by
the "tabs" permission). Do you think it'd be too confusing to just call it
"permissions"? (even if you later add "origins" as a separate field?)

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:749: ExtensionPermissionSet*
ExtensionPrefs::GetActivePermissions(
This seems pretty similar to the granted permission get/set methods. Might be
worth it to have some shared utility functions for converting to/from Value.

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:1155: "type": "object",
Shouldn't this be of type Permissions?

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/permissions/optional/background.html
(right):

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional/background.html:24:
var empty_permissions = {apis: []};
I think we generally use the JS naming scheme in tests (this should be
emptyPermissions)

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional/background.html:60: },
pass(function(result) {
Seems like this should be intended more, since it's one of the arguments to
chrome.permissions.contains.

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional/background.html:94:
assertTrue(false);
Something like chrome.test.fail("Should not have tabs API permission") seems
clearer.

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional/background.html:140:
assertTrue(false);
Ditto about using chrome.test.fail("Should not have tabs API permission").

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/permissions/optional/manifest.json
(right):

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional/manifest.json:4:
"key":
"MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDkprt3BRSqoikAhSygI6VUzDLt18cKODYmkaa/dwPp4dboyz93RSB+v76grbqsNWrJjkrEwRD3QFeBYBq7h27XAMV4X5XvWjmWQBkRTBQyQI8cZd7M9dgfKrI3EqX9OJvd/wTJkC0dgF47nwWRa/Tvwl7Y66GwEEUjpn2MTv4klwIDAQAB",
Is the key necessary?

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/permissions/optional_deny/background.html
(right):

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional_deny/background.html:24:
assertTrue(false);
Ditto about using chrome.test.fail("Should not have tabs API permission").

[jstritar, 2011-07-21 15:43:12]

On 2011/07/20 22:03:42, Mihai Parparita wrote:
>
http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_permissions_api_constants.cc:9: const char
> kApisKey[] = "apis";
> Not sure "apis" is the best name, since it's not always a 1:1 mapping between
> APIs and permissions (e.g. chrome.tabs and chrome.windows are both controlled
by
> the "tabs" permission). Do you think it'd be too confusing to just call it
> "permissions"? (even if you later add "origins" as a separate field?)
> 

Hm... good point, and the intent is for these to be used more generally in the
future, so the manifest no longer has implicit permissions (like the "plugins"
or "devtools_url" manifest keys). Then the extension's permissions boil down to
these named permissions, origins, and content script origins rather than being a
function of anything in the manifest.

I was originally thinking of them as "named permissions" but that'd be confusing
too: {names: ['tabs', 'management']}. I think 'permissions' may work best even
though it'll look odd before we add the origins section.

{'permissions': ['tabs', 'management']}
{'permissions': ['tabs'], 'origins': ['http://*.google.com/*']}

What do you think about content scripts? In terms of naming, maybe something
like 'user_script_origins'? Do you think it should enable all the content
scripts for the given origin (they'd first be marked optional)?

{ 'user_script_origins': ['http://*.google.com/*'] }

This wouldn't provide that much granularity, but it would be in line with how we
calculate permissions. They could implement more fine grained controls
themselves inside of the content script.

[Mihai Parparita -not on Chrome, 2011-07-21 17:41:52]

On Thu, Jul 21, 2011 at 11:43 AM,  <jstritar@chromium.org> wrote:
> too: {names: ['tabs', 'management']}. I think 'permissions' may work best
> even
> though it'll look odd before we add the origins section.
>
> {'permissions': ['tabs', 'management']}
> {'permissions': ['tabs'], 'origins': ['http://*.google.com/*']}

Yeah, I think that's fine. Generally, I think it's preferable to mimic
the manifest structure as much as possible. I see that that the bug
for splitting up permissions and origins in the manifest
(http://crbug.com/62898) doesn't have a naming proposal, but
"permissions" and "origins" seems reasonable to me.

> What do you think about content scripts? In terms of naming, maybe something
> like 'user_script_origins'? Do you think it should enable all the content
> scripts for the given origin (they'd first be marked optional)?
>
> { 'user_script_origins': ['http://*.google.com/*'] }
>
> This wouldn't provide that much granularity, but it would be in line with
> how we calculate permissions. They could implement more fine grained
> controls themselves inside of the content script.

Along the same lines as above (being consistent with the manifest),
what if we instead had the full content_scripts section from the
manifest. I.e. if an extension could optionally modify Gmail, its
manifest would look like:

{
  ..
  "optional_permissions": ["contentScripts"],
  // no content_scripts permission by default
}

And then the call would look like:

chrome.permissions.request({content_scripts: [{
  "matches": ["https://mail.google.com/*"],
  "css": ["mystyles.css"],
  "js": ["jquery.js", "myscript.js"]
}]}

Or do you think it's not worth losing the declarative mode (in the
manifest) for knowing which origins are affected by optional user
scripts?

Also, how did you envision optional origin acces working? Would the
optional_permissions entry just say "origins" and the extension could
request access for any origin, or would the extension have to specify
upfront which origins it wants optional access for? The former would
allow more flexibility, so that instead of listing and requesting
"<all_urls>" for an extension that could conceivably affect every page
(but in practice would only affect a few user-chosen ones) it could
just get access to a user-chosen subset.

Mihai

[jstritar, 2011-07-21 18:29:01]

On Thu, Jul 21, 2011 at 1:41 PM, Mihai Parparita <mihaip@chromium.org>wrote:

> On Thu, Jul 21, 2011 at 11:43 AM,  <jstritar@chromium.org> wrote:
> > too: {names: ['tabs', 'management']}. I think 'permissions' may work best
> > even
> > though it'll look odd before we add the origins section.
> >
> > {'permissions': ['tabs', 'management']}
> > {'permissions': ['tabs'], 'origins': ['http://*.google.com/*']}
>
> Yeah, I think that's fine. Generally, I think it's preferable to mimic
> the manifest structure as much as possible. I see that that the bug
> for splitting up permissions and origins in the manifest
> (http://crbug.com/62898) doesn't have a naming proposal, but
> "permissions" and "origins" seems reasonable to me.
>
> > What do you think about content scripts? In terms of naming, maybe
> something
> > like 'user_script_origins'? Do you think it should enable all the content
> > scripts for the given origin (they'd first be marked optional)?
> >
> > { 'user_script_origins': ['http://*.google.com/*'] }
> >
> > This wouldn't provide that much granularity, but it would be in line with
> > how we calculate permissions. They could implement more fine grained
> > controls themselves inside of the content script.
>
> Along the same lines as above (being consistent with the manifest),
> what if we instead had the full content_scripts section from the
> manifest. I.e. if an extension could optionally modify Gmail, its
> manifest would look like:
>
> {
>  ..
>  "optional_permissions": ["contentScripts"],
>  // no content_scripts permission by default
> }
>
> And then the call would look like:
>
> chrome.permissions.request({content_scripts: [{
>  "matches": ["https://mail.google.com/*"],
>  "css": ["mystyles.css"],
>  "js": ["jquery.js", "myscript.js"]
> }]}
>
> Or do you think it's not worth losing the declarative mode (in the
> manifest) for knowing which origins are affected by optional user
> scripts?
>
> Also, how did you envision optional origin acces working? Would the
> optional_permissions entry just say "origins" and the extension could
> request access for any origin, or would the extension have to specify
> upfront which origins it wants optional access for? The former would
> allow more flexibility, so that instead of listing and requesting
> "<all_urls>" for an extension that could conceivably affect every page
> (but in practice would only affect a few user-chosen ones) it could
> just get access to a user-chosen subset.
>
> Mihai
>

We were thinking something along the lines of this for origins:

"optional_permissions": ["tabs", "management", 'https://someorigin.com"]
"permissions": ["permissions"]

chrome.permissions.request({origins: ['https://someorigin.com']}, ...}

We want to declare optional permissions up-front in the manifest to put a
limit on what an extension can request (e.g. in case its compromised). It
would be nice if you could enable origins that weren't specified upfront in
the manifest (or were subsets of those in the manifest) since that fits more
use cases, but we decided to start with this because there are separate
efforts to let users disable scripts on certain sites. We should be able to
migrate towards that in the future though.

Part of the confusion related to content scripts is that this is an API for
granting permissions, but permissions don't line up exactly with content
scripts. With respect to permissions, an extension that runs 5 content
scripts on https://mail.google.com/ is the same as an extension that runs 1
content script, or even 1 that has the https://mail.google.com/ host
permission and no content scripts. The extension could silently upgrade to a
new version that switches between these.

We may need to separate permissions from these specific features.
Permissions could be strictly (a) named permissions and (b) origins. You
need to have the origin permission before injecting a content script:

chrome.permissions.request({origin: ["https://mail.google.com/"]},
function(granted) {
  if (granted) {
    chrome.contentScripts.enable({
      "matches": ["https://mail.google.com/*"],
      "css": ["mystyles.css"],
      "js": ["jquery.js", "myscript.js"]
    });
  }
});

(chrome.contentScripts.enable could even call the permissions request method
internally)

Specifying the content script directly in the manifest would implicitly
grant the origin permission. We could do the same thing with plugins:

chrome.permissions.request({permission: ["plugin"]}, function(granted) {
 if (granted)
   chrome.plugins.start("content_plugin.dll");
});

This might be a slightly cleaner approach.

--Jon

[Mihai Parparita -not on Chrome, 2011-07-21 20:39:42]

On Thu, Jul 21, 2011 at 2:28 PM, Jon Stritar <jstritar@chromium.org> wrote:
> This might be a slightly cleaner approach.

Yeah, this all makes sense.

Mihai

[Mihai Parparita -not on Chrome, 2011-07-21 21:18:13]

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_permissions_api.cc (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:102: const
ExtensionPermissionSet* existing = extension->GetActivePermissions();
I the type of existing should be scoped_refptr<ExtensionPermissionSet>.
Otherwise its ref count is not actually increased while you're using it, so it
could go away if the active permission set changes while this function is
running.

This probably applies to other places in the CL where you call
GetActivePermissions().

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2506: LOG(INFO) << "BLAH1: " << key;
Did you want to keep this logging statement?

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2515: LOG(INFO) << "BLAH2: " << key;
Ditto.

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
chrome/common/extensions/extension.h:567: class RuntimeData : public
base::RefCountedThreadSafe<RuntimeData> {
I don't think making this ref counted buys you anything, since you never hand
out instances of it to anyone else, it seems like the lifetime of runtime_data_
is 1:1 with the lifetime of the Extension instance that it's in.

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
File chrome/common/extensions/extension_messages.cc (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
chrome/common/extensions/extension_messages.cc:141: WriteParam(m, (int)p);
static_cast<int>(p) is the casting style preferred by the C++ style guide
(applies below too).

[jstritar, 2011-07-22 19:21:55]

Back to you Mihai. It'd be great if you have time to review it before Monday,
but don't worry about it if you can't.

http://codereview.chromium.org/7432006/diff/2002/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:3667: <message
name="IDS_EXTENSION_PERMISSIONS_PROMPT_TITLE" desc="Titlebar of the extenxion or
app permissions prompt window">
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Typo (extenxion).

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_permissions_api.cc (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:54: bool
UnpackPermissionSet(DictionaryValue* value,
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Seems like CreatePermissionsValue and UnpackPermissionSet should have
> symmetrical names, since they do ExtensionPermissionSet <-> Value conversion.
> PackPermissionsToValue and UnpackPermissionsFromValue?

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:102: const
ExtensionPermissionSet* existing = extension->GetActivePermissions();
On 2011/07/21 21:18:13, Mihai Parparita wrote:
> I the type of existing should be scoped_refptr<ExtensionPermissionSet>.
> Otherwise its ref count is not actually increased while you're using it, so it
> could go away if the active permission set changes while this function is
> running.
> 
> This probably applies to other places in the CL where you call
> GetActivePermissions().

Woops, nice catch. Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:111: if
(extension->location() == Extension::INTERNAL)
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Not sure I understand the location check. Don't we want to update the granted
> permissions for all extension types?

We only prompt the user to confirm permissions for INTERNAL extensions (the
others can silently increase privilege levels). To make this easier to follow, I
added Extension::CanSilentlyIncreasePermissions and made this an early return
inside GrantPermissions (there used to be a CHECK). I also updated
RequestPermissionsFunction to skip the prompt if that is true.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:137: if (!permissions ||
permissions->IsEmpty())
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Any reason why this check is here, and not earlier in NotifyPermissionsUpdated
> (presumably you don't want to send notifications on empty changes either)?
Also,
> the argument name should probably be changed_permissions, to make it clearer
> that it's just a delta.

Just a relic from some last minute shuffling. I've moved this to the top of
NotifyPermissionsUpdated.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.cc:315: // of the granted
permissions set.
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Would it make sense to filter out the requested permissions if some are
already
> granted, so that the prompt only has newly requested ones?

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_permissions_api.h (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.h:30: // and sends the
relevant messages and notifications.
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Add a comment that it's assumed the user has already been prompted (if
> necessary) for the extra permissions?

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.h:41: const
ExtensionPermissionSet& white_list() const { return *white_list_; }
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> "whitelist" (no dash/underscore) seems to be the preferred spelling in
Chromium.

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.h:76: virtual bool
RunImpl();
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Add OVERRIDE to all the RunImpls.

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api.h:98: // FOR TESTS ONLY to
by pass the confirmation UI.
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> "bypass" is one word.

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_permissions_api_constants.cc (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_permissions_api_constants.cc:9: const char
kApisKey[] = "apis";
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Not sure "apis" is the best name, since it's not always a 1:1 mapping between
> APIs and permissions (e.g. chrome.tabs and chrome.windows are both controlled
by
> the "tabs" permission). Do you think it'd be too confusing to just call it
> "permissions"? (even if you later add "origins" as a separate field?)

Changed this to "permissions". I'll update the terminology in
ExtensionPermissionSet in a follow up CL.

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:749: ExtensionPermissionSet*
ExtensionPrefs::GetActivePermissions(
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> This seems pretty similar to the granted permission get/set methods. Might be
> worth it to have some shared utility functions for converting to/from Value.

I factored these out into {Set,Read}ExtensionPrefPermissionSet. Once the
permissions API supports origins, we can probably share the Unpack/Pack methods
between here and the extension_permissions_api.cc.

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:1151: "description": "Requests
access to the specified permissions.",
On 2011/07/19 17:15:13, jstritar wrote:
> Mention the optional_permissions field of the manifest here and in the
manifest
> section of documentation.

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:1155: "type": "object",
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Shouldn't this be of type Permissions?

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2506: LOG(INFO) << "BLAH1: " << key;
On 2011/07/21 21:18:13, Mihai Parparita wrote:
> Did you want to keep this logging statement?

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2515: LOG(INFO) << "BLAH2: " << key;
On 2011/07/21 21:18:13, Mihai Parparita wrote:
> Ditto.

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
chrome/common/extensions/extension.h:567: class RuntimeData : public
base::RefCountedThreadSafe<RuntimeData> {
On 2011/07/21 21:18:13, Mihai Parparita wrote:
> I don't think making this ref counted buys you anything, since you never hand
> out instances of it to anyone else, it seems like the lifetime of
runtime_data_
> is 1:1 with the lifetime of the Extension instance that it's in.

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
File chrome/common/extensions/extension_messages.cc (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/common/extensions/ext...
chrome/common/extensions/extension_messages.cc:141: WriteParam(m, (int)p);
On 2011/07/21 21:18:13, Mihai Parparita wrote:
> static_cast<int>(p) is the casting style preferred by the C++ style guide
> (applies below too).

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/renderer/resources/re...
File chrome/renderer/resources/renderer_extension_bindings.js (right):

http://codereview.chromium.org/7432006/diff/2002/chrome/renderer/resources/re...
chrome/renderer/resources/renderer_extension_bindings.js:332: "permissions",
On 2011/07/19 17:10:53, jstritar wrote:
> I plan on making chrome.permissions accessible by web apps, but it isn't right
> now.

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/permissions/optional/background.html
(right):

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional/background.html:24:
var empty_permissions = {apis: []};
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> I think we generally use the JS naming scheme in tests (this should be
> emptyPermissions)

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional/background.html:60: },
pass(function(result) {
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Seems like this should be intended more, since it's one of the arguments to
> chrome.permissions.contains.

Looked kind of strange that way, since the contents of the callback would then
be indented 4 spaces. I updated this to be consistent with how other tests, like
the tabs ones, are indented.

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional/background.html:94:
assertTrue(false);
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Something like chrome.test.fail("Should not have tabs API permission") seems
> clearer.

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional/background.html:140:
assertTrue(false);
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Ditto about using chrome.test.fail("Should not have tabs API permission").

Done.

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/permissions/optional/manifest.json
(right):

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional/manifest.json:4:
"key":
"MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDkprt3BRSqoikAhSygI6VUzDLt18cKODYmkaa/dwPp4dboyz93RSB+v76grbqsNWrJjkrEwRD3QFeBYBq7h27XAMV4X5XvWjmWQBkRTBQyQI8cZd7M9dgfKrI3EqX9OJvd/wTJkC0dgF47nwWRa/Tvwl7Y66GwEEUjpn2MTv4klwIDAQAB",
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Is the key necessary?

The key makes sure the extension has the same ID, so we can initialize the
extension's permission prefs before running the tests. (One test verifies the
extension can request permissions with no user prompt if they are already in the
granted permissions list)

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/permissions/optional_deny/background.html
(right):

http://codereview.chromium.org/7432006/diff/2002/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/permissions/optional_deny/background.html:24:
assertTrue(false);
On 2011/07/20 22:03:43, Mihai Parparita wrote:
> Ditto about using chrome.test.fail("Should not have tabs API permission").

Done.

[Mihai Parparita -not on Chrome, 2011-07-23 22:54:52]

LGTM with some small issues, feel free to fix them and land this if you want to
make sure that this makes the M14 branch.

http://codereview.chromium.org/7432006/diff/18053/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7432006/diff/18053/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:980: if (extension->location() !=
Extension::INTERNAL)
Not seeing the Extension::CanSilentlyIncreasePermissions method that you said
you had added.

http://codereview.chromium.org/7432006/diff/18053/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/7432006/diff/18053/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1918: // Initialize the optional
permissions (optional).
Shouldn't this only be done if the experimental permissions flag is passed in?

[jstritar, 2011-07-25 19:15:23]

Thanks for taking a look at it. I addressed your comments, and also realized
that I can't use scoped_ptr at all if we're ref counting ExtensionPermissionSet
(throws a CHECK in debug mode), so I updated that everywhere.

http://codereview.chromium.org/7432006/diff/18053/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7432006/diff/18053/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:980: if (extension->location() !=
Extension::INTERNAL)
On 2011/07/23 22:54:53, Mihai Parparita wrote:
> Not seeing the Extension::CanSilentlyIncreasePermissions method that you said
> you had added.

Woops. Done.

http://codereview.chromium.org/7432006/diff/18053/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/7432006/diff/18053/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:1918: // Initialize the optional
permissions (optional).
On 2011/07/23 22:54:53, Mihai Parparita wrote:
> Shouldn't this only be done if the experimental permissions flag is passed in?

Done.

[Mihai Parparita -not on Chrome, 2011-07-25 19:55:30]

Still LGTM

[jstritar, 2011-07-26 15:31:20]

Unfortunately I didn't make this in M14 due to a couple obscure IPC errors. I
originally thought the failures were flake because I didn't see them on my local
machine, but it was because I was compiling Release builds.

The errors (one of them at least) appears related to deserializing
ExtensionMsg_Load_Params for certain user script values (like when file://
schemes are used).

[commit-bot: I haz the power, 2011-07-26 23:13:03]

Can't apply patch for file
chrome/test/data/extensions/api_test/permissions/experimental_disabled/background.html.
While running patch -p1 --forward --force;
patching file
chrome/test/data/extensions/api_test/permissions/experimental_disabled/background.html
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/data/extensions/api_test/permissions/experimental_disabled/background.html.rej