Require confirmation for writable directory access.

Previously, an app could request just the fileSystem.write permission,
followed by just the fileSystem.directory permission. This would prompt
the user to allow write access to files and read-only access to
directories, respectively. If the app were to later request both at the
same time, this would allow write access to directories without further
user confirmation.

This change adds an implicit fileSystem.writeDirectory if both
fileSystem.write and fileSystem.directory are requested at the same
time. This requires the user to confirm write access to directories.

BUG=148486

Review URL: https://chromiumcodereview.appspot.com/23506021

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@221620 0039d316-1c4b-4281-b951-d872f2087c98

chrome/common/extensions/permissions/api_permission.h

@@ -83,6 +83,7 @@ class APIPermission {
     kFileSystemDirectory,
     kFileSystemRetainEntries,
     kFileSystemWrite,
+    kFileSystemWriteDirectory,
     kFontSettings,
     kFullscreen,
     kGeolocation,

chrome/common/extensions/permissions/api_permission_set.cc

@@ -5,6 +5,7 @@
 #include "chrome/common/extensions/permissions/api_permission_set.h"
 
 #include "base/logging.h"
+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 #include "chrome/common/extensions/extension_manifest_constants.h"
@@ -335,4 +336,14 @@ bool APIPermissionSet::ParseFromJSON(
   return true;
 }
 
+void APIPermissionSet::AddImpliedPermissions() {
+  // The fileSystem.write and fileSystem.directory permissions imply
+  // fileSystem.writeDirectory.
+  // TODO(sammc): Remove this. See http://crbug.com/284849.
+  if (ContainsKey(map_, APIPermission::kFileSystemWrite) &&
+      ContainsKey(map_, APIPermission::kFileSystemDirectory)) {
+    insert(APIPermission::kFileSystemWriteDirectory);
+  }
+}
+
 }  // namespace extensions

chrome/common/extensions/permissions/api_permission_set.h

@@ -162,6 +162,8 @@ class APIPermissionSet {
       string16* error,
       std::vector<std::string>* unhandled_permissions);
 
+  void AddImpliedPermissions();
+
  private:
   APIPermissionMap map_;
 };

chrome/common/extensions/permissions/api_permission_set_unittest.cc

@@ -301,4 +301,33 @@ TEST(APIPermissionSetTest, IPC) {
   EXPECT_EQ(apis, expected_apis);
 }
 
+TEST(APIPermissionSetTest, ImplicitPermissions) {
+  APIPermissionSet apis;
+  apis.insert(APIPermission::kFileSystemWrite);
+  apis.AddImpliedPermissions();
+
+  EXPECT_EQ(apis.find(APIPermission::kFileSystemWrite)->id(),
+            APIPermission::kFileSystemWrite);
+  EXPECT_EQ(apis.size(), 1u);
+
+  apis.erase(APIPermission::kFileSystemWrite);
+  apis.insert(APIPermission::kFileSystemDirectory);
+  apis.AddImpliedPermissions();
+
+  EXPECT_EQ(apis.find(APIPermission::kFileSystemDirectory)->id(),
+            APIPermission::kFileSystemDirectory);
+  EXPECT_EQ(apis.size(), 1u);
+
+  apis.insert(APIPermission::kFileSystemWrite);
+  apis.AddImpliedPermissions();
+
+  EXPECT_EQ(apis.find(APIPermission::kFileSystemWrite)->id(),
+            APIPermission::kFileSystemWrite);
+  EXPECT_EQ(apis.find(APIPermission::kFileSystemDirectory)->id(),
+            APIPermission::kFileSystemDirectory);
+  EXPECT_EQ(apis.find(APIPermission::kFileSystemWriteDirectory)->id(),
+            APIPermission::kFileSystemWriteDirectory);
+  EXPECT_EQ(apis.size(), 3u);
+}
+
 }  // namespace extensions

chrome/common/extensions/permissions/chrome_api_permissions.cc

@@ -288,6 +288,10 @@ std::vector<APIPermissionInfo*> ChromeAPIPermissions::GetAllPermissions()
       APIPermissionInfo::kFlagNone,
       IDS_EXTENSION_PROMPT_WARNING_FILE_SYSTEM_WRITE,
       PermissionMessage::kFileSystemWrite },
