Reland restrict extension features based on the extension type.

Reland restrict extension features based on the extension type.

The "chrome_url_overrides" manifest key is now accessible by packaged apps.

BUG=101992, 104103
TEST=existing, ManifestTest

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

[Aaron Boodman, 2011-11-23 01:45:25]

Nice!

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

http://codereview.chromium.org/8654001/diff/5014/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1104:
extension->manifest_value()->value()->DeepCopy());
Yeah, calling this thing just "manifest" would make this read better
(extension->manifest()->value()...)

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

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:229: if
(!extension->InitFromValue(ManifestValue(value.DeepCopy()), flags, error))
This kinda sucks, we end up copying this same DictionaryValue several times.
(once here, once into ManifestValue's member, and then we copy the ManifestValue
into Extension).

If either InitFromValue() or ManifestValue() took pointers to their param
instead of const-ref, then they could just take ownership of them instead of
copying.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:876: is_app_ =
manifest->HasKey(keys::kApp);
Can you get rid of all the is_*_ members in Extension and change all the is_*()
methods to delegate to the new class, so that we have a single source of truth?

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/ext...
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/ext...
chrome/common/extensions/extension_constants.cc:169: "Access to '*' denied.";
Would be nice to have a slightly more descriptive error, like:

Feature 'foo' is not allowed in this type of manifest.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
File chrome/common/extensions/manifest_value.cc (right):

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
Shame to pull out this nice, encapsulated class and not have any unit tests for
it.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:79:
restrictions_[kFeatureRestrictions[i].key] =
Sucks to have a separate copy of this for every instance of the object.

Instead, use base::LazyInstance to have one static instance. You also don't need
the big array that you translate into a map: You can just have like

namespace {

typedef ... RestrictionMap;

struct Restrictions {
  Restrictions() {
    map[foo] = bar;
    map[monkey] = balls;
    ...
  }

  RestrictionMap map;
};

base::LazyInstance<Restrictions> g_restrictions;

}

Then, in your code, use g_restrictions.Get().

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:84: if (IsTheme())
Consider omitting the type_ member and instead having a GetType() method that
does the work to figure out the type and then have Is*() helpers that just
delegate to GetType().

Otherwise you are essentially duplicating information - the type is represented
in both type_ and in the dictionary.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:102: // recognized for compatibility.
For clarity: s/for compatibility/for forward compatibility/

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:103: if (!IsKnownKey(*key))
Add:

