Restrict access to permissions based on extension types.

Restrict access to permissions based on extension types.

BUG=101992
TEST=existing

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

[Aaron Boodman, 2011-11-22 09:40:32]

drive-by

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

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2858: if (creation_flags_ &
STRICT_ERROR_CHECKS) {
I don't think we want to be lose with the errors for new types right? The
behavior with hosted apps was a mistake.

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2905: switch (GetType()) {
This is so short, you could just fold it into CanSpecifyAPIPermission() to
address my 'how is this different than...' comment.

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

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.h:693: bool CanSpecifyPermissionForType(
It isn't clear to me from just these declarations how this is different than
CanSpecifyAPIPermission().

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_manifests_unittest.cc (right):

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension_manifests_unittest.cc:656: } else if
(!permission->supports_hosted_apps()) {
I like the 'supports' nomenclature better - nice change.

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.h:235: // Registers all the
permissions.
I sorta feel like this should be:

// Register ALL the permissions!

[Aaron Boodman, 2011-11-22 09:41:17]

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

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2858: if (creation_flags_ &
STRICT_ERROR_CHECKS) {
On 2011/11/22 09:40:32, Aaron Boodman wrote:
> I don't think we want to be lose with the errors for new types right? The
> behavior with hosted apps was a mistake.

sigh, *loose*

[jstritar, 2011-11-22 15:58:29]

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

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2858: if (creation_flags_ &
STRICT_ERROR_CHECKS) {
Won't we run into this again?

http://b/issue?id=5514150

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:2905: switch (GetType()) {
On 2011/11/22 09:40:32, Aaron Boodman wrote:
> This is so short, you could just fold it into CanSpecifyAPIPermission() to
> address my 'how is this different than...' comment.

Done.

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

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension.h:693: bool CanSpecifyPermissionForType(
On 2011/11/22 09:40:32, Aaron Boodman wrote:
> It isn't clear to me from just these declarations how this is different than
> CanSpecifyAPIPermission().

Done- just rolled it into CanSpeicfyAPIPermission.

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_manifests_unittest.cc (right):

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension_manifests_unittest.cc:656: } else if
(!permission->supports_hosted_apps()) {
On 2011/11/22 09:40:32, Aaron Boodman wrote:
> I like the 'supports' nomenclature better - nice change.

Nice, thx.

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_permission_set.h (right):

http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
chrome/common/extensions/extension_permission_set.h:235: // Registers all the
permissions.
On 2011/11/22 09:40:32, Aaron Boodman wrote:
> I sorta feel like this should be:
> 
> // Register ALL the permissions!

Haha, done. I also renamed the method to RegisterAllPermissions.

[Aaron Boodman, 2011-11-22 17:16:26]

> On 2011/11/22 15:58:29, jstritar wrote:
http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
> chrome/common/extensions/extension.cc:2858: if (creation_flags_ &
> STRICT_ERROR_CHECKS) {
> Won't we run into this again?
> 
> http://b/issue?id=5514150

I thought that only applied to hosted apps because we had some bad legacy data
for them. We can still be strict about the packaged apps, and platform apps,
right?

>
http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
> chrome/common/extensions/extension.cc:2905: switch (GetType()) {
> On 2011/11/22 09:40:32, Aaron Boodman wrote:
> > This is so short, you could just fold it into CanSpecifyAPIPermission() to
> > address my 'how is this different than...' comment.
> 
> Done.
> 
>
http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
> File chrome/common/extensions/extension.h (right):
> 
>
http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
> chrome/common/extensions/extension.h:693: bool CanSpecifyPermissionForType(
> On 2011/11/22 09:40:32, Aaron Boodman wrote:
> > It isn't clear to me from just these declarations how this is different than
> > CanSpecifyAPIPermission().
> 
> Done- just rolled it into CanSpeicfyAPIPermission.
> 
>
http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
> File chrome/common/extensions/extension_manifests_unittest.cc (right):
> 
>
http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
> chrome/common/extensions/extension_manifests_unittest.cc:656: } else if
> (!permission->supports_hosted_apps()) {
> On 2011/11/22 09:40:32, Aaron Boodman wrote:
> > I like the 'supports' nomenclature better - nice change.
> 
> Nice, thx.
> 
>
http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
> File chrome/common/extensions/extension_permission_set.h (right):
> 
>
http://codereview.chromium.org/8598022/diff/7001/chrome/common/extensions/ext...
> chrome/common/extensions/extension_permission_set.h:235: // Registers all the
> permissions.
> On 2011/11/22 09:40:32, Aaron Boodman wrote:
> > I sorta feel like this should be:
> > 
> > // Register ALL the permissions!
> 
> Haha, done. I also renamed the method to RegisterAllPermissions.

[jstritar, 2011-11-22 18:07:50]

On 2011/11/22 17:16:26, Aaron Boodman wrote: 
> I thought that only applied to hosted apps because we had some bad legacy data
> for them. We can still be strict about the packaged apps, and platform apps,
> right?

We can definitely be strict on platform apps since non exist yet, but couldn't
we run into the same problem with packaged apps?

Can we rely on the hosted apps to update their manifests to remove the bad data?

[jstritar, 2011-11-22 20:59:30]

Back to you Aaron-- I special cased the hosted apps for the strict check.

[Aaron Boodman, 2011-11-22 21:32:31]

lgtm

[jstritar, 2011-11-30 18:56:20]

I added TYPE_PLATFORM_APP to Extension::GetType() while rebasing and had to make
a couple changes to chrome_render_view_host_observer.cc. Could you take a look
at that file?

[Aaron Boodman, 2011-11-30 21:32:16]

http://codereview.chromium.org/8598022/diff/24001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/8598022/diff/24001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:116: if (type
== Extension::TYPE_EXTENSION ||
Why did you add this check?

[commit-bot: I haz the power, 2011-12-01 19:58:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/8598022/27001

[commit-bot: I haz the power, 2011-12-01 22:02:41]

Change committed as 112548