Adding file access permissions to fileBrowserHandler manifest.

Adding read, read-write and create file access permission to fileBrowserHandler manifest.

added file_access property to file_browser_handlers

the property is list of enum {"read", "read-write", "create"} values.

The property is optional and it's default value is read-write

"create" value must be alone in the list.

If "create" is specified, file_filters property is ignored.

design doc (part 1 only): https://docs.google.com/a/google.com/document/d/10ZWbisYfxz4jSvvN534p7fPyFFRrmwDtA1RaJN7VDl4/edit
(part 2 needs some more work, it's targeted for R20)

BUG=chromium-os:26106
TEST=unit/browser : *FileBrowser*

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

[tbarzic, 2012-03-20 18:56:53]

Aaron, can you take a look?

This is for the part of the api review that was resolved (at least that was my
impression:) )
the rest of the api changes have been punted to M20..

Thanks

[Aaron Boodman, 2012-03-26 18:11:12]

.redirect("yoz")

[Yoyo Zhou, 2012-03-26 22:42:58]

Mostly LG, but with a few comments.

http://codereview.chromium.org/9741002/diff/19004/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/9741002/diff/19004/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:113: if
(action_iter->get()->id() == action_id)
These ids look like they're user-provided, but I don't see what ensures the ids
are unique.

http://codereview.chromium.org/9741002/diff/19004/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:130: // We don't handle
Create yet.
nit: can you write TODO(tbarzic):

http://codereview.chromium.org/9741002/diff/19004/chrome/common/extensions/fi...
File chrome/common/extensions/file_browser_handler.cc (right):

