Add the basic infrastructure for the Behavior feature type: BehaviorF…

…eature and _behavior_features.json. Arbitrarily use it to implement the allow-in-incognito whitelist. BUG=440194 R=rockot@chromium.org Review URL: https://codereview.chromium.org/789383002 Cr-Commit-Position: refs/heads/master@{#308311}
280723148 · Dec 15, 2014 · 6421032 · 6421032
1 parent 60e5f11
commit 6421032
Show file tree

Hide file tree

Showing 16 changed files with 130 additions and 23 deletions.
diff --git a/chrome/browser/extensions/extension_browsertest.cc b/chrome/browser/extensions/extension_browsertest.cc
@@ -234,7 +234,8 @@ ExtensionBrowserTest::LoadExtensionWithInstallParam(
     content::WindowedNotificationObserver load_signal(
         extensions::NOTIFICATION_EXTENSION_LOADED_DEPRECATED,
         content::Source<Profile>(profile()));
-    CHECK(!extensions::util::IsIncognitoEnabled(extension_id, profile()));
+    CHECK(!extensions::util::IsIncognitoEnabled(extension_id, profile()))
+        << extension_id << " is enabled in incognito, but shouldn't be";
 
     if (flags & kFlagEnableIncognito) {
       extensions::util::SetIsIncognitoEnabled(extension_id, profile(), true);

diff --git a/chrome/browser/extensions/extension_util.cc b/chrome/browser/extensions/extension_util.cc
@@ -24,7 +24,8 @@
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_icon_set.h"
 #include "extensions/common/feature_switch.h"
-#include "extensions/common/features/simple_feature.h"
+#include "extensions/common/features/behavior_feature.h"
+#include "extensions/common/features/feature_provider.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/manifest_handlers/incognito_info.h"
 #include "extensions/common/permissions/permissions_data.h"
@@ -41,20 +42,12 @@ namespace {
 const char kExtensionAllowedOnAllUrlsPrefName[] =
     "extension_can_script_all_urls";
 
-// Returns true if |extension_id| for an external component extension should
-// always be enabled in incognito windows.
-bool IsWhitelistedForIncognito(const std::string& extension_id) {
-  static const char* const kExtensionWhitelist[] = {
-    "D5736E4B5CF695CB93A2FB57E4FDC6E5AFAB6FE2",  // http://crbug.com/312900
-    "D57DE394F36DC1C3220E7604C575D29C51A6C495",  // http://crbug.com/319444
-    "3F65507A3B39259B38C8173C6FFA3D12DF64CCE9"   // http://crbug.com/371562
-  };
-
-  return extensions::SimpleFeature::IsIdInList(
-      extension_id,
-      std::set<std::string>(
-          kExtensionWhitelist,
-          kExtensionWhitelist + arraysize(kExtensionWhitelist)));
+// Returns true if |extension| should always be enabled in incognito mode.
+bool IsWhitelistedForIncognito(const Extension* extension) {
+  return FeatureProvider::GetBehaviorFeatures()
+      ->GetFeature(BehaviorFeature::kWhitelistedForIncognito)
+      ->IsAvailableToExtension(extension)
+      .is_available();
 }
 
 // Returns |extension_id|. See note below.
@@ -90,12 +83,9 @@ bool IsIncognitoEnabled(const std::string& extension_id,
     // work in incognito mode.
     if (extension->location() == Manifest::COMPONENT)
       return true;
-    if (extension->location() == Manifest::EXTERNAL_COMPONENT &&
-        IsWhitelistedForIncognito(extension_id)) {
+    if (IsWhitelistedForIncognito(extension))
       return true;
-    }
   }
-
   return ExtensionPrefs::Get(context)->IsIncognitoEnabled(extension_id);
 }
 

diff --git a/chrome/common/extensions/chrome_extensions_client.cc b/chrome/common/extensions/chrome_extensions_client.cc
@@ -32,6 +32,7 @@
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/features/api_feature.h"
 #include "extensions/common/features/base_feature_provider.h"
+#include "extensions/common/features/behavior_feature.h"
 #include "extensions/common/features/feature_provider.h"
 #include "extensions/common/features/json_feature_provider_source.h"
 #include "extensions/common/features/manifest_feature.h"
@@ -144,6 +145,9 @@ scoped_ptr<FeatureProvider> ChromeExtensionsClient::CreateFeatureProvider(
   } else if (name == "permission") {
     provider.reset(new BaseFeatureProvider(source->dictionary(),
                                            CreateFeature<PermissionFeature>));
+  } else if (name == "behavior") {
+    provider.reset(new BaseFeatureProvider(source->dictionary(),
+                                           CreateFeature<BehaviorFeature>));
   } else {
     NOTREACHED();
   }
@@ -164,6 +168,8 @@ ChromeExtensionsClient::CreateFeatureProviderSource(
   } else if (name == "permission") {
     source->LoadJSON(IDR_EXTENSION_PERMISSION_FEATURES);
     source->LoadJSON(IDR_CHROME_EXTENSION_PERMISSION_FEATURES);
+  } else if (name == "behavior") {
+    source->LoadJSON(IDR_EXTENSION_BEHAVIOR_FEATURES);
   } else {
     NOTREACHED();
     source.reset();

diff --git a/extensions/common/BUILD.gn b/extensions/common/BUILD.gn
@@ -100,6 +100,8 @@ if (enable_extensions) {
       "features/simple_feature.h",
       "features/simple_feature_filter.cc",
       "features/simple_feature_filter.h",
+      "features/behavior_feature.cc",
+      "features/behavior_feature.h",
       "file_util.cc",
       "file_util.h",
       "guest_view/guest_view_constants.cc",

diff --git a/extensions/common/api/_behavior_features.json b/extensions/common/api/_behavior_features.json
@@ -0,0 +1,33 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// This features file defines switches used to control Extension behaviour,
+// typically whitelist configuration.
+//
+// See extensions/common/features/* to understand this file, in particular
+// feature.h, simple_feature.h, and base_feature_provider.h.
+//
+// To add a new whitelisted ID, SHA-1 it and force it to uppercase. In Bash:
+//
+// $ echo -n "aaaabbbbccccddddeeeeffffgggghhhh" | \
+// sha1sum | tr '[:lower:]' '[:upper:]'
+// 9A0417016F345C934A1A88F55CA17C05014EEEBA  -
+//
+// Google employees: please update http://go/chrome-api-whitelist to map
+// hashes back to ids.
+
+{
+  "whitelisted_for_incognito": {
+    "channel": "stable",
+    "extension_types": "all",
+    // This is "external_component" for legacy reasons; it should be
+    // unnecessary given there's a whitelist.
+    "location": "external_component",
+    "whitelist": [
+      "D5736E4B5CF695CB93A2FB57E4FDC6E5AFAB6FE2",  // http://crbug.com/312900
+      "D57DE394F36DC1C3220E7604C575D29C51A6C495",  // http://crbug.com/319444
+      "3F65507A3B39259B38C8173C6FFA3D12DF64CCE9"   // http://crbug.com/371562
+    ]
+  }
+}
diff --git a/extensions/common/features/behavior_feature.cc b/extensions/common/features/behavior_feature.cc
@@ -0,0 +1,12 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/common/features/behavior_feature.h"
+
+namespace extensions {
+
+const char* BehaviorFeature::kWhitelistedForIncognito =
+    "whitelisted_for_incognito";
+
+}  // namespace extensions
diff --git a/extensions/common/features/behavior_feature.h b/extensions/common/features/behavior_feature.h
@@ -0,0 +1,27 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef EXTENSIONS_COMMON_FEATURES_BEHAVIOR_FEATURE_H_
+#define EXTENSIONS_COMMON_FEATURES_BEHAVIOR_FEATURE_H_
+
+#include <string>
+
+#include "extensions/common/features/simple_feature.h"
+
+namespace extensions {
+
+// Implementation of the features in _behavior_features.json.
+//
+// For now, this is just constants + a vacuous implementation of SimpleFeature,
+// for consistency with the other Feature types. One day we may add some
+// additional functionality. One day we may also generate the feature names.
+class BehaviorFeature : public SimpleFeature {
+ public:
+  // Constants corresponding to keys in _behavior_features.json.
+  static const char* kWhitelistedForIncognito;
+};
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_COMMON_FEATURES_BEHAVIOR_FEATURE_H_
diff --git a/extensions/common/features/feature_provider.cc b/extensions/common/features/feature_provider.cc
@@ -34,6 +34,8 @@ class Static {
         make_linked_ptr(client->CreateFeatureProvider("manifest").release());
     feature_providers_["permission"] =
         make_linked_ptr(client->CreateFeatureProvider("permission").release());
+    feature_providers_["behavior"] =
+        make_linked_ptr(client->CreateFeatureProvider("behavior").release());
   }
 
   typedef std::map<std::string, linked_ptr<FeatureProvider> >
@@ -66,4 +68,8 @@ const FeatureProvider* FeatureProvider::GetPermissionFeatures() {
   return GetByName("permission");
 }
 
+const FeatureProvider* FeatureProvider::GetBehaviorFeatures() {
+  return GetByName("behavior");
+}
+
 }  // namespace extensions
diff --git a/extensions/common/features/feature_provider.h b/extensions/common/features/feature_provider.h
@@ -37,6 +37,7 @@ class FeatureProvider {
   static const FeatureProvider* GetAPIFeatures();
   static const FeatureProvider* GetManifestFeatures();
   static const FeatureProvider* GetPermissionFeatures();
+  static const FeatureProvider* GetBehaviorFeatures();
 };
 
 }  // namespace extensions

diff --git a/extensions/common/features/simple_feature.cc b/extensions/common/features/simple_feature.cc
@@ -59,6 +59,8 @@ struct Mappings {
     contexts["webui"] = Feature::WEBUI_CONTEXT;
 
     locations["component"] = SimpleFeature::COMPONENT_LOCATION;
+    locations["external_component"] =
+        SimpleFeature::EXTERNAL_COMPONENT_LOCATION;
     locations["policy"] = SimpleFeature::POLICY_LOCATION;
 
     platforms["chromeos"] = Feature::CHROMEOS_PLATFORM;
@@ -570,8 +572,9 @@ bool SimpleFeature::MatchesManifestLocation(
     case SimpleFeature::UNSPECIFIED_LOCATION:
       return true;
     case SimpleFeature::COMPONENT_LOCATION:
-      // TODO(kalman/asargent): Should this include EXTERNAL_COMPONENT too?
       return manifest_location == Manifest::COMPONENT;
+    case SimpleFeature::EXTERNAL_COMPONENT_LOCATION:
+      return manifest_location == Manifest::EXTERNAL_COMPONENT;
     case SimpleFeature::POLICY_LOCATION:
       return manifest_location == Manifest::EXTERNAL_POLICY ||
              manifest_location == Manifest::EXTERNAL_POLICY_DOWNLOAD;

diff --git a/extensions/common/features/simple_feature.h b/extensions/common/features/simple_feature.h
@@ -27,15 +27,15 @@ class SimpleFeature : public Feature {
   ~SimpleFeature() override;
 
   // Similar to Manifest::Location, these are the classes of locations
-  // supported in feature files; "component" implies
-  // COMPONENT/EXTERNAL_COMPONENT manifest location types, etc.
+  // supported in feature files.
   //
   // This is only public for testing. Production code should never access it,
   // nor should it really have any reason to access the SimpleFeature class
   // directly, it should be dealing with the Feature interface.
   enum Location {
     UNSPECIFIED_LOCATION,
     COMPONENT_LOCATION,
+    EXTERNAL_COMPONENT_LOCATION,
     POLICY_LOCATION,
   };
 

diff --git a/extensions/common/features/simple_feature_unittest.cc b/extensions/common/features/simple_feature_unittest.cc
@@ -380,6 +380,8 @@ TEST(SimpleFeatureTest, Location) {
   // Component extensions can access any location.
   EXPECT_TRUE(LocationIsAvailable(SimpleFeature::COMPONENT_LOCATION,
                                   Manifest::COMPONENT));
+  EXPECT_TRUE(LocationIsAvailable(SimpleFeature::EXTERNAL_COMPONENT_LOCATION,
+                                  Manifest::COMPONENT));
   EXPECT_TRUE(
       LocationIsAvailable(SimpleFeature::POLICY_LOCATION, Manifest::COMPONENT));
   EXPECT_TRUE(LocationIsAvailable(SimpleFeature::UNSPECIFIED_LOCATION,
@@ -390,6 +392,8 @@ TEST(SimpleFeatureTest, Location) {
                                    Manifest::INVALID_LOCATION));
   EXPECT_FALSE(LocationIsAvailable(SimpleFeature::COMPONENT_LOCATION,
                                    Manifest::UNPACKED));
+  EXPECT_FALSE(LocationIsAvailable(SimpleFeature::COMPONENT_LOCATION,
+                                   Manifest::EXTERNAL_COMPONENT));
   EXPECT_FALSE(LocationIsAvailable(SimpleFeature::COMPONENT_LOCATION,
                                    Manifest::EXTERNAL_PREF_DOWNLOAD));
   EXPECT_FALSE(LocationIsAvailable(SimpleFeature::COMPONENT_LOCATION,
@@ -404,12 +408,19 @@ TEST(SimpleFeatureTest, Location) {
                                   Manifest::EXTERNAL_POLICY_DOWNLOAD));
 
   // Non-policy (except component) extensions cannot access policy.
+  EXPECT_FALSE(LocationIsAvailable(SimpleFeature::POLICY_LOCATION,
+                                   Manifest::EXTERNAL_COMPONENT));
   EXPECT_FALSE(LocationIsAvailable(SimpleFeature::POLICY_LOCATION,
                                    Manifest::INVALID_LOCATION));
   EXPECT_FALSE(
       LocationIsAvailable(SimpleFeature::POLICY_LOCATION, Manifest::UNPACKED));
   EXPECT_FALSE(LocationIsAvailable(SimpleFeature::POLICY_LOCATION,
                                    Manifest::EXTERNAL_PREF_DOWNLOAD));
+
+  // External component extensions can access the "external_component"
+  // location.
+  EXPECT_TRUE(LocationIsAvailable(SimpleFeature::EXTERNAL_COMPONENT_LOCATION,
+                                  Manifest::EXTERNAL_COMPONENT));
 }
 
 TEST(SimpleFeatureTest, Platform) {

diff --git a/extensions/extensions.gyp b/extensions/extensions.gyp
@@ -144,6 +144,8 @@
         'common/features/simple_feature.h',
         'common/features/simple_feature_filter.cc',
         'common/features/simple_feature_filter.h',
+        'common/features/behavior_feature.cc',
+        'common/features/behavior_feature.h',
         'common/file_util.cc',
         'common/file_util.h',
         'common/guest_view/guest_view_constants.cc',

diff --git a/extensions/extensions_resources.grd b/extensions/extensions_resources.grd
@@ -13,6 +13,7 @@
       <include name="IDR_EXTENSION_API_JSON_WEB_VIEW_REQUEST" file="common\api\web_view_request.json" type="BINDATA" />
       <include name="IDR_EXTENSION_MANIFEST_FEATURES" file="common\api\_manifest_features.json" type="BINDATA" />
       <include name="IDR_EXTENSION_PERMISSION_FEATURES" file="common\api\_permission_features.json" type="BINDATA" />
+      <include name="IDR_EXTENSION_BEHAVIOR_FEATURES" file="common\api\_behavior_features.json" type="BINDATA" />
     </includes>
   </release>
 </grit>
diff --git a/extensions/shell/common/shell_extensions_client.cc b/extensions/shell/common/shell_extensions_client.cc
@@ -11,6 +11,7 @@
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/features/api_feature.h"
 #include "extensions/common/features/base_feature_provider.h"
+#include "extensions/common/features/behavior_feature.h"
 #include "extensions/common/features/json_feature_provider_source.h"
 #include "extensions/common/features/manifest_feature.h"
 #include "extensions/common/features/permission_feature.h"
@@ -115,6 +116,9 @@ scoped_ptr<FeatureProvider> ShellExtensionsClient::CreateFeatureProvider(
   } else if (name == "permission") {
     provider.reset(new BaseFeatureProvider(source->dictionary(),
                                            CreateFeature<PermissionFeature>));
+  } else if (name == "behavior") {
+    provider.reset(new BaseFeatureProvider(source->dictionary(),
+                                           CreateFeature<BehaviorFeature>));
   } else {
     NOTREACHED();
   }
@@ -133,6 +137,8 @@ ShellExtensionsClient::CreateFeatureProviderSource(
     source->LoadJSON(IDR_EXTENSION_MANIFEST_FEATURES);
   } else if (name == "permission") {
     source->LoadJSON(IDR_EXTENSION_PERMISSION_FEATURES);
+  } else if (name == "behavior") {
+    source->LoadJSON(IDR_EXTENSION_BEHAVIOR_FEATURES);
   } else {
     NOTREACHED();
     source.reset();

diff --git a/extensions/test/test_extensions_client.cc b/extensions/test/test_extensions_client.cc
@@ -9,6 +9,7 @@
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/features/api_feature.h"
 #include "extensions/common/features/base_feature_provider.h"
+#include "extensions/common/features/behavior_feature.h"
 #include "extensions/common/features/feature_provider.h"
 #include "extensions/common/features/json_feature_provider_source.h"
 #include "extensions/common/features/manifest_feature.h"
@@ -74,6 +75,9 @@ scoped_ptr<FeatureProvider> TestExtensionsClient::CreateFeatureProvider(
   } else if (name == "permission") {
     provider.reset(new BaseFeatureProvider(source->dictionary(),
                                            CreateFeature<PermissionFeature>));
+  } else if (name == "behavior") {
+    provider.reset(new BaseFeatureProvider(source->dictionary(),
+                                           CreateFeature<BehaviorFeature>));
   } else {
     NOTREACHED();
   }
@@ -91,6 +95,8 @@ TestExtensionsClient::CreateFeatureProviderSource(
     source->LoadJSON(IDR_EXTENSION_MANIFEST_FEATURES);
   } else if (name == "permission") {
     source->LoadJSON(IDR_EXTENSION_PERMISSION_FEATURES);
+  } else if (name == "behavior") {
+    source->LoadJSON(IDR_EXTENSION_BEHAVIOR_FEATURES);
   } else {
     NOTREACHED();
     source.reset();