TODO(aa): Consider having an error here in the case of strict error checking to
let developers know when they screw up.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:106: if (!CanAccessKey(*key)) {
I think that there will probably be legacy data here, too, right? So we'll need
the same strategy as from the other place.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
File chrome/common/extensions/manifest_value.h (right):

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.h:20: 
Put this in namespace extensions { ?

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.h:21: // Represents an extension
manifest and enforces manifest schemas on top of
Suggest:

Lightweight wrapper around a DictionaryValue representing an 
extension's manifest. Currently enforces access to properties of 
the manifest based on manifest type.

TODO(aa): Move more smarts about manifest into this class over 
time.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.h:23: class ManifestValue {
Consider just calling this Manifest.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.h:49: // These access the manifest
value, and restrict access (with CHECKs)
I would not enforce with CHECK because that makes it hard to unit test.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.h:95: kTypeAllApps = kTypeHostedApp |
kTypePackagedApp | kTypePlatformApp,
Wow, there is a feature that is supported by all app types, but not extensions?

[jstritar, 2011-11-28 23:09:56]

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

http://codereview.chromium.org/8654001/diff/5014/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_prefs.cc:1104:
extension->manifest_value()->value()->DeepCopy());
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> Yeah, calling this thing just "manifest" would make this read better
> (extension->manifest()->value()...)

Yeah, much better. Done.

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

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:229: if
(!extension->InitFromValue(ManifestValue(value.DeepCopy()), flags, error))
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> This kinda sucks, we end up copying this same DictionaryValue several times.
> (once here, once into ManifestValue's member, and then we copy the
ManifestValue
> into Extension).
> 
> If either InitFromValue() or ManifestValue() took pointers to their param
> instead of const-ref, then they could just take ownership of them instead of
> copying.

Done, we only copy the DictionaryValue once now.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/ext...
chrome/common/extensions/extension.cc:876: is_app_ =
manifest->HasKey(keys::kApp);
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> Can you get rid of all the is_*_ members in Extension and change all the
is_*()
> methods to delegate to the new class, so that we have a single source of
truth?

Done.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/ext...
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/ext...
chrome/common/extensions/extension_constants.cc:169: "Access to '*' denied.";
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> Would be nice to have a slightly more descriptive error, like:
> 
> Feature 'foo' is not allowed in this type of manifest.

Done.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
File chrome/common/extensions/manifest_value.cc (right):

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> Shame to pull out this nice, encapsulated class and not have any unit tests
for
> it.

Done.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:79:
restrictions_[kFeatureRestrictions[i].key] =
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> Sucks to have a separate copy of this for every instance of the object.
> 
> Instead, use base::LazyInstance to have one static instance. You also don't
need
> the big array that you translate into a map: You can just have like
> 
> namespace {
> 
> typedef ... RestrictionMap;
> 
> struct Restrictions {
>   Restrictions() {
>     map[foo] = bar;
>     map[monkey] = balls;
>     ...
>   }
> 
>   RestrictionMap map;
> };
> 
> base::LazyInstance<Restrictions> g_restrictions;
> 
> }
> 
> Then, in your code, use g_restrictions.Get().
> 

Done.. LazyInstance is sweet!

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:84: if (IsTheme())
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> Consider omitting the type_ member and instead having a GetType() method that
> does the work to figure out the type and then have Is*() helpers that just
> delegate to GetType().
> 
> Otherwise you are essentially duplicating information - the type is
represented
> in both type_ and in the dictionary.

Done.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:102: // recognized for compatibility.
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> For clarity: s/for compatibility/for forward compatibility/

Done.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:103: if (!IsKnownKey(*key))
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> Add:
> 
> TODO(aa): Consider having an error here in the case of strict error checking
to
> let developers know when they screw up.

Done.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.cc:106: if (!CanAccessKey(*key)) {
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> I think that there will probably be legacy data here, too, right? So we'll
need
> the same strategy as from the other place.

I think we're okay here because we parsed hosted app manifests with a whitelist
before (See the old Extension::EnsureNotHybridApp), and we also had the
ContainsNonThemeKeys and HasMultipleUISurfaces checks.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
File chrome/common/extensions/manifest_value.h (right):

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.h:20: 
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> Put this in namespace extensions { ?

Done.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.h:21: // Represents an extension
manifest and enforces manifest schemas on top of
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> Suggest:
> 
> Lightweight wrapper around a DictionaryValue representing an 
> extension's manifest. Currently enforces access to properties of 
> the manifest based on manifest type.
> 
> TODO(aa): Move more smarts about manifest into this class over 
> time.

Done.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.h:23: class ManifestValue {
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> Consider just calling this Manifest.

Done.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.h:49: // These access the manifest
value, and restrict access (with CHECKs)
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> I would not enforce with CHECK because that makes it hard to unit test.

Done. We just return false now.

http://codereview.chromium.org/8654001/diff/5014/chrome/common/extensions/man...
chrome/common/extensions/manifest_value.h:95: kTypeAllApps = kTypeHostedApp |
kTypePackagedApp | kTypePlatformApp,
On 2011/11/23 01:45:25, Aaron Boodman wrote:
> Wow, there is a feature that is supported by all app types, but not
extensions?

The 'app' manifest key technically is. I changed this up a bit and no longer
have this in the latest revision.

[Aaron Boodman, 2011-12-01 21:32:04]

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
File chrome/common/extensions/manifest.cc (right):

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
This class turned out really tight and clean. Nice.

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:186: if (value_->HasKey(keys::kTheme) &&
value_->GetDictionary(keys::kTheme, NULL))
Why do you test that the value is a dictionary for theme, but not the other
types?

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
File chrome/common/extensions/manifest.h (right):

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest.h:31: // Flags for matching types of extensino
manifests.
typo: extensino

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest.h:82: bool HasKey(const std::string& key)
const;
Something to consider for the future:

This API from DictionaryValue is terrible. It requires two lines for every call,
even if you know that it won't fail, or don't care about failures.

It would be nice if there was a convenience form of each of these getters too,
that simply returned the value (or a default in the case of failure).

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
File chrome/common/extensions/manifest_unittest.cc (right):

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest_unittest.cc:33: const char* kNotThemeKeys[] =
{
Instead of duplicating these lists, can you expose it from manifest.h (I realize
this may require undoing some of what I suggested wrt the LazyInstance, sorry).

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest_unittest.cc:264: // Verifies that hosted apps
can access the right keys.
At least for hosted apps (maybe other types too), I think we have some other
tests similar to this in extension_manifest_unittest.cc. If they overlap, can
you remove those in favor of this?

[jstritar, 2011-12-02 16:26:32]

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
File chrome/common/extensions/manifest.cc (right):

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/12/01 21:32:04, Aaron Boodman wrote:
> This class turned out really tight and clean. Nice.

Nice, thanks!

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:186: if (value_->HasKey(keys::kTheme) &&
value_->GetDictionary(keys::kTheme, NULL))
On 2011/12/01 21:32:04, Aaron Boodman wrote:
> Why do you test that the value is a dictionary for theme, but not the other
> types?

I did that to be consistent with the previous behavior. It's probably not
necessary since other things will fail if it's not the correct type... removed.

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
File chrome/common/extensions/manifest.h (right):

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest.h:31: // Flags for matching types of extensino
manifests.
On 2011/12/01 21:32:04, Aaron Boodman wrote:
> typo: extensino

Done.

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest.h:82: bool HasKey(const std::string& key)
const;
On 2011/12/01 21:32:04, Aaron Boodman wrote:
> Something to consider for the future:
> 
> This API from DictionaryValue is terrible. It requires two lines for every
call,
> even if you know that it won't fail, or don't care about failures.
> 
> It would be nice if there was a convenience form of each of these getters too,
> that simply returned the value (or a default in the case of failure).

Agreed, the DictionaryValue API is really annoying.

I'd eventually like to hide the fact that this is a DictionaryValue, and only
expose higher level accessors like IsPlatformApp() and GetDefaultPermissions().
Then we can support manifest versions by having subclasses that override the old
accessors. This would also let us get rid of the duplicate data (members on
Extension and the data in DictionaryValue) as you mentioned in the other code
review.

We'd still want to simplify these methods though (and make them private).

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
File chrome/common/extensions/manifest_unittest.cc (right):

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest_unittest.cc:33: const char* kNotThemeKeys[] =
{
On 2011/12/01 21:32:04, Aaron Boodman wrote:
> Instead of duplicating these lists, can you expose it from manifest.h (I
realize
> this may require undoing some of what I suggested wrt the LazyInstance,
sorry).

These don't test much if we use the values from manifest.cc. I did this on
purpose to help make sure features aren't inadvertently made available.

http://codereview.chromium.org/8654001/diff/14005/chrome/common/extensions/ma...
chrome/common/extensions/manifest_unittest.cc:264: // Verifies that hosted apps
can access the right keys.
On 2011/12/01 21:32:04, Aaron Boodman wrote:
> At least for hosted apps (maybe other types too), I think we have some other
> tests similar to this in extension_manifest_unittest.cc. If they overlap, can
> you remove those in favor of this?

There wasn't too much overlap, but I got rid of DisallowHybridApp and part of
DisallowMultipleUISurfaces.

[Aaron Boodman, 2011-12-02 17:06:52]

LGTM

[commit-bot: I haz the power, 2011-12-02 18:19:25]

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

[commit-bot: I haz the power, 2011-12-02 19:49:49]

Change committed as 112764

[commit-bot: I haz the power, 2011-12-03 15:00:39]

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

[commit-bot: I haz the power, 2011-12-03 16:13:51]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

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

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

[commit-bot: I haz the power, 2011-12-04 00:55:35]

Change committed as 112914

[Mihai Parparita -not on Chrome, 2011-12-05 23:24:07]

+Mike for my drive-by platform app restriction musings.

http://codereview.chromium.org/8654001/diff/30001/chrome/common/extensions/ma...
File chrome/common/extensions/manifest.cc (right):

http://codereview.chromium.org/8654001/diff/30001/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:43: // keys::kPlatformApp holds a boolean,
so all types can define it.
If this makes platform apps weird (because all the other app types are implied
by the presence of specific manifest keys), perhaps this should be changed.

http://codereview.chromium.org/8654001/diff/30001/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:57: map[keys::kOfflineEnabled] =
all_but_themes;
Does this make sense for extensions? I guess we weren't forbidding it before, so
it might be too late to restrict it now.

http://codereview.chromium.org/8654001/diff/30001/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:60: map[keys::kConvertedFromUserScript] =
all_but_themes;
I don't think this makes sense for apps (of any kind).

http://codereview.chromium.org/8654001/diff/30001/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:62: map[keys::kPlugins] = all_but_themes;
I think we're going to try removing NPAPI plugin support from platform apps.

[jstritar, 2011-12-14 19:00:26]

http://codereview.chromium.org/8654001/diff/30001/chrome/common/extensions/ma...
File chrome/common/extensions/manifest.cc (right):

http://codereview.chromium.org/8654001/diff/30001/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:43: // keys::kPlatformApp holds a boolean,
so all types can define it.
On 2011/12/05 23:24:07, Mihai Parparita wrote:
> If this makes platform apps weird (because all the other app types are implied
> by the presence of specific manifest keys), perhaps this should be changed.

If you can think of any fields that could go under platform_app, we could make
it a dictionary like app.

http://codereview.chromium.org/8654001/diff/30001/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:57: map[keys::kOfflineEnabled] =
all_but_themes;
On 2011/12/05 23:24:07, Mihai Parparita wrote:
> Does this make sense for extensions? I guess we weren't forbidding it before,
so
> it might be too late to restrict it now.

Not sure, I don't think it'll hurt, but yeah it wasn't forbidden before.

http://codereview.chromium.org/8654001/diff/30001/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:60: map[keys::kConvertedFromUserScript] =
all_but_themes;
On 2011/12/05 23:24:07, Mihai Parparita wrote:
> I don't think this makes sense for apps (of any kind).

This is kind of like platform_app where it's a boolean. There are some sync unit
tests that set it to false, though I'm not sure if that happens in real life. We
might be able to do better validation (e.g. not set to true for apps) when we
switch to JSON schema.

http://codereview.chromium.org/8654001/diff/30001/chrome/common/extensions/ma...
chrome/common/extensions/manifest.cc:62: map[keys::kPlugins] = all_but_themes;
On 2011/12/05 23:24:07, Mihai Parparita wrote:
> I think we're going to try removing NPAPI plugin support from platform apps.

I'll fix this in another CL.