Deprecate MountPointProvider::IsAccessAllowed in favor of GetPermissionPolicy

Deprecate MountPointProvider::IsAccessAllowed in favor of GetPermissionPolicy

This deprecates:
- FileSystemMountPointProvider::IsAccessAllowed
- FileSystemMountPointProvider::IsRestrictedFileName

and merge the access checks used to be done by them into a single method:
FileSystemMountPointProvider::GetPermissionPolicy().

BUG=174550
TEST=CrosMountPointProviderTest.AccessPermissions, SandboxMountPointProviderTest.AccessPermissions

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

[kinuko, 2013-02-04 05:12:50]

Toni, I'm also thinking about removing access checks from
LocalFileSystemOperation::SetUp. Since your change will be affected by this
change can you also take a look at this change and let me know WDYT?

I think you'll be interested in the changes in following files:
webkit/chromeos/fileapi/cros_mount_point_provider.cc
webkit/fileapi/local_file_system_operation.cc

[tonibarzic, 2013-02-04 08:01:53]

I'll take a closer look tomorrow..

https://codereview.chromium.org/12193007/diff/17/webkit/chromeos/fileapi/cros...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/12193007/diff/17/webkit/chromeos/fileapi/cros...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:215:
DCHECK_EQ(fileapi::kFileSystemTypeDrive, url.type());
I think FILE_PERMISSION_USE_FILE_PERMISSIONS should br returned for drive file
system too

[kinuko, 2013-02-04 08:58:25]

Thanks!

https://codereview.chromium.org/12193007/diff/17/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/12193007/diff/17/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_handler_util.cc:664:
fileapi::FILE_PERMISSION_ALWAYS_DENY)) {
Maybe this change needs some more care too

https://codereview.chromium.org/12193007/diff/17/webkit/chromeos/fileapi/cros...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/12193007/diff/17/webkit/chromeos/fileapi/cros...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:215:
DCHECK_EQ(fileapi::kFileSystemTypeDrive, url.type());
On 2013/02/04 08:01:53, tonibarzic wrote:
> I think FILE_PERMISSION_USE_FILE_PERMISSIONS should br returned for drive file
> system too

Done.

[tbarzic, 2013-02-05 00:09:34]

ChromeOS part lgtm

https://codereview.chromium.org/12193007/diff/17/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/12193007/diff/17/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_handler_util.cc:664:
fileapi::FILE_PERMISSION_ALWAYS_DENY)) {
On 2013/02/04 08:58:25, kinuko wrote:
> Maybe this change needs some more care too

hmm, this should be ok.. to be honest I've started thinking about removing this
part of code and replacing it with something like:
CHECK(TheExtensionHasFileBrowserPrivatePermission(file_browser_id_), but that's
not much related to this cl.

[tbarzic, 2013-02-05 00:36:35]

https://codereview.chromium.org/12193007/diff/17001/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/12193007/diff/17001/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:180: permissions !=
fileapi::kReadFilePermissions) {
oh, I missed something:

this should be

if (url.type() == fileapi:kFileSystemTypeRestrictedNativeLocal &&
    (permissions & ~fileapi::kReadPermissions)) {
...
}

[kinuko, 2013-02-05 11:31:20]

https://codereview.chromium.org/12193007/diff/17001/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/12193007/diff/17001/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:180: permissions !=
fileapi::kReadFilePermissions) {
Good catch, fixed.

[kinuko, 2013-02-05 12:18:34]

Eric/Taiju, can you review?  We used to do some security check in
FileSystemOperation but the check has become obsolete and I want to cleanup the
part.

[tbarzic, 2013-02-05 19:37:28]

lgtm (again) :)

https://codereview.chromium.org/12193007/diff/34001/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/12193007/diff/34001/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:180: (permissions &~
fileapi::kReadFilePermissions)) {
personally, I would find " & ~x" more natural than " &~ x" given that ~ is
actually an unary operator on x and & operator takes ~x as an argument.

[ericu, 2013-02-05 21:51:57]

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:664:
fileapi::FILE_PERMISSION_ALWAYS_DENY)) {
This seems potentially more fragile than asking IsAccessAllowed, which is nicely
intuitive.  What if this returns FILE_PERMISSION_USE_FILESYSTEM_PERMISSION or
FILE_PERMISSION_USE_FILE_PERMISSION?  Then it's OK?  That's far less obvious.

I'd much rather pass in the required permission(s) and get a boolean response
than get an enum response and have to check it...and then worry about whether
the check should be == or !=.

https://codereview.chromium.org/12193007/diff/34001/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/12193007/diff/34001/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:180: (permissions &~
fileapi::kReadFilePermissions)) {
On 2013/02/05 19:37:28, tbarzic wrote:
> personally, I would find " & ~x" more natural than " &~ x" given that ~ is
> actually an unary operator on x and & operator takes ~x as an argument.


+1.  However, wouldn't != here be appropriate instead of "& ~"?  Should
permissions ever be 0?

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/isolated_m...
File webkit/fileapi/isolated_mount_point_provider.cc (right):

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/isolated_m...
webkit/fileapi/isolated_mount_point_provider.cc:117: if (permissions &~
fileapi::kReadFilePermissions)
Same again.

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/sandbox_mo...
File webkit/fileapi/sandbox_mount_point_provider.cc (right):

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/sandbox_mo...
webkit/fileapi/sandbox_mount_point_provider.cc:250: (permissions &~
kReadFilePermissions))
Ditto.

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/sandbox_mo...
File webkit/fileapi/sandbox_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/sandbox_mo...
webkit/fileapi/sandbox_mount_point_provider_unittest.cc:182:
kCreateFilePermissions));
I'd like to keep around FileSystemMountPointProviderTest.IsRestrictedName, which
tries to create a bunch of files illegal on Windows, and also tests that we
properly restrict "/" in filenames and "\\" everywhere.  Those have caught
multiple platform-specific bugs.

