Enable/disable extensions upon changes in blacklist.

chrome/browser/extensions/extension_service.cc

Index: chrome/browser/extensions/extension_service.cc
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index b9c734071674fc1a52e7e0df6d7d737803cc8c26..ffd03d062a5c2658ac3a3d299b2dbe121ba70ad8 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -565,12 +565,24 @@ void ExtensionService::Init() {
     }
     system_->management_policy()->RegisterProvider(
         shared_module_policy_provider_.get());
+
+    LoadGreylistFromPrefs();
   }
 
   UMA_HISTOGRAM_TIMES("Extensions.ExtensionServiceInitTime",
                       base::Time::Now() - begin_time);
 }
 
+void ExtensionService::LoadGreylistFromPrefs() {
+  scoped_ptr<ExtensionSet> all_extensions = GenerateInstalledExtensionsSet();
+
+  for (ExtensionSet::const_iterator it = all_extensions->begin();
+       it != all_extensions->end(); ++it) {
+    if (extension_prefs_->IsExtensionGreylisted((*it)->id()))
+      greylist_.Insert(*it);
+  }
+}
+
 void ExtensionService::VerifyAllExtensions() {
   ExtensionIdSet to_add;
   scoped_ptr<ExtensionSet> all_extensions = GenerateInstalledExtensionsSet();
@@ -2759,27 +2771,73 @@ void ExtensionService::MaybeFinishDelayedInstallations() {
 }
 
 void ExtensionService::OnBlacklistUpdated() {
-  blacklist_->GetMalwareIDs(
+  blacklist_->GetBlacklistedIDs(
       GenerateInstalledExtensionsSet()->GetIDs(),
       base::Bind(&ExtensionService::ManageBlacklist, AsWeakPtr()));
 }
 
-void ExtensionService::ManageBlacklist(const std::set<std::string>& updated) {
+void ExtensionService::ManageBlacklist(
+    const extensions::Blacklist::BlacklistStateMap& state_map) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  std::set<std::string> before = registry_->blacklisted_extensions().GetIDs();
-  std::set<std::string> no_longer_blacklisted =
-      base::STLSetDifference<std::set<std::string> >(before, updated);
-  std::set<std::string> not_yet_blacklisted =
-      base::STLSetDifference<std::set<std::string> >(updated, before);
+  std::set<std::string> blocked;

[not at google - send to devlin, 2014/01/22 00:33:14]

(following on from comment in header file) personally I prefer the term
"malware" here since it's more specific than just "blocked"... term is a bit
abstract to me... but not a big deal.

[Oleg Eterevsky, 2014/01/22 21:25:04]

I think we should fix the terminology everywhere, but it will touch so many
files, that I'd prefer to do it in a separate CL. How about:

blacklist -- refer to Blacklist with all the possible states,

malware, potentially unwanted, ... -- refer to the reason for the action we
take, but not to the actual categories of extensions in service and
ExtensionRegistry

blocked -- what was formerly blacklisted. If you can come up with a better term,
I'm willing to change

greylist -- disabled once

This way, if we decide to change how we treat some type of extensions (like, we
decide to permanently block extensions violating web store policy), we'll just
change a could of switches and ifs.

[not at google - send to devlin, 2014/01/22 22:24:38]

I think this is actually the thing that needs to change. And I think based on
your comment you agree? Blacklist is no longer the right term for that class; to
me a blacklist really does mean a single list of things that are never allowed;
pulling a greylist out of that is odd.

As you say we can worry about that / discuss naming later.
I'm ok leaving this as blocked for now, but I'd ideally like to see this back as
blacklisted (see above comment).

[Oleg Eterevsky, 2014/01/23 14:48:54]

Ok, let's commit it like this and then discuss the best terminology on
chrome-apps-team@ or apps-dev@.

+  std::set<std::string> greylist;
+  std::set<std::string> unknown;

[not at google - send to devlin, 2014/01/22 00:33:14]

nit: could you call this "ignore" or "skip" everywhere rather than "unknown"?
I'm finding the code hard to parse as "unknown", it's too overloaded a term.

[Oleg Eterevsky, 2014/01/22 21:25:04]

Changed to 'unchanged'.

+  for (extensions::Blacklist::BlacklistStateMap::const_iterator it =
+           state_map.begin();
+       it != state_map.end();
+       ++it) {
+    switch (it->second) {
+      case extensions::NOT_BLACKLISTED:
+        break;
+
+      case extensions::BLACKLISTED_MALWARE:
+        blocked.insert(it->first);
+        break;
 
-  for (std::set<std::string>::iterator it = no_longer_blacklisted.begin();
-       it != no_longer_blacklisted.end(); ++it) {
+      case extensions::BLACKLISTED_SECURITY_VULNERABILITY:
+      case extensions::BLACKLISTED_CWS_POLICY_VIOLATION:
+      case extensions::BLACKLISTED_POTENTIALLY_UNWANTED:
+        greylist.insert(it->first);
+        break;
+
+      case extensions::BLACKLISTED_UNKNOWN:
+        unknown.insert(it->first);
+        break;
+    }
+  }
+
+  UpdateBlockedExtensions(blocked, unknown);
+  UpdateGreylistedExtensions(greylist, unknown);
+
+  IdentifyAlertableExtensions();
+}
+
+void ExtensionService::UpdateBlockedExtensions(
+    const std::set<std::string>& blocked,
+    const std::set<std::string>& unknown) {
+  std::set<std::string> blocked_and_unknown;
+  std::set_union(
+      blocked.begin(), blocked.end(), unknown.begin(), unknown.end(),
+      std::inserter(blocked_and_unknown, blocked_and_unknown.end()));
+
+  // Extensions with unknown blacklist state will remain blocked or unblocked as
+  // before this update.
+  std::set<std::string> blocked_before =
+      registry_->blacklisted_extensions().GetIDs();
+  std::set<std::string> no_longer_blocked =
+      base::STLSetDifference<std::set<std::string> >(
+          blocked_before, blocked_and_unknown);
+  std::set<std::string> not_yet_blocked =
+      base::STLSetDifference<std::set<std::string> >(blocked, blocked_before);
+
+  for (std::set<std::string>::iterator it = no_longer_blocked.begin();
+       it != no_longer_blocked.end(); ++it) {
     scoped_refptr<const Extension> extension =
         registry_->blacklisted_extensions().GetByID(*it);
     if (!extension.get()) {
-      NOTREACHED() << "Extension " << *it << " no longer blacklisted, "
-                   << "but it was never blacklisted.";
+      NOTREACHED() << "Extension " << *it << " no longer blocked, "
+                   << "but it was never blocked.";
       continue;
     }
     registry_->RemoveBlacklisted(*it);
@@ -2790,8 +2848,8 @@ void ExtensionService::ManageBlacklist(const std::set<std::string>& updated) {
                               Manifest::NUM_LOCATIONS);
   }
 
-  for (std::set<std::string>::iterator it = not_yet_blacklisted.begin();
-       it != not_yet_blacklisted.end(); ++it) {
+  for (std::set<std::string>::iterator it = not_yet_blocked.begin();
+       it != not_yet_blocked.end(); ++it) {
     scoped_refptr<const Extension> extension = GetInstalledExtension(*it);
     if (!extension.get()) {
       NOTREACHED() << "Extension " << *it << " needs to be "
@@ -2805,7 +2863,50 @@ void ExtensionService::ManageBlacklist(const std::set<std::string>& updated) {
                               extension->location(), Manifest::NUM_LOCATIONS);
   }
 
-  IdentifyAlertableExtensions();
+}
+
+void ExtensionService::UpdateGreylistedExtensions(

[not at google - send to devlin, 2014/01/22 00:33:14]

seems like this method is basically the same as UpdateBlockedExtensions just
dealing with different sets, and with UMA logging above (should you be doing UMA
logging here too?).

it's probably too contrived to actually make them share all of the code, but you
should at least be able to pull out the set division (lines 2871 - 2882) into a
sensible common place.

[Oleg Eterevsky, 2014/01/22 21:25:04]

I'm not sure what exactly to log. Is it ok if I add a todo, and do it in a
separate CL?
Well, if there were just one resulting set, it would simplify the code, but I
feel that in the current state an extra function will not be much help. If you
wish, I can add STLSetUnion to stl_unil add use it.

[not at google - send to devlin, 2014/01/22 22:24:38]

I suppose you might as well do it in a follow up. I just mean to say that
tracking this stuff in UMA would be useful, and it occurs to me that greylisting
is even more subtle than blacklisting. How often are extensions disabled from
the greylist? How often are they re-enabled by the user? Etc. Based on that a
follow-up SGTM.
I'm imagining something like:

void Partition(const ExtensionIdSet& before,
               const ExtensionIdSet& after,
               const ExtensionIdSet& unchanged,
               ExtensionIdSet* no_longer,
               ExtensionIdSet* not_yet) {
  ExtensionIdSet before_and_unchanged;
  std::set_union(...);
  *no_longer = base::STLSetDifference(...);
  *not_yet = base::STLSetDifference(...);
}

void UpdateGreylistedExtensions(const ExtensionIdSet& greylist,
                                const ExtensionIdSet& unchanged) {
  ExtensionIdSet not_yet, no_longer;
  Partition(greylist_.GetIDs(), greylist, unchanged,
            &not_yet, &no_longer);
}

[Oleg Eterevsky, 2014/01/23 14:48:54]

Done. It actually did make the code simpler.

+    const std::set<std::string>& greylist,
+    const std::set<std::string>& unknown) {
+  std::set<std::string> greylist_and_unknown;
+  std::set_union(
+      greylist.begin(), greylist.end(), unknown.begin(), unknown.end(),
+      std::inserter(greylist_and_unknown, greylist_and_unknown.end()));
+
+  std::set<std::string> greylisted_before = greylist_.GetIDs();
+  std::set<std::string> no_longer_greylisted =
+      base::STLSetDifference<std::set<std::string> >(
+          greylisted_before, greylist_and_unknown);
+  std::set<std::string> not_yet_greylisted =
+      base::STLSetDifference<std::set<std::string> >(
+          greylist, greylisted_before);
+
+  for (std::set<std::string>::iterator it = no_longer_greylisted.begin();
+       it != no_longer_greylisted.end(); ++it) {
+    scoped_refptr<const Extension> extension = greylist_.GetByID(*it);
+    if (!extension.get()) {
+      NOTREACHED() << "Extension " << *it << " no longer greylisted, "
+                   << "but it was not marked as greylisted.";
+      continue;
+    }
+
+    greylist_.Remove(*it);
+    extension_prefs_->SetExtensionGreylisted(extension->id(), false);
+    EnableExtension(*it);
+  }
+
+  for (std::set<std::string>::iterator it = not_yet_greylisted.begin();
+       it != not_yet_greylisted.end(); ++it) {
+    scoped_refptr<const Extension> extension = GetInstalledExtension(*it);
+    if (!extension.get()) {
+      NOTREACHED() << "Extension " << *it << " needs to be "
+                   << "disabled, but it's not installed.";
+      continue;
+    }
+    greylist_.Insert(extension);
+    extension_prefs_->SetExtensionGreylisted(extension->id(), true);
+    DisableExtension(*it, extensions::Extension::DISABLE_GREYLISTED);
+  }
 }
 
 void ExtensionService::AddUpdateObserver(extensions::UpdateObserver* observer) {