Implement unlimitedStorage content capability

Implement unlimitedStorage content capability

This turns on functional support for the unlimitedStorage
content_capabilities grant.

BUG=409272
R=kalman@chromium.org

Committed: https://crrev.com/d3af38a5d2588854cbeeb7f02f6411965a161e6e
Cr-Commit-Position: refs/heads/master@{#308192}

[Ken Rockot(use gerrit already), 2014-12-12 02:11:45]

The storage policy code seems kinda ugly to me, but I tried to avoid the need
for any substantial refactoring.

[not at google - send to devlin, 2014-12-12 16:33:40]

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:159: if
(extensions::ContentCapabilitiesInfo::Get(extension).permissions.count(
Am I missing something; what guarantees the extension has a non-null
ContentCapabilitiesInfo?

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:161:
unlimited_extensions_.Add(extension);
I'm not sure; am I reading this correctly, and does this mean that I can do:

{
  "permissions": ["storage"],  // for example
  "content_capabilities": {
    "matches": ["https://google.com"],
    "permissions": ["unlimitedStorage"]
  }
}

and get unlimited storage for my own extension, in addition to for google.com?

[Ken Rockot(use gerrit already), 2014-12-12 16:39:08]

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:159: if
(extensions::ContentCapabilitiesInfo::Get(extension).permissions.count(
On 2014/12/12 16:33:40, kalman wrote:
> Am I missing something; what guarantees the extension has a non-null
> ContentCapabilitiesInfo?

It's lazily constructed as needed. An extension without a content_capabilities
manifest entry will just have a CCI with empty permissions and empty
url_patterns.

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:161:
unlimited_extensions_.Add(extension);
On 2014/12/12 16:33:40, kalman wrote:
> I'm not sure; am I reading this correctly, and does this mean that I can do:
> 
> {
>   "permissions": ["storage"],  // for example
>   "content_capabilities": {
>     "matches": ["https://google.com"],
>     "permissions": ["unlimitedStorage"]
>   }
> }
> 
> and get unlimited storage for my own extension, in addition to for google.com?

This only adds the extension to the unlimited_extensions_ set. Actual granting
of unlimited storage still happens per-origin according to the logic in
IsStorageUnlimited.

The presence of a content capability for unlimitedStorage at an origin will not
make IsStorageUnlimited("chrome-extension://foo/") pass.

Good test case though, so I'll add a test for reassurance.

[not at google - send to devlin, 2014-12-12 17:35:40]

lgtm

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:159: if
(extensions::ContentCapabilitiesInfo::Get(extension).permissions.count(
On 2014/12/12 16:39:07, Ken Rockot wrote:
> On 2014/12/12 16:33:40, kalman wrote:
> > Am I missing something; what guarantees the extension has a non-null
> > ContentCapabilitiesInfo?
> 
> It's lazily constructed as needed. An extension without a content_capabilities
> manifest entry will just have a CCI with empty permissions and empty
> url_patterns.

Is that always the way these things work?

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:161:
unlimited_extensions_.Add(extension);
On 2014/12/12 16:39:08, Ken Rockot wrote:
> On 2014/12/12 16:33:40, kalman wrote:
> > I'm not sure; am I reading this correctly, and does this mean that I can do:
> > 
> > {
> >   "permissions": ["storage"],  // for example
> >   "content_capabilities": {
> >     "matches": ["https://google.com"],
> >     "permissions": ["unlimitedStorage"]
> >   }
> > }
> > 
> > and get unlimited storage for my own extension, in addition to for
google.com?
> 
> This only adds the extension to the unlimited_extensions_ set. Actual granting
> of unlimited storage still happens per-origin according to the logic in
> IsStorageUnlimited.
> 
> The presence of a content capability for unlimitedStorage at an origin will
not
> make IsStorageUnlimited("chrome-extension://foo/") pass.
> 
> Good test case though, so I'll add a test for reassurance.

Oh, I see. So unlimited extensions is just some kind of optimisation rather than
checking the installed extension set every time. Lame, ok. Test would be nice.

[Ken Rockot(use gerrit already), 2014-12-12 20:51:13]

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:159: if
(extensions::ContentCapabilitiesInfo::Get(extension).permissions.count(
On 2014/12/12 17:35:39, kalman wrote:
> On 2014/12/12 16:39:07, Ken Rockot wrote:
> > On 2014/12/12 16:33:40, kalman wrote:
> > > Am I missing something; what guarantees the extension has a non-null
> > > ContentCapabilitiesInfo?
> > 
> > It's lazily constructed as needed. An extension without a
content_capabilities
> > manifest entry will just have a CCI with empty permissions and empty
> > url_patterns.
> 
> Is that always the way these things work?

It's the way some of them work. Others may be OK returning NULL.

I think I'd rather have a sane default object created lazily though.

https://codereview.chromium.org/802593003/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:161:
unlimited_extensions_.Add(extension);
On 2014/12/12 17:35:39, kalman wrote:
> On 2014/12/12 16:39:08, Ken Rockot wrote:
> > On 2014/12/12 16:33:40, kalman wrote:
> > > I'm not sure; am I reading this correctly, and does this mean that I can
do:
> > > 
> > > {
> > >   "permissions": ["storage"],  // for example
> > >   "content_capabilities": {
> > >     "matches": ["https://google.com"],
> > >     "permissions": ["unlimitedStorage"]
> > >   }
> > > }
> > > 
> > > and get unlimited storage for my own extension, in addition to for
> google.com?
> > 
> > This only adds the extension to the unlimited_extensions_ set. Actual
granting
> > of unlimited storage still happens per-origin according to the logic in
> > IsStorageUnlimited.
> > 
> > The presence of a content capability for unlimitedStorage at an origin will
> not
> > make IsStorageUnlimited("chrome-extension://foo/") pass.
> > 
> > Good test case though, so I'll add a test for reassurance.
> 
> Oh, I see. So unlimited extensions is just some kind of optimisation rather
than
> checking the installed extension set every time. Lame, ok. Test would be nice.

It was pretty ugly, so I changed it. There's a separate set to track content
capabilities grantors now. More tests added for privilege boundaries.

[not at google - send to devlin, 2014-12-12 21:03:06]

The whole thread safety thing is suspect to me anyway. I don't know why you need
to synchronise access at the ExtensionSpecialStoragePolicy level, rather than
just making the SpecialCollections thread-safe. Whatever.

https://codereview.chromium.org/802593003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/802593003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_special_storage_policy.cc:172:
extension->is_app()) {
Bleh, I don't like this code. It's basically:

if (A || B || C || D || E) {
  if (A && A') ...;
  if (B) ...;
  if (C) ...;
  if (D) ...;
  if (E) ...;
}

The only reason for the highly redundant looking outer check is so that you only
need to Lock under slightly tighter conditions, but those "conditions" include
it being an app. I really don't see the point.

Which makes me realise that the change you made:

content_capabilities_unlimited_extensions_.Add(extension);

is, in theory, not thread safe. However, neither are some of the other methods
in this class (like HasSessionOnlyOrigins). How strange.

I vote:
1. Put the AutoLock(lock_) at the top of this method.
2. Delete the if (A || B || C || D || E) outer check.

https://codereview.chromium.org/802593003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_special_storage_policy.cc:219:
extension->is_app()) {
Here too.

https://codereview.chromium.org/802593003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_special_storage_policy.cc:313: for
(scoped_refptr<const Extension> extension : extensions_) {
Should this be content_capabilities_unlimited_extensions_?

(Also same thread safety question).

[Ken Rockot(use gerrit already), 2014-12-12 21:10:18]

https://codereview.chromium.org/802593003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/802593003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_special_storage_policy.cc:172:
extension->is_app()) {
On 2014/12/12 21:03:06, kalman wrote:
> Bleh, I don't like this code. It's basically:
> 
> if (A || B || C || D || E) {
>   if (A && A') ...;
>   if (B) ...;
>   if (C) ...;
>   if (D) ...;
>   if (E) ...;
> }
> 
> The only reason for the highly redundant looking outer check is so that you
only
> need to Lock under slightly tighter conditions, but those "conditions" include
> it being an app. I really don't see the point.
> 
> Which makes me realise that the change you made:
> 
> content_capabilities_unlimited_extensions_.Add(extension);
> 
> is, in theory, not thread safe. However, neither are some of the other methods
> in this class (like HasSessionOnlyOrigins). How strange.
> 
> I vote:
> 1. Put the AutoLock(lock_) at the top of this method.
> 2. Delete the if (A || B || C || D || E) outer check.

Doh, you're right. Moving the lock.

https://codereview.chromium.org/802593003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_special_storage_policy.cc:313: for
(scoped_refptr<const Extension> extension : extensions_) {
On 2014/12/12 21:03:06, kalman wrote:
> Should this be content_capabilities_unlimited_extensions_?
> 
> (Also same thread safety question).

No, we're in a different data structure here. There is only extensions_, and
this operation is meant to be called from within a held lock scope (which it
is).

[Ken Rockot(use gerrit already), 2014-12-12 21:41:06]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-12-12 21:42:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/802593003/40001

[not at google - send to devlin, 2014-12-12 23:10:13]

https://codereview.chromium.org/802593003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/802593003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_special_storage_policy.cc:173:
extension->is_app()) {
(note that with the locking change I'm pretty sure you can just delete this
entire if-block, same below).

[commit-bot: I haz the power, 2014-12-12 23:23:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-12 23:24:02]

Patchset 3 (id:??) landed as
https://crrev.com/d3af38a5d2588854cbeeb7f02f6411965a161e6e
Cr-Commit-Position: refs/heads/master@{#308192}