Support a "policy" extension location in extension features files. At the same

Support a "policy" extension location in extension features files.

BUG=364536
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264826

[not at google - send to devlin, 2014-04-17 21:33:13]

The formatting changes are from "git cl format", sorry.

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
File extensions/common/features/feature.h (right):

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
extensions/common/features/feature.h:133: Availability
IsAvailableToExtension(const Extension* extension);
always irritated me that we don't have this.

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
extensions/common/features/simple_feature.cc:24:
extension_types["extension"].push_back(Manifest::TYPE_EXTENSION);
I'm not... super happy about these changes.

[not at google - send to devlin, 2014-04-17 21:36:02]

btw the bug talks about making "location" actually be "locations". I thought
about that, decided against it, but I'm not so sure anymore. I'm pretty sure
that we won't want different APIs for those subtly different extension
locations, but only pretty sure.

+asargent for opinion re component location.

[Yoyo Zhou, 2014-04-17 21:53:12]

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
extensions/common/features/simple_feature.cc:24:
extension_types["extension"].push_back(Manifest::TYPE_EXTENSION);
On 2014/04/17 21:33:14, kalman wrote:
> I'm not... super happy about these changes.

Can you replace Manifest::Location with something like a LocationClass that
matches multiple Manifest::Locations? We already have a custom notion of
location for Features, so why not replace it with a different custom notion?

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
extensions/common/features/simple_feature.cc:303: if (!locations_.empty() &&
locations_.find(location) == locations_.end())
Change this test to be something like location_class_.Contains(location)

[Yoyo Zhou, 2014-04-17 21:53:49]

P.S. I think we shouldn't support complex patterns of valid locations, like
"locations": [...]; certainly, at least, not until we need them.

[not at google - send to devlin, 2014-04-17 21:59:30]

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
extensions/common/features/simple_feature.cc:24:
extension_types["extension"].push_back(Manifest::TYPE_EXTENSION);
On 2014/04/17 21:53:12, Yoyo Zhou wrote:
> On 2014/04/17 21:33:14, kalman wrote:
> > I'm not... super happy about these changes.
> 
> Can you replace Manifest::Location with something like a LocationClass that
> matches multiple Manifest::Locations? We already have a custom notion of
> location for Features, so why not replace it with a different custom notion?

I found the old way of doing that a bit awkward; exposing a utility method and
calling it from the right places.

but it would be a less extreme change.

[Yoyo Zhou, 2014-04-17 22:00:29]

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
extensions/common/features/simple_feature.cc:24:
extension_types["extension"].push_back(Manifest::TYPE_EXTENSION);
On 2014/04/17 21:59:31, kalman wrote:
> On 2014/04/17 21:53:12, Yoyo Zhou wrote:
> > On 2014/04/17 21:33:14, kalman wrote:
> > > I'm not... super happy about these changes.
> > 
> > Can you replace Manifest::Location with something like a LocationClass that
> > matches multiple Manifest::Locations? We already have a custom notion of
> > location for Features, so why not replace it with a different custom notion?
> 
> I found the old way of doing that a bit awkward; exposing a utility method and
> calling it from the right places.
> 
> but it would be a less extreme change.

You don't have to expose it. Just pass in the Manifest::Location to
IsAvailable...().

[not at google - send to devlin, 2014-04-17 22:01:17]

good point.

[not at google - send to devlin, 2014-04-17 23:18:11]

ok... rather different now. I maintained part of the original advantage which
was hiding the Location enum from the interface.

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
File extensions/common/features/simple_feature.cc (right):

https://codereview.chromium.org/241673002/diff/20001/extensions/common/featur...
extensions/common/features/simple_feature.cc:303: if (!locations_.empty() &&
locations_.find(location) == locations_.end())
On 2014/04/17 21:53:12, Yoyo Zhou wrote:
> Change this test to be something like location_class_.Contains(location)

done sort of. I went back to how the code was before with the Location and move
the util in here. I didn't like the idea of updating that test again this time
with a "LocationClass" that's effectively an implementation detail.

[Yoyo Zhou, 2014-04-17 23:48:21]

LGTM

https://codereview.chromium.org/241673002/diff/30001/extensions/common/featur...
File extensions/common/features/simple_feature_unittest.cc (right):

https://codereview.chromium.org/241673002/diff/30001/extensions/common/featur...
extensions/common/features/simple_feature_unittest.cc:289:
TEST_F(ExtensionSimpleFeatureTest, Location) {
There should be a test in here for the new locations.

[asargent_no_longer_on_chrome, 2014-04-18 00:16:48]

The only concern I can come up with is more of a general one that might be
somewhat orthogonal to this CL. 

Imagine malicious software that tampers with the Preferences file to change the
location from a regular install to external component. Is that a way to easily
get super powers for their force-installed extension (ie will we put the
"location": "component" restriction on some powerful APIs thinking this prevents
abuse of them?). In general I'm becoming increasingly worried about the notion
of external component extensions being conflated with component ones, and
thinking about ways we can make it harder to abuse this mechanism. 

As far as this CL is concerned, I guess the only thing to think about is whether
we want to split the "component" location up into "component" and
"external_component".

[not at google - send to devlin, 2014-04-18 01:50:20]

On 2014/04/18 00:16:48, Antony Sargent wrote:
> The only concern I can come up with is more of a general one that might be
> somewhat orthogonal to this CL. 
> 
> Imagine malicious software that tampers with the Preferences file to change
the
> location from a regular install to external component. Is that a way to easily
> get super powers for their force-installed extension (ie will we put the
> "location": "component" restriction on some powerful APIs thinking this
prevents
> abuse of them?). In general I'm becoming increasingly worried about the notion
> of external component extensions being conflated with component ones, and
> thinking about ways we can make it harder to abuse this mechanism. 
> 
> As far as this CL is concerned, I guess the only thing to think about is
whether
> we want to split the "component" location up into "component" and
> "external_component".

Yes, fiddling with the location in the preferences file would give an extension
absolute and total power.

See
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
for some code I tried valiantly to remove. We not only put
{"location":"component"} on dangerous APIs but there are plenty of existing
dangerous APIs which are automatically granted access.

Some triage of what "component" means sounds like a good idea.

[not at google - send to devlin, 2014-04-18 01:57:45]

https://codereview.chromium.org/241673002/diff/30001/extensions/common/featur...
File extensions/common/features/simple_feature_unittest.cc (right):

https://codereview.chromium.org/241673002/diff/30001/extensions/common/featur...
extensions/common/features/simple_feature_unittest.cc:289:
TEST_F(ExtensionSimpleFeatureTest, Location) {
On 2014/04/17 23:48:21, Yoyo Zhou wrote:
> There should be a test in here for the new locations.

Done.

[not at google - send to devlin, 2014-04-18 01:57:52]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-04-18 01:58:19]

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

[commit-bot: I haz the power, 2014-04-18 03:34:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 03:34:32]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[not at google - send to devlin, 2014-04-18 16:27:23]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-04-18 16:27:34]

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

[commit-bot: I haz the power, 2014-04-18 19:19:06]

Change committed as 264826