[Native File System] Make sure all the operations check permissions.

Permissions are still auto-granted, but this at least hooks up the various
mojo exposed operations to check for read or write access.

Bug: 878585
Change-Id: I99cfde0451adc87f39ae8314881d0671dd06a168
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1636399
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#666566}

content/browser/native_file_system/native_file_system_directory_handle_impl.cc

@@ -89,12 +89,27 @@ void NativeFileSystemDirectoryHandleImpl::GetFile(const std::string& name,
     return;
   }
 
+  if (GetReadPermissionStatus() != PermissionStatus::GRANTED) {
+    std::move(callback).Run(
+        NativeFileSystemError::New(base::File::FILE_ERROR_ACCESS_DENIED),
+        nullptr);
+    return;
+  }
+
   if (create) {
-    operation_runner()->CreateFile(
-        child_url, /*exclusive=*/false,
-        base::BindOnce(&NativeFileSystemDirectoryHandleImpl::DidGetFile,
-                       weak_factory_.GetWeakPtr(), child_url,
-                       std::move(callback)));
+    // If |create| is true, write permission is required unconditionally, i.e.
+    // even if the file already exists. This is intentional, and matches the
+    // behavior that is specified in the spec.
+    RunWithWritePermission(
+        base::BindOnce(
+            &NativeFileSystemDirectoryHandleImpl::GetFileWithWritePermission,
+            weak_factory_.GetWeakPtr(), child_url),
+        base::BindOnce([](GetFileCallback callback) {
+          std::move(callback).Run(
+              NativeFileSystemError::New(base::File::FILE_ERROR_ACCESS_DENIED),
+              nullptr);
+        }),
+        std::move(callback));
   } else {
     operation_runner()->FileExists(
         child_url,
@@ -117,12 +132,27 @@ void NativeFileSystemDirectoryHandleImpl::GetDirectory(
     return;
   }
 
+  if (GetReadPermissionStatus() != PermissionStatus::GRANTED) {
+    std::move(callback).Run(
+        NativeFileSystemError::New(base::File::FILE_ERROR_ACCESS_DENIED),
+        nullptr);
+    return;
+  }
+
   if (create) {
-    operation_runner()->CreateDirectory(
-        child_url, /*exclusive=*/false, /*recursive=*/false,
-        base::BindOnce(&NativeFileSystemDirectoryHandleImpl::DidGetDirectory,
-                       weak_factory_.GetWeakPtr(), child_url,
-                       std::move(callback)));
+    // If |create| is true, write permission is required unconditionally, i.e.
+    // even if the file already exists. This is intentional, and matches the
+    // behavior that is specified in the spec.
+    RunWithWritePermission(
+        base::BindOnce(&NativeFileSystemDirectoryHandleImpl::
+                           GetDirectoryWithWritePermission,
+                       weak_factory_.GetWeakPtr(), child_url),
+        base::BindOnce([](GetDirectoryCallback callback) {
+          std::move(callback).Run(
+              NativeFileSystemError::New(base::File::FILE_ERROR_ACCESS_DENIED),
+              nullptr);
+        }),
+        std::move(callback));
   } else {
     operation_runner()->DirectoryExists(
         child_url,
@@ -147,13 +177,14 @@ void NativeFileSystemDirectoryHandleImpl::Remove(bool recurse,
                                                  RemoveCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  operation_runner()->Remove(
-      url(), recurse,
-      base::BindOnce(
-          [](RemoveCallback callback, base::File::Error result) {
-            std::move(callback).Run(NativeFileSystemError::New(result));
-          },
-          std::move(callback)));
+  RunWithWritePermission(
+      base::BindOnce(&NativeFileSystemDirectoryHandleImpl::RemoveImpl,
+                     weak_factory_.GetWeakPtr(), recurse),
+      base::BindOnce([](RemoveCallback callback) {
+        std::move(callback).Run(
+            NativeFileSystemError::New(base::File::FILE_ERROR_ACCESS_DENIED));
+      }),
+      std::move(callback));
 }
 
 void NativeFileSystemDirectoryHandleImpl::Transfer(
@@ -163,7 +194,22 @@ void NativeFileSystemDirectoryHandleImpl::Transfer(
   manager()->CreateTransferToken(*this, std::move(token));
 }
 
-void NativeFileSystemDirectoryHandleImpl::DidGetFile(storage::FileSystemURL url,
+void NativeFileSystemDirectoryHandleImpl::GetFileWithWritePermission(
+    const storage::FileSystemURL& child_url,
+    GetFileCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_EQ(GetWritePermissionStatus(),
+            blink::mojom::PermissionStatus::GRANTED);
+
+  operation_runner()->CreateFile(
+      child_url, /*exclusive=*/false,
+      base::BindOnce(&NativeFileSystemDirectoryHandleImpl::DidGetFile,
+                     weak_factory_.GetWeakPtr(), child_url,
+                     std::move(callback)));
+}
+
+void NativeFileSystemDirectoryHandleImpl::DidGetFile(
+    const storage::FileSystemURL& url,
     GetFileCallback callback,
     base::File::Error result) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -178,8 +224,22 @@ void NativeFileSystemDirectoryHandleImpl::DidGetFile(storage::FileSystemURL url,
       manager()->CreateFileHandle(context(), url, file_system()));
 }
 
+void NativeFileSystemDirectoryHandleImpl::GetDirectoryWithWritePermission(
+    const storage::FileSystemURL& child_url,
+    GetDirectoryCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_EQ(GetWritePermissionStatus(),
+            blink::mojom::PermissionStatus::GRANTED);
+
+  operation_runner()->CreateDirectory(
+      child_url, /*exclusive=*/false, /*recursive=*/false,
+      base::BindOnce(&NativeFileSystemDirectoryHandleImpl::DidGetDirectory,
+                     weak_factory_.GetWeakPtr(), child_url,
+                     std::move(callback)));
+}
+
 void NativeFileSystemDirectoryHandleImpl::DidGetDirectory(
-    storage::FileSystemURL url,
+    const storage::FileSystemURL& url,
     GetDirectoryCallback callback,
     base::File::Error result) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -231,6 +291,21 @@ void NativeFileSystemDirectoryHandleImpl::DidReadDirectory(
   }
 }
 
+void NativeFileSystemDirectoryHandleImpl::RemoveImpl(bool recurse,
+                                                     RemoveCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_EQ(GetWritePermissionStatus(),
+            blink::mojom::PermissionStatus::GRANTED);
+
+  operation_runner()->Remove(
+      url(), recurse,
+      base::BindOnce(
+          [](RemoveCallback callback, base::File::Error result) {
+            std::move(callback).Run(NativeFileSystemError::New(result));
+          },
+          std::move(callback)));
+}
+
 base::File::Error NativeFileSystemDirectoryHandleImpl::GetChildURL(
     const std::string& name,
     storage::FileSystemURL* result) {

content/browser/native_file_system/native_file_system_directory_handle_impl.h

@@ -52,10 +52,18 @@ class NativeFileSystemDirectoryHandleImpl
   // State that is kept for the duration of a GetEntries/ReadDirectory call.
   struct ReadDirectoryState;
 
-  void DidGetFile(storage::FileSystemURL url,
+  // This method creates the file if it does not currently exists. I.e. it is
+  // the implementation for passing create=true to GetFile.
+  void GetFileWithWritePermission(const storage::FileSystemURL& child_url,
+                                  GetFileCallback callback);
+  void DidGetFile(const storage::FileSystemURL& url,
                   GetFileCallback callback,
                   base::File::Error result);
-  void DidGetDirectory(storage::FileSystemURL url,
+  // This method creates the directory if it does not currently exists. I.e. it
+  // is the implementation for passing create=true to GetDirectory.
+  void GetDirectoryWithWritePermission(const storage::FileSystemURL& child_url,
+                                       GetDirectoryCallback callback);
+  void DidGetDirectory(const storage::FileSystemURL& url,
                        GetDirectoryCallback callback,
                        base::File::Error result);
   void DidReadDirectory(
@@ -64,6 +72,8 @@ class NativeFileSystemDirectoryHandleImpl
       std::vector<filesystem::mojom::DirectoryEntry> file_list,
       bool has_more);
 
+  void RemoveImpl(bool recurse, RemoveCallback callback);
+
   // Calculates a FileSystemURL for a (direct) child of this directory with the
   // given name.  Returns an error when |name| includes invalid input like "/".
   base::File::Error GetChildURL(const std::string& name,

content/browser/native_file_system/native_file_system_file_handle_impl.cc

@@ -52,6 +52,13 @@ void NativeFileSystemFileHandleImpl::RequestPermission(
 
 void NativeFileSystemFileHandleImpl::AsBlob(AsBlobCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (GetReadPermissionStatus() != PermissionStatus::GRANTED) {
+    std::move(callback).Run(
+        NativeFileSystemError::New(base::File::FILE_ERROR_ACCESS_DENIED),
+        nullptr);
+    return;
+  }
+
   // TODO(mek): Check backend::SupportsStreaming and create snapshot file if
   // streaming is not supported.
   operation_runner()->GetMetadata(
@@ -66,12 +73,14 @@ void NativeFileSystemFileHandleImpl::AsBlob(AsBlobCallback callback) {
 void NativeFileSystemFileHandleImpl::Remove(RemoveCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  operation_runner()->RemoveFile(
-      url(), base::BindOnce(
-                 [](RemoveCallback callback, base::File::Error result) {
-                   std::move(callback).Run(NativeFileSystemError::New(result));
-                 },
-                 std::move(callback)));
+  RunWithWritePermission(
+      base::BindOnce(&NativeFileSystemFileHandleImpl::RemoveImpl,
+                     weak_factory_.GetWeakPtr()),
+      base::BindOnce([](RemoveCallback callback) {
+        std::move(callback).Run(
+            NativeFileSystemError::New(base::File::FILE_ERROR_ACCESS_DENIED));
+      }),
+      std::move(callback));
 }
 
 void NativeFileSystemFileHandleImpl::Write(uint64_t offset,
@@ -79,10 +88,15 @@ void NativeFileSystemFileHandleImpl::Write(uint64_t offset,
                                            WriteCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  blob_context()->GetBlobDataFromBlobPtr(
-      std::move(data),
-      base::BindOnce(&NativeFileSystemFileHandleImpl::DoWriteBlob,
-                     weak_factory_.GetWeakPtr(), std::move(callback), offset));
+  RunWithWritePermission(
+      base::BindOnce(&NativeFileSystemFileHandleImpl::WriteImpl,
+                     weak_factory_.GetWeakPtr(), offset, std::move(data)),
+      base::BindOnce([](WriteCallback callback) {
+        std::move(callback).Run(
+            NativeFileSystemError::New(base::File::FILE_ERROR_ACCESS_DENIED),
+            0);
+      }),
+      std::move(callback));
 }
 
 void NativeFileSystemFileHandleImpl::WriteStream(
@@ -91,33 +105,29 @@ void NativeFileSystemFileHandleImpl::WriteStream(
     WriteStreamCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  // FileSystemOperationRunner assumes that positions passed to Write are always
-  // valid, and will NOTREACHED() if that is not the case, so first check the
-  // size of the file to make sure the position passed in from the renderer is
-  // in fact valid.
-  // Of course the file could still change between checking its size and the
-  // write operation being started, but this is at least a lot better than the
-  // old implementation where the renderer only checks against how big it thinks
-  // the file currently is.
-  // TODO(https://crbug.com/957214): Fix this situation.
-  operation_runner()->GetMetadata(
-      url(), FileSystemOperation::GET_METADATA_FIELD_SIZE,
-      base::BindOnce(&NativeFileSystemFileHandleImpl::DoWriteStreamWithFileInfo,
-                     weak_factory_.GetWeakPtr(), std::move(callback), offset,
-                     std::move(stream)));
+  RunWithWritePermission(
+      base::BindOnce(&NativeFileSystemFileHandleImpl::WriteStreamImpl,
+                     weak_factory_.GetWeakPtr(), offset, std::move(stream)),
+      base::BindOnce([](WriteStreamCallback callback) {
+        std::move(callback).Run(
+            NativeFileSystemError::New(base::File::FILE_ERROR_ACCESS_DENIED),
+            0);
+      }),
+      std::move(callback));
 }
 
 void NativeFileSystemFileHandleImpl::Truncate(uint64_t length,
                                               TruncateCallback callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  operation_runner()->Truncate(
-      url(), length,
-      base::BindOnce(
-          [](TruncateCallback callback, base::File::Error result) {
-            std::move(callback).Run(NativeFileSystemError::New(result));
-          },
-          std::move(callback)));
+  RunWithWritePermission(
+      base::BindOnce(&NativeFileSystemFileHandleImpl::TruncateImpl,
+                     weak_factory_.GetWeakPtr(), length),
+      base::BindOnce([](TruncateCallback callback) {
+        std::move(callback).Run(
+            NativeFileSystemError::New(base::File::FILE_ERROR_ACCESS_DENIED));
+      }),
+      std::move(callback));
 }
 
 void NativeFileSystemFileHandleImpl::Transfer(
@@ -164,6 +174,31 @@ void NativeFileSystemFileHandleImpl::DidGetMetaDataForBlob(
       blink::mojom::SerializedBlob::New(uuid, "application/octet-stream",
                                         info.size, blob_ptr.PassInterface()));
 }
+void NativeFileSystemFileHandleImpl::RemoveImpl(RemoveCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_EQ(GetWritePermissionStatus(),
+            blink::mojom::PermissionStatus::GRANTED);
+
+  operation_runner()->RemoveFile(
+      url(), base::BindOnce(
+                 [](RemoveCallback callback, base::File::Error result) {
+                   std::move(callback).Run(NativeFileSystemError::New(result));
+                 },
+                 std::move(callback)));
+}
+
+void NativeFileSystemFileHandleImpl::WriteImpl(uint64_t offset,
+                                               blink::mojom::BlobPtr data,
+                                               WriteCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_EQ(GetWritePermissionStatus(),
+            blink::mojom::PermissionStatus::GRANTED);
+
+  blob_context()->GetBlobDataFromBlobPtr(
+      std::move(data),
+      base::BindOnce(&NativeFileSystemFileHandleImpl::DoWriteBlob,
+                     weak_factory_.GetWeakPtr(), std::move(callback), offset));
+}
 
 void NativeFileSystemFileHandleImpl::DoWriteBlob(
     WriteCallback callback,
@@ -214,6 +249,30 @@ void NativeFileSystemFileHandleImpl::DoWriteBlobWithFileInfo(
                           base::Owned(new WriteState{std::move(callback)})));
 }
 
+void NativeFileSystemFileHandleImpl::WriteStreamImpl(
+    uint64_t offset,
+    mojo::ScopedDataPipeConsumerHandle stream,
+    WriteStreamCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_EQ(GetWritePermissionStatus(),
+            blink::mojom::PermissionStatus::GRANTED);
+
+  // FileSystemOperationRunner assumes that positions passed to Write are always
+  // valid, and will NOTREACHED() if that is not the case, so first check the
+  // size of the file to make sure the position passed in from the renderer is
+  // in fact valid.
+  // Of course the file could still change between checking its size and the
+  // write operation being started, but this is at least a lot better than the
+  // old implementation where the renderer only checks against how big it thinks
+  // the file currently is.
+  // TODO(https://crbug.com/957214): Fix this situation.
+  operation_runner()->GetMetadata(
+      url(), FileSystemOperation::GET_METADATA_FIELD_SIZE,
+      base::BindOnce(&NativeFileSystemFileHandleImpl::DoWriteStreamWithFileInfo,
+                     weak_factory_.GetWeakPtr(), std::move(callback), offset,
+                     std::move(stream)));
+}
+
 void NativeFileSystemFileHandleImpl::DoWriteStreamWithFileInfo(
     WriteStreamCallback callback,
     uint64_t position,
@@ -249,4 +308,19 @@ void NativeFileSystemFileHandleImpl::DidWrite(WriteState* state,
   }
 }
 
+void NativeFileSystemFileHandleImpl::TruncateImpl(uint64_t length,
+                                                  TruncateCallback callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK_EQ(GetWritePermissionStatus(),
+            blink::mojom::PermissionStatus::GRANTED);
+
+  operation_runner()->Truncate(
+      url(), length,
+      base::BindOnce(
+          [](TruncateCallback callback, base::File::Error result) {
+            std::move(callback).Run(NativeFileSystemError::New(result));
+          },
+          std::move(callback)));
+}
+
 }  // namespace content

content/browser/native_file_system/native_file_system_file_handle_impl.h

@@ -68,6 +68,11 @@ class CONTENT_EXPORT NativeFileSystemFileHandleImpl
                              base::File::Error result,
                              const base::File::Info& info);
 
+  void RemoveImpl(RemoveCallback callback);
+
+  void WriteImpl(uint64_t offset,
+                 blink::mojom::BlobPtr data,
+                 WriteCallback callback);
   void DoWriteBlob(WriteCallback callback,
                    uint64_t position,
                    std::unique_ptr<storage::BlobDataHandle> blob);
@@ -76,6 +81,9 @@ class CONTENT_EXPORT NativeFileSystemFileHandleImpl
                                std::unique_ptr<storage::BlobDataHandle> blob,
                                base::File::Error result,
                                const base::File::Info& file_info);
+  void WriteStreamImpl(uint64_t offset,
+                       mojo::ScopedDataPipeConsumerHandle stream,
+                       WriteStreamCallback callback);
   void DoWriteStreamWithFileInfo(WriteStreamCallback callback,
                                  uint64_t position,
                                  mojo::ScopedDataPipeConsumerHandle data_pipe,
@@ -86,6 +94,8 @@ class CONTENT_EXPORT NativeFileSystemFileHandleImpl
                 int64_t bytes,
                 bool complete);
 
+  void TruncateImpl(uint64_t length, TruncateCallback callback);
+
   base::WeakPtrFactory<NativeFileSystemFileHandleImpl> weak_factory_{this};
   DISALLOW_COPY_AND_ASSIGN(NativeFileSystemFileHandleImpl);
 };

content/browser/native_file_system/native_file_system_handle_base.h

@@ -62,6 +62,15 @@ class CONTENT_EXPORT NativeFileSystemHandleBase {
   void DoRequestPermission(bool writable,
                            base::OnceCallback<void(PermissionStatus)> callback);
 
+  // Invokes |callback|, possibly after first requesting write permission. If
+  // permission isn't granted, |permission_denied| is invoked instead. The
+  // callbacks can be invoked synchronously.
+  template <typename CallbackArgType>
+  void RunWithWritePermission(
+      base::OnceCallback<void(CallbackArgType)> callback,
+      base::OnceCallback<void(CallbackArgType)> no_permission_callback,
+      CallbackArgType callback_arg);
+
  protected:
   NativeFileSystemManagerImpl* manager() { return manager_; }
   const BindingContext& context() { return context_; }
@@ -90,6 +99,28 @@ class CONTENT_EXPORT NativeFileSystemHandleBase {
   DISALLOW_COPY_AND_ASSIGN(NativeFileSystemHandleBase);
 };
 
+template <typename CallbackArgType>
+void NativeFileSystemHandleBase::RunWithWritePermission(
+    base::OnceCallback<void(CallbackArgType)> callback,
+    base::OnceCallback<void(CallbackArgType)> no_permission_callback,
+    CallbackArgType callback_arg) {
+  DoRequestPermission(
+      /*writable=*/true,
+      base::BindOnce(
+          [](base::OnceCallback<void(CallbackArgType)> callback,
+             base::OnceCallback<void(CallbackArgType)> no_permission_callback,
+             CallbackArgType callback_arg,
+             blink::mojom::PermissionStatus status) {
+            if (status == blink::mojom::PermissionStatus::GRANTED) {
+              std::move(callback).Run(std::move(callback_arg));
+              return;
+            }
+            std::move(no_permission_callback).Run(std::move(callback_arg));
+          },
+          std::move(callback), std::move(no_permission_callback),
+          std::move(callback_arg)));
+}
+
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_NATIVE_FILE_SYSTEM_NATIVE_FILE_SYSTEM_HANDLE_BASE_H_