ChildProcessSecurityPolicy: Deprecate bitmask-based permissions checks for files.

content/browser/child_process_security_policy_unittest.cc

Index: content/browser/child_process_security_policy_unittest.cc
diff --git a/content/browser/child_process_security_policy_unittest.cc b/content/browser/child_process_security_policy_unittest.cc
index e6473e9278911ea37ec6614e3c499cf6f9079f51..5f489e6d98eabb533cab489877979dd55a0310d1 100644
--- a/content/browser/child_process_security_policy_unittest.cc
+++ b/content/browser/child_process_security_policy_unittest.cc
@@ -13,6 +13,9 @@
 #include "content/test/test_content_browser_client.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
+#include "webkit/browser/fileapi/file_permission_policy.h"
+#include "webkit/browser/fileapi/file_system_url.h"
+#include "webkit/common/fileapi/file_system_types.h"
 
 namespace content {
 namespace {
@@ -90,6 +93,83 @@ class ChildProcessSecurityPolicyTest : public testing::Test {
   ContentBrowserClient* old_browser_client_;
 };
 
+class PermissionsSet {
+ public:
+  PermissionsSet()
+      : can_read(false),
+        can_write(false),
+        can_create(false),
+        can_create_read_write(false) {
+  }
+
+  PermissionsSet& EnableRead() {
+    can_read = true;
+    return *this;
+  }
+
+  PermissionsSet& EnableWrite() {
+    can_write = true;
+    return *this;
+  }
+
+  PermissionsSet& EnableCreate() {
+    can_create = true;
+    return *this;
+  }
+
+  PermissionsSet& EnableCreateReadWrite() {
+    can_create_read_write = true;

[vandebo (ex-Chrome), 2013/07/18 16:15:58]

Should there only be three bools with this one setting all of them?

[tommycli, 2013/07/18 16:23:45]

Unfortunately, CreateReadWrite is almost a misnomer.

Create|Read|Write != CreateReadWrite. Check out the top of
child_process_security_policy_impl.cc for the specific flags...

Should that above relationship hold? It probably should, or we should rename it
a bit... Thoughts?

+    return *this;
+  }
+
+  bool operator==(const PermissionsSet& o) const {
+    return can_read == o.can_read &&
+        can_write == o.can_write &&
+        can_create == o.can_create &&
+        can_create_read_write == o.can_create_read_write;
+  }
+
+private:
+  bool can_read;
+  bool can_write;
+  bool can_create;
+  bool can_create_read_write;
+};
+
+PermissionsSet GetAllPermissions(ChildProcessSecurityPolicyImpl* p,
+                                 int child_id, const base::FilePath& file) {
+  PermissionsSet permissions;
+
+  if (p->CanReadFile(child_id, file))
+    permissions.EnableRead();
+  if (p->CanWriteFile(child_id, file))
+    permissions.EnableWrite();
+  if (p->CanCreateFile(child_id, file))
+    permissions.EnableCreate();
+  if (p->CanCreateReadWriteFile(child_id, file))
+    permissions.EnableCreateReadWrite();
+
+  return permissions;
+}
+
+PermissionsSet GetAllPermissionsForURL(
+    ChildProcessSecurityPolicyImpl* p,
+    int child_id,
+    const fileapi::FileSystemURL& url) {
+  PermissionsSet permissions;
+
+  if (p->CanReadFileSystemFile(child_id, url))
+    permissions.EnableRead();
+  if (p->CanWriteFileSystemFile(child_id, url))
+    permissions.EnableWrite();
+  if (p->CanCreateFileSystemFile(child_id, url))
+    permissions.EnableCreate();
+  if (p->CanCreateReadWriteFileSystemFile(child_id, url))
+    permissions.EnableCreateReadWrite();
+
+  return permissions;
+}
+
 TEST_F(ChildProcessSecurityPolicyTest, IsWebSafeSchemeTest) {
   ChildProcessSecurityPolicyImpl* p =
       ChildProcessSecurityPolicyImpl::GetInstance();
@@ -278,28 +358,68 @@ TEST_F(ChildProcessSecurityPolicyTest, SpecificFile) {
   p->Remove(kRendererID);
 }
 
-TEST_F(ChildProcessSecurityPolicyTest, CanReadFiles) {
+TEST_F(ChildProcessSecurityPolicyTest, PermissionGrantingAndRevoking) {
   ChildProcessSecurityPolicyImpl* p =
       ChildProcessSecurityPolicyImpl::GetInstance();
 
+  p->RegisterFileSystemPermissionPolicy(
+      fileapi::kFileSystemTypeTest,
+      fileapi::FILE_PERMISSION_USE_FILE_PERMISSION);
+
   p->Add(kRendererID);
+  base::FilePath file(TEST_PATH("/dir/testfile"));
+  fileapi::FileSystemURL url = fileapi::FileSystemURL::CreateForTest(
+      GURL("http://foo/"), fileapi::kFileSystemTypeTest, file);
 
-  EXPECT_FALSE(p->CanReadFile(kRendererID,
-                              base::FilePath(TEST_PATH("/etc/passwd"))));
-  p->GrantReadFile(kRendererID, base::FilePath(TEST_PATH("/etc/passwd")));
-  EXPECT_TRUE(p->CanReadFile(kRendererID,
-                             base::FilePath(TEST_PATH("/etc/passwd"))));
-  EXPECT_FALSE(p->CanReadFile(kRendererID,
-                              base::FilePath(TEST_PATH("/etc/shadow"))));
+  PermissionsSet all_denied;
 
+  // Test initially having no permissions.
+  EXPECT_EQ(all_denied, GetAllPermissions(p, kRendererID, file));
+  EXPECT_EQ(all_denied, GetAllPermissionsForURL(p, kRendererID, url));
+
+  // Testing every combination of permissions granting and revoking.
+  PermissionsSet read_only;
+  read_only.EnableRead();
+  p->GrantReadFile(kRendererID, file);
+  EXPECT_EQ(read_only, GetAllPermissions(p, kRendererID, file));
+  EXPECT_EQ(read_only, GetAllPermissionsForURL(p, kRendererID, url));
+  p->RevokeAllPermissionsForFile(kRendererID, file);
+  EXPECT_EQ(all_denied, GetAllPermissions(p, kRendererID, file));
+  EXPECT_EQ(all_denied, GetAllPermissionsForURL(p, kRendererID, url));
+
+  PermissionsSet create_read_write;
+  create_read_write.EnableRead().EnableWrite().EnableCreate()
+      .EnableCreateReadWrite();
+  p->GrantCreateReadWriteFile(kRendererID, file);
+  EXPECT_EQ(create_read_write, GetAllPermissions(p, kRendererID, file));
+  EXPECT_EQ(create_read_write, GetAllPermissionsForURL(p, kRendererID, url));
+  p->RevokeAllPermissionsForFile(kRendererID, file);
+  EXPECT_EQ(all_denied, GetAllPermissions(p, kRendererID, file));
+  EXPECT_EQ(all_denied, GetAllPermissionsForURL(p, kRendererID, url));
+
+  PermissionsSet create_write;
+  create_write.EnableCreate().EnableWrite();
+  p->GrantCreateWriteFile(kRendererID, file);
+  EXPECT_EQ(create_write, GetAllPermissions(p, kRendererID, file));
+  EXPECT_EQ(create_write, GetAllPermissionsForURL(p, kRendererID, url));
+  p->RevokeAllPermissionsForFile(kRendererID, file);
+  EXPECT_EQ(all_denied, GetAllPermissions(p, kRendererID, file));
+  EXPECT_EQ(all_denied, GetAllPermissionsForURL(p, kRendererID, url));
+
+  // Test revoke permissions on renderer ID removal.
+  p->GrantCreateReadWriteFile(kRendererID, file);
+  EXPECT_EQ(create_read_write, GetAllPermissions(p, kRendererID, file));
+  EXPECT_EQ(create_read_write, GetAllPermissionsForURL(p, kRendererID, url));
   p->Remove(kRendererID);
-  p->Add(kRendererID);
+  EXPECT_EQ(all_denied, GetAllPermissions(p, kRendererID, file));
+  EXPECT_EQ(all_denied, GetAllPermissionsForURL(p, kRendererID, url));
 
-  EXPECT_FALSE(p->CanReadFile(kRendererID,
-                              base::FilePath(TEST_PATH("/etc/passwd"))));
-  EXPECT_FALSE(p->CanReadFile(kRendererID,
-                              base::FilePath(TEST_PATH("/etc/shadow"))));
+  // Test having no permissions upon re-adding same renderer ID.
+  p->Add(kRendererID);
+  EXPECT_EQ(all_denied, GetAllPermissions(p, kRendererID, file));
+  EXPECT_EQ(all_denied, GetAllPermissionsForURL(p, kRendererID, url));
 
+  // Cleanup.
   p->Remove(kRendererID);
 }