[tbarzic, 2013-02-05 22:21:25]

https://codereview.chromium.org/12193007/diff/34001/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/12193007/diff/34001/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:180: (permissions &~
fileapi::kReadFilePermissions)) {
On 2013/02/05 21:51:57, ericu wrote:
> On 2013/02/05 19:37:28, tbarzic wrote:
> > personally, I would find " & ~x" more natural than " &~ x" given that ~ is
> > actually an unary operator on x and & operator takes ~x as an argument.
> 
> 
> +1.  However, wouldn't != here be appropriate instead of "& ~"?  Should
> permissions ever be 0?

!= wouldn't work if |permissions| has (strict) subset of permissions allowed by
fileapi::kReadFilePermissions.

[ericu, 2013-02-05 22:31:41]

https://codereview.chromium.org/12193007/diff/34001/webkit/chromeos/fileapi/c...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

https://codereview.chromium.org/12193007/diff/34001/webkit/chromeos/fileapi/c...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:180: (permissions &~
fileapi::kReadFilePermissions)) {
On 2013/02/05 22:21:25, tbarzic wrote:
> On 2013/02/05 21:51:57, ericu wrote:
> > On 2013/02/05 19:37:28, tbarzic wrote:
> > > personally, I would find " & ~x" more natural than " &~ x" given that ~ is
> > > actually an unary operator on x and & operator takes ~x as an argument.
> > 
> > 
> > +1.  However, wouldn't != here be appropriate instead of "& ~"?  Should
> > permissions ever be 0?
> 
> != wouldn't work if |permissions| has (strict) subset of permissions allowed
by
> fileapi::kReadFilePermissions.

Ah, my bad.  I was thinking it was a single bit, not a mask.  That's fine, then
[except that I agree about the formatting change].

[kinuko, 2013-02-06 03:36:17]

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:664:
fileapi::FILE_PERMISSION_ALWAYS_DENY)) {
On 2013/02/05 21:51:57, ericu wrote:
> This seems potentially more fragile than asking IsAccessAllowed, which is
nicely
> intuitive.  What if this returns FILE_PERMISSION_USE_FILESYSTEM_PERMISSION or
> FILE_PERMISSION_USE_FILE_PERMISSION?  Then it's OK?  That's far less obvious.

I think actually we should also check ChildProcessSecurityPolicy here based on
the returned policy but not fully sure. Toni, what do you think?

As for &~ notation I'll change in the next patch.

> I'd much rather pass in the required permission(s) and get a boolean response
> than get an enum response and have to check it...and then worry about whether
> the check should be == or !=.

[tonibarzic, 2013-02-06 06:20:17]

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:664:
fileapi::FILE_PERMISSION_ALWAYS_DENY)) {
On 2013/02/06 03:36:17, kinuko wrote:
> On 2013/02/05 21:51:57, ericu wrote:
> > This seems potentially more fragile than asking IsAccessAllowed, which is
> nicely
> > intuitive.  What if this returns FILE_PERMISSION_USE_FILESYSTEM_PERMISSION
or
> > FILE_PERMISSION_USE_FILE_PERMISSION?  Then it's OK?  That's far less
obvious.
> 
> I think actually we should also check ChildProcessSecurityPolicy here based on
> the returned policy but not fully sure. Toni, what do you think?
> 
> As for &~ notation I'll change in the next patch.
> 
> > I'd much rather pass in the required permission(s) and get a boolean
response
> > than get an enum response and have to check it...and then worry about
whether
> > the check should be == or !=.
> 

yeah, checking ChildProcessSecurityPolicy would probably be resonable.

another thing I was thinking about is to check that the extension with id
file_browser_id has fileBrowserPrivate permission (which would apply that it has
full access permissions) and that the origin matches the extensions base url .
What do you think about this option?

[kinuko, 2013-02-06 09:24:50]

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:664:
fileapi::FILE_PERMISSION_ALWAYS_DENY)) {
On 2013/02/06 06:20:17, tonibarzic wrote:
> On 2013/02/06 03:36:17, kinuko wrote:
> > On 2013/02/05 21:51:57, ericu wrote:
> > > This seems potentially more fragile than asking IsAccessAllowed, which is
> > nicely
> > > intuitive.  What if this returns FILE_PERMISSION_USE_FILESYSTEM_PERMISSION
> or
> > > FILE_PERMISSION_USE_FILE_PERMISSION?  Then it's OK?  That's far less
> obvious.
> > 
> > I think actually we should also check ChildProcessSecurityPolicy here based
on
> > the returned policy but not fully sure. Toni, what do you think?
> > 
> > As for &~ notation I'll change in the next patch.
> > 
> > > I'd much rather pass in the required permission(s) and get a boolean
> response
> > > than get an enum response and have to check it...and then worry about
> whether
> > > the check should be == or !=.
> > 
> 
> yeah, checking ChildProcessSecurityPolicy would probably be resonable.
> 
> another thing I was thinking about is to check that the extension with id
> file_browser_id has fileBrowserPrivate permission (which would apply that it
has
> full access permissions) and that the origin matches the extensions base url .
> What do you think about this option?

Yes, that sounds reasonable to me too. My intention was to have the same
method/steps whenever we want to apply regular permission check, so if this
isn't the case I can leave it.

How would you like me to do for this part in this patch? I can:
1) revive IsAccessAllowed() method (maybe exposed only on
ExternalMountPointProvider) and call it here, or
2) leave this line as is with a specific TODO comment

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/sandbox_mo...
File webkit/fileapi/sandbox_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/sandbox_mo...
webkit/fileapi/sandbox_mount_point_provider_unittest.cc:182:
kCreateFilePermissions));
On 2013/02/05 21:51:57, ericu wrote:
> I'd like to keep around FileSystemMountPointProviderTest.IsRestrictedName,
which
> tries to create a bunch of files illegal on Windows, and also tests that we
> properly restrict "/" in filenames and "\\" everywhere.  Those have caught
> multiple platform-specific bugs.

