File system API: whitelist the Downloads dir on Chrome OS.

File system API: whitelist the Downloads dir on Chrome OS.

Due to crrev.com/203393 we now blacklist the user data dir in
DoCheckWritableFile(). However, on Chrome OS the user Downloads
folder (~/Downloads) is a subdirectory of the user data dir and
serves the role of local storage. This should be writable for
file system API.

BUG=246339
TEST=verified Downloads folder is writable by v2 apps on Chrome OS.

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

[hshi1, 2013-06-03 22:48:55]

benwells@ - PTAL (OWNER)
sammc@ - FYI

[hshi1, 2013-06-03 22:55:51]

Note that trybots are all failing now (infrastructure issues? bots failing with
credential errors), but I've verified this locally on Chrome OS device.

Also adding Matt for comments. I feel that the current blacklisting logic is not
flexible enough. Ideally we want to blacklist everything under the user data dir
except the Downloads folder.

[Matt Perry, 2013-06-03 23:04:33]

I assume that ChromeOS has other protections against someone overwriting a file
in the user-data-dir? If that's the case, this seems reasonable.

BTW, see http://dev.chromium.org/developers/contributing-code#TOC-GCL for an
explanation of the TEST= line.

[hshi1, 2013-06-03 23:15:47]

On 2013/06/03 23:04:33, Matt Perry wrote:
> I assume that ChromeOS has other protections against someone overwriting a
file
> in the user-data-dir? If that's the case, this seems reasonable.
> 
> BTW, see http://dev.chromium.org/developers/contributing-code#TOC-GCL for an
> explanation of the TEST= line.

Updated TEST= field.
On Chrome OS the user data dir (except Downloads/) is not accessible via the
Files app by design.

[hshi1, 2013-06-03 23:47:52]

PTAL Patch Set #2. I have decided to apply a whitelist specifically for the
Downloads directory on Chrome OS, so that we still restrict access to the rest
of the user data dir (such as profiles and extensions). Even though they are not
user accessible via UI, I do not want to unnecessarily weaken the existing
defence put in place.

Also +Satorux@ as a FYI.

[Sam McNally, 2013-06-04 01:09:00]

Please add a test for a file in a blacklisted and whitelisted path.

[hshi1, 2013-06-04 01:18:23]

On 2013/06/04 01:09:00, Sam McNally wrote:
> Please add a test for a file in a blacklisted and whitelisted path.

Sam, is Patch Set# 3 something you're looking for? Sorry I'm not very familiar
with this piece of code and I'm leaving office shortly, but it is a fairly bad
regression for Chrome OS, if possible can you please make changes as you desire.
Thanks.

[Sam McNally, 2013-06-04 01:25:58]

lgtm

https://codereview.chromium.org/15896035/diff/19001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/15896035/diff/19001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:465:
chrome::DIR_DEFAULT_DOWNLOADS_SAFE, test_file.DirName(), false));
Add
ASSERT_TRUE(PathService::OverrideAndCreateIfNeeded(
    chrome::DIR_USER_DATA, test_file.DirName(), false));

[hshi1, 2013-06-04 01:33:53]

https://codereview.chromium.org/15896035/diff/19001/chrome/browser/extensions...
File chrome/browser/extensions/api/file_system/file_system_apitest.cc (right):

https://codereview.chromium.org/15896035/diff/19001/chrome/browser/extensions...
chrome/browser/extensions/api/file_system/file_system_apitest.cc:465:
chrome::DIR_DEFAULT_DOWNLOADS_SAFE, test_file.DirName(), false));
On 2013/06/04 01:25:58, Sam McNally wrote:
> Add
> ASSERT_TRUE(PathService::OverrideAndCreateIfNeeded(
>     chrome::DIR_USER_DATA, test_file.DirName(), false));

Done.

[benwells, 2013-06-04 01:35:12]

lgtm

[benwells, 2013-06-04 01:35:27]

and thanks for fixing so quickly!

[commit-bot: I haz the power, 2013-06-04 01:36:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hshi@chromium.org/15896035/27001

[commit-bot: I haz the power, 2013-06-04 10:51:52]

Change committed as 203919