Deprecate MountPointProvider::IsAccessAllowed in favor of GetPermissionPolicy

webkit/fileapi/sandbox_mount_point_provider_unittest.cc

 // Copyright (c) 2012 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/sandbox_mount_point_provider.h"
 
 #include <set>
 
 #include "base/basictypes.h"
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/message_loop_proxy.h"
 #include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "webkit/fileapi/file_system_mount_point_provider.h"
+#include "webkit/fileapi/file_system_url.h"
 #include "webkit/fileapi/file_system_util.h"
 #include "webkit/fileapi/mock_file_system_options.h"
 
 namespace fileapi {
 
+namespace {
+
+FileSystemURL CreateFileSystemURL(const char* path) {
+  const GURL kOrigin("http://foo/");
+  return FileSystemURL::CreateForTest(
+      kOrigin, kFileSystemTypeTemporary, FilePath::FromUTF8Unsafe(path));
+}
+
+}  // namespace
+
 class SandboxMountPointProviderOriginEnumeratorTest : public testing::Test {
  public:
   void SetUp() {
     ASSERT_TRUE(data_dir_.CreateUniqueTempDir());
     sandbox_provider_.reset(
         new SandboxMountPointProvider(
             NULL,
             base::MessageLoopProxy::current(),
             data_dir_.path(),
             CreateAllowFileAccessOptions()));
@@ skipping 64 matching lines @@
     if (enumerator->HasFileSystemType(kFileSystemTypePersistent)) {
       ASSERT_TRUE(persistent_set.find(current) != persistent_set.end());
       ++persistent_actual_size;
     }
   }
 
   EXPECT_EQ(temporary_size, temporary_actual_size);
   EXPECT_EQ(persistent_size, persistent_actual_size);
 }
 
+TEST(SandboxMountPointProviderTest, AccessPermissions) {
+  MessageLoop message_loop_;
+  SandboxMountPointProvider provider(
+      NULL, base::MessageLoopProxy::current(), FilePath(),
+      CreateAllowFileAccessOptions());
+
+  // Any access should be allowed in sandbox directory.
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_ALLOW,
+            provider.GetPermissionPolicy(CreateFileSystemURL("foo"),
+                                         kReadFilePermissions));
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_ALLOW,
+            provider.GetPermissionPolicy(CreateFileSystemURL("foo"),
+                                         kWriteFilePermissions));
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_ALLOW,
+            provider.GetPermissionPolicy(CreateFileSystemURL("foo"),
+                                         kCreateFilePermissions));
+
+  // Access to a path with parent references ('..') should be disallowed.
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_DENY,
+            provider.GetPermissionPolicy(CreateFileSystemURL("a/../b"),
+                                         kReadFilePermissions));
+
+  // Access from non-allowed scheme should be disallowed.
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_DENY,
+            provider.GetPermissionPolicy(
+                FileSystemURL::CreateForTest(
+                    GURL("unknown://bar"), kFileSystemTypeTemporary,
+                    FilePath::FromUTF8Unsafe("foo")),
+                kReadFilePermissions));
+
+  // Access for non-sandbox type should be disallowed.
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_DENY,
+            provider.GetPermissionPolicy(
+                FileSystemURL::CreateForTest(
+                    GURL("http://foo/"), kFileSystemTypeTest,
+                    FilePath::FromUTF8Unsafe("foo")),
+                kReadFilePermissions));
+
+  // Create access with ristricted name should be disallowed.
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_DENY,
+            provider.GetPermissionPolicy(CreateFileSystemURL(".."),
+                                         kCreateFilePermissions));
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_DENY,
+            provider.GetPermissionPolicy(CreateFileSystemURL("."),
+                                         kCreateFilePermissions));
+
+  // Similar but safe cases.
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_ALLOW,
+            provider.GetPermissionPolicy(CreateFileSystemURL(" ."),
+                                         kCreateFilePermissions));
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_ALLOW,
+            provider.GetPermissionPolicy(CreateFileSystemURL(". "),
+                                         kCreateFilePermissions));
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_ALLOW,
+            provider.GetPermissionPolicy(CreateFileSystemURL(" .."),
+                                         kCreateFilePermissions));
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_ALLOW,
+            provider.GetPermissionPolicy(CreateFileSystemURL(".. "),
+                                         kCreateFilePermissions));
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_ALLOW,
+            provider.GetPermissionPolicy(CreateFileSystemURL("b."),
+                                         kCreateFilePermissions));
+  EXPECT_EQ(FILE_PERMISSION_ALWAYS_ALLOW,
+            provider.GetPermissionPolicy(CreateFileSystemURL(".b"),
+                                         kCreateFilePermissions));

[ericu, 2013/02/05 21:51:57]

I'd like to keep around FileSystemMountPointProviderTest.IsRestrictedName, which
tries to create a bunch of files illegal on Windows, and also tests that we
properly restrict "/" in filenames and "\\" everywhere.  Those have caught
multiple platform-specific bugs.

[kinuko, 2013/02/06 09:24:50]

They were important when we used to restrict them, but I think we're ready to
deprecate them.  The code we have today only performs simple string-matching
that has nothing to do with platform-specific illegal filenames, and as for '/'
and '\\' they're normalize in FileSystemURL constructor so the test result
wouldn't be as we expect.  Instead we have layout tests for all of those cases
which run in content_browsertests (thanks to your change) so that we still can
quickly spot problems.

[ericu, 2013/02/11 22:29:17]

The massive flakiness on some of our layout tests, which I fixed just a couple
of months ago, was due to us accidentally calling a FilePath method on a
filesystem path.  It only showed up when the path started with e.g. "c:", on
Windows.  That test passed, but then killed the next few tests due to leftover
files that were hard to clean up.  These kinds of tests are really useful for
those kinds of bugs.

We probably don't need all of them, but at least keep the
looks-like-a-drive-letter test and a handful of others.

[kinuko, 2013/02/12 08:19:52]

Ok, I added a test for that.

+}
+
 }  // namespace fileapi