Allow re-enabling of DISABLE_NOT_VERIFIED extensions if appropriate

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 f8245a56f3b091a404eb01a27badbf9469934199..30fb8d390fc448bb067a91913e275f4d8332b476 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -888,8 +888,17 @@ void ExtensionService::EnableExtension(const std::string& extension_id) {
   const Extension* extension = disabled_extensions_.GetByID(extension_id);
 
   ManagementPolicy* policy = system_->management_policy();
-  if (extension && policy->MustRemainDisabled(extension, NULL, NULL)) {
+  Extension::DisableReason reason;
+  if (extension && policy->MustRemainDisabled(extension, &reason, NULL)) {
     UMA_HISTOGRAM_COUNTS_100("Extensions.EnableDeniedByPolicy", 1);
+    if (reason == Extension::DISABLE_NOT_VERIFIED) {
+      // Try adding this id to the verified list, and if it succeeds we may
+      // be able to enable it.
+      extensions::ExtensionSystem::Get(profile_)->install_verifier()->Add(
+          extension->id(), base::Bind(&ExtensionService::EnableOnVerifySuccess,
+                                      base::Unretained(this),
+                                      extension_id));

[Finnur, 2013/12/11 17:28:20]

I'm worried this will slam the webstore with lots of requests from users. Is
that worry warranted? We have a large user base and almost all of the time this
is just going to waste CPU cycles on the server (I could imagine a lot of devs
are not going to bother migrating to the store). This is also not
time-sensitive, per se. Should we do something here to address this? What do you
think?

[asargent_no_longer_on_chrome, 2013/12/11 21:26:05]

I think this should only happen as a result of the user manually trying to
re-enable, since the API to do this in management_api.cc already early-returns
by checking the policy MustRemainDisabled function. 

I suppose we could rate-limit it somehow, but my guess is that most people would
try checking the "enable" checkbox a few times, and if it didn't work give up. 

[Finnur, 2013/12/11 21:46:31]

Does the only codepath through this function involve the Enable checkbox on the
Extension Settings page?

+    }
     return;
   }
 
@@ -2788,6 +2797,17 @@ void ExtensionService::ManageBlacklist(const std::set<std::string>& updated) {
   IdentifyAlertableExtensions();
 }
 
+void ExtensionService::EnableOnVerifySuccess(std::string extension_id,
+                                             bool verify_success) {
+  ManagementPolicy* policy = system_->management_policy();
+  const Extension* extension = GetExtensionById(extension_id, true);
+  if (verify_success && !IsExtensionEnabled(extension_id) &&
+      !policy->MustRemainDisabled(extension, NULL, NULL)) {

[Finnur, 2013/12/11 17:28:20]

We're guaranteed to have this check in other locations too, right? Shouldn't
there be one chokepoint to make sure they don't get out of sync?

[asargent_no_longer_on_chrome, 2013/12/11 21:26:05]

Yeah, I agree this is a little awkward. The problem I was trying to solve is
that the code above in EnableExtension tests "MustRemainDisabled", and if that
comes back as DISABLE_NOT_VERIFIED then it kicks off the verification test that
gets us into this function. If |verify_success| is true then when we call
EnableExtension again it *shouldn't* be the case that the MustRemainDisabled
check it repeats will return DISABLE_NOT_VERIFIED again, but I was worried about
a bug creating an infinite loop of requests here. 

Does that make sense (and should I add a comment here to this effect)? Or do you
think I should be more explicit and refactor things so I have a way to call into
the same logic as EnableExtension but eliminating the possibility of generating
another verification attempt?

[Finnur, 2013/12/11 21:46:31]

As discussed in person I had a concern that we have the same if checks in
multiple locations and that they might get out of sync.

+    EnableExtension(extension_id);
+  }

[Finnur, 2013/12/11 17:28:20]

nit: Single line if body, no braces.

[asargent_no_longer_on_chrome, 2013/12/11 21:26:05]

Done.

+}
+
+
 void ExtensionService::AddUpdateObserver(extensions::UpdateObserver* observer) {
   update_observers_.AddObserver(observer);
 }