This CL integrates the new SafeBrowsing download service class

chrome/browser/download/chrome_download_manager_delegate.cc

 // Copyright (c) 2011 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/download/chrome_download_manager_delegate.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/rand_util.h"
 #include "base/stringprintf.h"
+#include "base/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_crx_util.h"
 #include "chrome/browser/download/download_extensions.h"
 #include "chrome/browser/download/download_file_picker.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_prefs.h"
-#include "chrome/browser/download/download_safe_browsing_client.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/download/save_package_file_picker.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/prefs/pref_member.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/extensions/user_script.h"
 #include "chrome/common/pref_names.h"
 #include "content/browser/download/download_file.h"
 #include "content/browser/download/download_item.h"
 #include "content/browser/download/download_manager.h"
 #include "content/browser/download/download_status_updater.h"
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/public/browser/notification_source.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 using content::BrowserThread;
+using safe_browsing::DownloadProtectionService;
 
 ChromeDownloadManagerDelegate::ChromeDownloadManagerDelegate(Profile* profile)
     : profile_(profile),
       download_prefs_(new DownloadPrefs(profile->GetPrefs())) {
 }
 
 ChromeDownloadManagerDelegate::~ChromeDownloadManagerDelegate() {
 }
 
 bool ChromeDownloadManagerDelegate::IsExtensionDownload(
@@ skipping 24 matching lines @@
   // history service thread is still reading the persistent state, we do not
   // insert the new DownloadItem into 'history_downloads_' or inform our
   // observers at this point. OnCreateDownloadEntryComplete() handles that
   // finalization of the the download creation as a callback from the history
   // thread.
   DownloadItem* download =
       download_manager_->GetActiveDownloadItem(download_id);
   if (!download)
     return false;
 
 #if defined(ENABLE_SAFE_BROWSING)

[Randy Smith (Not in Mondays), 2011/11/14 19:43:00]

Remind me what the context around this define is?  Under what circumstances do
we want to compile out safe browsing?  (Just asking for context).

[noelutz, 2011/11/14 20:30:42]

SafeBrowsing is currently disabled (and compiled out) for ChromeOs.  There might
also be other targets that do not compile SafeBrowsing but ChromeOs is the main
one.

-  // Create a client to verify download URL with safebrowsing.
-  // It deletes itself after the callback.
-  scoped_refptr<DownloadSBClient> sb_client = new DownloadSBClient(
-      download_id, download->url_chain(), download->referrer_url(),
-          profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled));
-  sb_client->CheckDownloadUrl(
-      base::Bind(&ChromeDownloadManagerDelegate::CheckDownloadUrlDone,
-                 base::Unretained(this)));
-#else
-  CheckDownloadUrlDone(download_id, false);
+  SafeBrowsingService* sb_service =
+      g_browser_process->safe_browsing_service();
+  if (sb_service && sb_service->download_protection_service() &&
+      profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
+    VLOG(2) << __FUNCTION__ << "() Start SB URL check for download = "
+            << download->DebugString(false);
+    sb_service->download_protection_service()->CheckDownloadUrl(
+        DownloadProtectionService::DownloadInfo::FromDownloadItem(*download),
+        base::Bind(
+            &ChromeDownloadManagerDelegate::CheckDownloadUrlDone,
+            this,
+            download->id()));
+    return false;
+  }
 #endif