They were important when we used to restrict them, but I think we're ready to
deprecate them.  The code we have today only performs simple string-matching
that has nothing to do with platform-specific illegal filenames, and as for '/'
and '\\' they're normalize in FileSystemURL constructor so the test result
wouldn't be as we expect.  Instead we have layout tests for all of those cases
which run in content_browsertests (thanks to your change) so that we still can
quickly spot problems.

[kinuko, 2013-02-06 09:31:51]

Updated the patch, I think it looks better. PTAL thanks,

1. Revived IsAccessAllowed() method for now (only on ExternalMountPointProvider)
and reverted the change in
chrome/browser/chromeos/extensions/file_handler_util.cc.
2. Replaced all "a &~ b" with "a & ~b".

[ericu, 2013-02-11 22:29:17]

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:664:
fileapi::FILE_PERMISSION_ALWAYS_DENY)) {
On 2013/02/06 09:24:50, kinuko wrote:
> On 2013/02/06 06:20:17, tonibarzic wrote:
> > On 2013/02/06 03:36:17, kinuko wrote:
> > > On 2013/02/05 21:51:57, ericu wrote:
> > > > This seems potentially more fragile than asking IsAccessAllowed, which
is
> > > nicely
> > > > intuitive.  What if this returns
FILE_PERMISSION_USE_FILESYSTEM_PERMISSION
> > or
> > > > FILE_PERMISSION_USE_FILE_PERMISSION?  Then it's OK?  That's far less
> > obvious.
> > > 
> > > I think actually we should also check ChildProcessSecurityPolicy here
based
> on
> > > the returned policy but not fully sure. Toni, what do you think?
> > > 
> > > As for &~ notation I'll change in the next patch.
> > > 
> > > > I'd much rather pass in the required permission(s) and get a boolean
> > response
> > > > than get an enum response and have to check it...and then worry about
> > whether
> > > > the check should be == or !=.
> > > 
> > 
> > yeah, checking ChildProcessSecurityPolicy would probably be resonable.
> > 
> > another thing I was thinking about is to check that the extension with id
> > file_browser_id has fileBrowserPrivate permission (which would apply that it
> has
> > full access permissions) and that the origin matches the extensions base url
.
> > What do you think about this option?
> 
> Yes, that sounds reasonable to me too. My intention was to have the same
> method/steps whenever we want to apply regular permission check, so if this
> isn't the case I can leave it.
> 
> How would you like me to do for this part in this patch? I can:
> 1) revive IsAccessAllowed() method (maybe exposed only on
> ExternalMountPointProvider) and call it here, or
> 2) leave this line as is with a specific TODO comment

