[Extensions] Add a SimpleFeature::Validate function

Currently, most feature validation happens in Parse(). Add a Validate()
function that can be called after Parse() so that if a feature is
generated, validation can still be performed (independent of whether or
not the feature is created from a JSON value).  In the future, these
validations should be performed as part of the compile process, but this
is a starting point.

Also update tests that parsed features and used JSONFeatureProvider only
to test this validation. This way, once we move away from
JSON-based features and JSONFeatureProvider, these tests are still
active.

BUG=280286

Review-Url: https://codereview.chromium.org/2150193002
Cr-Commit-Position: refs/heads/master@{#406076}

chrome/common/extensions/api/common_extension_api_unittest.cc

@@ -676,36 +676,16 @@ TEST(ExtensionAPITest, DefaultConfigurationFeatures) {
   }
 }
 
-TEST(ExtensionAPITest, FeaturesRequireContexts) {
-  // TODO(cduvall): Make this check API featues.
-  std::unique_ptr<base::DictionaryValue> api_features1(
+TEST(ExtensionAPITest, JSONFeatureProviderDoesNotStoreInvalidFeatures) {
+  base::DictionaryValue features;
+  std::unique_ptr<base::DictionaryValue> feature_value(
       new base::DictionaryValue());
-  std::unique_ptr<base::DictionaryValue> api_features2(
-      new base::DictionaryValue());
-  base::DictionaryValue* test1 = new base::DictionaryValue();
-  base::DictionaryValue* test2 = new base::DictionaryValue();
-  base::ListValue* contexts = new base::ListValue();
-  contexts->AppendString("content_script");
-  test1->Set("contexts", contexts);
-  test1->SetString("channel", "stable");
-  test2->SetString("channel", "stable");
-  api_features1->Set("test", test1);
-  api_features2->Set("test", test2);
-
-  struct {
-    base::DictionaryValue* api_features;
-    bool expect_success;
-  } test_data[] = {
-    { api_features1.get(), true },
-    { api_features2.get(), false }
-  };
-
-  for (size_t i = 0; i < arraysize(test_data); ++i) {
-    JSONFeatureProvider api_feature_provider(*test_data[i].api_features,
-                                             CreateAPIFeature);
-    Feature* feature = api_feature_provider.GetFeature("test");
-    EXPECT_EQ(test_data[i].expect_success, feature != NULL) << i;
-  }
+  // This feature is invalid (it needs an extension context), so the
+  // JSONFeatureProvider should not store it.
+  feature_value->SetString("channel", "stable");
+  features.Set("test", std::move(feature_value));
+  JSONFeatureProvider api_feature_provider(features, CreateAPIFeature);
+  EXPECT_FALSE(api_feature_provider.GetFeature("test"));
 }
 
 static void GetDictionaryFromList(const base::DictionaryValue* schema,

extensions/common/features/api_feature.cc

@@ -6,27 +6,21 @@
 
 namespace extensions {
 
-APIFeature::APIFeature()
-    : internal_(false) {}
+APIFeature::APIFeature() {}
 
 APIFeature::~APIFeature() {
 }
 
-bool APIFeature::IsInternal() const {
-  return internal_;
-}
-
-std::string APIFeature::Parse(const base::DictionaryValue* value) {
-  std::string error = SimpleFeature::Parse(value);
-  if (!error.empty())
-    return error;
-
-  value->GetBoolean("internal", &internal_);
+bool APIFeature::Validate(std::string* error) {
+  if (!SimpleFeature::Validate(error))
+    return false;
 
-  if (contexts()->empty())
-    return name() + ": API features must specify at least one context.";
+  if (contexts()->empty()) {
+    *error = name() + ": API features must specify at least one context.";
+    return false;
+  }
 
-  return std::string();
+  return true;
 }
 
 }  // namespace extensions

extensions/common/features/api_feature.h

@@ -17,12 +17,7 @@ class APIFeature : public SimpleFeature {
   ~APIFeature() override;
 
   // extensions::Feature:
-  bool IsInternal() const override;
-
-  std::string Parse(const base::DictionaryValue* value) override;
-
- private:
-  bool internal_;
+  bool Validate(std::string* error) override;
 };
 
 }  // namespace extensions

extensions/common/features/json_feature_provider.cc

@@ -35,10 +35,12 @@ bool ParseFeature(const base::DictionaryValue* value,
                   const std::string& name,
                   SimpleFeature* feature) {
   feature->set_name(name);
-  std::string error = feature->Parse(value);
-  if (!error.empty())
+  feature->Parse(value);
+  std::string error;
+  bool valid = feature->Validate(&error);
+  if (!valid)
     LOG(ERROR) << error;
-  return error.empty();
+  return valid;
 }
 
 }  // namespace

extensions/common/features/manifest_feature.cc

@@ -34,20 +34,22 @@ Feature::Availability ManifestFeature::IsAvailableToContext(
   return CreateAvailability(IS_AVAILABLE);
 }
 
-std::string ManifestFeature::Parse(const base::DictionaryValue* value) {
-  std::string error = SimpleFeature::Parse(value);
-  if (!error.empty())
-    return error;
+bool ManifestFeature::Validate(std::string* error) {
+  if (!SimpleFeature::Validate(error))
+    return false;
 
   if (extension_types()->empty()) {
-    return name() + ": Manifest features must specify at least one " +
-        "value for extension_types.";
+    *error = name() + ": Manifest features must specify at least one " +
+             "value for extension_types.";
+    return false;
   }
 
-  if (value->HasKey("contexts"))
-    return name() + ": Manifest features do not support contexts.";
+  if (!contexts()->empty()) {
+    *error = name() + ": Manifest features do not support contexts.";
+    return false;
+  }
 
-  return std::string();
+  return true;
 }
 
 }  // namespace extensions

extensions/common/features/manifest_feature.h

@@ -22,7 +22,7 @@ class ManifestFeature : public SimpleFeature {
       const GURL& url,
       Feature::Platform platform) const override;
 
-  std::string Parse(const base::DictionaryValue* value) override;
+  bool Validate(std::string* error) override;
 };
 
 }  // namespace extensions

