Add removal filter support for Cookies, Storage, and Content Settings.

chrome/browser/browsing_data/origin_filter_builder.cc

Index: chrome/browser/browsing_data/origin_filter_builder.cc
diff --git a/chrome/browser/browsing_data/origin_filter_builder.cc b/chrome/browser/browsing_data/origin_filter_builder.cc
index 900b018310032b7c25264d00343dea8c920a9868..11dc84c79a9d38076a88310fc630fff3022a6bdc 100644
--- a/chrome/browser/browsing_data/origin_filter_builder.cc
+++ b/chrome/browser/browsing_data/origin_filter_builder.cc
@@ -8,6 +8,8 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "components/content_settings/core/common/content_settings_pattern.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 
 namespace {
 
@@ -47,6 +49,10 @@ void OriginFilterBuilder::SetMode(Mode mode) {
   mode_ = mode;
 }
 
+bool OriginFilterBuilder::IsEmptyBlacklist() const {
+  return mode_ == Mode::BLACKLIST && origin_list_.empty();
+}
+
 base::Callback<bool(const GURL&)>
     OriginFilterBuilder::BuildSameOriginFilter() const {
   std::set<url::Origin>* origins = new std::set<url::Origin>(origin_list_);
@@ -54,6 +60,20 @@ base::Callback<bool(const GURL&)>
                     base::Owned(origins), mode_);
 }
 
+base::Callback<bool(const ContentSettingsPattern& pattern)>
+    OriginFilterBuilder::BuildSameOriginContentSettingsFilter() const {

[msramek, 2016/03/10 20:10:08]

Consider renaming to BuildWebsiteSettingsFilter().

Two reasons:
1. The matching semantics here are not really about same origin, but about
ContentSettingsPattern::Matches(). The pattern can be much broader than just
origin (such as [*.]google.com or *:80).

2. s/ContentSettings/WebsiteSettings/
The state of the code here is a bit unfortunate. The component is called
content_settings. The class is called ContentSettingsPattern. However, the
nomenclature has recently changed. We call the settings "website settings", and
"content settings" are a special subclass that only support ContentSetting as a
value. This is well illustrated in the classes WebsiteSettingsRegistry and
ContentSettingsRegistry.

Most importantly - we use this method on APP_BANNER and SITE_ENGAGEMENT, which
are website settings, but NOT content settings.   In fact, we really don't want
to use this on actual content settings. If I, as a user, set up a setting for
pattern "https://*" and then delete the origin "https://google.com", I would be
really surprised if that broad setting was deleted. But no such surprises should
happen for APP_BANNER and SITE_ENGAGEMENT, since they are not manually settable
by the user, and thus (to my knowledge) only use origins as their patterns. But
I really appreciate that you did test various possible patterns in this method's
unittest.

[dmurph, 2016/03/10 23:00:11]

Hmm.... sounds good, should I add more documentation around this filter to make
sure this isn't misused? What do you recommend?

[msramek, 2016/03/11 20:54:50]

We're currently only dealing with website settings (and not content settings) in
BrowsingDataRemover, so it isn't that likely to be misused. If the method is
called BuildWebsiteSettingsFilter and its parameter is ContentSettingsPattern,
it will hopefully be clear that it matches according to pattern.

But you might want to add a comment to the tests, noting that we're testing
broader-scoped patterns just for the sake of completeness, and we don't expect
website settings to actually use them.

+  std::set<url::Origin>* origins = new std::set<url::Origin>(origin_list_);
+  return base::Bind(&OriginFilterBuilder::MatchesContentSettingsPattern,
+                    base::Owned(origins), mode_);
+}
+
+base::Callback<bool(const net::CanonicalCookie& pattern)>
+    OriginFilterBuilder::BuildDomainCookieFilter() const {
+  std::set<url::Origin>* origins = new std::set<url::Origin>(origin_list_);
+  return base::Bind(&OriginFilterBuilder::MatchesCookieForTLDPlusOne,
+                    base::Owned(origins), mode_);
+}
+
 base::Callback<bool(const GURL&)>
     OriginFilterBuilder::BuildDomainFilter() const {
   std::set<url::Origin>* origins = new std::set<url::Origin>(origin_list_);
@@ -74,6 +94,53 @@ bool OriginFilterBuilder::MatchesURL(
 }
 
 // static
+bool OriginFilterBuilder::MatchesContentSettingsPattern(
+    std::set<url::Origin>* origins,
+    Mode mode,
+    const ContentSettingsPattern& pattern) {
+  for (const url::Origin& origin : *origins) {
+    if (pattern.Matches(GURL(origin.Serialize())))
+      return mode == WHITELIST;
+  }
+  return mode != WHITELIST;
+}
+
+// static
+bool OriginFilterBuilder::MatchesCookieForTLDPlusOne(
+    std::set<url::Origin>* origins,
+    Mode mode,
+    const net::CanonicalCookie& cookie) {
+  if (origins->empty())
+    return mode == BLACKLIST;
+
+  bool found = false;
+  std::string cookie_domain = cookie.Domain();
+  if (cookie.IsDomainCookie()) {
+    cookie_domain = cookie_domain.substr(1);
+  }

[Mike West, 2016/03/11 08:36:10]

Nit: No {} for single-line clauses.

[dmurph, 2016/04/05 00:16:28]

Done.

+  std::string cookie_tld_plus_one =
+      net::registry_controlled_domains::GetDomainAndRegistry(

[msramek, 2016/03/10 20:10:08]

I'm not really familiar with cookies, so I'll trust bsittler@'s judgement.

However, as a sanity check. My understanding is that some cookies, those where
Set-Cookie header was empty, are only scoped to the particular domain, and not
shared with superdomains. Do we still want to do eTLD+1 comparison for those?

[dmurph, 2016/03/10 23:00:11]

We basically break websites unless we treat this as eTLD+1 (we can't clear a
subdomain and leave the domain still there, and vice versa). I would also like
Mike West's feedback here.

+          cookie_domain,
+          net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
+  for (const url::Origin& origin : *origins) {
+    GURL origin_url(origin.Serialize());
+    if (origin_url.HostIsIPAddress()) {
+      if (origin_url.host() == cookie_domain) {
+        found = true;
+        break;
+      }
+    } else if (cookie_tld_plus_one ==
+               net::registry_controlled_domains::GetDomainAndRegistry(
+                   origin.host(), net::registry_controlled_domains::
+                                      INCLUDE_PRIVATE_REGISTRIES)) {
+      found = true;
+      break;
+    }
+  }
+  return (mode == WHITELIST) == found;
+}
+
+// static
 bool OriginFilterBuilder::MatchesURLWithSubdomains(
     std::set<url::Origin>* origins, Mode mode, const GURL& url) {
   if (origins->empty())