Add support for command line switches to Features, and as proof that it works,

use it for the experimental permission.

BUG=440194
R=rockot@chromium.org

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

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

chrome/browser/extensions/crx_installer_browsertest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/extensions/crx_installer.h"
 
 #include "base/at_exit.h"
+#include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/download/download_crx_util.h"
@@ -37,7 +38,9 @@
 #include "extensions/common/extension.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/file_util.h"
+#include "extensions/common/permissions/api_permission.h"
 #include "extensions/common/permissions/permission_set.h"
+#include "extensions/common/permissions/permissions_data.h"
 #include "extensions/common/switches.h"
 #include "ui/base/l10n/l10n_util.h"
 
@@ -159,10 +162,27 @@ class ManagementPolicyMock : public extensions::ManagementPolicy::Provider {
   }
 };
 
+// Appends "enable-experimental-extension-apis" to the command line for the
+// lifetime of this class.
+class ScopedExperimentalCommandLine {
+ public:
+  ScopedExperimentalCommandLine() : saved_(*CommandLine::ForCurrentProcess()) {
+    CommandLine::ForCurrentProcess()->AppendSwitch(
+        switches::kEnableExperimentalExtensionApis);
+  }
+
+  ~ScopedExperimentalCommandLine() {
+    *CommandLine::ForCurrentProcess() = saved_;
+  }
+
+ private:
+  CommandLine saved_;
+};
+
 }  // namespace
 
 class ExtensionCrxInstallerTest : public ExtensionBrowserTest {
- public:
+ protected:
   scoped_ptr<WebstoreInstaller::Approval> GetApproval(
       const char* manifest_dir,
       const std::string& id,
@@ -219,8 +239,7 @@ class ExtensionCrxInstallerTest : public ExtensionBrowserTest {
   // |record_oauth2_grant| is true.
   void CheckHasEmptyScopesAfterInstall(const std::string& ext_relpath,
                                        bool record_oauth2_grant) {
-    CommandLine::ForCurrentProcess()->AppendSwitch(
-        switches::kEnableExperimentalExtensionApis);
+    ScopedExperimentalCommandLine scope;
 
     scoped_refptr<MockPromptProxy> mock_prompt =
         CreateMockPromptProxyForBrowser(browser());
@@ -233,6 +252,18 @@ class ExtensionCrxInstallerTest : public ExtensionBrowserTest {
             ->GetGrantedPermissions(mock_prompt->extension_id());
     ASSERT_TRUE(permissions.get());
   }
+
+  // Returns a FilePath to an unpacked "experimental" extension (a test
+  // Extension which requests the "experimental" permission).
+  base::FilePath PackExperimentalExtension() {
+    // We must modify the command line temporarily in order to pack an
+    // extension that requests the experimental permission.
+    ScopedExperimentalCommandLine scope;
+    base::FilePath test_path = test_data_dir_.AppendASCII("experimental");
+    base::FilePath crx_path = PackExtension(test_path);
+    CHECK(!crx_path.empty()) << "Extension not found at " << test_path.value();
+    return crx_path;
+  }
 };
 
 // This test is skipped on ChromeOS because it requires the NPAPI,
@@ -253,27 +284,41 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest, Whitelisting) {
 #endif
 
 IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
-                       GalleryInstallGetsExperimental) {
-  // We must modify the command line temporarily in order to pack an extension
-  // that requests the experimental permission.
-  CommandLine* command_line = CommandLine::ForCurrentProcess();
-  CommandLine old_command_line = *command_line;
-  command_line->AppendSwitch(switches::kEnableExperimentalExtensionApis);
-  base::FilePath crx_path = PackExtension(
-      test_data_dir_.AppendASCII("experimental"));
-  ASSERT_FALSE(crx_path.empty());
+                       ExperimentalExtensionFromGallery) {
+  // Gallery-installed extensions should have their experimental permission
+  // preserved, since we allow the Webstore to make that decision.
+  base::FilePath crx_path = PackExperimentalExtension();
+  const Extension* extension = InstallExtensionFromWebstore(crx_path, 1);
+  ASSERT_TRUE(extension);
+  EXPECT_TRUE(extension->permissions_data()->HasAPIPermission(
+      APIPermission::kExperimental));
+}
 
-  // Now reset the command line so that we are testing specifically whether
-  // installing from webstore enables experimental permissions.
-  *(CommandLine::ForCurrentProcess()) = old_command_line;
+IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
+                       ExperimentalExtensionFromOutsideGallery) {
+  // Non-gallery-installed extensions should lose their experimental
+  // permission if the flag isn't enabled.
+  base::FilePath crx_path = PackExperimentalExtension();
+  const Extension* extension = InstallExtension(crx_path, 1);
+  ASSERT_TRUE(extension);
+  EXPECT_FALSE(extension->permissions_data()->HasAPIPermission(
+      APIPermission::kExperimental));
+}
 
-  EXPECT_FALSE(InstallExtension(crx_path, 0));
-  EXPECT_TRUE(InstallExtensionFromWebstore(crx_path, 1));
+IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest,
+                       ExperimentalExtensionFromOutsideGalleryWithFlag) {
+  // Non-gallery-installed extensions should maintain their experimental
+  // permission if the flag is enabled.
+  base::FilePath crx_path = PackExperimentalExtension();
+  ScopedExperimentalCommandLine scope;
+  const Extension* extension = InstallExtension(crx_path, 1);
+  ASSERT_TRUE(extension);
+  EXPECT_TRUE(extension->permissions_data()->HasAPIPermission(
+      APIPermission::kExperimental));
 }
 
 IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest, PlatformAppCrx) {
-  CommandLine::ForCurrentProcess()->AppendSwitch(
-      switches::kEnableExperimentalExtensionApis);
+  ScopedExperimentalCommandLine scope;
   EXPECT_TRUE(InstallExtension(
       test_data_dir_.AppendASCII("minimal_platform_app.crx"), 1));
 }