extensions/common/features/permission_feature.cc

@@ -33,20 +33,22 @@ Feature::Availability PermissionFeature::IsAvailableToContext(
   return CreateAvailability(IS_AVAILABLE);
 }
 
-std::string PermissionFeature::Parse(const base::DictionaryValue* value) {
-  std::string error = SimpleFeature::Parse(value);
-  if (!error.empty())
-    return error;
+bool PermissionFeature::Validate(std::string* error) {
+  if (!SimpleFeature::Validate(error))
+    return false;
 
   if (extension_types()->empty()) {
-    return name() + ": Permission features must specify at least one " +
-        "value for extension_types.";
+    *error = name() + ": Permission features must specify at least one " +
+             "value for extension_types.";
+    return false;
   }
 
-  if (value->HasKey("contexts"))
-    return name() + ": Permission features do not support contexts.";
+  if (!contexts()->empty()) {
+    *error = name() + ": Permission features do not support contexts.";
+    return false;
+  }
 
-  return std::string();
+  return true;
 }
 
 }  // namespace extensions

extensions/common/features/permission_feature.h

@@ -21,8 +21,7 @@ class PermissionFeature : public SimpleFeature {
       Feature::Context context,
       const GURL& url,
       Feature::Platform platform) const override;
-
-  std::string Parse(const base::DictionaryValue* value) override;
+  bool Validate(std::string* error) override;
 };
 
 }  // namespace extensions

extensions/common/features/simple_feature.cc

@@ -310,11 +310,12 @@ SimpleFeature::SimpleFeature()
     : location_(UNSPECIFIED_LOCATION),
       min_manifest_version_(0),
       max_manifest_version_(0),
