Deprecate direct use of legacy extension management preference

chrome/browser/download/download_crx_util.cc

 // Copyright (c) 2012 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.
 //
 // Download code which handles CRX files (extensions, themes, apps, ...).
 
 #include "chrome/browser/download/download_crx_util.h"
 
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/extension_install_prompt.h"
+#include "chrome/browser/extensions/extension_management.h"
 #include "chrome/browser/extensions/webstore_installer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/notification_service.h"
-#include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/common/user_script.h"
 
 using content::BrowserThread;
 using content::DownloadItem;
 using extensions::WebstoreInstaller;
 
 namespace download_crx_util {
 
 namespace {
@@ skipping 94 matching lines @@
   if (download_item.GetMimeType() == extensions::Extension::kMimeType ||
       extensions::UserScript::IsURLUserScript(download_item.GetURL(),
                                               download_item.GetMimeType())) {
     return true;
   } else {
     return false;
   }
 }
 
 bool OffStoreInstallAllowedByPrefs(Profile* profile, const DownloadItem& item) {
-  extensions::ExtensionPrefs* prefs = extensions::ExtensionPrefs::Get(profile);
-  CHECK(prefs);
+  extensions::ExtensionManagement* extension_management =
+      extensions::ExtensionManagementFactory::GetForBrowserContext(profile);
 
-  extensions::URLPatternSet url_patterns = prefs->GetAllowedInstallSites();
+  // No allowed install sites specified, disallow by default.

[binjin, 2014/09/09 23:57:51]

It turns out that it's how ExtensionInstallSources policy is being handled now.

The preference is not forced to be managed before, so overriding the default
behavior with user preference is possible. It might be a good idea to set
forced-managed for this preference to false in this CL as well.

But I would like to eventually drop all these legacy policies and preferences,
after that, disallowing be default looks too strict to me.

What do you think?

[Joao da Silva, 2014/09/10 10:05:54]

I think this is fine. This preference was never meant to work as a user
preference, only as a managed setting.

+  if (!extension_management->ReadGlobalSettings()
+           .has_restricted_install_sources) {
+    return false;
+  }
+
+  const extensions::URLPatternSet& url_patterns =
+      extension_management->ReadGlobalSettings().install_sources;
 
   if (!url_patterns.MatchesURL(item.GetURL()))
     return false;
 
   // The referrer URL must also be whitelisted, unless the URL has the file
   // scheme (there's no referrer for those URLs).
   // TODO(aa): RefererURL is cleared in some cases, for example when going
   // between secure and non-secure URLs. It would be better if DownloadItem
   // tracked the initiating page explicitly.
   return url_patterns.MatchesURL(item.GetReferrerUrl()) ||
          item.GetURL().SchemeIsFile();

[Joao da Silva, 2014/09/10 10:05:54]

What do you think of moving all this logic to ExtensionManagement?

Then the code from here would be just:

  return ExtensionManagementFactory::GetForBrowserContext(profile)
      ->IsOffstoreInstallAllowed(item.GetURL(), item.GetReferrerUrl());

Having all the logic around the policies in ExtensionManagement would make it
easier to unit test and to see how all the policies affect the browser.

[asanka, 2014/09/10 16:14:02]

+1. The only use of this function is in
DownloadTargetDeterminer::IsDangerousFile() which is trying to decide whether a
given download is considered dangerous or not (for the purpose of displaying a
warning to the user).

The logic being enforced by invoking this method is "extension downloads from
pref-whitelisted sources are not considered dangerous". I'd be happy if the
question asked by downloads is "does the extension subsystem consider this
extension download to be dangerous?" and have ExtensionManagement (or somewhere
else) encapsulate the details of how that determination is made.

[binjin, 2014/09/12 11:46:14]

Done.

 }
 
 }  // namespace download_crx_util