V4LDBM: Get response from GetHashManager, detect severest result

components/safe_browsing_db/v4_local_database_manager.cc

Index: components/safe_browsing_db/v4_local_database_manager.cc
diff --git a/components/safe_browsing_db/v4_local_database_manager.cc b/components/safe_browsing_db/v4_local_database_manager.cc
index ba9336a007fff7fa69364017fda5c904753b556b..4d0b56816cb38909398f0407e74700ab534522b2 100644
--- a/components/safe_browsing_db/v4_local_database_manager.cc
+++ b/components/safe_browsing_db/v4_local_database_manager.cc
@@ -9,7 +9,9 @@
 
 #include <vector>
 
+#include "base/bind_helpers.h"
 #include "base/callback.h"
+#include "base/memory/ptr_util.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
@@ -18,16 +20,60 @@ namespace safe_browsing {
 
 namespace {
 
+const ThreatSeverity kLeastSeverity =
+    std::numeric_limits<ThreatSeverity>::max();
+
 // TODO(vakh): Implement this to populate the vector appopriately.
 // Filed as http://crbug.com/608075
+// Any stores added/removed to/from here likely need an update in
+// GetSBThreatTypeForList

[Nathan Parker, 2016/09/20 17:51:09]

Maybe you can enforce that with a compile-time assert or dcheck

[vakh (use Gerrit instead), 2016/09/20 18:21:16]

I am using the GetChromeUrlApiId store in tests but I can't add it here since we
don't want to create a file on disk for that store.
I've added a TODO for that here. I'll address it when I add code to handle
stores that don't have an associated file on disk.

 StoreIdAndFileNames GetStoreIdAndFileNames() {
   return StoreIdAndFileNames(
       {StoreIdAndFileName(GetUrlMalwareId(), "UrlMalware.store"),
        StoreIdAndFileName(GetUrlSocEngId(), "UrlSoceng.store")});
 }
 
+// Returns the SBThreatType corresponding to a given SafeBrowsing list.
+SBThreatType GetSBThreatTypeForList(const UpdateListIdentifier& list_id) {
+  if (list_id == GetChromeUrlApiId()) {

[Nathan Parker, 2016/09/20 17:51:09]

nit: When this list gets longer, you might want an unordered map.

[vakh (use Gerrit instead), 2016/09/20 18:21:17]

Acknowledged.

[Scott Hess - ex-Googler, 2016/09/20 21:55:55]

This kind of stuff in the old code feels like the messy-polymorphism
anti-pattern.  Like maybe the selector thingie belongs in UpdateListIdentifier
so it can be effectively optimized.

[vakh (use Gerrit instead), 2016/09/21 17:43:40]

You're right. I did not put it there because the SB_THREAT_TYPE_ values are used
only in the V4LocalDatabaseManager so I wasn't sure if I should add them to
UpdateListIdentifier. However, I do realize that this if-else block is much more
messy so I've changed ListIdentifier to also contain a member of type
SBThreatType.

+    return SB_THREAT_TYPE_API_ABUSE;
+  } else if (list_id == GetUrlMalwareId()) {
+    return SB_THREAT_TYPE_URL_MALWARE;
+  } else if (list_id == GetUrlSocEngId()) {
+    return SB_THREAT_TYPE_URL_PHISHING;
+  } else {
+    NOTREACHED() << "Unknown list encountered in GetSBThreatTypeForList";
+    return SB_THREAT_TYPE_SAFE;
+  }
+}
+
+// Returns the severity information about a given SafeBrowsing list. The lowest
+// value is 0, which represents the most severe list.
+ThreatSeverity GetThreatSeverity(const UpdateListIdentifier& list_id) {
+  switch (list_id.threat_type) {
+    case MALWARE_THREAT:
+    case SOCIAL_ENGINEERING_PUBLIC:
+      return 0;
+    case API_ABUSE:
+      return 1;
+    default:
+      NOTREACHED() << "Unexpected ThreatType encountered in GetThreatSeverity";
+      return kLeastSeverity;
+  }
+}
+
 }  // namespace
 
+V4LocalDatabaseManager::PendingCheck::PendingCheck(CheckType check_type,
+                                                   Client* client,
+                                                   const GURL& url)
+    : check_type(check_type),
+      client(client),
+      url(url),
+      result_threat_type(SB_THREAT_TYPE_SAFE) {}
+
+V4LocalDatabaseManager::PendingCheck::~PendingCheck() {}
+
 V4LocalDatabaseManager::V4LocalDatabaseManager(const base::FilePath& base_path)
     : base_path_(base_path),
       enabled_(false),
@@ -140,6 +186,7 @@ bool V4LocalDatabaseManager::IsCsdWhitelistKillSwitchOn() {
 
 bool V4LocalDatabaseManager::CheckBrowseUrl(const GURL& url, Client* client) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   if (!enabled_ || !CanCheckUrl(url)) {
     return true;
   }
@@ -152,27 +199,32 @@ bool V4LocalDatabaseManager::CheckBrowseUrl(const GURL& url, Client* client) {
         {GetUrlMalwareId(), GetUrlSocEngId()});
     std::unordered_set<HashPrefix> matched_hash_prefixes;
     std::unordered_set<UpdateListIdentifier> matched_stores;
-    StoreAndHashPrefixes matched_store_and_full_hashes;
+    StoreAndHashPrefixes matched_store_and_hash_prefixes;
     FullHashToStoreAndHashPrefixesMap full_hash_to_store_and_hash_prefixes;
     for (const auto& full_hash : full_hashes) {
-      matched_store_and_full_hashes.clear();
+      matched_store_and_hash_prefixes.clear();
       v4_database_->GetStoresMatchingFullHash(full_hash, stores_to_look,
-                                              &matched_store_and_full_hashes);
-      if (!matched_store_and_full_hashes.empty()) {
+                                              &matched_store_and_hash_prefixes);
+      if (!matched_store_and_hash_prefixes.empty()) {
         full_hash_to_store_and_hash_prefixes[full_hash] =
-            matched_store_and_full_hashes;
+            matched_store_and_hash_prefixes;
       }
     }
 
     if (full_hash_to_store_and_hash_prefixes.empty()) {
       return true;
     } else {
-      // TODO(vakh): Pass more information to the callback for it to be able to
-      // do something meaningful.
+      std::unique_ptr<PendingCheck> pending_check =
+          base::MakeUnique<PendingCheck>(CheckType::CHECK_BROWSE_URL, client,
+                                         url);
+
       v4_get_hash_protocol_manager_->GetFullHashes(
           full_hash_to_store_and_hash_prefixes,
           base::Bind(&V4LocalDatabaseManager::OnFullHashResponse,
-                     base::Unretained(this)));
+                     base::Unretained(this), base::Passed(&pending_check)));
+
+      pending_clients_.insert(client);
+
       return false;
     }
   } else {
@@ -182,15 +234,73 @@ bool V4LocalDatabaseManager::CheckBrowseUrl(const GURL& url, Client* client) {
   }
 }
 
