Allow re-enabling of DISABLE_NOT_VERIFIED extensions if appropriate

chrome/browser/extensions/install_verifier.cc

 // Copyright 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/install_verifier.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
@@ skipping 64 matching lines @@
       DVLOG(1) << "Init - ignoring invalid signature";
     } else {
       signature_ = signature_from_prefs.Pass();
       UMA_HISTOGRAM_COUNTS("InstallVerifier.InitGoodSignature",
                            signature_->ids.size());
       GarbageCollect();
     }
   } else {
     UMA_HISTOGRAM_BOOLEAN("InstallVerifier.InitNoSignature", true);
   }
+}
 
-  if (!signature_.get() && ShouldFetchSignature()) {
-    // We didn't have any signature but are in fetch mode, so do a request for
-    // a signature if needed.
-    scoped_ptr<ExtensionPrefs::ExtensionsInfo> all_info =
-        prefs_->GetInstalledExtensionsInfo();
-    ExtensionIdSet to_add;
-    if (all_info.get()) {
-      for (ExtensionPrefs::ExtensionsInfo::const_iterator i = all_info->begin();
-           i != all_info->end(); ++i) {
-        const ExtensionInfo& info = **i;
-        const base::DictionaryValue* dictionary = info.extension_manifest.get();
-        if (dictionary && ManifestURL::UpdatesFromGallery(dictionary)) {
-          Manifest manifest(info.extension_location,
-                            scoped_ptr<DictionaryValue>(
-                                dictionary->DeepCopy()));
-          if (manifest.is_extension())
-            to_add.insert(info.extension_id);
-        }
-      }
-    }
-    if (to_add.empty()) {
-      // Write an empty signature so we don't have to redo this at next Init.
-      signature_.reset(new InstallSignature());
-      SaveToPrefs();
-    } else {
-      AddMany(to_add, AddResultCallback());
-    }
-  }
+bool InstallVerifier::NeedsBootstrap() {
+  return signature_.get() == NULL && ShouldFetchSignature();
 }
 
 void InstallVerifier::Add(const std::string& id,
                           const AddResultCallback& callback) {
   ExtensionIdSet ids;
   ids.insert(id);
   AddMany(ids, callback);
 }
 
 void InstallVerifier::AddMany(const ExtensionIdSet& ids,
                               const AddResultCallback& callback) {
-  if (!ShouldFetchSignature())
+  if (!ShouldFetchSignature()) {
+    if (!callback.is_null())
+      callback.Run(true);
     return;
+  }
 
   if (signature_.get()) {
     ExtensionIdSet not_allowed_yet =
         base::STLSetDifference<ExtensionIdSet>(ids, signature_->ids);
     if (not_allowed_yet.empty()) {
       if (!callback.is_null())
         callback.Run(true);
       return;
     }
   }
@@ skipping 58 matching lines @@
 bool InstallVerifier::MustRemainDisabled(const Extension* extension,
                                          Extension::DisableReason* reason,
                                          base::string16* error) const {
   if (!ShouldEnforce() || !extension->is_extension() ||
       Manifest::IsUnpackedLocation(extension->location()) ||
       AllowedByEnterprisePolicy(extension->id()))
     return false;
 
   bool verified =
       FromStore(extension) &&
-      IsVerified(extension->id()) &&
+      (signature_.get() == NULL || IsVerified(extension->id())) &&

[Finnur, 2013/12/16 14:21:51]

Wait... it is verified if there's no signature? Where you testing something or
is this actually intended? If so, document why this makes sense...

[asargent_no_longer_on_chrome, 2013/12/16 18:47:10]

It's temporarily allowed - the idea is that if someone gets thrown into
enforcement mode but has never done a check to the server before, we want to
hold off on disabling their extensions until we can successfully contact the
server once. That's what the "NeedsBootstrap" method above is for. 

Anyhow, I'll add a comment. (We may eventually need to make this more strict to
prevent abuse since there is an obvious attack vector here, but in the short
term we want to avoid the 100% false positive rate for existing profiles)

       !ContainsKey(InstallSigner::GetForcedNotFromWebstore(), extension->id());
 
   if (!verified) {
     if (reason)
       *reason = Extension::DISABLE_NOT_VERIFIED;
     if (error)
       *error = l10n_util::GetStringFUTF16(
           IDS_EXTENSIONS_ADDED_WITHOUT_KNOWLEDGE,
           l10n_util::GetStringUTF16(IDS_EXTENSION_WEB_STORE_TITLE));
   }
@@ skipping 124 matching lines @@
   } else {
     signature_ = signature.Pass();
     SaveToPrefs();
 
     if (!provisional_.empty()) {
       // Update |provisional_| to remove ids that were successfully signed.
       provisional_ = base::STLSetDifference<ExtensionIdSet>(
           provisional_, signature_->ids);
     }
 
-    // See if we were able to sign all of |ids|.
-    ExtensionIdSet not_allowed =
-        base::STLSetDifference<ExtensionIdSet>(operation->ids,
-                                               signature_->ids);
-
-    UMA_HISTOGRAM_COUNTS_100("InstallVerifier.CallbackNotAllowed",
-                             not_allowed.size());

[asargent_no_longer_on_chrome, 2013/12/14 00:42:39]

note: I removed this block of code because it was not correct and the histogram
would be misleading - it's possible the operation could have been to remove
operation->ids() from the signature, not add them. 

-
     if (!operation->callback.is_null())
-      operation->callback.Run(not_allowed.empty());

[asargent_no_longer_on_chrome, 2013/12/14 00:42:39]

This got updated to return the success of the network request instead of whether
we thought we properly signed all of the ids that may have been requested to be
added. 

It turns out callers are more interested in the network request success/failure.

+      operation->callback.Run(success);
   }
 
   if (!operation_queue_.empty())
     BeginFetch();
 }
 
 
 }  // namespace extensions