+    { APIPermission::kFileSystemWriteDirectory, "fileSystem.writeDirectory",
+      APIPermissionInfo::kFlagNone,
+      IDS_EXTENSION_PROMPT_WARNING_FILE_SYSTEM_WRITE_DIRECTORY,
+      PermissionMessage::kFileSystemWriteDirectory },
     // Because warning messages for the "mediaGalleries" permission vary based
     // on the permissions parameters, no message ID or message text is
     // specified here.

chrome/common/extensions/permissions/permission_set.cc

@@ -482,21 +482,18 @@ std::set<PermissionMessage> PermissionSet::GetAPIPermissionMessages() const {
     }
   }
 
-  // A special hack: If both kFileSystemDirectory and and kFileSystemWrite
-  // would be displayed, instead show kFileSystemWriteDirectory.
-  // TODO(sammc): Remove this when http://crbug.com/282118 is fixed.
-  std::set<PermissionMessage>::iterator read_directory_message = messages.find(
-      PermissionMessage(PermissionMessage::kFileSystemDirectory, string16()));
-  std::set<PermissionMessage>::iterator write_message = messages.find(
-      PermissionMessage(PermissionMessage::kFileSystemWrite, string16()));
-  if (read_directory_message != messages.end() &&
-      write_message != messages.end()) {
-    messages.erase(read_directory_message);
-    messages.erase(write_message);
-    messages.insert(PermissionMessage(
-        PermissionMessage::kFileSystemWriteDirectory,
-        l10n_util::GetStringUTF16(
-            IDS_EXTENSION_PROMPT_WARNING_FILE_SYSTEM_WRITE_DIRECTORY)));
+  // A special hack: If kFileSystemWriteDirectory would be displayed, hide
+  // kFileSystemDirectory and and kFileSystemWrite as the write directory
+  // message implies the other two.
+  // TODO(sammc): Remove this. See http://crbug.com/284849.
+  std::set<PermissionMessage>::iterator write_directory_message =
+      messages.find(PermissionMessage(
+          PermissionMessage::kFileSystemWriteDirectory, string16()));
+  if (write_directory_message != messages.end()) {
+    messages.erase(
+        PermissionMessage(PermissionMessage::kFileSystemWrite, string16()));
+    messages.erase(
+        PermissionMessage(PermissionMessage::kFileSystemDirectory, string16()));
   }
   return messages;
 }