chrome/common/extensions/api/_permission_features.json

@@ -379,6 +379,7 @@
   },
   "experimental": {
     "channel": "stable",
+    "command_line_switch": "experimental-extension-apis",
     "extension_types": [
       "extension", "legacy_packaged_app", "hosted_app", "platform_app"
     ]

chrome/common/extensions/manifest_tests/extension_manifests_experimental_unittest.cc

@@ -14,7 +14,10 @@
 namespace errors = extensions::manifest_errors;
 
 TEST_F(ChromeManifestTest, ExperimentalPermission) {
-  LoadAndExpectError("experimental.json", errors::kExperimentalFlagRequired);
+  LoadAndExpectWarning(
+      "experimental.json",
+      "'experimental' requires the 'experimental-extension-apis' "
+      "command line switch to be enabled.");
   LoadAndExpectSuccess("experimental.json", extensions::Manifest::COMPONENT);
   LoadAndExpectSuccess("experimental.json", extensions::Manifest::INTERNAL,
                        extensions::Extension::FROM_WEBSTORE);

chrome/common/extensions/manifest_tests/extension_manifests_isolatedapp_unittest.cc

@@ -19,9 +19,10 @@ class IsolatedAppsManifestTest : public ChromeManifestTest {
 };
 
 TEST_F(IsolatedAppsManifestTest, IsolatedApps) {
-  // Requires --enable-experimental-extension-apis
-  LoadAndExpectError("isolated_app_valid.json",
-                     errors::kExperimentalFlagRequired);
+  LoadAndExpectWarning(
+      "isolated_app_valid.json",
+      "'experimental' requires the 'experimental-extension-apis' "
+      "command line switch to be enabled.");
 
   CommandLine::ForCurrentProcess()->AppendSwitch(
       extensions::switches::kEnableExperimentalExtensionApis);

chrome/common/extensions/manifest_tests/extension_manifests_platformapp_unittest.cc

@@ -129,10 +129,12 @@ TEST_F(PlatformAppsManifestTest, CertainApisRequirePlatformApps) {
     manifests.push_back(make_linked_ptr(manifest->DeepCopy()));
   }
 
-  // First try to load without any flags. This should fail for every API.
+  // First try to load without any flags. This should warn for every API.
   for (size_t i = 0; i < arraysize(kPlatformAppExperimentalApis); ++i) {
-    LoadAndExpectError(ManifestData(manifests[i].get(), ""),
-                       errors::kExperimentalFlagRequired);
+    LoadAndExpectWarning(
+        ManifestData(manifests[i].get(), ""),
+        "'experimental' requires the 'experimental-extension-apis' "
+        "command line switch to be enabled.");
   }
 
   // Now try again with the experimental flag set.

extensions/common/features/feature.cc

@@ -11,6 +11,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/manifest.h"
 
 namespace extensions {
 
@@ -43,6 +44,13 @@ Feature::Availability Feature::IsAvailableToExtension(
                                extension->manifest_version());
 }
 
+Feature::Availability Feature::IsAvailableToEnvironment() const {
+  return IsAvailableToManifest("",  // extension_id
+                               Manifest::TYPE_UNKNOWN,
+                               Manifest::INVALID_LOCATION,
+                               -1);  // manifest_version
+}
+
 Feature::Feature() : no_parent_(false) {}
 
 Feature::~Feature() {}

extensions/common/features/feature.h

@@ -73,6 +73,7 @@ class Feature {
     NOT_PRESENT,
     UNSUPPORTED_CHANNEL,
     FOUND_IN_BLACKLIST,
+    MISSING_COMMAND_LINE_SWITCH,
   };
 
   // Container for AvailabiltyResult that also exposes a user-visible error
@@ -149,6 +150,17 @@ class Feature {
                                              const GURL& url,
                                              Context context) const = 0;
 
+  // Returns true if the feature is available to the current environment,
+  // without needing to know information about an Extension or any other
+  // contextual information. Typically used when the Feature is purely
+  // configured by command line flags and/or Chrome channel.
+  //
+  // Generally try not to use this function. Even if you don't think a Feature
+  // relies on an Extension now - maybe it will, one day, so if there's an
+  // Extension available (or a runtime context, etc) then use the more targeted
+  // method instead.
+  Availability IsAvailableToEnvironment() const;
+
   virtual bool IsIdInBlacklist(const std::string& extension_id) const = 0;
   virtual bool IsIdInWhitelist(const std::string& extension_id) const = 0;
 

extensions/common/features/simple_feature.cc

@@ -248,13 +248,21 @@ std::string HashExtensionId(const std::string& extension_id) {
   return base::HexEncode(id_hash.c_str(), id_hash.length());
 }
 
+bool IsCommandLineSwitchEnabled(const std::string& switch_name) {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  if (command_line->HasSwitch(switch_name + "=1"))
+    return true;
+  if (command_line->HasSwitch(std::string("enable-") + switch_name))
+    return true;
+  return false;
+}
+
 }  // namespace
 
 SimpleFeature::SimpleFeature()
     : location_(UNSPECIFIED_LOCATION),
       min_manifest_version_(0),
       max_manifest_version_(0),
-      has_parent_(false),
       component_extensions_auto_granted_(true) {}
 
 SimpleFeature::~SimpleFeature() {}
@@ -286,10 +294,11 @@ std::string SimpleFeature::Parse(const base::DictionaryValue* value) {
   no_parent_ = false;
   value->GetBoolean("noparent", &no_parent_);
 
-  component_extensions_auto_granted_ = true;
   value->GetBoolean("component_extensions_auto_granted",
                     &component_extensions_auto_granted_);
 
+  value->GetString("command_line_switch", &command_line_switch_);
+
   // NOTE: ideally we'd sanity check that "matches" can be specified if and
   // only if there's a "web_page" or "webui" context, but without
   // (Simple)Features being aware of their own heirarchy this is impossible.
@@ -367,6 +376,11 @@ Feature::Availability SimpleFeature::IsAvailableToManifest(
   if (max_manifest_version_ != 0 && manifest_version > max_manifest_version_)
     return CreateAvailability(INVALID_MAX_MANIFEST_VERSION, type);
 
+  if (!command_line_switch_.empty() &&
+      !IsCommandLineSwitchEnabled(command_line_switch_)) {
+    return CreateAvailability(MISSING_COMMAND_LINE_SWITCH, type);
+  }
+
   for (FilterList::const_iterator filter_iter = filters_.begin();
        filter_iter != filters_.end();
        ++filter_iter) {
@@ -481,6 +495,10 @@ std::string SimpleFeature::GetAvailabilityMessage(
       return base::StringPrintf(
           "'%s' is unsupported in this version of the platform.",
           name().c_str());
+    case MISSING_COMMAND_LINE_SWITCH:
+      return base::StringPrintf(
+          "'%s' requires the '%s' command line switch to be enabled.",
+          name().c_str(), command_line_switch_.c_str());
   }
 
   NOTREACHED();

extensions/common/features/simple_feature.h

@@ -41,6 +41,10 @@ class SimpleFeature : public Feature {
 
   // Accessors defined for testing. See comment above about not directly using
   // SimpleFeature in production code.
+  std::set<std::string>* blacklist() { return &blacklist_; }
+  std::set<std::string>* whitelist() { return &whitelist_; }
+  std::set<Manifest::Type>* extension_types() { return &extension_types_; }
+  std::set<Context>* contexts() { return &contexts_; }
   Location location() const { return location_; }
   void set_location(Location location) { location_ = location; }
   int min_manifest_version() const { return min_manifest_version_; }
@@ -51,11 +55,12 @@ class SimpleFeature : public Feature {
   void set_max_manifest_version(int max_manifest_version) {
     max_manifest_version_ = max_manifest_version;
   }
-
-  std::set<std::string>* blacklist() { return &blacklist_; }
-  std::set<std::string>* whitelist() { return &whitelist_; }
-  std::set<Manifest::Type>* extension_types() { return &extension_types_; }
-  std::set<Context>* contexts() { return &contexts_; }
+  const std::string& command_line_switch() const {
+    return command_line_switch_;
+  }
+  void set_command_line_switch(const std::string& command_line_switch) {
+    command_line_switch_ = command_line_switch;
+  }
 
   // Dependency resolution is a property of Features that is preferrably
   // handled internally to avoid temptation, but FeatureFilters may need
@@ -141,8 +146,8 @@ class SimpleFeature : public Feature {
   std::set<Platform> platforms_;
   int min_manifest_version_;
   int max_manifest_version_;
-  bool has_parent_;
   bool component_extensions_auto_granted_;
+  std::string command_line_switch_;
 
   typedef std::vector<linked_ptr<SimpleFeatureFilter> > FilterList;
   FilterList filters_;

extensions/common/features/simple_feature_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "base/command_line.h"
 #include "base/values.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/value_builder.h"
@@ -37,6 +38,21 @@ bool LocationIsAvailable(SimpleFeature::Location feature_location,
   return availability_result == Feature::IS_AVAILABLE;
 }
 
+class ScopedCommandLineSwitch {
+ public:
+  explicit ScopedCommandLineSwitch(const std::string& arg)
+      : original_command_line_(*base::CommandLine::ForCurrentProcess()) {
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(arg);
+  }
+
+  ~ScopedCommandLineSwitch() {
+    *base::CommandLine::ForCurrentProcess() = original_command_line_;
+  }
+
+ private:
+  base::CommandLine original_command_line_;
+};
+
 }  // namespace
 
 TEST(SimpleFeatureTest, IsAvailableNullCase) {
@@ -649,4 +665,38 @@ TEST(SimpleFeatureTest, Inheritance) {
   EXPECT_EQ(3, feature.max_manifest_version());
 }
 
+TEST(SimpleFeatureTest, CommandLineSwitch) {
+  SimpleFeature feature;
+  feature.set_command_line_switch("laser-beams");
+  {
+    EXPECT_EQ(Feature::MISSING_COMMAND_LINE_SWITCH,
+              feature.IsAvailableToEnvironment().result());
+  }
+  {
+    ScopedCommandLineSwitch scoped_switch("laser-beams");
+    EXPECT_EQ(Feature::MISSING_COMMAND_LINE_SWITCH,
+              feature.IsAvailableToEnvironment().result());
+  }
+  {
+    ScopedCommandLineSwitch scoped_switch("enable-laser-beams");
+    EXPECT_EQ(Feature::IS_AVAILABLE,
+              feature.IsAvailableToEnvironment().result());
+  }
+  {
+    ScopedCommandLineSwitch scoped_switch("disable-laser-beams");
+    EXPECT_EQ(Feature::MISSING_COMMAND_LINE_SWITCH,
+              feature.IsAvailableToEnvironment().result());
+  }
+  {
+    ScopedCommandLineSwitch scoped_switch("laser-beams=1");
+    EXPECT_EQ(Feature::IS_AVAILABLE,
+              feature.IsAvailableToEnvironment().result());
+  }
+  {
+    ScopedCommandLineSwitch scoped_switch("laser-beams=0");
+    EXPECT_EQ(Feature::MISSING_COMMAND_LINE_SWITCH,
+              feature.IsAvailableToEnvironment().result());
+  }
+}
+
 }  // namespace extensions

extensions/common/manifest_constants.cc

@@ -271,10 +271,6 @@ const char kChromeVersionTooLow[] =
 const char kDisabledByPolicy[] =
     "This extension has been disabled by your administrator.";
 const char kExpectString[] = "Expect string value.";
-const char kExperimentalFlagRequired[] =
-    "Loading extensions with 'experimental' permission is turned off by "
-    "default. You can enable 'Experimental Extension APIs' "
-    "by visiting chrome://flags.";
 const char kInvalidAboutPage[] = "Invalid value for 'about_page'.";
 const char kInvalidAboutPageExpectRelativePath[] =
     "Invalid value for 'about_page'. Value must be a relative path.";

extensions/common/manifest_constants.h

@@ -255,7 +255,6 @@ extern const char kChromeVersionTooLow[];
 extern const char kDevToolsExperimental[];
 extern const char kDisabledByPolicy[];
 extern const char kExpectString[];
-extern const char kExperimentalFlagRequired[];
 extern const char kInvalidAboutPage[];
 extern const char kInvalidAboutPageExpectRelativePath[];
 extern const char kInvalidAllFrames[];

extensions/common/manifest_handlers/permissions_parser.cc

@@ -46,26 +46,6 @@ ManifestPermissions::ManifestPermissions(
 ManifestPermissions::~ManifestPermissions() {
 }
 
-// Custom checks for the experimental permission that can't be expressed in
-// _permission_features.json.
-bool CanSpecifyExperimentalPermission(const Extension* extension) {
-  if (extension->location() == Manifest::COMPONENT)
-    return true;
-
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableExperimentalExtensionApis)) {
-    return true;
-  }
-
-  // We rely on the webstore to check access to experimental. This way we can
-  // whitelist extensions to have access to experimental in just the store, and
-  // not have to push a new version of the client.
-  if (extension->from_webstore())
-    return true;
-
-  return false;
-}
-
 // Checks whether the host |pattern| is allowed for the given |extension|,
 // given API permissions |permissions|.
 bool CanSpecifyHostPermission(const Extension* extension,
@@ -146,6 +126,16 @@ bool ParseHelper(Extension* extension,
       continue;
     }
 
+    // Sneaky check for "experimental", which we always allow for extensions
+    // installed from the Webstore. This way we can whitelist extensions to
+    // have access to experimental in just the store, and not have to push a
+    // new version of the client. Otherwise, experimental goes through the
+    // usual features check.
+    if (iter->id() == APIPermission::kExperimental &&
+        extension->from_webstore()) {
+      continue;
+    }
+
     Feature::Availability availability =
         feature->IsAvailableToExtension(extension);
     if (!availability.is_available()) {
@@ -157,13 +147,6 @@ bool ParseHelper(Extension* extension,
       to_remove.push_back(iter->id());
       continue;
     }
-
-    if (iter->id() == APIPermission::kExperimental) {
-      if (!CanSpecifyExperimentalPermission(extension)) {
-        *error = base::ASCIIToUTF16(errors::kExperimentalFlagRequired);
-        return false;
-      }
-    }
   }
 
   api_permissions->AddImpliedPermissions();