-      component_extensions_auto_granted_(true) {}
+      component_extensions_auto_granted_(true),
+      internal_(false) {}
 
 SimpleFeature::~SimpleFeature() {}
 
-std::string SimpleFeature::Parse(const base::DictionaryValue* dictionary) {
+void SimpleFeature::Parse(const base::DictionaryValue* dictionary) {
   static base::LazyInstance<SimpleFeature::Mappings> mappings =
       LAZY_INSTANCE_INITIALIZER;
 
@@ -358,6 +359,8 @@ std::string SimpleFeature::Parse(const base::DictionaryValue* dictionary) {
       channel_.reset(new version_info::Channel(version_info::Channel::UNKNOWN));
       ParseEnum<version_info::Channel>(value, channel_.get(),
                                        mappings.Get().channels);
+    } else if (key == "internal") {
+      value->GetAsBoolean(&internal_);
     }
   }
 
@@ -369,13 +372,18 @@ std::string SimpleFeature::Parse(const base::DictionaryValue* dictionary) {
   // and "matches" google.com/*. Then a sub-feature "foo.bar" might override
   // "matches" to be chromium.org/*. That sub-feature doesn't need to specify
   // "web_page" context because it's inherited, but we don't know that here.
+}
 
+bool SimpleFeature::Validate(std::string* error) {
+  DCHECK(error);
   // All features must be channel-restricted, either directly or through
   // dependents.
-  if (!channel_ && dependencies_.empty())
-    return name() + ": Must supply a value for channel or dependencies.";
+  if (!channel_ && dependencies_.empty()) {
+    *error = name() + ": Must supply a value for channel or dependencies.";
+    return false;
+  }
 
-  return std::string();
+  return true;
 }
 
 Feature::Availability SimpleFeature::IsAvailableToManifest(
@@ -583,7 +591,7 @@ Feature::Availability SimpleFeature::CreateAvailability(
 }
 
 bool SimpleFeature::IsInternal() const {
-  return false;
+  return internal_;
 }
 
 bool SimpleFeature::IsIdInBlacklist(const std::string& extension_id) const {

extensions/common/features/simple_feature.h

@@ -47,10 +47,13 @@ class SimpleFeature : public Feature {
   ~SimpleFeature() override;
 
   // Parses the JSON representation of a feature into the fields of this object.
-  // Unspecified values in the JSON are not modified in the object. This allows
-  // us to implement inheritance by parsing one value after another. Returns
-  // the error found, or an empty string on success.
-  virtual std::string Parse(const base::DictionaryValue* dictionary);
+  // Note: Validate() should be called after this.
+  void Parse(const base::DictionaryValue* dictionary);
+
+  // Checks whether the feature is valid. Invalid features should not be used.
+  // Subclasses can override to implement specific checking, but should always
+  // call this method as well.
+  virtual bool Validate(std::string* error);
 
   Availability IsAvailableToContext(const Extension* extension,
                                     Context context) const {
@@ -153,7 +156,9 @@ class SimpleFeature : public Feature {
   FRIEND_TEST_ALL_PREFIXES(ManifestUnitTest, Extension);
   FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, Blacklist);
   FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, CommandLineSwitch);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, ComplexFeatureAvailability);
   FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, Context);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, FeatureValidation);
   FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, HashedIdBlacklist);
   FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, HashedIdWhitelist);
   FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, Inheritance);
@@ -168,6 +173,7 @@ class SimpleFeature : public Feature {
   FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, ParsePlatforms);
   FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, ParseWhitelist);
   FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, Platform);
+  FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, SimpleFeatureAvailability);
   FRIEND_TEST_ALL_PREFIXES(SimpleFeatureTest, Whitelist);
 
   // Holds String to Enum value mappings.
@@ -200,6 +206,7 @@ class SimpleFeature : public Feature {
   bool component_extensions_auto_granted_;
   std::string command_line_switch_;
   std::unique_ptr<version_info::Channel> channel_;
+  bool internal_;
 
   DISALLOW_COPY_AND_ASSIGN(SimpleFeature);
 };