+  CheckDownloadUrlDone(download_id, DownloadProtectionService::SAFE);
   return false;
 }
 
 void ChromeDownloadManagerDelegate::ChooseDownloadPath(
     TabContents* tab_contents,
     const FilePath& suggested_path,
     void* data) {
   // Deletes itself.
   new DownloadFilePicker(
       download_manager_, tab_contents, suggested_path, data);
@@ skipping 30 matching lines @@
   if (extension.empty())
     return false;
   if (Extension::IsExtension(path))
     return false;
   DCHECK(extension[0] == FilePath::kExtensionSeparator);
   extension.erase(0, 1);
   return download_prefs_->IsAutoOpenEnabledForExtension(extension);
 }
 
 bool ChromeDownloadManagerDelegate::ShouldCompleteDownload(DownloadItem* item) {
+#if defined(ENABLE_SAFE_BROWSING)
+  // See if there is already a pending SafeBrowsing check for that download.
+  SafeBrowsingStateMap::iterator it = safe_browsing_state_.find(item->id());
+  if (it != safe_browsing_state_.end()) {
+    SafeBrowsingState state = it->second;
+    if (!state.pending) {
+      safe_browsing_state_.erase(it);
+    }
+    return !state.pending;
+  }
+  // Begin the safe browsing download protection check.
+  SafeBrowsingService* sb_service =
+      g_browser_process->safe_browsing_service();
+  if (sb_service && sb_service->download_protection_service() &&
+      profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
+    VLOG(2) << __FUNCTION__ << "() Start SB download check for download = "
+            << item->DebugString(false);
+    sb_service->download_protection_service()->CheckClientDownload(
+        DownloadProtectionService::DownloadInfo::FromDownloadItem(*item),
+        base::Bind(
+            &ChromeDownloadManagerDelegate::CheckClientDownloadDone,
+            this,
+            item->id()));
+    SafeBrowsingState state;
+    state.pending = true;
+    safe_browsing_state_[item->id()] = state;
+    return false;
+  }
+#endif
   return true;
 }
 
 bool ChromeDownloadManagerDelegate::ShouldOpenDownload(DownloadItem* item) {
   if (!IsExtensionDownload(item)) {
-#if defined(ENABLE_SAFE_BROWSING)
-    // Begin the safe browsing download protection check.
-    SafeBrowsingService* sb_service =
-        g_browser_process->safe_browsing_service();
-    if (sb_service && sb_service->download_protection_service() &&
-        profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
-      using safe_browsing::DownloadProtectionService;
-      sb_service->download_protection_service()->CheckClientDownload(
-          DownloadProtectionService::DownloadInfo::FromDownloadItem(*item),
-          base::Bind(
-              &ChromeDownloadManagerDelegate::CheckClientDownloadDone,
-              this, item->id()));
-      // For now, we won't delay the download for this.
-    }
-#else
-    // Assume safe.
-#endif
-
     return true;
   }
 
   scoped_refptr<CrxInstaller> crx_installer =
       download_crx_util::OpenChromeExtension(profile_, *item);
 
   // CRX_INSTALLER_DONE will fire when the install completes.  Observe()
   // will call DelayedDownloadOpened() on this item.  If this DownloadItem is
   // not around when CRX_INSTALLER_DONE fires, Complete() will not be called.
   registrar_.Add(this,
@@ skipping 11 matching lines @@
 bool ChromeDownloadManagerDelegate::GenerateFileHash() {
 #if defined(ENABLE_SAFE_BROWSING)
   return profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled) &&
       g_browser_process->safe_browsing_service()->DownloadBinHashNeeded();
 #else
   return false;
 #endif
 }
 
 void ChromeDownloadManagerDelegate::OnResponseCompleted(DownloadItem* item) {
-#if defined(ENABLE_SAFE_BROWSING)
-  // When hash is not available, it means either it is not calculated
-  // or there is error while it is calculated. We will skip the download hash
-  // check in that case.
-  if (item->hash().empty())
-    return;
-
-  scoped_refptr<DownloadSBClient> sb_client =
-      new DownloadSBClient(item->id(),
-                           item->url_chain(),
-                           item->referrer_url(),
-                           profile_->GetPrefs()->GetBoolean(
-                               prefs::kSafeBrowsingEnabled));
-  sb_client->CheckDownloadHash(
-      item->hash(),
-      base::Bind(&ChromeDownloadManagerDelegate::CheckDownloadHashDone,
-                 base::Unretained(this)));
-#endif
+  // TODO(noelutz): remove this method from the delegate API.
 }
 
 void ChromeDownloadManagerDelegate::AddItemToPersistentStore(
     DownloadItem* item) {
   download_history_->AddEntry(item,
       base::Bind(&ChromeDownloadManagerDelegate::OnItemAddedToPersistentStore,
                  base::Unretained(this)));
 }
 
 void ChromeDownloadManagerDelegate::UpdateItemInPersistentStore(
     DownloadItem* item) {
   download_history_->UpdateEntry(item);
 }
 
 void ChromeDownloadManagerDelegate::UpdatePathForItemInPersistentStore(
     DownloadItem* item,
     const FilePath& new_path) {
   download_history_->UpdateDownloadPath(item, new_path);
 }
 
-void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
-    int32 download_id,
-    safe_browsing::DownloadProtectionService::DownloadCheckResult result) {
-  // TODO(bryner): notify the user based on this result
-}
-
 void ChromeDownloadManagerDelegate::RemoveItemFromPersistentStore(
     DownloadItem* item) {
   download_history_->RemoveEntry(item);
 }
 
 void ChromeDownloadManagerDelegate::RemoveItemsFromPersistentStoreBetween(
     const base::Time remove_begin,
     const base::Time remove_end) {
   download_history_->RemoveEntriesBetween(remove_begin, remove_end);
 }
