Revert 57816 - disallow extension permissions in hosted apps

BUG=53323
TEST=ExtensionManifestTest.DisallowExtensionPermissions

Review URL: http://codereview.chromium.org/3228005

TBR=erikkay@chromium.org
Review URL: http://codereview.chromium.org/3235006

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

3 files changed, 36 insertions, 138 deletions

diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index 6f4de9a..0a0a9c4 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -155,20 +155,9 @@ const char* Extension::kPermissionNames[] = {
 const size_t Extension::kNumPermissions =
     arraysize(Extension::kPermissionNames);
 
-const char* Extension::kHostedAppPermissionNames[] = {
-  Extension::kBackgroundPermission,
-  Extension::kGeolocationPermission,
-  Extension::kNotificationPermission,
-  Extension::kUnlimitedStoragePermission,
-  Extension::kNativeClientPermission
-};
-const size_t Extension::kNumHostedAppPermissions =
-    arraysize(Extension::kHostedAppPermissionNames);
-
 // We purposefully don't put this into kPermissionNames.
 const char* Extension::kOldUnlimitedStoragePermission = "unlimited_storage";
 
-// static
 const Extension::SimplePermissions& Extension::GetSimplePermissions() {
   static SimplePermissions permissions;
   if (permissions.empty()) {
@@ -182,16 +171,6 @@ const Extension::SimplePermissions& Extension::GetSimplePermissions() {
   return permissions;
 }
 
-// static
-bool Extension::IsHostedAppPermission(const std::string& str) {
-  for (size_t i = 0; i < Extension::kNumHostedAppPermissions; ++i) {
-    if (str == Extension::kHostedAppPermissionNames[i]) {
-      return true;
-    }
-  }
-  return false;
-}
-
 Extension::~Extension() {
 }
 
@@ -1408,20 +1387,6 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_key,
       return false;  // Failed to parse browser action definition.
   }
 
-  // Load App settings.
-  if (!LoadIsApp(manifest_value_.get(), error) ||
-      !LoadExtent(manifest_value_.get(), keys::kWebURLs, &web_extent_,
-                  errors::kInvalidWebURLs, errors::kInvalidWebURL, error) ||
-      !LoadExtent(manifest_value_.get(), keys::kBrowseURLs, &browse_extent_,
-                  errors::kInvalidBrowseURLs, errors::kInvalidBrowseURL,
-                  error) ||
-      !EnsureNotHybridApp(manifest_value_.get(), error) ||
-      !LoadLaunchURL(manifest_value_.get(), error) ||
-      !LoadLaunchContainer(manifest_value_.get(), error) ||
-      !LoadLaunchFullscreen(manifest_value_.get(), error)) {
-    return false;
-  }
-
   // Initialize the permissions (optional).
   if (source.HasKey(keys::kPermissions)) {
     ListValue* permissions = NULL;
@@ -1443,18 +1408,10 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_key,
       if (permission_str == kOldUnlimitedStoragePermission)
         permission_str = kUnlimitedStoragePermission;
 
-      if (web_extent().is_empty()) {
-        // Check if it's a module permission.  If so, enable that permission.
-        if (IsAPIPermission(permission_str)) {
-          api_permissions_.push_back(permission_str);
-          continue;
-        }
-      } else {
-        // Hosted apps only get access to a subset of the valid permissions.
-        if (IsHostedAppPermission(permission_str)) {
-          api_permissions_.push_back(permission_str);
-          continue;
-        }
+      // Check if it's a module permission.  If so, enable that permission.
+      if (IsAPIPermission(permission_str)) {
+        api_permissions_.push_back(permission_str);
+        continue;
       }
 
       // Otherwise, it's a host pattern permission.
@@ -1547,6 +1504,19 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_key,
     devtools_url_ = GetResourceURL(devtools_str);
   }
 
+  if (!LoadIsApp(manifest_value_.get(), error) ||
+      !LoadExtent(manifest_value_.get(), keys::kWebURLs, &web_extent_,
+                  errors::kInvalidWebURLs, errors::kInvalidWebURL, error) ||
+      !LoadExtent(manifest_value_.get(), keys::kBrowseURLs, &browse_extent_,
+                  errors::kInvalidBrowseURLs, errors::kInvalidBrowseURL,
+                  error) ||
+      !EnsureNotHybridApp(manifest_value_.get(), error) ||
+      !LoadLaunchURL(manifest_value_.get(), error) ||
+      !LoadLaunchContainer(manifest_value_.get(), error) ||
+      !LoadLaunchFullscreen(manifest_value_.get(), error)) {
+    return false;
+  }
+
   // Although |source| is passed in as a const, it's still possible to modify
   // it.  This is dangerous since the utility process re-uses |source| after
   // it calls InitFromValue, passing it up to the browser process which calls
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index d8ce48d..f8c977d9 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -113,8 +113,6 @@ class Extension {
 
   static const char* kPermissionNames[];
   static const size_t kNumPermissions;
-  static const char* kHostedAppPermissionNames[];
-  static const size_t kNumHostedAppPermissions;
 
   // The old name for the unlimited storage permission, which is deprecated but
   // still accepted as meaning the same thing as kUnlimitedStoragePermission.
@@ -127,10 +125,6 @@ class Extension {
   typedef std::map<std::string, string16> SimplePermissions;
   static const SimplePermissions& GetSimplePermissions();
 
-  // Returns true if the string is one of the known hosted app permissions (see
-  // kHostedAppPermissionNames).
-  static bool IsHostedAppPermission(const std::string& permission);
-
   // An NPAPI plugin included in the extension.
   struct PluginInfo {
     FilePath path;  // Path to the plugin.
diff --git a/chrome/common/extensions/extension_manifests_unittest.cc b/chrome/common/extensions/extension_manifests_unittest.cc
index eb07b6d..c2ebfaa 100644
--- a/chrome/common/extensions/extension_manifests_unittest.cc
+++ b/chrome/common/extensions/extension_manifests_unittest.cc
@@ -17,32 +17,32 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace errors = extension_manifest_errors;
-namespace keys = extension_manifest_keys;
 
 class ExtensionManifestTest : public testing::Test {
  public:
   ExtensionManifestTest() : enable_apps_(true) {}
 
  protected:
-  DictionaryValue* LoadManifestFile(const std::string& filename,
-                                    std::string* error) {
+  Extension* LoadExtension(const std::string& name,
+                           std::string* error) {
+    return LoadExtensionWithLocation(name, Extension::INTERNAL, error);
+  }
+
+  Extension* LoadExtensionWithLocation(const std::string& name,
+                                       Extension::Location location,
+                                       std::string* error) {
     FilePath path;
     PathService::Get(chrome::DIR_TEST_DATA, &path);
     path = path.AppendASCII("extensions")
         .AppendASCII("manifest_tests")
-        .AppendASCII(filename.c_str());
+        .AppendASCII(name.c_str());
     EXPECT_TRUE(file_util::PathExists(path));
 
     JSONFileValueSerializer serializer(path);
-    return static_cast<DictionaryValue*>(serializer.Deserialize(NULL, error));
-  }
-
-  Extension* LoadExtensionWithLocation(DictionaryValue* value,
-                                       Extension::Location location,
-                                       std::string* error) {
-    FilePath path;
-    PathService::Get(chrome::DIR_TEST_DATA, &path);
-    path = path.AppendASCII("extensions").AppendASCII("manifest_tests");
+    scoped_ptr<DictionaryValue> value(
+        static_cast<DictionaryValue*>(serializer.Deserialize(NULL, error)));
+    if (!value.get())
+      return NULL;
 
     scoped_ptr<Extension> extension(new Extension(path.DirName()));
     extension->set_location(location);
@@ -54,68 +54,25 @@ class ExtensionManifestTest : public testing::Test {
     return extension.release();
   }
 
-  Extension* LoadExtension(const std::string& name,
-                           std::string* error) {
-    return LoadExtensionWithLocation(name, Extension::INTERNAL, error);
-  }
-
-  Extension* LoadExtension(DictionaryValue* value,
-                           std::string* error) {
-    return LoadExtensionWithLocation(value, Extension::INTERNAL, error);
-  }
-
-  Extension* LoadExtensionWithLocation(const std::string& name,
-                                       Extension::Location location,
-                                       std::string* error) {
-    scoped_ptr<DictionaryValue> value(LoadManifestFile(name, error));
-    if (!value.get())
-      return NULL;
-    return LoadExtensionWithLocation(value.get(), location, error);
-  }
-
   Extension* LoadAndExpectSuccess(const std::string& name) {
     std::string error;
     Extension* extension = LoadExtension(name, &error);
-    EXPECT_TRUE(extension) << name;
-    EXPECT_EQ("", error) << name;
+    EXPECT_TRUE(extension);
+    EXPECT_EQ("", error);
     return extension;
   }
 
-  Extension* LoadAndExpectSuccess(DictionaryValue* manifest,
-                                  const std::string& name) {
+  void LoadAndExpectError(const std::string& name,
+                          const std::string& expected_error) {
     std::string error;
-    Extension* extension = LoadExtension(manifest, &error);
-    EXPECT_TRUE(extension) << "Unexpected success for " << name;
-    EXPECT_EQ("", error) << "Unexpected no error for " << name;
-    return extension;
-  }
-
-  void VerifyExpectedError(Extension* extension,
-                           const std::string& name,
-                           const std::string& error,
-                           const std::string& expected_error) {
-    EXPECT_FALSE(extension) <<
+    scoped_ptr<Extension> extension(LoadExtension(name, &error));
+    EXPECT_FALSE(extension.get()) <<
         "Expected failure loading extension '" << name <<
         "', but didn't get one.";
     EXPECT_TRUE(MatchPatternASCII(error, expected_error)) << name <<
         " expected '" << expected_error << "' but got '" << error << "'";
   }
 
-  void LoadAndExpectError(const std::string& name,
-                          const std::string& expected_error) {
-    std::string error;
-    scoped_ptr<Extension> extension(LoadExtension(name, &error));
-    VerifyExpectedError(extension.get(), name, error, expected_error);
-  }
-
-  void LoadAndExpectError(DictionaryValue* manifest,
-                          const std::string& name,
-                          const std::string& expected_error) {
-    std::string error;
-    scoped_ptr<Extension> extension(LoadExtension(manifest, &error));
-    VerifyExpectedError(extension.get(), name, error, expected_error);
-  }
-
   bool enable_apps_;
 };
 
@@ -312,26 +269,3 @@ TEST_F(ExtensionManifestTest, DisallowHybridApps) {
   LoadAndExpectError("disallow_hybrid_2.json",
                      errors::kHostedAppsCannotIncludeExtensionFeatures);
 }
-
-TEST_F(ExtensionManifestTest, DisallowExtensionPermissions) {
-  std::string error;
-  scoped_ptr<DictionaryValue> manifest(
-      LoadManifestFile("valid_app.json", &error));
-  ASSERT_TRUE(manifest.get());
-
-  ListValue *permissions = new ListValue();
-  manifest->Set(keys::kPermissions, permissions);
-  for (size_t i = 0; i < Extension::kNumPermissions; i++) {
-    const char* name = Extension::kPermissionNames[i];
-    StringValue* p = new StringValue(name);
-    permissions->Clear();
-    permissions->Append(p);
-    std::string message_name = StringPrintf("permission-%s", name);
-    if (Extension::IsHostedAppPermission(name)) {
-      LoadAndExpectSuccess(manifest.get(), message_name);
-    } else {
-      LoadAndExpectError(manifest.get(), message_name, 
-                         errors::kInvalidPermission);
-    }
-  }
-}