Add a warning for missing fileBrowserHandler permission.

If file browser handlers are specified in the manifest, and the permission is
missing, then they will be ignored. This CL adds a warning so users are aware
of it.

TEST=unit_tests: *FileBrowserHandlerManifestTest.InvalidFileBrowserHandlers*
BUG=470494

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

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

chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc

@@ -11,6 +11,7 @@
 #include "base/values.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "extensions/common/error_utils.h"
+#include "extensions/common/install_warning.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/manifest_handlers/permissions_parser.h"
@@ -131,6 +132,7 @@ FileBrowserHandlerParser::FileBrowserHandlerParser() {
 FileBrowserHandlerParser::~FileBrowserHandlerParser() {
 }
 
+#if defined(OS_CHROMEOS)
 namespace {
 
 FileBrowserHandler* LoadFileBrowserHandler(
@@ -268,32 +270,43 @@ bool LoadFileBrowserHandlers(
 }
 
 }  // namespace
+#endif
 
 bool FileBrowserHandlerParser::Parse(extensions::Extension* extension,
                                      base::string16* error) {
-  // If the permission is missing, then skip parsing the list of handlers.
+#if !defined(OS_CHROMEOS)
+  return true;
+#else
+  const base::Value* file_browser_handlers_value = nullptr;
+  if (!extension->manifest()->Get(keys::kFileBrowserHandlers,
+                                  &file_browser_handlers_value)) {
+    return true;
+  }
+
   if (!extensions::PermissionsParser::HasAPIPermission(
           extension, extensions::APIPermission::ID::kFileBrowserHandler)) {
+    extension->AddInstallWarning(extensions::InstallWarning(
+        errors::kInvalidFileBrowserHandlerMissingPermission));
     return true;
   }
 
-  const base::ListValue* file_browser_handlers_value = NULL;
-  if (!extension->manifest()->GetList(keys::kFileBrowserHandlers,
-                                      &file_browser_handlers_value)) {
+  const base::ListValue* file_browser_handlers_list_value = nullptr;
+  if (!file_browser_handlers_value->GetAsList(
+          &file_browser_handlers_list_value)) {
     *error = base::ASCIIToUTF16(errors::kInvalidFileBrowserHandler);
     return false;
   }
 
   scoped_ptr<FileBrowserHandlerInfo> info(new FileBrowserHandlerInfo);
   if (!LoadFileBrowserHandlers(extension->id(),
-                               file_browser_handlers_value,
-                               &info->file_browser_handlers,
-                               error)) {
+                               file_browser_handlers_list_value,
+                               &info->file_browser_handlers, error)) {
     return false;  // Failed to parse file browser actions definition.
   }
 
   extension->SetManifestData(keys::kFileBrowserHandlers, info.release());
   return true;
+#endif
 }
 
 const std::vector<std::string> FileBrowserHandlerParser::Keys() const {

chrome/common/extensions/api/file_browser_handlers/file_browser_handler_manifest_unittest.cc

@@ -73,31 +73,35 @@ TEST_F(FileBrowserHandlerManifestTest, GetHandlersRequiresPermission) {
 
 TEST_F(FileBrowserHandlerManifestTest, InvalidFileBrowserHandlers) {
   Testcase testcases[] = {
-    Testcase("filebrowser_invalid_access_permission.json",
-             extensions::ErrorUtils::FormatErrorMessage(
-                 errors::kInvalidFileAccessValue, base::IntToString(1))),
-    Testcase("filebrowser_invalid_access_permission_list.json",
-             errors::kInvalidFileAccessList),
-    Testcase("filebrowser_invalid_empty_access_permission_list.json",
-             errors::kInvalidFileAccessList),
-    Testcase("filebrowser_invalid_actions_1.json",
-             errors::kInvalidFileBrowserHandler),
-    Testcase("filebrowser_invalid_actions_2.json",
-             errors::kInvalidFileBrowserHandler),
-    Testcase("filebrowser_invalid_action_id.json",
-             errors::kInvalidPageActionId),
-    Testcase("filebrowser_invalid_action_title.json",
-             errors::kInvalidPageActionDefaultTitle),
-    Testcase("filebrowser_invalid_file_filters_1.json",
-             errors::kInvalidFileFiltersList),
-    Testcase("filebrowser_invalid_file_filters_2.json",
-             extensions::ErrorUtils::FormatErrorMessage(
-                errors::kInvalidFileFilterValue, base::IntToString(0))),
-    Testcase("filebrowser_invalid_file_filters_url.json",
-             extensions::ErrorUtils::FormatErrorMessage(
-                errors::kInvalidURLPatternError, "http:*.html"))
-  };
+      Testcase("filebrowser_invalid_access_permission.json",
+               extensions::ErrorUtils::FormatErrorMessage(
+                   errors::kInvalidFileAccessValue, base::IntToString(1))),
+      Testcase("filebrowser_invalid_access_permission_list.json",
+               errors::kInvalidFileAccessList),
+      Testcase("filebrowser_invalid_empty_access_permission_list.json",
+               errors::kInvalidFileAccessList),
+      Testcase("filebrowser_invalid_value.json",
+               errors::kInvalidFileBrowserHandler),
+      Testcase("filebrowser_invalid_actions_1.json",
+               errors::kInvalidFileBrowserHandler),
+      Testcase("filebrowser_invalid_actions_2.json",
+               errors::kInvalidFileBrowserHandler),
+      Testcase("filebrowser_invalid_action_id.json",
+               errors::kInvalidPageActionId),
+      Testcase("filebrowser_invalid_action_title.json",
+               errors::kInvalidPageActionDefaultTitle),
+      Testcase("filebrowser_invalid_file_filters_1.json",
+               errors::kInvalidFileFiltersList),
+      Testcase("filebrowser_invalid_file_filters_2.json",
+               extensions::ErrorUtils::FormatErrorMessage(
+                   errors::kInvalidFileFilterValue, base::IntToString(0))),
+      Testcase("filebrowser_invalid_file_filters_url.json",
+               extensions::ErrorUtils::FormatErrorMessage(
+                   errors::kInvalidURLPatternError, "http:*.html"))};
   RunTestcases(testcases, arraysize(testcases), EXPECT_TYPE_ERROR);
+  RunTestcase(Testcase("filebrowser_missing_permission.json",
+                       errors::kInvalidFileBrowserHandlerMissingPermission),
+              EXPECT_TYPE_WARNING);
 }
 
 TEST_F(FileBrowserHandlerManifestTest, ValidFileBrowserHandler) {

chrome/test/data/extensions/manifest_tests/filebrowser_invalid_value.json

+{
+  "name": "test",
+  "version": "1",
+  "permissions": [
+    "fileBrowserHandler"
+  ],
+  "file_browser_handlers": 123
+}

chrome/test/data/extensions/manifest_tests/filebrowser_missing_permission.json

+{
+  "name": "test",
+  "version": "1",
+  "file_browser_handlers": [
+    {
+      "id": "test",
+      "default_title" : "Test",
+      "default_icon" : "icon.png",
+      "file_filters" : [
+        "filesystem:*.txt"
+      ]
+    }
+  ]
+}

extensions/common/manifest_constants.cc

@@ -361,6 +361,9 @@ const char kInvalidFileAccessValue[] =
     "Invalid value for 'file_access[*]'.";
 const char kInvalidFileBrowserHandler[] =
     "Invalid value for 'file_browser_handlers'.";
+const char kInvalidFileBrowserHandlerMissingPermission[] =
+    "Declaring file browser handlers requires the fileBrowserHandler manifest "
+    "permission.";
 const char kInvalidFileFiltersList[] =
     "Invalid value for 'file_filters'.";
 const char kInvalidFileFilterValue[] =

extensions/common/manifest_constants.h

@@ -302,7 +302,7 @@ extern const char kInvalidExportWhitelistString[];
 extern const char kInvalidFileAccessList[];
 extern const char kInvalidFileAccessValue[];
 extern const char kInvalidFileBrowserHandler[];
-extern const char kInvalidFileBrowserHandlerMIMETypes[];
+extern const char kInvalidFileBrowserHandlerMissingPermission[];
 extern const char kInvalidFileFiltersList[];
 extern const char kInvalidFileFilterValue[];
 extern const char kInvalidFileHandlers[];