Cleanup file permission check code in FileAPIMessageFilter

- Move detailed permission check code into each MountPointProvider
- Add common enum for fileapi permission policy (webkit/fileapi/file_permission_policy.{h,cc})

BUG=none, cleanup only
TEST=content_browsertests:FileSystemLayoutTest.*,browser_tests:FileBrowser*)

Review URL: https://codereview.chromium.org/11804005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@176014 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/fileapi/fileapi_message_filter.cc

@@ -27,6 +27,7 @@
 #include "webkit/blob/blob_storage_controller.h"
 #include "webkit/blob/shareable_file_reference.h"
 #include "webkit/fileapi/file_observers.h"
+#include "webkit/fileapi/file_permission_policy.h"
 #include "webkit/fileapi/file_system_context.h"
 #include "webkit/fileapi/file_system_types.h"
 #include "webkit/fileapi/file_system_util.h"
@@ -45,29 +46,8 @@ using webkit_blob::BlobData;
 using webkit_blob::BlobStorageController;
 
 namespace content {
-namespace {
-
-const int kReadFilePermissions = base::PLATFORM_FILE_OPEN |
-                                 base::PLATFORM_FILE_READ |
-                                 base::PLATFORM_FILE_EXCLUSIVE_READ |
-                                 base::PLATFORM_FILE_ASYNC;
-
-const int kWriteFilePermissions = base::PLATFORM_FILE_OPEN |
-                                  base::PLATFORM_FILE_WRITE |
-                                  base::PLATFORM_FILE_EXCLUSIVE_WRITE |
-                                  base::PLATFORM_FILE_ASYNC |
-                                  base::PLATFORM_FILE_WRITE_ATTRIBUTES;
 
-const int kCreateFilePermissions = base::PLATFORM_FILE_CREATE;
-
-const int kOpenFilePermissions = base::PLATFORM_FILE_CREATE |
-                                 base::PLATFORM_FILE_OPEN_ALWAYS |
-                                 base::PLATFORM_FILE_CREATE_ALWAYS |
-                                 base::PLATFORM_FILE_OPEN_TRUNCATED |
-                                 base::PLATFORM_FILE_WRITE |
-                                 base::PLATFORM_FILE_EXCLUSIVE_WRITE |
-                                 base::PLATFORM_FILE_DELETE_ON_CLOSE |
-                                 base::PLATFORM_FILE_WRITE_ATTRIBUTES;
+namespace {
 
 void RevokeFilePermission(int child_id, const FilePath& path) {
   ChildProcessSecurityPolicyImpl::GetInstance()->RevokeAllPermissionsForFile(
@@ -229,9 +209,11 @@ void FileAPIMessageFilter::OnMove(
   base::PlatformFileError error;
   FileSystemURL src_url(src_path);
   FileSystemURL dest_url(dest_path);
-  const int src_permissions = kReadFilePermissions | kWriteFilePermissions;
+  const int src_permissions =
+      fileapi::kReadFilePermissions | fileapi::kWriteFilePermissions;
   if (!HasPermissionsForFile(src_url, src_permissions, &error) ||
-      !HasPermissionsForFile(dest_url, kCreateFilePermissions, &error)) {
+      !HasPermissionsForFile(
+          dest_url, fileapi::kCreateFilePermissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
     return;
   }
@@ -250,8 +232,9 @@ void FileAPIMessageFilter::OnCopy(
   base::PlatformFileError error;
   FileSystemURL src_url(src_path);
   FileSystemURL dest_url(dest_path);
-  if (!HasPermissionsForFile(src_url, kReadFilePermissions, &error) ||
-      !HasPermissionsForFile(dest_url, kCreateFilePermissions, &error)) {
+  if (!HasPermissionsForFile(src_url, fileapi::kReadFilePermissions, &error) ||
+      !HasPermissionsForFile(
+          dest_url, fileapi::kCreateFilePermissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
     return;
   }
@@ -269,7 +252,7 @@ void FileAPIMessageFilter::OnRemove(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   base::PlatformFileError error;
   FileSystemURL url(path);
-  if (!HasPermissionsForFile(url, kWriteFilePermissions, &error)) {
+  if (!HasPermissionsForFile(url, fileapi::kWriteFilePermissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
     return;
   }
@@ -287,7 +270,7 @@ void FileAPIMessageFilter::OnReadMetadata(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   base::PlatformFileError error;
   FileSystemURL url(path);
-  if (!HasPermissionsForFile(url, kReadFilePermissions, &error)) {
+  if (!HasPermissionsForFile(url, fileapi::kReadFilePermissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
     return;
   }
@@ -306,7 +289,7 @@ void FileAPIMessageFilter::OnCreate(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   base::PlatformFileError error;
   FileSystemURL url(path);
-  if (!HasPermissionsForFile(url, kCreateFilePermissions, &error)) {
+  if (!HasPermissionsForFile(url, fileapi::kCreateFilePermissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
     return;
   }
@@ -330,7 +313,7 @@ void FileAPIMessageFilter::OnExists(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   base::PlatformFileError error;
   FileSystemURL url(path);
-  if (!HasPermissionsForFile(url, kReadFilePermissions, &error)) {
+  if (!HasPermissionsForFile(url, fileapi::kReadFilePermissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
     return;
   }
@@ -354,7 +337,7 @@ void FileAPIMessageFilter::OnReadDirectory(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   base::PlatformFileError error;
   FileSystemURL url(path);
-  if (!HasPermissionsForFile(url, kReadFilePermissions, &error)) {
+  if (!HasPermissionsForFile(url, fileapi::kReadFilePermissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
     return;
   }
@@ -381,7 +364,7 @@ void FileAPIMessageFilter::OnWrite(
 
   FileSystemURL url(path);
   base::PlatformFileError error;
-  if (!HasPermissionsForFile(url, kWriteFilePermissions, &error)) {
+  if (!HasPermissionsForFile(url, fileapi::kWriteFilePermissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
     return;
   }
@@ -400,7 +383,7 @@ void FileAPIMessageFilter::OnTruncate(
     int64 length) {
   base::PlatformFileError error;
   FileSystemURL url(path);
-  if (!HasPermissionsForFile(url, kWriteFilePermissions, &error)) {
+  if (!HasPermissionsForFile(url, fileapi::kWriteFilePermissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
     return;
   }
@@ -421,7 +404,7 @@ void FileAPIMessageFilter::OnTouchFile(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   FileSystemURL url(path);
   base::PlatformFileError error;
-  if (!HasPermissionsForFile(url, kCreateFilePermissions, &error)) {
+  if (!HasPermissionsForFile(url, fileapi::kCreateFilePermissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
     return;
   }
@@ -456,7 +439,7 @@ void FileAPIMessageFilter::OnOpenFile(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   base::PlatformFileError error;
   const int open_permissions = base::PLATFORM_FILE_OPEN |
-                               (file_flags & kOpenFilePermissions);
+                               (file_flags & fileapi::kOpenFilePermissions);
   FileSystemURL url(path);
   if (!HasPermissionsForFile(url, open_permissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
@@ -528,7 +511,7 @@ void FileAPIMessageFilter::OnSyncGetPlatformPath(
   // which means roughly same as the renderer is allowed to get the platform
   // path to the file).
   base::PlatformFileError error;
-  if (!HasPermissionsForFile(url, kReadFilePermissions, &error))
+  if (!HasPermissionsForFile(url, fileapi::kReadFilePermissions, &error))
     return;
 
   // This is called only by pepper plugin as of writing to get the
@@ -567,7 +550,7 @@ void FileAPIMessageFilter::OnCreateSnapshotFile(
   // called when the renderer is about to create a new File object
   // (for reading the file).
   base::PlatformFileError error;
-  if (!HasPermissionsForFile(url, kReadFilePermissions, &error)) {
+  if (!HasPermissionsForFile(url, fileapi::kReadFilePermissions, &error)) {
     Send(new FileSystemMsg_DidFail(request_id, error));
     return;
   }
@@ -832,47 +815,31 @@ bool FileAPIMessageFilter::HasPermissionsForFile(
   ChildProcessSecurityPolicyImpl* policy =
       ChildProcessSecurityPolicyImpl::GetInstance();
 
-  // Special handling for filesystems whose mount type is isolated.
-  // (See ChildProcessSecurityPolicy::GrantReadFileSystem for more
-  // details about access permission for isolated filesystem.)
-  if (url.mount_type() == fileapi::kFileSystemTypeIsolated) {
-    // The root directory of the dragged filesystem is read-only.
-    if (url.type() == fileapi::kFileSystemTypeDragged && url.path().empty()) {
-      if (permissions != kReadFilePermissions) {
+  switch (mount_point_provider->GetPermissionPolicy(url, permissions)) {
+    case fileapi::FILE_PERMISSION_ALWAYS_DENY:
       *error = base::PLATFORM_FILE_ERROR_SECURITY;
       return false;
-      }
+    case fileapi::FILE_PERMISSION_ALWAYS_ALLOW:
+      CHECK(mount_point_provider == context_->sandbox_provider());
       return true;
+    case fileapi::FILE_PERMISSION_USE_FILE_PERMISSION: {
+      const bool success = policy->HasPermissionsForFile(
+          process_id_, url.path(), permissions);
+      if (!success)
+        *error = base::PLATFORM_FILE_ERROR_SECURITY;
+      return success;
     }
-
-    // Access permission to the file system overrides the file permission
-    // (if and only if they accessed via an isolated file system).
-    bool success = policy->HasPermissionsForFileSystem(
+    case fileapi::FILE_PERMISSION_USE_FILESYSTEM_PERMISSION: {
+      const bool success = policy->HasPermissionsForFileSystem(
           process_id_, url.filesystem_id(), permissions);
       if (!success)
         *error = base::PLATFORM_FILE_ERROR_SECURITY;
       return success;
     }
-
-  if (fileapi::SandboxMountPointProvider::CanHandleType(url.type())) {
-    // Sandboxed file system permissions should be implicitly granted.
-    // (And the application should not be given direct permission to the actual
-    // data directory in the sandboxed area.)
-    CHECK(mount_point_provider == context_->sandbox_provider());
-    return true;
   }
-
-  file_path = mount_point_provider->GetPathForPermissionsCheck(url.path());
-  if (file_path.empty()) {
+  NOTREACHED();
   *error = base::PLATFORM_FILE_ERROR_SECURITY;
   return false;
-  }
-
-  bool success = policy->HasPermissionsForFile(
-      process_id_, file_path, permissions);
-  if (!success)
-    *error = base::PLATFORM_FILE_ERROR_SECURITY;
-  return success;
 }
 
 FileSystemOperation* FileAPIMessageFilter::GetNewOperation(

webkit/chromeos/fileapi/cros_mount_point_provider.cc

@@ -235,9 +235,14 @@ fileapi::FileSystemFileUtil* CrosMountPointProvider::GetFileUtil(
   return local_file_util_.get();
 }
 
-FilePath CrosMountPointProvider::GetPathForPermissionsCheck(
-    const FilePath& virtual_path) const {
-  return virtual_path;
+fileapi::FilePermissionPolicy CrosMountPointProvider::GetPermissionPolicy(
+    const fileapi::FileSystemURL& url, int permissions) const {
+  if (url.mount_type() == fileapi::kFileSystemTypeIsolated) {
+    // Permissions in isolated filesystems should be examined with
+    // FileSystem permission.
+    return fileapi::FILE_PERMISSION_USE_FILESYSTEM_PERMISSION;
+  }
+  return fileapi::FILE_PERMISSION_USE_FILE_PERMISSION;
 }
 
 fileapi::FileSystemOperation* CrosMountPointProvider::CreateFileSystemOperation(

webkit/chromeos/fileapi/cros_mount_point_provider.h

@@ -57,8 +57,9 @@ class WEBKIT_STORAGE_EXPORT CrosMountPointProvider
   virtual bool IsRestrictedFileName(const FilePath& filename) const OVERRIDE;
   virtual fileapi::FileSystemFileUtil* GetFileUtil(
       fileapi::FileSystemType type) OVERRIDE;
-  virtual FilePath GetPathForPermissionsCheck(const FilePath& virtual_path)
-      const OVERRIDE;
+  virtual fileapi::FilePermissionPolicy GetPermissionPolicy(
+      const fileapi::FileSystemURL& url,
+      int permissions) const OVERRIDE;
   virtual fileapi::FileSystemOperation* CreateFileSystemOperation(
       const fileapi::FileSystemURL& url,
       fileapi::FileSystemContext* context,

webkit/fileapi/file_permission_policy.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "webkit/fileapi/file_permission_policy.h"
+
+#include "base/platform_file.h"
+
+namespace fileapi {
+
+const int kReadFilePermissions = base::PLATFORM_FILE_OPEN |
+                                 base::PLATFORM_FILE_READ |
+                                 base::PLATFORM_FILE_EXCLUSIVE_READ |
+                                 base::PLATFORM_FILE_ASYNC;
+
+const int kWriteFilePermissions = base::PLATFORM_FILE_OPEN |
+                                  base::PLATFORM_FILE_WRITE |
+                                  base::PLATFORM_FILE_EXCLUSIVE_WRITE |
+                                  base::PLATFORM_FILE_ASYNC |
+                                  base::PLATFORM_FILE_WRITE_ATTRIBUTES;
+
+const int kCreateFilePermissions = base::PLATFORM_FILE_CREATE;
+
+const int kOpenFilePermissions = base::PLATFORM_FILE_CREATE |
+                                 base::PLATFORM_FILE_OPEN_ALWAYS |
+                                 base::PLATFORM_FILE_CREATE_ALWAYS |
+                                 base::PLATFORM_FILE_OPEN_TRUNCATED |
+                                 base::PLATFORM_FILE_WRITE |
+                                 base::PLATFORM_FILE_EXCLUSIVE_WRITE |
+                                 base::PLATFORM_FILE_DELETE_ON_CLOSE |
+                                 base::PLATFORM_FILE_WRITE_ATTRIBUTES;
+
+
+}  // namespace fileapi

webkit/fileapi/file_permission_policy.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef WEBKIT_FILEAPI_FILE_PERMISSION_POLICY_H_
+#define WEBKIT_FILEAPI_FILE_PERMISSION_POLICY_H_
+
+#include "webkit/storage/webkit_storage_export.h"
+
+namespace fileapi {
+
+WEBKIT_STORAGE_EXPORT extern const int kReadFilePermissions;
+WEBKIT_STORAGE_EXPORT extern const int kWriteFilePermissions;
+WEBKIT_STORAGE_EXPORT extern const int kCreateFilePermissions;
+WEBKIT_STORAGE_EXPORT extern const int kOpenFilePermissions;
+
+enum FilePermissionPolicy {
+  // Any access should be always denied.
+  FILE_PERMISSION_ALWAYS_DENY,
+
+  // Any access should be always allowed. (This should be used only for
+  // access to sandbox directories.)
+  FILE_PERMISSION_ALWAYS_ALLOW,
+
+  // Access should be examined by per-file permission policy.
+  FILE_PERMISSION_USE_FILE_PERMISSION,
+
+  // Access should be examined by per-filesystem permission policy.
+  FILE_PERMISSION_USE_FILESYSTEM_PERMISSION,
+};
+
+}  // namespace fileapi
+
+#endif  // WEBKIT_FILEAPI_FILE_PERMISSION_POLICY_H_

webkit/fileapi/file_system_mount_point_provider.h

@@ -11,6 +11,7 @@
 #include "base/callback_forward.h"
 #include "base/file_path.h"
 #include "base/platform_file.h"
+#include "webkit/fileapi/file_permission_policy.h"
 #include "webkit/fileapi/file_system_types.h"
 #include "webkit/storage/webkit_storage_export.h"
 
@@ -68,10 +69,10 @@ class WEBKIT_STORAGE_EXPORT FileSystemMountPointProvider {
   // Returns the specialized FileSystemFileUtil for this mount point.
   virtual FileSystemFileUtil* GetFileUtil(FileSystemType type) = 0;
 
-  // Returns file path we should use to check access permissions for
-  // |virtual_path|.
-  virtual FilePath GetPathForPermissionsCheck(const FilePath& virtual_path)
-      const = 0;
+  // Returns file permission policy we should apply for the given |url|.
+  virtual FilePermissionPolicy GetPermissionPolicy(
+      const FileSystemURL& url,
+      int permissions) const = 0;
 
   // Returns a new instance of the specialized FileSystemOperation for this
   // mount point based on the given triplet of |origin_url|, |file_system_type|

webkit/fileapi/isolated_mount_point_provider.cc

@@ -100,10 +100,16 @@ FileSystemFileUtil* IsolatedMountPointProvider::GetFileUtil(
   return NULL;
 }
 
-FilePath IsolatedMountPointProvider::GetPathForPermissionsCheck(
-    const FilePath& virtual_path) const {
-  // For isolated filesystems we only check per-filesystem permissions.
-  return FilePath();
+FilePermissionPolicy IsolatedMountPointProvider::GetPermissionPolicy(
+    const FileSystemURL& url, int permissions) const {
+  if (url.type() == kFileSystemTypeDragged && url.path().empty()) {
+    // The root directory of the dragged filesystem must be always read-only.
+    if (permissions != kReadFilePermissions)
+      return FILE_PERMISSION_ALWAYS_DENY;
+  }
+  // Access to isolated file systems should be checked using per-filesystem
+  // access permission.
+  return FILE_PERMISSION_USE_FILESYSTEM_PERMISSION;
 }
 
 FileSystemOperation* IsolatedMountPointProvider::CreateFileSystemOperation(

webkit/fileapi/isolated_mount_point_provider.h

@@ -38,8 +38,9 @@ class IsolatedMountPointProvider : public FileSystemMountPointProvider {
   virtual bool IsAccessAllowed(const FileSystemURL& url) OVERRIDE;
   virtual bool IsRestrictedFileName(const FilePath& filename) const OVERRIDE;
   virtual FileSystemFileUtil* GetFileUtil(FileSystemType type) OVERRIDE;
-  virtual FilePath GetPathForPermissionsCheck(const FilePath& virtual_path)
-      const OVERRIDE;
+  virtual FilePermissionPolicy GetPermissionPolicy(
+      const FileSystemURL& url,
+      int permissions) const OVERRIDE;
   virtual FileSystemOperation* CreateFileSystemOperation(
       const FileSystemURL& url,
       FileSystemContext* context,

webkit/fileapi/sandbox_mount_point_provider.cc

@@ -255,11 +255,13 @@ FileSystemFileUtil* SandboxMountPointProvider::GetFileUtil(
   return sandbox_file_util_.get();
 }
 
-FilePath SandboxMountPointProvider::GetPathForPermissionsCheck(
-    const FilePath& virtual_path) const {
-  // Sandbox provider shouldn't directly grant permissions for its
-  // data directory.
-  return FilePath();
+FilePermissionPolicy SandboxMountPointProvider::GetPermissionPolicy(
+    const FileSystemURL& url, int permissions) const {
+  // Access to the sandbox directory (and only to the directory) should be
+  // always allowed.
+  CHECK(CanHandleType(url.type()));
+  CHECK(!url.path().ReferencesParent());
+  return FILE_PERMISSION_ALWAYS_ALLOW;
 }
 
 FileSystemOperation* SandboxMountPointProvider::CreateFileSystemOperation(

webkit/fileapi/sandbox_mount_point_provider.h

@@ -83,8 +83,9 @@ class WEBKIT_STORAGE_EXPORT SandboxMountPointProvider
   virtual bool IsAccessAllowed(const FileSystemURL& url) OVERRIDE;
   virtual bool IsRestrictedFileName(const FilePath& filename) const OVERRIDE;
   virtual FileSystemFileUtil* GetFileUtil(FileSystemType type) OVERRIDE;
-  virtual FilePath GetPathForPermissionsCheck(const FilePath& virtual_path)
-      const OVERRIDE;
+  virtual FilePermissionPolicy GetPermissionPolicy(
+      const FileSystemURL& url,
+      int permissions) const OVERRIDE;
   virtual FileSystemOperation* CreateFileSystemOperation(
       const FileSystemURL& url,
       FileSystemContext* context,

webkit/fileapi/test_mount_point_provider.cc

@@ -114,9 +114,9 @@ FileSystemFileUtil* TestMountPointProvider::GetFileUtil(FileSystemType type) {
   return local_file_util_.get();
 }
 
-FilePath TestMountPointProvider::GetPathForPermissionsCheck(
-    const FilePath& virtual_path) const {
-  return base_path_.Append(virtual_path);
+FilePermissionPolicy TestMountPointProvider::GetPermissionPolicy(
+    const FileSystemURL& url, int permissions) const {
+  return FILE_PERMISSION_ALWAYS_DENY;
 }
 
 FileSystemOperation* TestMountPointProvider::CreateFileSystemOperation(

webkit/fileapi/test_mount_point_provider.h

@@ -44,8 +44,9 @@ class WEBKIT_STORAGE_EXPORT_PRIVATE TestMountPointProvider
   virtual bool IsAccessAllowed(const FileSystemURL& url) OVERRIDE;
   virtual bool IsRestrictedFileName(const FilePath& filename) const OVERRIDE;
   virtual FileSystemFileUtil* GetFileUtil(FileSystemType type) OVERRIDE;
-  virtual FilePath GetPathForPermissionsCheck(const FilePath& virtual_path)
-      const OVERRIDE;
+  virtual FilePermissionPolicy GetPermissionPolicy(
+      const FileSystemURL& url,
+      int permissions) const OVERRIDE;
   virtual FileSystemOperation* CreateFileSystemOperation(
       const FileSystemURL& url,
       FileSystemContext* context,

webkit/fileapi/webkit_fileapi.gypi

@@ -6,6 +6,8 @@
   'variables': {
     'webkit_fileapi_sources': [
       '../fileapi/file_observers.h',
+      '../fileapi/file_permission_policy.cc',
+      '../fileapi/file_permission_policy.h',
       '../fileapi/file_stream_writer.h',
       '../fileapi/file_system_callback_dispatcher.cc',
       '../fileapi/file_system_callback_dispatcher.h',