Extension white and force lists (set by policy) should have priority over auto-updated Google black…

chrome/browser/extensions/extension_prefs.cc

Index: chrome/browser/extensions/extension_prefs.cc
diff --git a/chrome/browser/extensions/extension_prefs.cc b/chrome/browser/extensions/extension_prefs.cc
index e2ed4ea0e9b6caf7dfc7ddb998348ab0974e2f88..ee66e8416a4c3f02cb4376ada20a1cd1b85935af 100644
--- a/chrome/browser/extensions/extension_prefs.cc
+++ b/chrome/browser/extensions/extension_prefs.cc
@@ -271,6 +271,31 @@ bool ScopeToPrefKey(ExtensionPrefsScope scope, std::string* result) {
   return false;
 }
 
+// Reads a boolean pref from |ext| with key |pref_key|.
+// Return false if the value is false or |pref_key| does not exist.
+bool ReadBooleanFromPref(const DictionaryValue* ext,
+                         const std::string& pref_key) {
+  bool bool_value = false;
+  ext->GetBoolean(pref_key, &bool_value);
+  return bool_value;
+}
+
+// Reads an integer pref from |ext| with key |pref_key|.
+// Return false if the value does not exist.
+bool ReadIntegerFromPref(const DictionaryValue* ext,
+                         const std::string& pref_key, int* out_value) {

[Mattias Nissler (ping if slow), 2012/08/08 08:08:37]

all parameters on separate lines

[qfel, 2012/08/13 16:03:01]

Done.

+  if (!ext->GetInteger(pref_key, out_value))
+    return false;
+  return out_value != NULL;
+}
+
+// Checks if kPrefBlacklist is set to true in the DictionaryValue.
+// Return false if the value is false or kPrefBlacklist does not exist.
+// This is used to decide if an extension is blacklisted.
+bool IsBlacklistBitSet(const DictionaryValue* ext) {
+  return ReadBooleanFromPref(ext, kPrefBlacklist);
+}
+
 }  // namespace
 
 ExtensionPrefs::ExtensionPrefs(
@@ -398,14 +423,6 @@ DictionaryValue* ExtensionPrefs::CopyCurrentExtensions() {
   return new DictionaryValue;
 }
 
-// static
-bool ExtensionPrefs::ReadBooleanFromPref(
-    const DictionaryValue* ext, const std::string& pref_key) {
-  bool bool_value = false;
-  ext->GetBoolean(pref_key, &bool_value);
-  return bool_value;
-}
-
 bool ExtensionPrefs::ReadExtensionPrefBoolean(
     const std::string& extension_id, const std::string& pref_key) const {
   const DictionaryValue* ext = GetExtensionPref(extension_id);
@@ -416,15 +433,6 @@ bool ExtensionPrefs::ReadExtensionPrefBoolean(
   return ReadBooleanFromPref(ext, pref_key);
 }
 
-// static
-bool ExtensionPrefs::ReadIntegerFromPref(
-    const DictionaryValue* ext, const std::string& pref_key, int* out_value) {
-  if (!ext->GetInteger(pref_key, out_value))
-    return false;
-
-  return out_value != NULL;
-}
-
 bool ExtensionPrefs::ReadExtensionPrefInteger(
     const std::string& extension_id, const std::string& pref_key,
     int* out_value) const {
@@ -549,13 +557,33 @@ void ExtensionPrefs::SetExtensionPrefPermissionSet(
   }
 }
 
-// static
-bool ExtensionPrefs::IsBlacklistBitSet(const DictionaryValue* ext) {
-  return ReadBooleanFromPref(ext, kPrefBlacklist);
+bool ExtensionPrefs::IsExtensionBlacklisted(
+      const std::string& extension_id) const {
+  return IsExtensionBlacklisted(extension_id, prefs_);
 }
 
-bool ExtensionPrefs::IsExtensionBlacklisted(const std::string& extension_id) {
-  return ReadExtensionPrefBoolean(extension_id, kPrefBlacklist);
+// static
+bool ExtensionPrefs::IsExtensionBlacklisted(const std::string& extension_id,
+                                            const PrefService* pref_service) {
+  const DictionaryValue* dict = pref_service->GetDictionary(kExtensionsPref);
+  if (!dict)
+    return false;
+  const DictionaryValue* ext = NULL;
+  if (!dict->GetDictionary(extension_id, &ext)) {
+    // No such extension yet.
+    return false;
+  }
+  if (!IsBlacklistBitSet(ext))
+    return false;
+
+  // Check if policy settings override the blacklist.
+  const base::ListValue* whitelist =
+      pref_service->GetList(prefs::kExtensionInstallAllowList);
+  const base::ListValue* forcelist =
+      pref_service->GetList(prefs::kExtensionInstallForceList);
+  base::StringValue id(extension_id);
+  return (!whitelist || whitelist->Find(id) == whitelist->end()) &&
+         (!forcelist || forcelist->Find(id) == forcelist->end());

[Mattias Nissler (ping if slow), 2012/08/08 08:08:37]

Blacklisting has recently been abstracted into
chrome/browser/extensions/management_policy.h and we should use that for this as
well.

In fact, the main caller of this in crx_installer does the blacklist check and
the policy check right immediately after each other, so I suggest we consolidate
there.

 }
 
 bool ExtensionPrefs::IsExtensionOrphaned(const std::string& extension_id) {
@@ -1516,26 +1544,17 @@ const DictionaryValue* ExtensionPrefs::GetExtensionPref(
   return extension;
 }
 
-// Helper function for GetInstalledExtensionsInfo.
-static ExtensionInfo* GetInstalledExtensionInfoImpl(
+ExtensionInfo* ExtensionPrefs::GetInstalledExtensionInfoImpl(
     DictionaryValue* extension_data,
-    DictionaryValue::key_iterator extension_id) {
+    DictionaryValue::key_iterator extension_id) const {

[Mattias Nissler (ping if slow), 2012/08/08 08:08:37]

any reason to pass an iterator here instead of the id?

[qfel, 2012/08/13 16:03:01]

I guess the original author did it mechanically when factoring out common code,
but doesn't seem to serve a deeper purpose. Changed.

   DictionaryValue* ext;
   if (!extension_data->GetDictionaryWithoutPathExpansion(*extension_id, &ext)) {
     LOG(WARNING) << "Invalid pref for extension " << *extension_id;
     NOTREACHED();
     return NULL;
   }
-  if (ext->HasKey(kPrefBlacklist)) {
-    bool is_blacklisted = false;
-    if (!ext->GetBoolean(kPrefBlacklist, &is_blacklisted)) {
-      NOTREACHED() << "Invalid blacklist pref:" << *extension_id;
-      return NULL;
-    }
-    if (is_blacklisted) {
+  if (IsExtensionBlacklisted(*extension_id))

[Mattias Nissler (ping if slow), 2012/08/08 08:08:37]

so you get the extension dictionary passed, and you still make another lookup?
That doesn't seem right.

       return NULL;
-    }
-  }
   int state_value;
   if (!ext->GetInteger(kPrefState, &state_value)) {
     // This can legitimately happen if we store preferences for component
@@ -1804,16 +1823,20 @@ void ExtensionPrefs::GetExtensions(ExtensionIdSet* out) {
 
 // static
 ExtensionPrefs::ExtensionIdSet ExtensionPrefs::GetExtensionsFrom(
-    const base::DictionaryValue* extension_prefs) {
+    const PrefService* pref_service) {
   ExtensionIdSet result;
+
+  const base::DictionaryValue* extension_prefs;
+  const base::Value* extension_prefs_value =
+      pref_service->GetUserPrefValue(kExtensionsPref);
+  if (!extension_prefs_value ||
+      !extension_prefs_value->GetAsDictionary(&extension_prefs)) {
+    return result;  // Empty set
+  }
+
   for (base::DictionaryValue::key_iterator it = extension_prefs->begin_keys();
        it != extension_prefs->end_keys(); ++it) {
-    const DictionaryValue* ext;
-    if (!extension_prefs->GetDictionaryWithoutPathExpansion(*it, &ext)) {
-      NOTREACHED() << "Invalid pref for extension " << *it;
-      continue;
-    }
-    if (!IsBlacklistBitSet(ext))
+    if (!IsExtensionBlacklisted(*it, pref_service))
       result.push_back(*it);
   }
   return result;