@@ -549,7 +546,7 @@ bool PermissionSet::HasLessAPIPrivilegesThan(
 
   // A special hack: kFileSystemWriteDirectory implies kFileSystemDirectory and
   // kFileSystemWrite.
-  // TODO(sammc): Remove this when http://crbug.com/282118 is fixed.
+  // TODO(sammc): Remove this. See http://crbug.com/284849.
   if (current_warnings.find(PermissionMessage(
           PermissionMessage::kFileSystemWriteDirectory, string16())) !=
       current_warnings.end()) {

chrome/common/extensions/permissions/permission_set_unittest.cc

@@ -762,32 +762,30 @@ TEST(PermissionsTest, PermissionMessages) {
   }
 }
 
-TEST(PermissionsTest, FileSystemWritePermissionMessages) {
+TEST(PermissionsTest, FileSystemPermissionMessages) {
   APIPermissionSet api_permissions;
   api_permissions.insert(APIPermission::kFileSystemWrite);
-  scoped_refptr<PermissionSet> permissions(
-      new PermissionSet(api_permissions, URLPatternSet(), URLPatternSet()));
-  PermissionMessages messages =
-      permissions->GetPermissionMessages(Manifest::TYPE_PLATFORM_APP);
-  ASSERT_EQ(1u, messages.size());
-  EXPECT_EQ(PermissionMessage::kFileSystemWrite, messages[0].id());
-}
-
-TEST(PermissionsTest, FileSystemDirectoryPermissionMessages) {
-  APIPermissionSet api_permissions;
   api_permissions.insert(APIPermission::kFileSystemDirectory);
   scoped_refptr<PermissionSet> permissions(
       new PermissionSet(api_permissions, URLPatternSet(), URLPatternSet()));
   PermissionMessages messages =
       permissions->GetPermissionMessages(Manifest::TYPE_PLATFORM_APP);
-  ASSERT_EQ(1u, messages.size());
-  EXPECT_EQ(PermissionMessage::kFileSystemDirectory, messages[0].id());
+  ASSERT_EQ(2u, messages.size());
+  std::sort(messages.begin(), messages.end());
+  std::set<PermissionMessage::ID> ids;
+  for (PermissionMessages::const_iterator it = messages.begin();
+       it != messages.end(); ++it) {
+    ids.insert(it->id());
+  }
+  EXPECT_TRUE(ContainsKey(ids, PermissionMessage::kFileSystemDirectory));
+  EXPECT_TRUE(ContainsKey(ids, PermissionMessage::kFileSystemWrite));
 }
 
-TEST(PermissionsTest, MergedFileSystemPermissionMessages) {
+TEST(PermissionsTest, HiddenFileSystemPermissionMessages) {
   APIPermissionSet api_permissions;
   api_permissions.insert(APIPermission::kFileSystemWrite);
   api_permissions.insert(APIPermission::kFileSystemDirectory);
+  api_permissions.insert(APIPermission::kFileSystemWriteDirectory);
   scoped_refptr<PermissionSet> permissions(
       new PermissionSet(api_permissions, URLPatternSet(), URLPatternSet()));
   PermissionMessages messages =
@@ -807,24 +805,24 @@ TEST(PermissionsTest, MergedFileSystemPermissionComparison) {
   scoped_refptr<PermissionSet> directory_permissions(new PermissionSet(
       directory_api_permissions, URLPatternSet(), URLPatternSet()));
 
-  APIPermissionSet both_api_permissions;
-  both_api_permissions.insert(APIPermission::kFileSystemWrite);
-  both_api_permissions.insert(APIPermission::kFileSystemDirectory);
-  scoped_refptr<PermissionSet> both_permissions(new PermissionSet(
-      both_api_permissions, URLPatternSet(), URLPatternSet()));
+  APIPermissionSet write_directory_api_permissions;
+  write_directory_api_permissions.insert(
+      APIPermission::kFileSystemWriteDirectory);
+  scoped_refptr<PermissionSet> write_directory_permissions(new PermissionSet(
+      write_directory_api_permissions, URLPatternSet(), URLPatternSet()));
 
-  EXPECT_FALSE(both_permissions->HasLessPrivilegesThan(
+  EXPECT_FALSE(write_directory_permissions->HasLessPrivilegesThan(
       write_permissions, Manifest::TYPE_PLATFORM_APP));
-  EXPECT_FALSE(both_permissions->HasLessPrivilegesThan(
+  EXPECT_FALSE(write_directory_permissions->HasLessPrivilegesThan(
       directory_permissions, Manifest::TYPE_PLATFORM_APP));
   EXPECT_TRUE(write_permissions->HasLessPrivilegesThan(
       directory_permissions, Manifest::TYPE_PLATFORM_APP));
   EXPECT_TRUE(write_permissions->HasLessPrivilegesThan(
-      both_permissions, Manifest::TYPE_PLATFORM_APP));
+      write_directory_permissions, Manifest::TYPE_PLATFORM_APP));
   EXPECT_TRUE(directory_permissions->HasLessPrivilegesThan(
       write_permissions, Manifest::TYPE_PLATFORM_APP));
   EXPECT_TRUE(directory_permissions->HasLessPrivilegesThan(
-      both_permissions, Manifest::TYPE_PLATFORM_APP));
+      write_directory_permissions, Manifest::TYPE_PLATFORM_APP));
 }
 
 TEST(PermissionsTest, GetWarningMessages_ManyHosts) {

chrome/common/extensions/permissions/permissions_data.cc

@@ -172,6 +172,8 @@ bool ParseHelper(Extension* extension,
     }
   }
 
+  api_permissions->AddImpliedPermissions();
+
   // Remove permissions that are not available to this extension.
   for (std::vector<APIPermission::ID>::const_iterator iter = to_remove.begin();
        iter != to_remove.end(); ++iter) {