Require confirmation for writable directory access.

Require confirmation for writable directory access.

Previously, an app could request just the fileSystem.write permission,
followed by just the fileSystem.directory permission. This would prompt
the user to allow write access to files and read-only access to
directories, respectively. If the app were to later request both at the
same time, this would allow write access to directories without further
user confirmation.

This change adds an implicit fileSystem.writeDirectory if both
fileSystem.write and fileSystem.directory are requested at the same
time. This requires the user to confirm write access to directories.

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

[Matt Giuca, 2013-09-03 08:24:22]

lgtm

[benwells, 2013-09-03 11:38:54]

lgtm, but please wait to see what kalman say.

kalman: i think we're trying to use the permissions system in a way it wasn't
designed for by implying permissions, but maybe there is already a nice way to
do this.

We had already added hacks in the permission message system but this feels
cleaner and fixes a problem with the previous hackery. This hack is a little
deeper (but feels better) in the extensions system, so I wanted to get your
opinion.

Later we want to make this more systematic, i.e. have a generic way of implying
permissions instead of hardcoding it, and similar a more generic way of
suppressing implying permission messages.

[not at google - send to devlin, 2013-09-03 17:07:04]

lgtm

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:283: }],
For the record: [{...}] in these files is equivalent to just {...} so you can
leave out the []s here and for fileSystem.directory.

In any case: for specifying these for-warning-only permissions you don't
currently need an entry in this file. See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
for example. 

However, when http://crbug.com/280286 is partially in effect, you will.

However #2, I will try and get the arbitrary-install-prompt logic working, and
if so you won't.

So for now I'd suggest you just remove this.

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.h (right):

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:85:
kFileSystemWriteDirectory,
(you do need this)

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission_set.cc (right):

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission_set.cc:344:
insert(APIPermission::kFileSystemWriteDirectory);
Ugh so busted. But I agree that there doesn't seem to be a better way to do this
in our current permission warning infrastructure.

But please file a specific bug to remove this + the similar code in
permission_set.cc and set that general permission bug as blocking it. This code
is going to block the extensions refactor.

[Sam McNally, 2013-09-04 00:21:30]

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:283: }],
On 2013/09/03 17:07:04, kalman wrote:
> For the record: [{...}] in these files is equivalent to just {...} so you can
> leave out the []s here and for fileSystem.directory.
Having fileSystem with the [] and a child permission without the [] CHECK fails
at
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext....
> 
> In any case: for specifying these for-warning-only permissions you don't
> currently need an entry in this file. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> for example. 
> 
> However, when http://crbug.com/280286 is partially in effect, you will.
> 
> However #2, I will try and get the arbitrary-install-prompt logic working, and
> if so you won't.
> 
> So for now I'd suggest you just remove this.

Done.

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission_set.cc (right):

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission_set.cc:344:
insert(APIPermission::kFileSystemWriteDirectory);
On 2013/09/03 17:07:04, kalman wrote:
> Ugh so busted. But I agree that there doesn't seem to be a better way to do
this
> in our current permission warning infrastructure.
> 
> But please file a specific bug to remove this + the similar code in
> permission_set.cc and set that general permission bug as blocking it. This
code
> is going to block the extensions refactor.

Done.

[commit-bot: I haz the power, 2013-09-04 00:23:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/23506021/37001

[not at google - send to devlin, 2013-09-04 00:23:06]

lgtm

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/a...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/23506021/diff/3001/chrome/common/extensions/a...
chrome/common/extensions/api/_permission_features.json:283: }],
On 2013/09/04 00:21:30, Sam McNally wrote:
> On 2013/09/03 17:07:04, kalman wrote:
> > For the record: [{...}] in these files is equivalent to just {...} so you
can
> > leave out the []s here and for fileSystem.directory.
> Having fileSystem with the [] and a child permission without the [] CHECK
fails
> at
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext....
> > 
> > In any case: for specifying these for-warning-only permissions you don't
> > currently need an entry in this file. See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...
> > for example. 
> > 
> > However, when http://crbug.com/280286 is partially in effect, you will.
> > 
> > However #2, I will try and get the arbitrary-install-prompt logic working,
and
> > if so you won't.
> > 
> > So for now I'd suggest you just remove this.
> 
> Done.

Oh. I didn't even realised that those complex permissions worked with the child
permission logic.

[commit-bot: I haz the power, 2013-09-04 02:19:56]

Failed to trigger a try job on win_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-09-04 02:20:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/23506021/59001

[commit-bot: I haz the power, 2013-09-04 16:41:54]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-06 04:08:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/23506021/59001

[commit-bot: I haz the power, 2013-09-06 07:21:33]

Change committed as 221620