ChildProcessSecurityPolicy: Port FileAPIMessageFilter to use new checks

content/browser/renderer_host/pepper/pepper_security_helper.cc

 // Copyright 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 "content/browser/renderer_host/pepper/pepper_security_helper.h"
 
 #include "base/logging.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "ppapi/c/ppb_file_io.h"
 
@@ skipping 10 matching lines @@
   bool pp_truncate = !!(pp_open_flags & PP_FILEOPENFLAG_TRUNCATE);
   bool pp_exclusive = !!(pp_open_flags & PP_FILEOPENFLAG_EXCLUSIVE);
   bool pp_append = !!(pp_open_flags & PP_FILEOPENFLAG_APPEND);
 
   if (pp_read && !policy->CanReadFile(child_id, file))
     return false;
 
   if (pp_write && !policy->CanWriteFile(child_id, file))
     return false;
 
-  if (pp_append) {
-    // Given ChildSecurityPolicyImpl's current definition of permissions,
-    // APPEND is never supported.
+  // TODO(tommycli): Maybe tighten up required permission. crbug.com/284792
+  if (pp_append && !policy->CanCreateWriteFile(child_id, file))
     return false;
-  }
 
   if (pp_truncate && !pp_write)
     return false;
 
   if (pp_create) {
     if (pp_exclusive) {
       return policy->CanCreateFile(child_id, file);
     } else {
       // Asks for too much, but this is the only grant that allows overwrite.
       return policy->CanCreateWriteFile(child_id, file);
     }
   } else if (pp_truncate) {
     return policy->CanCreateWriteFile(child_id, file);
   }
 
   return true;
 }
 
+bool CanOpenFileSystemURLWithPepperFlags(int pp_open_flags, int child_id,
+                                         const fileapi::FileSystemURL& url) {
+  ChildProcessSecurityPolicyImpl* policy =
+      ChildProcessSecurityPolicyImpl::GetInstance();
+
+  bool pp_read = !!(pp_open_flags & PP_FILEOPENFLAG_READ);

[Tom Sepez, 2013/09/04 22:22:45]

Seems a shame to have this same logic here and above. It would be nice if one of
these called into the other or some common routines.

[tommycli, 2013/09/04 23:21:27]

I agree it's a shame. The only obvious way I saw to combine these a bit would be
to do something like:

// (Create|Write != CreateWrite) See crbug.com/263150
struct PermissionsSet {
  bool can_read;
  bool can_write;
  bool can_create;
  bool can_create_write;
}

PermissionSet GetPermissionSetForFile(...);
PermissionSet GetPermissionSetForFileSystemFile(...);

But that didn't seem to save any lines of code.

[kinuko, 2013/09/05 03:49:43]

Could we make the common logic a template?  I think something like following
code would work:

namespace {

template <typename CanRead, typename CanWrite,
          typename CanCreate, typename CanCreateWrite,
          typename FileID>
bool CanOpenFileWithPepperFlags(CanRead can_read,
                                CanWrite can_write,
                                CanCreate can_create,
                                CanCreateWrite can_create_write,
                                int pp_open_flags,
                                int child_id,
                                const FileID& file) {
  ChildProcessSecurityPolicyImpl* policy = ...
  bool pp_read = !!(...);
  ...

  if (pp_read && !(policy->*can_read)(child_id, file))
    return false;
  if (pp_write && !(policy->*can_write)(child_id, file))
    return false;
  ...
}

}  // namespace

bool CanOpenWithPepperFlags(int pp_open_flags, int child_id, 
                            const base::FilePath& file) { 
  return CanOpenFileWithPepperFlags(
      &ChildProcessSecurityPolicyImpl::CanReadFile,
      &ChildProcessSecurityPolicyImpl::CanWriteFile,
      &ChildProcessSecurityPolicyImpl::CanCreateFile,
      &ChildProcessSecurityPolicyImpl::CanCreateWriteFile,
      pp_open_flags, child_id, file);
}
  
bool CanOpenFileSystemURLWithPepperFlags(int pp_open_flags, int child_id,
                                         const fileapi::FileSystemURL& url) {
  return CanOpenFileWithPepperFlags(
      &ChildProcessSecurityPolicyImpl::CanReadFileSystemFile,
      &ChildProcessSecurityPolicyImpl::CanWriteFileSystemFile,
      &ChildProcessSecurityPolicyImpl::CanCreateFileSystemFile,
      &ChildProcessSecurityPolicyImpl::CanCreateWriteFileSystemFile,
      pp_open_flags, child_id, url);
}

Other than SLOC bloat I'm afraid having duplicated logic makes the code
error-prone.  (We likely tend to change only one method and forget the other)

[tommycli, 2013/09/06 01:41:43]

Done. I had no idea you could do this with templates. Great idea!

+  bool pp_write = !!(pp_open_flags & PP_FILEOPENFLAG_WRITE);
+  bool pp_create = !!(pp_open_flags & PP_FILEOPENFLAG_CREATE);
+  bool pp_truncate = !!(pp_open_flags & PP_FILEOPENFLAG_TRUNCATE);
+  bool pp_exclusive = !!(pp_open_flags & PP_FILEOPENFLAG_EXCLUSIVE);
+  bool pp_append = !!(pp_open_flags & PP_FILEOPENFLAG_APPEND);
+
+  if (pp_read && !policy->CanReadFileSystemFile(child_id, url))
+    return false;
+
+  if (pp_write && !policy->CanWriteFileSystemFile(child_id, url))
+    return false;
+
+  // TODO(tommycli): Maybe tighten up required permission. crbug.com/284792
+  if (pp_append && !policy->CanCreateWriteFileSystemFile(child_id, url))
+    return false;
+
+  if (pp_truncate && !pp_write)
+    return false;
+
+  if (pp_create) {
+    if (pp_exclusive) {
+      return policy->CanCreateFileSystemFile(child_id, url);
+    } else {
+      // Asks for too much, but this is the only grant that allows overwrite.
+      return policy->CanCreateWriteFileSystemFile(child_id, url);
+    }
+  } else if (pp_truncate) {
+    return policy->CanCreateWriteFileSystemFile(child_id, url);
+  }
+
+  return true;
+}
+
 }  // namespace content