PasswordProtectionService verdict cache management

components/safe_browsing/password_protection/password_protection_service.cc

 // Copyright 2017 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 "components/safe_browsing/password_protection/password_protection_service.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
 #include "components/safe_browsing_db/database_manager.h"
+#include "components/safe_browsing_db/v4_protocol_manager_util.h"
 #include "content/public/browser/browser_thread.h"
 
 using content::BrowserThread;
 
 namespace safe_browsing {
 
+namespace {
+
+// Keys for storing password protection verdict into a DictionaryValue.
+static const char kCacheTTL[] = "cache_ttl";
+static const char kCacheCreationTime[] = "cache_creation_time";
+static const char kVerdictType[] = "verdict";
+
+// Helper function to determine if the given origin matches content settings
+// map's patterns.
+bool OriginMatchPrimaryPattern(
+    const GURL& origin,
+    const ContentSettingsPattern& primary_pattern,
+    const ContentSettingsPattern& secondary_pattern_unused) {
+  return ContentSettingsPattern::FromURLNoWildcard(origin) == primary_pattern;
+}
+
+// Convert a LoginReputationClientResponse proto into a DictionaryValue.
+std::unique_ptr<base::DictionaryValue> CreateDictionaryFromVerdict(
+    LoginReputationClientResponse* verdict,

[vakh (use Gerrit instead), 2017/03/16 22:04:30]

this can be const

[Jialiu Lin, 2017/03/18 23:46:31]

Done.

+    const base::Time& receive_time) {
+  std::unique_ptr<base::DictionaryValue> result =
+      base::MakeUnique<base::DictionaryValue>();
+  if (verdict) {
+    result->SetIntegerWithoutPathExpansion(kCacheTTL,
+                                           verdict->cache_duration_sec());
+    result->SetIntegerWithoutPathExpansion(
+        kCacheCreationTime, static_cast<int>(receive_time.ToDoubleT()));
+    result->SetIntegerWithoutPathExpansion(kVerdictType,
+                                           verdict->verdict_type());
+  }
+  return result;
+}
+
+}  // namespace
+
 PasswordProtectionService::PasswordProtectionService(
     const scoped_refptr<SafeBrowsingDatabaseManager>& database_manager)
     : database_manager_(database_manager), weak_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 }
 
 PasswordProtectionService::~PasswordProtectionService() {
   weak_factory_.InvalidateWeakPtrs();
 }
 
 void PasswordProtectionService::RecordPasswordReuse(const GURL& url) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(database_manager_);
   if (!url.is_valid())
     return;
 
   BrowserThread::PostTaskAndReplyWithResult(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&SafeBrowsingDatabaseManager::MatchCsdWhitelistUrl,
                  database_manager_, url),
       base::Bind(&PasswordProtectionService::OnMatchCsdWhiteListResult,
                  GetWeakPtr()));
 }
 