http://codereview.chromium.org/9741002/diff/19004/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_handler.cc:86: bool
FileBrowserHandler::CanCreate() const {
Does write access imply being able to create? Do you intend for this to be used
to check "has the create file access permission" or to check "can create files"?
(In other words, if I want to be able to both edit files and create new files,
what should I specify?)

http://codereview.chromium.org/9741002/diff/19004/chrome/common/extensions/fi...
File chrome/common/extensions/file_browser_handler.h (right):

http://codereview.chromium.org/9741002/diff/19004/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_handler.h:54: bool
AddFileAccessPermission(const std::string& permission_str);
Both these functions should have some documentation here: at the very least,
what they return.

http://codereview.chromium.org/9741002/diff/19004/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_handler.h:55: bool
InvalidateFileAccessPermissions();
I think it'd be clearer if it was called ValidateFileAccessPermissions.

[tbarzic, 2012-03-26 23:22:51]

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/browser/chro...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:113: if
(action_iter->get()->id() == action_id)
On 2012/03/26 22:42:58, Yoyo Zhou wrote:
> These ids look like they're user-provided, but I don't see what ensures the
ids
> are unique.

I don't think there is anything..

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:130: // We don't handle
Create yet.
On 2012/03/26 22:42:58, Yoyo Zhou wrote:
> nit: can you write TODO(tbarzic):

Done.

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/common/exten...
File chrome/common/extensions/file_browser_handler.cc (right):

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/common/exten...
chrome/common/extensions/file_browser_handler.cc:86: bool
FileBrowserHandler::CanCreate() const {
On 2012/03/26 22:42:58, Yoyo Zhou wrote:
> Does write access imply being able to create? Do you intend for this to be
used
> to check "has the create file access permission" or to check "can create
files"?
> (In other words, if I want to be able to both edit files and create new files,
> what should I specify?)

I plan to use this as check that the handler "has the create file access
permission".
The write permission could also imply create permission (in save as use case),
but I'm not completely sure how we're going to implement this yet (we may also
check that the handler has at least on "create" handler specified).

Maybe it would be good to rename this?

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/common/exten...
File chrome/common/extensions/file_browser_handler.h (right):

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/common/exten...
chrome/common/extensions/file_browser_handler.h:54: bool
AddFileAccessPermission(const std::string& permission_str);
On 2012/03/26 22:42:58, Yoyo Zhou wrote:
> Both these functions should have some documentation here: at the very least,
> what they return.

Done.

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/common/exten...
chrome/common/extensions/file_browser_handler.h:55: bool
InvalidateFileAccessPermissions();
On 2012/03/26 22:42:58, Yoyo Zhou wrote:
> I think it'd be clearer if it was called ValidateFileAccessPermissions.

Done.

[Yoyo Zhou, 2012-03-27 00:54:19]

LGTM

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/browser/chro...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/browser/chro...
chrome/browser/chromeos/extensions/file_handler_util.cc:113: if
(action_iter->get()->id() == action_id)
On 2012/03/26 23:22:51, tbarzic wrote:
> On 2012/03/26 22:42:58, Yoyo Zhou wrote:
> > These ids look like they're user-provided, but I don't see what ensures the
> ids
> > are unique.
> 
> I don't think there is anything..

So you may have unexpected results if the ids are not unique. But this is
probably not a big deal.

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/common/exten...
File chrome/common/extensions/file_browser_handler.cc (right):

https://chromiumcodereview.appspot.com/9741002/diff/19004/chrome/common/exten...
chrome/common/extensions/file_browser_handler.cc:86: bool
FileBrowserHandler::CanCreate() const {
On 2012/03/26 23:22:51, tbarzic wrote:
> On 2012/03/26 22:42:58, Yoyo Zhou wrote:
> > Does write access imply being able to create? Do you intend for this to be
> used
> > to check "has the create file access permission" or to check "can create
> files"?
> > (In other words, if I want to be able to both edit files and create new
files,
> > what should I specify?)
> 
> I plan to use this as check that the handler "has the create file access
> permission".
> The write permission could also imply create permission (in save as use case),
> but I'm not completely sure how we're going to implement this yet (we may also
> check that the handler has at least on "create" handler specified).
> 
> Maybe it would be good to rename this?

Yeah, my comment is along the lines of: the functionality might not match the
name, so either the name or the functionality should change.

[tbarzic, 2012-03-27 01:10:59]

Thanks for the review!

http://codereview.chromium.org/9741002/diff/19004/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/9741002/diff/19004/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:113: if
(action_iter->get()->id() == action_id)
On 2012/03/27 00:54:20, Yoyo Zhou wrote:
> On 2012/03/26 23:22:51, tbarzic wrote:
> > On 2012/03/26 22:42:58, Yoyo Zhou wrote:
> > > These ids look like they're user-provided, but I don't see what ensures
the
> > ids
> > > are unique.
> > 
> > I don't think there is anything..
> 
> So you may have unexpected results if the ids are not unique. But this is
> probably not a big deal.

Yeah.. I'll try to address this as I implement new way of specifying file
filters (part 2 of the design doc).. should not be too hard

http://codereview.chromium.org/9741002/diff/19004/chrome/common/extensions/fi...
File chrome/common/extensions/file_browser_handler.cc (right):

http://codereview.chromium.org/9741002/diff/19004/chrome/common/extensions/fi...
chrome/common/extensions/file_browser_handler.cc:86: bool
FileBrowserHandler::CanCreate() const {
On 2012/03/27 00:54:20, Yoyo Zhou wrote:
> On 2012/03/26 23:22:51, tbarzic wrote:
> > On 2012/03/26 22:42:58, Yoyo Zhou wrote:
> > > Does write access imply being able to create? Do you intend for this to be
> > used
> > > to check "has the create file access permission" or to check "can create
> > files"?
> > > (In other words, if I want to be able to both edit files and create new
> files,
> > > what should I specify?)
> > 
> > I plan to use this as check that the handler "has the create file access
> > permission".
> > The write permission could also imply create permission (in save as use
case),
> > but I'm not completely sure how we're going to implement this yet (we may
also
> > check that the handler has at least on "create" handler specified).
> > 
> > Maybe it would be good to rename this?
> 
> Yeah, my comment is along the lines of: the functionality might not match the
> name, so either the name or the functionality should change.

Done.