I don't know about this specific case vs. others, but if we're deprecating a
simple boolean method in favor of one that returns a number of responses, I
think we're making the code more error-prone.  And if we're leaving both in
there, I think it's even worse.  Do I call IsAccessAllowed?  CanHandleURL? 
ChildProcessSecurityPolicy?  GetPermissionPolicy?

Is there a way to make a single function [or at least no more than two] that
will subsume the others and return a boolean?  That's what we should be using in
most cases.

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/sandbox_mo...
File webkit/fileapi/sandbox_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/sandbox_mo...
webkit/fileapi/sandbox_mount_point_provider_unittest.cc:182:
kCreateFilePermissions));
On 2013/02/06 09:24:50, kinuko wrote:
> On 2013/02/05 21:51:57, ericu wrote:
> > I'd like to keep around FileSystemMountPointProviderTest.IsRestrictedName,
> which
> > tries to create a bunch of files illegal on Windows, and also tests that we
> > properly restrict "/" in filenames and "\\" everywhere.  Those have caught
> > multiple platform-specific bugs.
> 
> They were important when we used to restrict them, but I think we're ready to
> deprecate them.  The code we have today only performs simple string-matching
> that has nothing to do with platform-specific illegal filenames, and as for
'/'
> and '\\' they're normalize in FileSystemURL constructor so the test result
> wouldn't be as we expect.  Instead we have layout tests for all of those cases
> which run in content_browsertests (thanks to your change) so that we still can
> quickly spot problems.

The massive flakiness on some of our layout tests, which I fixed just a couple
of months ago, was due to us accidentally calling a FilePath method on a
filesystem path.  It only showed up when the path started with e.g. "c:", on
Windows.  That test passed, but then killed the next few tests due to leftover
files that were hard to clean up.  These kinds of tests are really useful for
those kinds of bugs.