+LoginReputationClientResponse::VerdictType
+PasswordProtectionService::GetCachedVerdict(HostContentSettingsMap* settings,

[vakh (use Gerrit instead), 2017/03/16 22:04:29]

const *settings?

[Jialiu Lin, 2017/03/18 23:46:31]

Done.

+                                            const GURL& url) {
+  // TODO(jialiul): Add UMA metrics to track if verdict is available, not
+  // available, or expired.
+  if (!settings || !url.is_valid()) {

[Nathan Parker, 2017/03/16 21:27:11]

nit: do you want to DCHECK on settings, or is there a case where it's valid to
have it null?

[Jialiu Lin, 2017/03/18 23:46:31]

Done.

+    return LoginReputationClientResponse::VERDICT_TYPE_UNSPECIFIED;
+  }
+
+  GURL origin_url = url.GetOrigin();
+  std::unique_ptr<base::DictionaryValue> verdict_dictionary =
+      base::DictionaryValue::From(settings->GetWebsiteSetting(
+          origin_url, GURL(), CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
+          std::string(), nullptr));
+  // Return early if there is no verdict cached for this |origin_url|.
+  if (!verdict_dictionary.get() || verdict_dictionary->empty()) {
+    return LoginReputationClientResponse::VERDICT_TYPE_UNSPECIFIED;
+  }
+
+  // For all the verdicts of the same origin, we key them by cache_expression
+  // field. And its corresponding value is a DictionaryValue contains its ttl,
+  // creation time, and verdict type info.
+  int cache_creation_time;
+  int cache_duration;
+  int verdict_type_value;
+  base::DictionaryValue* verdict_entry = nullptr;
+  for (base::DictionaryValue::Iterator it(*verdict_dictionary.get());
+       !it.IsAtEnd(); it.Advance()) {
+    if (verdict_dictionary->GetDictionaryWithoutPathExpansion(it.key(),
+                                                              &verdict_entry) &&

[Nathan Parker, 2017/03/16 21:27:11]

Is it an error if ParseVerdictEntry() returns false here? If so, that could also
be a DCHECK (to help catch bugs)

[Jialiu Lin, 2017/03/18 23:46:31]

Done.

+        ParseVerdictEntry(verdict_entry, &cache_creation_time, &cache_duration,
+                          &verdict_type_value)) {
+      if (UrlMatchCacheExpression(url, it.key() /* cache_expression */) &&
+          !IsCacheExpired(cache_creation_time, cache_duration)) {

[Nathan Parker, 2017/03/16 21:27:11]

Do you think we need to remove expired entries, or just wait for them to get
refreshed? I think the latter might be sufficient.

[Jialiu Lin, 2017/03/18 23:46:31]

If the entry is expired, we are going to request a new verdict, which upon
receipt will override the expired one. So we don't need to remove them here.  

+        return static_cast<LoginReputationClientResponse::VerdictType>(
+            verdict_type_value);

[Nathan Parker, 2017/03/16 21:27:11]

Something to think about: If we were to add new verdict types, or add more
structure to the verdict, do we have a migration path?  One option is to add a
version number, but that seems like overkill.  The key is that users might have
very old verdicts that are getting read my a newer version of chrome.

[Jialiu Lin, 2017/03/18 23:46:31]

Agreed. In the latest patch, I cache the serialized verdict proto instead of
individual fields.  The tricky part is, base::DictionaryValue only accept UTF8
strings, so I need to serialized proto as binary value (instead of string).
Added a little bit conversions to handle that.

+      }
+    }
+  }
+  // We don't find any valid and matching verdict.
+  return LoginReputationClientResponse::VERDICT_TYPE_UNSPECIFIED;
+}
+
+void PasswordProtectionService::CacheVerdict(
+    HostContentSettingsMap* settings,

[vakh (use Gerrit instead), 2017/03/16 22:04:30]

Since |settings| is being set, it is an output, so it should be at the end of
the list. See:
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

[Jialiu Lin, 2017/03/18 23:46:31]

Done.

+    const GURL& url,
+    LoginReputationClientResponse* verdict,
+    const base::Time& receive_time) {
+  if (!verdict)

[Nathan Parker, 2017/03/16 21:27:11]

Again, how about DCHECK instead?

[Jialiu Lin, 2017/03/18 23:46:31]

Done.

+    return;
+
+  GURL origin_url = url.GetOrigin();
+  std::unique_ptr<base::DictionaryValue> verdict_dictionary =
+      base::DictionaryValue::From(settings->GetWebsiteSetting(
+          origin_url, GURL(), CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
+          std::string(), nullptr));
+
+  if (!verdict_dictionary.get())
+    verdict_dictionary = base::MakeUnique<base::DictionaryValue>();
+
+  std::unique_ptr<base::DictionaryValue> verdict_entry =
+      CreateDictionaryFromVerdict(verdict, receive_time);
+  // If same cache_expression is already in this verdict_dictionary, we simply
+  // override it with a fresher verdict.
+  // TODO(jialiul): We assume cache expressions returned by CSD server are
+  // not overlapping with each other and are stable. Need to confirm with SB

[Nathan Parker, 2017/03/16 21:27:11]

We could verify it on the client and do something predictable with it if there
is an overlap. That way we'd notice it happening (UMA metrics) and wouldn't get
undefined behaviour.

Also, the notes here imply it might be overlapping:
https://docs.google.com/a/google.com/document/d/13CW7q2_c1tUgAbZsCzVVnopKcJ4X...

[Jialiu Lin, 2017/03/18 23:46:31]

You're right. I just confirmed with Weining. Overlapping could happen.
This part if actually fine, I added more code in GetCachedVerdict to find the
most specific cached verdict.

+  // backend if this assumption is always true.
+  verdict_dictionary->SetWithoutPathExpansion(verdict->cache_expression(),
+                                              std::move(verdict_entry));
+  settings->SetWebsiteSettingDefaultScope(
+      origin_url, GURL(), CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
+      std::string(), std::move(verdict_dictionary));
+}
+
 void PasswordProtectionService::OnMatchCsdWhiteListResult(
     bool match_whitelist) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   UMA_HISTOGRAM_BOOLEAN(
       "PasswordManager.PasswordReuse.MainFrameMatchCsdWhitelist",
       match_whitelist);
 }
 