+// static
+void V4LocalDatabaseManager::GetSeverestThreatTypeAndMetadata(
+    SBThreatType* result_threat_type,
+    ThreatMetadata* metadata,
+    const std::vector<FullHashInfo>& full_hash_infos) {
+  DCHECK(result_threat_type);
+  DCHECK(metadata);
+
+  ThreatSeverity most_severe_yet = kLeastSeverity;
+  for (size_t i = 0; i < full_hash_infos.size(); i++) {

[Nathan Parker, 2016/09/20 17:51:09]

nit: for (const FullHashInfo& fi : full_hash_infos)

[vakh (use Gerrit instead), 2016/09/20 18:21:16]

Done.

+    const FullHashInfo& fhi = full_hash_infos[i];
+    ThreatSeverity severity = GetThreatSeverity(fhi.list_id);
+    if (severity < most_severe_yet) {
+      most_severe_yet = severity;
+      *result_threat_type = GetSBThreatTypeForList(fhi.list_id);
+      *metadata = fhi.metadata;
+    }
+  }
+}
+
 void V4LocalDatabaseManager::OnFullHashResponse(
+    std::unique_ptr<PendingCheck> pending_check,
     const std::vector<FullHashInfo>& full_hash_infos) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  if (!enabled_) {
+    DCHECK(pending_clients_.empty());
+    return;
+  }
+
+  auto it = pending_clients_.find(pending_check->client);
+  if (it == pending_clients_.end()) {
+    // The check has since been cancelled.
+    return;
+  }
+
+  if (full_hash_infos.empty()) {
+    // The resource is not known to be unsafe. Respond right away.
+    RespondToClient(std::move(pending_check));
+    return;
+  }
+
+  GetSeverestThreatTypeAndMetadata(&pending_check->result_threat_type,
+                                   &pending_check->url_metadata,
+                                   full_hash_infos);
+
+  RespondToClient(std::move(pending_check));
+  pending_clients_.erase(it);
+}
+
+void V4LocalDatabaseManager::RespondToClient(
+    std::unique_ptr<PendingCheck> pending_check) {
+  DCHECK(pending_check.get());
+  DCHECK_EQ(CheckType::CHECK_BROWSE_URL, pending_check->check_type);
   // TODO(vakh): Implement this skeleton.
 }
 
 void V4LocalDatabaseManager::CancelCheck(Client* client) {
-  // TODO(vakh): Implement this skeleton.
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(enabled_);
+
+  auto it = pending_clients_.find(client);
+  if (it != pending_clients_.end()) {
+    pending_clients_.erase(it);
+  }
+
+  // TODO(vakh): Handle the case of queued checks.
 }
 
 void V4LocalDatabaseManager::StartOnIOThread(
@@ -262,6 +372,8 @@ void V4LocalDatabaseManager::StopOnIOThread(bool shutdown) {
 
   enabled_ = false;
 
+  pending_clients_.clear();
+
   // Delete the V4Database. Any pending writes to disk are completed.
   // This operation happens on the task_runner on which v4_database_ operates
   // and doesn't block the IO thread.