Extracting and unittesting PrepareDropDataForChildProcess function.

content/browser/child_process_security_policy_impl.cc

Index: content/browser/child_process_security_policy_impl.cc
diff --git a/content/browser/child_process_security_policy_impl.cc b/content/browser/child_process_security_policy_impl.cc
index 28ce45ad809ad34567726a34478ff43627bf5f67..8b2b0808361fbc98a8dfa28c3c99163adefa695f 100644
--- a/content/browser/child_process_security_policy_impl.cc
+++ b/content/browser/child_process_security_policy_impl.cc
@@ -15,17 +15,21 @@
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/public/browser/child_process_data.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/storage_partition.h"
 #include "content/public/common/bindings_policy.h"
+#include "content/public/common/drop_data.h"
 #include "content/public/common/url_constants.h"
 #include "net/base/filename_util.h"
 #include "net/url_request/url_request.h"
 #include "storage/browser/fileapi/file_permission_policy.h"
+#include "storage/browser/fileapi/file_system_context.h"
 #include "storage/browser/fileapi/file_system_url.h"
 #include "storage/browser/fileapi/isolated_context.h"
 #include "storage/common/fileapi/file_system_util.h"
@@ -942,6 +946,85 @@ bool ChildProcessSecurityPolicyImpl::HasSpecificPermissionForOrigin(
   return state->second->CanCommitOrigin(origin);
 }
 
+void ChildProcessSecurityPolicyImpl::GrantFileAccessFromDropData(
+    int child_id,
+    const storage::FileSystemContext* file_system_context,
+    DropData* drop_data) {
+#if defined(OS_CHROMEOS)
+  // The externalfile:// scheme is used in Chrome OS to open external files in a
+  // browser tab.
+  if (drop_data->url.SchemeIs(content::kExternalFileScheme))
+    GrantRequestURL(child_id, drop_data->url);
+#endif
+
+  // The filenames vector represents a capability to access the given files.
+  storage::IsolatedContext::FileInfoSet files;
+  for (auto& filename : drop_data->filenames) {
+    // Make sure we have the same display_name as the one we register.
+    if (filename.display_name.empty()) {
+      std::string name;
+      files.AddPath(filename.path, &name);
+      filename.display_name = base::FilePath::FromUTF8Unsafe(name);

[ncarter (slow), 2017/04/24 18:54:46]

Mutating the |drop_data| seems like an unusual contract for a Grant operation. 

If you move this into a static function (as suggested below), I might consider
giving it a name more like "PrepareDropDataForProcess" to make it clearer that
DropData is an in/out parameter.

[Łukasz Anforowicz, 2017/04/24 21:30:23]

Good point.  I should have raised this as a concern, rather than quietly hiding
this in the CL.
Thanks for the suggestion.  Done (more or less - I went forward with
"PrepareDropDataForChildProcess").

+    } else {
+      files.AddPathWithName(filename.path,
+                            filename.display_name.AsUTF8Unsafe());
+    }
+    // A dragged file may wind up as the value of an input element, or it
+    // may be used as the target of a navigation instead.  We don't know
+    // which will happen at this point, so generously grant both access
+    // and request permissions to the specific file to cover both cases.
+    // We do not give it the permission to request all file:// URLs.
+    GrantRequestSpecificFileURL(child_id,
+                                net::FilePathToFileURL(filename.path));
+
+    // If the renderer already has permission to read these paths, we don't need
+    // to re-grant them. This prevents problems with DnD for files in the CrOS
+    // file manager--the file manager already had read/write access to those
+    // directories, but dragging a file would cause the read/write access to be
+    // overwritten with read-only access, making them impossible to delete or
+    // rename until the renderer was killed.
+    if (!CanReadFile(child_id, filename.path))
+      GrantReadFile(child_id, filename.path);
+  }
+
+  storage::IsolatedContext* isolated_context =
+      storage::IsolatedContext::GetInstance();
+  DCHECK(isolated_context);
+
+  if (!files.fileset().empty()) {
+    std::string filesystem_id =
+        isolated_context->RegisterDraggedFileSystem(files);

[ncarter (slow), 2017/04/24 18:54:46]

I am not convinced that CPSP is the right place for this operation to live. I
think of CPSP as a dumb recorder of state; the actual policy decisions are
supposed to live in the caller of CPSP. But this function has a lot of business
logic specific to drag and drop. Calling into IsolatedContext, in particular,
seems like a layering violation.

What do you think of making this a utility function in
content/browser/fileapi/browser_file_system_helper.h (it could even accept the
IsolatedContext and CPSP as arguments, to make explicit what it's mutating?)?

I also noticed some web_drag_ things in content/browser/web_contents/ but that
doesn't feel as appropriate.

[Łukasz Anforowicz, 2017/04/24 21:30:23]

Thanks for pointing this out - I haven't considered layering (nor was I aware of
where we try to place CPSP).  I like the suggestion to put the helper function
in content/browser/fileapi/browser_file_system_helper.h.  Done. 

PS. TBH it feels like the helper function should be [layer-wise] above *both* 1)
filesystem and 2) cpsp;  OTOH, I wasn't able to find a better place for the
helper and content/browser/fileapi/browser_file_system_helper.h seems okay.  The
only alternative I could think of is to create a new drag_helpers.h compilation
unit (but then putting it next to
content/browser/renderer_host/render_widget_host_impl.cc seems for some reason
icky to me).

[ncarter (slow), 2017/04/24 22:01:51]

I agree with the feeling that it ought to be both layer-wise above both
filesystem and cpsp.  //content/browser/fileapi is kind of there, in the sense
that it's in a layer above //storage.

So, I think fileapi is not such a bad place for this. The CPSP grants are all
file-system related, and the fileapi's scope has some overlap with file:///.

+    if (!filesystem_id.empty()) {
+      // Grant the permission iff the ID is valid.
+      GrantReadFileSystem(child_id, filesystem_id);
+    }
+    drop_data->filesystem_id = base::UTF8ToUTF16(filesystem_id);
+  }
+
+  for (auto& file_system_file : drop_data->file_system_files) {
+    storage::FileSystemURL file_system_url =
+        file_system_context->CrackURL(file_system_file.url);
+
+    std::string register_name;
+    std::string filesystem_id = isolated_context->RegisterFileSystemForPath(
+        file_system_url.type(), file_system_url.filesystem_id(),
+        file_system_url.path(), &register_name);
+
+    if (!filesystem_id.empty()) {
+      // Grant the permission iff the ID is valid.
+      GrantReadFileSystem(child_id, filesystem_id);
+    }
+
+    // Note: We are using the origin URL provided by the sender here. It may be
+    // different from the receiver's.
+    file_system_file.url =
+        GURL(storage::GetIsolatedFileSystemRootURIString(
+                 file_system_url.origin(), filesystem_id, std::string())
+                 .append(register_name));
+    file_system_file.filesystem_id = filesystem_id;
+  }
+}
+
 void ChildProcessSecurityPolicyImpl::LockToOrigin(int child_id,
                                                   const GURL& gurl) {
   // "gurl" can be currently empty in some cases, such as file://blah.