Deprecate MountPointProvider::IsAccessAllowed in favor of GetPermissionPolicy

chrome/browser/chromeos/extensions/file_handler_util.cc

Index: chrome/browser/chromeos/extensions/file_handler_util.cc
diff --git a/chrome/browser/chromeos/extensions/file_handler_util.cc b/chrome/browser/chromeos/extensions/file_handler_util.cc
index b6b56e4c97f31656e1a6116e5744107320fc8d2a..a688f78a9a9d2c25f67fa69f2ade85fbade1272e 100644
--- a/chrome/browser/chromeos/extensions/file_handler_util.cc
+++ b/chrome/browser/chromeos/extensions/file_handler_util.cc
@@ -659,7 +659,9 @@ bool FileTaskExecutor::FileBrowserHasAccessPermissionForFiles(
       return false;
 
     if (!chromeos::CrosMountPointProvider::CanHandleURL(files[i]) ||
-        !external_provider->IsAccessAllowed(files[i])) {
+        (external_provider->GetPermissionPolicy(
+            files[i], fileapi::kReadFilePermissions) ==
+            fileapi::FILE_PERMISSION_ALWAYS_DENY)) {

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

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 !=.

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

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.

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

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]

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

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

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.

[kinuko, 2013/02/12 08:19:52]

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)

       return false;
     }
   }