@@ skipping 41 matching lines @@
   int download_count = 0;
   bool progress_known =
       g_browser_process->download_status_updater()->GetProgress(
           &progress, &download_count);
   download_util::UpdateAppIconDownloadProgress(
       download_count, progress_known, progress);
 }
 
 void ChromeDownloadManagerDelegate::CheckDownloadUrlDone(
     int32 download_id,
-    bool is_dangerous_url) {
+    DownloadProtectionService::DownloadCheckResult result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
   DownloadItem* download =
       download_manager_->GetActiveDownloadItem(download_id);
   if (!download)
     return;
 
-  if (is_dangerous_url)
+  VLOG(2) << __FUNCTION__ << "() download = " << download->DebugString(false)
+          << " verdict = " << result;
+  if (result == DownloadProtectionService::DANGEROUS)
     download->MarkUrlDangerous();
 
   download_history_->CheckVisitedReferrerBefore(
       download_id, download->referrer_url(),
       base::Bind(&ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone,
                  base::Unretained(this)));
 }
 
+void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
+    int32 download_id,
+    DownloadProtectionService::DownloadCheckResult result) {
+  DownloadItem* item = download_manager_->GetActiveDownloadItem(download_id);
+  if (!item)

[Randy Smith (Not in Mondays), 2011/11/14 22:24:22]

There's a (minor) leak here if the DownloadItem's gone away; you haven't cleaned
up the state in the map.  That'll fix itself when we switch to the new storage
model, but you might want to fix it here as well.

[noelutz, 2011/11/14 22:26:35]

I think we're OK. There aren't any pointers in the map.  Everything should be
deleted by the destructor as far as I know.

[noelutz, 2011/11/15 00:14:10]

Fixed.

+    return;
+
+  VLOG(2) << __FUNCTION__ << "() download = " << item->DebugString(false)
+          << " verdict = " << result;
+  // TODO(noelutz):
+  //  1) display a warning if the result is DANGEROUS.
+  //  2) make sure we haven't already displayed a warning for the URL.
+  //  3) disable the existing dangerous file warning for executables.
+
+  SafeBrowsingStateMap::iterator it = safe_browsing_state_.find(item->id());
+  DCHECK(it != safe_browsing_state_.end() && it->second.pending);
+  if (it != safe_browsing_state_.end()) {
+    it->second.pending = false;
+    it->second.verdict = result;
+  }
+  download_manager_->MaybeCompleteDownload(item);
+}
+
 // content::NotificationObserver implementation.
 void ChromeDownloadManagerDelegate::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK(type == chrome::NOTIFICATION_CRX_INSTALLER_DONE);
 
   registrar_.Remove(this,
                     chrome::NOTIFICATION_CRX_INSTALLER_DONE,
                     source);
@@ skipping 211 matching lines @@
     int32 download_id, int64 db_handle) {
   // It's not immediately obvious, but HistoryBackend::CreateDownload() can
   // call this function with an invalid |db_handle|. For instance, this can
   // happen when the history database is offline. We cannot have multiple
   // DownloadItems with the same invalid db_handle, so we need to assign a
   // unique |db_handle| here.
   if (db_handle == DownloadItem::kUninitializedHandle)
     db_handle = download_history_->GetNextFakeDbHandle();
   download_manager_->OnItemAddedToPersistentStore(download_id, db_handle);
 }
-
-// TODO(noelutz): This function currently works as a callback place holder.
-// Once we decide the hash check is reliable, we could move the
-// MaybeCompleteDownload in OnAllDataSaved to this function.
-void ChromeDownloadManagerDelegate::CheckDownloadHashDone(
-    int32 download_id,
-    bool is_dangerous_hash) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DVLOG(1) << "CheckDownloadHashDone, download_id: " << download_id
-           << " is dangerous_hash: " << is_dangerous_hash;
-}