+HostContentSettingsMap*
+PasswordProtectionService::GetSettingMapForActiveProfile() {
+  return nullptr;
+}
+
+void PasswordProtectionService::OnURLsDeleted(
+    history::HistoryService* history_service,
+    bool all_history,
+    bool expired,
+    const history::URLRows& deleted_rows,
+    const std::set<GURL>& favicon_urls) {
+  HostContentSettingsMap* setting_map = GetSettingMapForActiveProfile();
+  if (!setting_map)
+    return;
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&PasswordProtectionService::RemoveContentSettingsOnURLsDeleted,
+                 GetWeakPtr(), setting_map, all_history, deleted_rows));
+}
+
+void PasswordProtectionService::RemoveContentSettingsOnURLsDeleted(
+    HostContentSettingsMap* setting_map,

[vakh (use Gerrit instead), 2017/03/16 22:04:30]

|setting_map| at the end

[Jialiu Lin, 2017/03/18 23:46:31]

Done.

+    bool all_history,
+    const history::URLRows& deleted_rows) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (all_history) {
+    setting_map->ClearSettingsForOneType(
+        CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION);
+    return;
+  }
+
+  // For now, if a URL is deleted from history, we simply remove all the
+  // cached verdicts of the same origin. This is a pretty aggressive deletion.
+  // We might revisit this logic later to decide if we want to only delete the
+  // cached verdict whose cache expression matches this URL.
+  for (const history::URLRow& row : deleted_rows) {
+    setting_map->ClearSettingsForOneTypeWithPredicate(
+        CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION,
+        base::Bind(&OriginMatchPrimaryPattern, row.url().GetOrigin()));
+  }
+}
+
+// static
+bool PasswordProtectionService::ParseVerdictEntry(
+    base::DictionaryValue* verdict_entry,

[vakh (use Gerrit instead), 2017/03/16 22:04:30]

|verdict_entry| const?

[Jialiu Lin, 2017/03/18 23:46:31]

acknowledged.  for some weird reason, base::DictionaryValue::Get(...) is not
marked as const.  So compiler will complain if I add a const  to verdict_entry. 

+    int* out_cache_creation_time,
+    int* out_cache_duration,
+    int* out_verdict_type) {
+  return verdict_entry &&
+         verdict_entry->GetInteger(kCacheCreationTime,
+                                   out_cache_creation_time) &&
+         verdict_entry->GetInteger(kCacheTTL, out_cache_duration) &&
+         verdict_entry->GetInteger(kVerdictType, out_verdict_type);
+}
+
+bool PasswordProtectionService::UrlMatchCacheExpression(
+    const GURL& url,
+    const std::string& cache_expression) {
+  std::string canon_host;
+  std::string canon_path;
+  std::string canon_query;
+  V4ProtocolManagerUtil::CanonicalizeUrl(url, &canon_host, &canon_path,
+                                         &canon_query);
+
+  std::vector<std::string> hosts;
+  if (url.HostIsIPAddress()) {
+    hosts.push_back(url.host());
+  } else {
+    V4ProtocolManagerUtil::GenerateHostVariantsToCheck(canon_host, &hosts);
+  }
+
+  std::vector<std::string> paths;
+  V4ProtocolManagerUtil::GeneratePathVariantsToCheck(canon_path, canon_query,
+                                                     &paths);
+
+  base::StringPiece cache_expression_trimed(base::TrimString(
+      cache_expression, "/", base::TrimPositions::TRIM_TRAILING));
+  for (const std::string& host : hosts) {

[Nathan Parker, 2017/03/16 21:27:11]

I'm a little worried about CPU/latency here, since this inner loop is called a
lot.  When clearing history will we check every cleared URL against every cached
entry for a given origin?

It may not be a big deal, but something to keep an eye on.

[Jialiu Lin, 2017/03/18 23:46:31]

Rewrote this function significantly. Now only one layer of loop is needed.

+    for (const std::string& path : paths) {

[vakh (use Gerrit instead), 2017/03/16 22:04:30]

Most code in this function is similar to V4ProtocolManagerUtil::UrlToFullHashes.

Please consider making a common function that's called in both places and
returns the combinations of canonical hosts and paths.

[Jialiu Lin, 2017/03/18 23:46:31]

This function is rewritten significantly in the latest patch. Now it does not
look like UrlToFullHashes anymore. 

+      std::string host_path_combine(host);
+      host_path_combine.append(path);
+      if (cache_expression_trimed ==
+          base::TrimString(host_path_combine, "/",
+                           base::TrimPositions::TRIM_TRAILING)) {
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
+bool PasswordProtectionService::IsCacheExpired(int cache_creation_time,
+                                               int cache_duration) {
+  // TODO(jialiul): For now, we assume client's clock is accurate or almost
+  // accurate. Need some logic to handle cases where client's clock is way off.
+  return base::Time::Now().ToDoubleT() >
+         static_cast<double>(cache_creation_time + cache_duration);
+}
+
 }  // namespace safe_browsing