Move the check for fileBrowserHandler permission to FileBrowserHandler.

The previous check location was forcing all apps and extensions which call
the chrome.fileSystem.requestFileSystem API, to have "fileBrowserHandler"
permission as well.

That however doesn't make much sense, as those apps are not always handlers.
This CL moves the check to FileBrowserHandler::GetHandler() so simply the
permission has to be used only if the app wants to be a handler.

TEST=unit_test: FileBrowserHandlerManifestTest.GetHandlersRequiresPermission
BUG=470494

Review URL: https://codereview.chromium.org/1030133002

Cr-Commit-Position: refs/heads/master@{#322343}

2 files changed, 104 insertions, 34 deletions

diff --git a/chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc b/chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc
index 92124d2..f722b1f 100644
--- a/chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc
+++ b/chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc
@@ -13,6 +13,8 @@
 #include "extensions/common/error_utils.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/manifest_constants.h"
+#include "extensions/common/manifest_handlers/permissions_parser.h"
+#include "extensions/common/permissions/api_permission.h"
 #include "extensions/common/url_pattern.h"
 #include "url/url_constants.h"
 
@@ -115,11 +117,12 @@ bool FileBrowserHandler::HasCreateAccessPermission() const {
 // static
 FileBrowserHandler::List*
 FileBrowserHandler::GetHandlers(const extensions::Extension* extension) {
-  FileBrowserHandlerInfo* info = static_cast<FileBrowserHandlerInfo*>(
+  FileBrowserHandlerInfo* const info = static_cast<FileBrowserHandlerInfo*>(
       extension->GetManifestData(keys::kFileBrowserHandlers));
-  if (info)
-    return &info->file_browser_handlers;
-  return NULL;
+  if (!info)
+    return nullptr;
+
+  return &info->file_browser_handlers;
 }
 
 FileBrowserHandlerParser::FileBrowserHandlerParser() {
@@ -268,12 +271,19 @@ bool LoadFileBrowserHandlers(
 
 bool FileBrowserHandlerParser::Parse(extensions::Extension* extension,
                                      base::string16* error) {
+  // If the permission is missing, then skip parsing the list of handlers.
+  if (!extensions::PermissionsParser::HasAPIPermission(
+          extension, extensions::APIPermission::ID::kFileBrowserHandler)) {
+    return true;
+  }
+
   const base::ListValue* file_browser_handlers_value = NULL;
   if (!extension->manifest()->GetList(keys::kFileBrowserHandlers,
                                       &file_browser_handlers_value)) {
     *error = base::ASCIIToUTF16(errors::kInvalidFileBrowserHandler);
     return false;
   }
+
   scoped_ptr<FileBrowserHandlerInfo> info(new FileBrowserHandlerInfo);
   if (!LoadFileBrowserHandlers(extension->id(),
                                file_browser_handlers_value,
diff --git a/chrome/common/extensions/api/file_browser_handlers/file_browser_handler_manifest_unittest.cc b/chrome/common/extensions/api/file_browser_handlers/file_browser_handler_manifest_unittest.cc
index ba36d5e..cfcaeef 100644
--- a/chrome/common/extensions/api/file_browser_handlers/file_browser_handler_manifest_unittest.cc
+++ b/chrome/common/extensions/api/file_browser_handlers/file_browser_handler_manifest_unittest.cc
@@ -24,6 +24,53 @@ namespace {
 class FileBrowserHandlerManifestTest : public ChromeManifestTest {
 };
 
+#if !defined(OS_CHROMEOS)
+TEST_F(FileBrowserHandlerManifestTest, PermissionNotAllowedOnNonChromeOS) {
+  RunTestcase(
+      Testcase("filebrowser_valid.json",
+               "'fileBrowserHandler' is not allowed for specified platform."),
+      EXPECT_TYPE_WARNING);
+}
+#else
+
+TEST_F(FileBrowserHandlerManifestTest, PermissionAllowed) {
+  RunTestcase(Testcase("filebrowser_valid.json"), EXPECT_TYPE_SUCCESS);
+}
+
+TEST_F(FileBrowserHandlerManifestTest, GetHandlersRequiresPermission) {
+  extensions::DictionaryBuilder bad_manifest_builder;
+  bad_manifest_builder.Set("name", "Foo")
+      .Set("version", "1.0.0")
+      .Set("manifest_version", 2)
+      .Set("file_browser_handlers",
+           extensions::ListBuilder().Append(
+               extensions::DictionaryBuilder()
+                   .Set("id", "open")
+                   .Set("default_title", "open")
+                   .Set("file_filters", extensions::ListBuilder()
+                                            .Append("filesystem:*.txt")
+                                            .Append("filesystem:*.html"))));
+  scoped_ptr<base::DictionaryValue> bad_manifest_value(
+      bad_manifest_builder.Build());
+
+  // Create a good manifest by extending the bad one with the missing
+  // permission.
+  extensions::DictionaryBuilder good_manifest_builder(
+      *make_scoped_ptr(bad_manifest_value->DeepCopy()).get());
+  good_manifest_builder.Set(
+      "permissions", extensions::ListBuilder().Append("fileBrowserHandler"));
+
+  extensions::ExtensionBuilder bad_app_builder;
+  bad_app_builder.SetManifest(bad_manifest_value.Pass());
+  scoped_refptr<extensions::Extension> bad_app(bad_app_builder.Build());
+  EXPECT_FALSE(FileBrowserHandler::GetHandlers(bad_app.get()));
+
+  extensions::ExtensionBuilder good_app_builder;
+  good_app_builder.SetManifest(good_manifest_builder);
+  scoped_refptr<extensions::Extension> good_app(good_app_builder.Build());
+  EXPECT_TRUE(FileBrowserHandler::GetHandlers(good_app.get()));
+}
+
 TEST_F(FileBrowserHandlerManifestTest, InvalidFileBrowserHandlers) {
   Testcase testcases[] = {
     Testcase("filebrowser_invalid_access_permission.json",
@@ -56,18 +103,22 @@ TEST_F(FileBrowserHandlerManifestTest, InvalidFileBrowserHandlers) {
 TEST_F(FileBrowserHandlerManifestTest, ValidFileBrowserHandler) {
   scoped_refptr<const Extension> extension =
       ExtensionBuilder()
-      .SetManifest(DictionaryBuilder()
+          .SetManifest(
+               DictionaryBuilder()
                    .Set("name", "file browser handler test")
                    .Set("version", "1.0.0")
                    .Set("manifest_version", 2)
-                   .Set("file_browser_handlers", ListBuilder()
-                       .Append(DictionaryBuilder()
-                           .Set("id", "ExtremelyCoolAction")
-                           .Set("default_title", "Be Amazed")
-                           .Set("default_icon", "icon.png")
-                           .Set("file_filters", ListBuilder()
-                               .Append("filesystem:*.txt")))))
-      .Build();
+                   .Set("permissions",
+                        extensions::ListBuilder().Append("fileBrowserHandler"))
+                   .Set("file_browser_handlers",
+                        ListBuilder().Append(
+                            DictionaryBuilder()
+                                .Set("id", "ExtremelyCoolAction")
+                                .Set("default_title", "Be Amazed")
+                                .Set("default_icon", "icon.png")
+                                .Set("file_filters", ListBuilder().Append(
+                                                         "filesystem:*.txt")))))
+          .Build();
 
   ASSERT_TRUE(extension.get());
   FileBrowserHandler::List* handlers =
@@ -91,19 +142,23 @@ TEST_F(FileBrowserHandlerManifestTest, ValidFileBrowserHandler) {
 TEST_F(FileBrowserHandlerManifestTest, ValidFileBrowserHandlerMIMETypes) {
   scoped_refptr<const Extension> extension =
       ExtensionBuilder()
-      .SetID(extension_misc::kQuickOfficeExtensionId)
-      .SetManifest(DictionaryBuilder()
+          .SetID(extension_misc::kQuickOfficeExtensionId)
+          .SetManifest(
+               DictionaryBuilder()
                    .Set("name", "file browser handler test")
                    .Set("version", "1.0.0")
                    .Set("manifest_version", 2)
-                   .Set("file_browser_handlers", ListBuilder()
-                       .Append(DictionaryBuilder()
-                           .Set("id", "ID")
-                           .Set("default_title", "Default title")
-                           .Set("default_icon", "icon.png")
-                           .Set("file_filters", ListBuilder()
-                               .Append("filesystem:*.txt")))))
-      .Build();
+                   .Set("permissions",
+                        extensions::ListBuilder().Append("fileBrowserHandler"))
+                   .Set("file_browser_handlers",
+                        ListBuilder().Append(
+                            DictionaryBuilder()
+                                .Set("id", "ID")
+                                .Set("default_title", "Default title")
+                                .Set("default_icon", "icon.png")
+                                .Set("file_filters", ListBuilder().Append(
+                                                         "filesystem:*.txt")))))
+          .Build();
 
   ASSERT_TRUE(extension.get());
   FileBrowserHandler::List* handlers =
@@ -121,20 +176,24 @@ TEST_F(FileBrowserHandlerManifestTest, ValidFileBrowserHandlerMIMETypes) {
 TEST_F(FileBrowserHandlerManifestTest, ValidFileBrowserHandlerWithCreate) {
   scoped_refptr<const Extension> extension =
       ExtensionBuilder()
-      .SetManifest(DictionaryBuilder()
+          .SetManifest(
+               DictionaryBuilder()
                    .Set("name", "file browser handler test create")
                    .Set("version", "1.0.0")
                    .Set("manifest_version", 2)
-                   .Set("file_browser_handlers", ListBuilder()
-                       .Append(DictionaryBuilder()
-                           .Set("id", "ID")
-                           .Set("default_title", "Default title")
-                           .Set("default_icon", "icon.png")
-                           .Set("file_filters", ListBuilder()
-                               .Append("filesystem:*.txt"))
-                           .Set("file_access", ListBuilder()
-                               .Append("create")))))
-      .Build();
+                   .Set("permissions",
+                        extensions::ListBuilder().Append("fileBrowserHandler"))
+                   .Set("file_browser_handlers",
+                        ListBuilder().Append(
+                            DictionaryBuilder()
+                                .Set("id", "ID")
+                                .Set("default_title", "Default title")
+                                .Set("default_icon", "icon.png")
+                                .Set("file_filters",
+                                     ListBuilder().Append("filesystem:*.txt"))
+                                .Set("file_access",
+                                     ListBuilder().Append("create")))))
+          .Build();
 
   ASSERT_TRUE(extension.get());
   FileBrowserHandler::List* handlers =
@@ -149,5 +208,6 @@ TEST_F(FileBrowserHandlerManifestTest, ValidFileBrowserHandlerWithCreate) {
   EXPECT_FALSE(action->CanRead());
   EXPECT_FALSE(action->CanWrite());
 }
+#endif
 
 }  // namespace