Allow re-enabling of DISABLE_NOT_VERIFIED extensions if appropriate

chrome/browser/extensions/extension_service.cc

 // Copyright (c) 2013 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 "chrome/browser/extensions/extension_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 
@@ skipping 870 matching lines @@
 }
 
 void ExtensionService::EnableExtension(const std::string& extension_id) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (IsExtensionEnabled(extension_id))
     return;
   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;
   }
 
   extension_prefs_->SetExtensionState(extension_id, Extension::ENABLED);
   extension_prefs_->ClearDisableReasons(extension_id);
 
   // This can happen if sync enables an extension that is not
   // installed yet.
   if (!extension)
     return;
@@ skipping 1878 matching lines @@
     blacklisted_extensions_.Insert(extension);
     extension_prefs_->SetExtensionBlacklisted(extension->id(), true);
     UnloadExtension(*it, UnloadedExtensionInfo::REASON_BLACKLIST);
     UMA_HISTOGRAM_ENUMERATION("ExtensionBlacklist.BlacklistInstalled",
                               extension->location(), Manifest::NUM_LOCATIONS);
   }
 
   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);
 }
 
 void ExtensionService::RemoveUpdateObserver(
     extensions::UpdateObserver* observer) {
   update_observers_.RemoveObserver(observer);
 }