We probably don't need all of them, but at least keep the
looks-like-a-drive-letter test and a handful of others.

https://codereview.chromium.org/12193007/diff/23019/webkit/fileapi/sandbox_mo...
File webkit/fileapi/sandbox_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/12193007/diff/23019/webkit/fileapi/sandbox_mo...
webkit/fileapi/sandbox_mount_point_provider_unittest.cc:156: // Create access
with ristricted name should be disallowed.
typo: restricted

[kinuko, 2013-02-12 08:19:51]

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

https://codereview.chromium.org/12193007/diff/34001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_handler_util.cc:664:
fileapi::FILE_PERMISSION_ALWAYS_DENY)) {
On 2013/02/11 22:29:17, ericu wrote:
> On 2013/02/06 09:24:50, kinuko wrote:
> > On 2013/02/06 06:20:17, tonibarzic wrote:
> > > On 2013/02/06 03:36:17, kinuko wrote:
> > > > On 2013/02/05 21:51:57, ericu wrote:
> > > > > This seems potentially more fragile than asking IsAccessAllowed, which
> is
> > > > nicely
> > > > > intuitive.  What if this returns
> FILE_PERMISSION_USE_FILESYSTEM_PERMISSION
> > > or
> > > > > FILE_PERMISSION_USE_FILE_PERMISSION?  Then it's OK?  That's far less
> > > obvious.
> > > > 
> > > > I think actually we should also check ChildProcessSecurityPolicy here
> based
> > on
> > > > the returned policy but not fully sure. Toni, what do you think?
> > > > 
> > > > As for &~ notation I'll change in the next patch.
> > > > 
> > > > > I'd much rather pass in the required permission(s) and get a boolean
> > > response
> > > > > than get an enum response and have to check it...and then worry about
> > > whether
> > > > > the check should be == or !=.
> > > > 
> > > 
> > > yeah, checking ChildProcessSecurityPolicy would probably be resonable.
> > > 
> > > another thing I was thinking about is to check that the extension with id
> > > file_browser_id has fileBrowserPrivate permission (which would apply that
it
> > has
> > > full access permissions) and that the origin matches the extensions base
url
> .
> > > What do you think about this option?
> > 
> > Yes, that sounds reasonable to me too. My intention was to have the same
> > method/steps whenever we want to apply regular permission check, so if this
> > isn't the case I can leave it.
> > 
> > How would you like me to do for this part in this patch? I can:
> > 1) revive IsAccessAllowed() method (maybe exposed only on
> > ExternalMountPointProvider) and call it here, or
> > 2) leave this line as is with a specific TODO comment
> 
> I don't know about this specific case vs. others, but if we're deprecating a
> simple boolean method in favor of one that returns a number of responses, I
> think we're making the code more error-prone.  And if we're leaving both in
> there, I think it's even worse.  Do I call IsAccessAllowed?  CanHandleURL? 
> ChildProcessSecurityPolicy?  GetPermissionPolicy?
> Is there a way to make a single function [or at least no more than two] that
> will subsume the others and return a boolean?  That's what we should be using
in
> most cases.

