Grant file access permissions for cached file paths to file browsers/handlers.

Grant file access permissions for cached file paths to file browsers/handlers.

File browser extensions are given access permissions for whole pinned and tmp cache dirs.

File handlers are given permissions during runtime for files for wich their task is being executed.
To simplify things they are given permissions for all possible file's cache paths.

Since we check permissions for cache paths only when performing entry.file(), read-only permissions
should be sufficient.

BUG=chromium-os:27926
TEST=manual. browser_tests:*FileSystemExtension*

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

[tbarzic, 2012-03-22 22:14:45]

https://chromiumcodereview.appspot.com/9808023/diff/6001/chrome/chrome_tests....
File chrome/chrome_tests.gypi (left):

https://chromiumcodereview.appspot.com/9808023/diff/6001/chrome/chrome_tests....
chrome/chrome_tests.gypi:2988:
'browser/chromeos/extensions/file_browser_private_apitest.cc',
whole browser/chromeos is already excluded, so we don't need these two lines..

[tbarzic, 2012-03-22 22:18:28]

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/external_filesystem_apitest.cc (right):

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/external_filesystem_apitest.cc:141:
ASSERT_TRUE(test_mount_point_.CreateUniqueTempDirUnderPath(tmp_dir_path));
I randomized this to make sure the cache paths don't end up under mount point
path (for not yet implemented test)

[zel, 2012-03-22 22:21:52]

This will work for file manager only. You also need to grant such permissions to
all file handler extensions as we execute tasks - imagine how an external file
editor might work.

https://chromiumcodereview.appspot.com/9808023/diff/6001/chrome/browser/chrom...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://chromiumcodereview.appspot.com/9808023/diff/6001/chrome/browser/chrom...
chrome/browser/chromeos/extensions/file_handler_util.cc:189:
std::vector<std::pair<FilePath, int> > cache_paths;
please move the block that gives you all file names to gdata_util.cc - that
logic should not be part of extensions layer. - L179-206

[satorux1, 2012-03-22 22:28:29]

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1048:
file_handler_util::GetReadOnlyPermissions());
PinnedDirectory only contains symlinks, and the real files stay in
PersistentDirectory. shouldn't we grant permissions for the persistent
directory?

Please add GDataFileSystem::GetGDataCachePersistentDirectory() if needed.

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_handler_util.cc:172: int pid,
indentation is off

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_handler_util.cc:453:
file_util::IsLink(final_file_path) ||
Why are we checking IsLink()? we are checking file_info.is_symbolic_link below.
Or we can keep it here, and stop checking file_info.is_symbolic_link

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_handler_util.h:21: int
GetReadOnlyPermissions();
While you are at it, please add some comments to these functions

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:325: CachedFileOrigin
file_orign) const = 0;
You should probably update mock_gdata_file_system.h

[zel, 2012-03-22 22:32:54]

ok, works for handlers too... need test for hanlders

[tonibarzic, 2012-03-22 23:38:20]

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:1048:
file_handler_util::GetReadOnlyPermissions());
On 2012/03/22 22:28:29, satorux1 wrote:
> PinnedDirectory only contains symlinks, and the real files stay in
> PersistentDirectory. shouldn't we grant permissions for the persistent
> directory?
> 
> Please add GDataFileSystem::GetGDataCachePersistentDirectory() if needed.

Yeah, my bad... that should have been Persistent..

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_handler_util.cc:172: int pid,
On 2012/03/22 22:28:29, satorux1 wrote:
> indentation is off

Done.

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_handler_util.cc:189:
std::vector<std::pair<FilePath, int> > cache_paths;
On 2012/03/22 22:21:53, zel wrote:
> please move the block that gives you all file names to gdata_util.cc - that
> logic should not be part of extensions layer. - L179-206

Done.

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_handler_util.cc:453:
file_util::IsLink(final_file_path) ||
On 2012/03/22 22:28:29, satorux1 wrote:
> Why are we checking IsLink()? we are checking file_info.is_symbolic_link
below.
> Or we can keep it here, and stop checking file_info.is_symbolic_link

I'm not sure why we check that, but removed symbolic link check

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_handler_util.h (right):

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_handler_util.h:21: int
GetReadOnlyPermissions();
On 2012/03/22 22:28:29, satorux1 wrote:
> While you are at it, please add some comments to these functions

Done.

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/9808023/diff/6001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:325: CachedFileOrigin
file_orign) const = 0;
On 2012/03/22 22:28:29, satorux1 wrote:
> You should probably update mock_gdata_file_system.h

Yeah, I usually remember this when try job fails :/

Done.

[zel, 2012-03-23 02:32:44]

LGTM


please add additional handler tests to another CL