The primary intention of this series of changes are to have the single function
to do the permission check for fileapi files, and I'm planning to add such
function after this refactoring.  Why I don't add the function in this patch
(and I can't add the single function at FileSystemMountPointProvider) is
layering.

As for the three methods let me try to clarify:

* ExternalMountPointProvider::IsAccessAllowed() is cros- (or
ExternalMountPointProvider-) specifi function that is only exposed to
ExternalMPP-specific handlers.  Other code that needs to deal with generic
fileapi layer does not (need to) know about this method.  This function may need
more cleanup, but I wanted to leave it to cros folks so I chose to leave the
function as is in this patch.

* FileSystemMountPointProvider::GetPermissionPolicy() is the single function
that is to be exposed from fileapi layer and should be called for permission
checks by all modules that deal with fileapi code.

* ChildProcessSecurityPolicy is not the resident of webkit/ but in the content/
layer, and we should perform further security check at this layer too when
fileapi layer is called from content/.  (This is a bit awkward and when we merge
webkit/ into content/ this separation should be cleaned up again but we still
have the two distinct layers for now)

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/sandbox_mo...
File webkit/fileapi/sandbox_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/12193007/diff/34001/webkit/fileapi/sandbox_mo...
webkit/fileapi/sandbox_mount_point_provider_unittest.cc:182:
kCreateFilePermissions));
On 2013/02/11 22:29:17, ericu wrote:
> On 2013/02/06 09:24:50, kinuko wrote:
> > On 2013/02/05 21:51:57, ericu wrote:
> > > I'd like to keep around FileSystemMountPointProviderTest.IsRestrictedName,
> > which
> > > tries to create a bunch of files illegal on Windows, and also tests that
we
> > > properly restrict "/" in filenames and "\\" everywhere.  Those have caught
> > > multiple platform-specific bugs.
> > 
> > They were important when we used to restrict them, but I think we're ready
to
> > deprecate them.  The code we have today only performs simple string-matching
> > that has nothing to do with platform-specific illegal filenames, and as for
> '/'
> > and '\\' they're normalize in FileSystemURL constructor so the test result
> > wouldn't be as we expect.  Instead we have layout tests for all of those
cases
> > which run in content_browsertests (thanks to your change) so that we still
can
> > quickly spot problems.
> 
> The massive flakiness on some of our layout tests, which I fixed just a couple
> of months ago, was due to us accidentally calling a FilePath method on a
> filesystem path.  It only showed up when the path started with e.g. "c:", on
> Windows.  That test passed, but then killed the next few tests due to leftover
> files that were hard to clean up.  These kinds of tests are really useful for
> those kinds of bugs.
> 
> We probably don't need all of them, but at least keep the
> looks-like-a-drive-letter test and a handful of others.

Ok, I added a test for that.

[ericu, 2013-02-13 01:33:15]

OK, that makes sense.  Thanks for the new test, too.

[ericu, 2013-02-13 01:45:55]

LGTM with nits.

https://codereview.chromium.org/12193007/diff/20007/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_mount_point_provider.h (right):

https://codereview.chromium.org/12193007/diff/20007/webkit/fileapi/file_syste...
webkit/fileapi/file_system_mount_point_provider.h:129: virtual bool
IsAccessAllowed(const fileapi::FileSystemURL& url) const = 0;
Given the other methods, please add a comment explaining how this one differs. 
"is allowed" by whom, and if this should override other checks or if it's an
additional requirement.

If this will only ever be used by implementers of this interface, we could also
make it protected to clarify that.

https://codereview.chromium.org/12193007/diff/20007/webkit/fileapi/sandbox_mo...
File webkit/fileapi/sandbox_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/12193007/diff/20007/webkit/fileapi/sandbox_mo...
webkit/fileapi/sandbox_mount_point_provider_unittest.cc:187:
kCreateFilePermissions));
Missing: test for modifying the root directory.

[kinuko, 2013-02-13 03:36:30]

Thanks!

https://codereview.chromium.org/12193007/diff/20007/webkit/fileapi/file_syste...
File webkit/fileapi/file_system_mount_point_provider.h (right):

https://codereview.chromium.org/12193007/diff/20007/webkit/fileapi/file_syste...
webkit/fileapi/file_system_mount_point_provider.h:129: virtual bool
IsAccessAllowed(const fileapi::FileSystemURL& url) const = 0;
On 2013/02/13 01:45:56, ericu wrote:
> Given the other methods, please add a comment explaining how this one differs.

> "is allowed" by whom, and if this should override other checks or if it's an
> additional requirement.
> 
> If this will only ever be used by implementers of this interface, we could
also
> make it protected to clarify that.

Done.

https://codereview.chromium.org/12193007/diff/20007/webkit/fileapi/sandbox_mo...
File webkit/fileapi/sandbox_mount_point_provider_unittest.cc (right):

https://codereview.chromium.org/12193007/diff/20007/webkit/fileapi/sandbox_mo...
webkit/fileapi/sandbox_mount_point_provider_unittest.cc:187:
kCreateFilePermissions));
On 2013/02/13 01:45:56, ericu wrote:
> Missing: test for modifying the root directory.

Done.