Handle deleting cookies with session-only policy in network service

The ContentSettings for cookies are now synced to the network service,
which allows us to delete cookies properly when the session ends.

SessionCleanupCookieStore and tests are mostly moved over from
QuotaPolicyCookieStore.

Bug: 848801
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.linux:linux_mojo;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I53f8d7f6cb6439d3b39fad22c406bf058b758e52
Reviewed-on: https://chromium-review.googlesource.com/1090035
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567428}

chrome/browser/extensions/DEPS

@@ -10,6 +10,7 @@ include_rules = [
   "+components/arc",
   "+components/vector_icons",
   "+services/network/public/mojom",
+  "+services/network/session_cleanup_cookie_store.h",
 
   # For access to testing command line switches.
   "+ppapi/shared_impl",

chrome/browser/extensions/extension_special_storage_policy.cc

@@ -113,10 +113,10 @@ bool ExtensionSpecialStoragePolicy::IsStorageSessionOnly(const GURL& origin) {
   return cookie_settings_->IsCookieSessionOnly(origin);
 }
 
-storage::SpecialStoragePolicy::DeleteCookiePredicate
+network::SessionCleanupCookieStore::DeleteCookiePredicate
 ExtensionSpecialStoragePolicy::CreateDeleteCookieOnExitPredicate() {
   if (cookie_settings_.get() == NULL)
-    return DeleteCookiePredicate();
+    return network::SessionCleanupCookieStore::DeleteCookiePredicate();
   // Fetch the list of cookies related content_settings and bind it
   // to CookieSettings::ShouldDeleteCookieOnExit to avoid fetching it on
   // every call.

chrome/browser/extensions/extension_special_storage_policy.h

@@ -11,6 +11,7 @@
 
 #include "base/synchronization/lock.h"
 #include "extensions/common/extension_set.h"
+#include "services/network/session_cleanup_cookie_store.h"
 #include "storage/browser/quota/special_storage_policy.h"
 #include "url/gurl.h"
 
@@ -42,7 +43,8 @@ class ExtensionSpecialStoragePolicy : public storage::SpecialStoragePolicy {
   bool HasIsolatedStorage(const GURL& origin) override;
   bool HasSessionOnlyOrigins() override;
   bool IsStorageDurable(const GURL& origin) override;
-  DeleteCookiePredicate CreateDeleteCookieOnExitPredicate() override;
+  network::SessionCleanupCookieStore::DeleteCookiePredicate
+  CreateDeleteCookieOnExitPredicate() override;
 
   // Methods used by the ExtensionService to populate this class.
   void GrantRightsForExtension(const extensions::Extension* extension,

chrome/browser/extensions/mock_extension_special_storage_policy.cc

@@ -24,9 +24,9 @@ bool MockExtensionSpecialStoragePolicy::HasSessionOnlyOrigins() {
   return false;
 }
 
-storage::SpecialStoragePolicy::DeleteCookiePredicate
+network::SessionCleanupCookieStore::DeleteCookiePredicate
 MockExtensionSpecialStoragePolicy::CreateDeleteCookieOnExitPredicate() {
-  return DeleteCookiePredicate();
+  return network::SessionCleanupCookieStore::DeleteCookiePredicate();
 }
 
 MockExtensionSpecialStoragePolicy::~MockExtensionSpecialStoragePolicy() {}

chrome/browser/extensions/mock_extension_special_storage_policy.h

@@ -10,6 +10,7 @@
 
 #include "base/macros.h"
 #include "chrome/browser/extensions/extension_special_storage_policy.h"
+#include "services/network/session_cleanup_cookie_store.h"
 #include "url/gurl.h"
 
 // This class is the same as MockSpecialStoragePolicy (in
@@ -24,7 +25,8 @@ class MockExtensionSpecialStoragePolicy : public ExtensionSpecialStoragePolicy {
   bool IsStorageUnlimited(const GURL& origin) override;
   bool IsStorageSessionOnly(const GURL& origin) override;
   bool HasSessionOnlyOrigins() override;
-  DeleteCookiePredicate CreateDeleteCookieOnExitPredicate() override;
+  network::SessionCleanupCookieStore::DeleteCookiePredicate
+  CreateDeleteCookieOnExitPredicate() override;
 
   void AddProtected(const GURL& origin) {
     protected_.insert(origin);

chrome/browser/policy/policy_browsertest.cc

@@ -30,6 +30,7 @@
 #include "base/metrics/statistics_recorder.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
+#include "base/strings/strcat.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -48,6 +49,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.h"
 #include "chrome/browser/component_updater/chrome_component_updater_configurator.h"
+#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/content_settings/tab_specific_content_settings.h"
 #include "chrome/browser/devtools/devtools_window_testing.h"
 #include "chrome/browser/download/download_prefs.h"
@@ -116,6 +118,7 @@
 #include "components/bookmarks/common/bookmark_pref_names.h"
 #include "components/browsing_data/core/pref_names.h"
 #include "components/component_updater/component_updater_service.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "components/content_settings/core/common/pref_names.h"
@@ -1224,7 +1227,7 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, PRE_PRE_DefaultCookiesSetting) {
   // No cookies at startup.
   EXPECT_TRUE(content::GetCookies(profile, url).empty());
   // Set a cookie now.
-  std::string value = std::string(kCookieValue) + std::string(kCookieOptions);
+  std::string value = base::StrCat({kCookieValue, kCookieOptions});
   EXPECT_TRUE(content::SetCookie(profile, url, value));
   // Verify it was set.
   EXPECT_EQ(kCookieValue, GetCookies(profile, url));
@@ -1237,7 +1240,8 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, PRE_DefaultCookiesSetting) {
   PolicyMap policies;
   policies.Set(key::kDefaultCookiesSetting, POLICY_LEVEL_MANDATORY,
                POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD,
-               std::make_unique<base::Value>(4), nullptr);
+               std::make_unique<base::Value>(CONTENT_SETTING_SESSION_ONLY),
+               nullptr);
   UpdateProviderPolicy(policies);
 }
 
@@ -1246,6 +1250,36 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, DefaultCookiesSetting) {
   EXPECT_TRUE(GetCookies(browser()->profile(), GURL(kURL)).empty());
 }
 
+IN_PROC_BROWSER_TEST_F(PolicyTest, PRE_PRE_WebsiteCookiesSetting) {
+  // Verifies that cookies are deleted on shutdown. This test is split in 3
+  // parts because it spans 2 browser restarts.
+
+  Profile* profile = browser()->profile();
+  GURL url(kURL);
+  // No cookies at startup.
+  EXPECT_TRUE(content::GetCookies(profile, url).empty());
+  // Set a cookie now.
+  std::string value = base::StrCat({kCookieValue, kCookieOptions});
+  EXPECT_TRUE(content::SetCookie(profile, url, value));
+  // Verify it was set.
+  EXPECT_EQ(kCookieValue, GetCookies(profile, url));
+}
+
+IN_PROC_BROWSER_TEST_F(PolicyTest, PRE_WebsiteCookiesSetting) {
+  // Verify that the cookie persists across restarts.
+  EXPECT_EQ(kCookieValue, GetCookies(browser()->profile(), GURL(kURL)));
+  // Now set the policy and the cookie should be gone after another restart.
+  HostContentSettingsMapFactory::GetForProfile(browser()->profile())
+      ->SetWebsiteSettingDefaultScope(
+          GURL(kURL), GURL(kURL), CONTENT_SETTINGS_TYPE_COOKIES, std::string(),
+          std::make_unique<base::Value>(CONTENT_SETTING_SESSION_ONLY));
+}
+
+IN_PROC_BROWSER_TEST_F(PolicyTest, WebsiteCookiesSetting) {
+  // Verify that the cookie is gone.
+  EXPECT_TRUE(GetCookies(browser()->profile(), GURL(kURL)).empty());
+}
+
 IN_PROC_BROWSER_TEST_F(PolicyTest, DefaultSearchProvider) {
   MakeRequestFail make_request_fail("search.example");
 

chrome/browser/profiles/profile_impl.cc

@@ -593,6 +593,11 @@ void ProfileImpl::DoFinalInit() {
         base::Bind(&ProfileImpl::UpdateBlockThirdPartyCookies,
                    base::Unretained(this)));
     UpdateBlockThirdPartyCookies();
+
+    // Observe content settings so they can be synced to the network service.
+    HostContentSettingsMapFactory::GetForProfile(this)->AddObserver(this);
+    OnContentSettingChanged(ContentSettingsPattern(), ContentSettingsPattern(),
+                            CONTENT_SETTINGS_TYPE_COOKIES, std::string());
   }
 
   media_device_id_salt_ = new MediaDeviceIDSalt(prefs_.get());
@@ -1424,6 +1429,31 @@ void ProfileImpl::UpdateBlockThirdPartyCookies() {
                 block_third_party_cookies));
 }
 
+void ProfileImpl::OnContentSettingChanged(
+    const ContentSettingsPattern& primary_pattern,
+    const ContentSettingsPattern& secondary_pattern,
+    ContentSettingsType content_type,
+    std::string resource_identifier) {
+  if (content_type != CONTENT_SETTINGS_TYPE_COOKIES &&
+      content_type != CONTENT_SETTINGS_TYPE_DEFAULT) {
+    return;
+  }
+
+  ContentSettingsForOneType settings;
+  HostContentSettingsMapFactory::GetForProfile(this)->GetSettingsForOneType(
+      CONTENT_SETTINGS_TYPE_COOKIES, std::string(), &settings);
+  content::BrowserContext::ForEachStoragePartition(
+      this, base::BindRepeating(
+                [](const ContentSettingsForOneType& settings,
+                   content::StoragePartition* partition) {
+                  network::mojom::CookieManagerPtr cookie_manager;
+                  partition->GetNetworkContext()->GetCookieManager(
+                      mojo::MakeRequest(&cookie_manager));
+                  cookie_manager->SetContentSettings(settings);
+                },
+                std::move(settings)));
+}
+
 // Gets the media cache parameters from the command line. |cache_path| will be
 // set to the user provided path, or will not be touched if there is not an
 // argument. |max_size| will be the user provided value or zero by default.

chrome/browser/profiles/profile_impl.h

@@ -20,6 +20,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_impl_io_data.h"
 #include "chrome/common/buildflags.h"
+#include "components/content_settings/core/browser/content_settings_observer.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "content/public/browser/content_browser_client.h"
 #include "extensions/buildflags/buildflags.h"
@@ -64,7 +65,7 @@ class PrefRegistrySyncable;
 }
 
 // The default profile implementation.
-class ProfileImpl : public Profile {
+class ProfileImpl : public Profile, public content_settings::Observer {
  public:
   // Value written to prefs when the exit type is EXIT_NORMAL. Public for tests.
   static const char kPrefExitTypeNormal[];
@@ -193,6 +194,12 @@ class ProfileImpl : public Profile {
 
   void GetMediaCacheParameters(base::FilePath* cache_path, int* max_size);
 
+  // content_settings::Observer:
+  void OnContentSettingChanged(const ContentSettingsPattern& primary_pattern,
+                               const ContentSettingsPattern& secondary_pattern,
+                               ContentSettingsType content_type,
+                               std::string resource_identifier) override;
+
   std::unique_ptr<domain_reliability::DomainReliabilityMonitor>
   CreateDomainReliabilityMonitor(PrefService* local_state);
 

components/content_settings/core/browser/cookie_settings.cc

@@ -16,25 +16,8 @@
 #include "extensions/buildflags/buildflags.h"
 #include "net/base/net_errors.h"
 #include "net/base/static_cookie_policy.h"
-#include "net/cookies/cookie_util.h"
 #include "url/gurl.h"
 
-namespace {
-
-bool IsValidSetting(ContentSetting setting) {
-  return (setting == CONTENT_SETTING_ALLOW ||
-          setting == CONTENT_SETTING_SESSION_ONLY ||
-          setting == CONTENT_SETTING_BLOCK);
-}
-
-bool IsAllowed(ContentSetting setting) {
-  DCHECK(IsValidSetting(setting));
-  return (setting == CONTENT_SETTING_ALLOW ||
-          setting == CONTENT_SETTING_SESSION_ONLY);
-}
-
-}  // namespace
-
 namespace content_settings {
 
 CookieSettings::CookieSettings(
@@ -72,36 +55,6 @@ bool CookieSettings::IsCookieSessionOnly(const GURL& origin) const {
   return (setting == CONTENT_SETTING_SESSION_ONLY);
 }
 
-bool CookieSettings::ShouldDeleteCookieOnExit(
-    const ContentSettingsForOneType& cookie_settings,
-    const std::string& domain,
-    bool is_https) const {
-  GURL origin = net::cookie_util::CookieOriginToURL(domain, is_https);
-  ContentSetting setting;
-  GetCookieSetting(origin, origin, nullptr, &setting);
-  DCHECK(IsValidSetting(setting));
-  if (setting == CONTENT_SETTING_ALLOW)
-    return false;
-  // Non-secure cookies are readable by secure sites. We need to check for
-  // https pattern if http is not allowed. The section below is independent
-  // of the scheme so we can just retry from here.
-  if (!is_https)
-    return ShouldDeleteCookieOnExit(cookie_settings, domain, true);
-  // Check if there is a more precise rule that "domain matches" this cookie.
-  bool matches_session_only_rule = false;
-  for (const auto& entry : cookie_settings) {
-    const std::string& host = entry.primary_pattern.GetHost();
-    if (net::cookie_util::IsDomainMatch(domain, host)) {
-      if (entry.GetContentSetting() == CONTENT_SETTING_ALLOW) {
-        return false;
-      } else if (entry.GetContentSetting() == CONTENT_SETTING_SESSION_ONLY) {
-        matches_session_only_rule = true;
-      }
-    }
-  }
-  return setting == CONTENT_SETTING_SESSION_ONLY || matches_session_only_rule;
-}
-
 void CookieSettings::GetCookieSettings(
     ContentSettingsForOneType* settings) const {
   host_content_settings_map_->GetSettingsForOneType(

components/content_settings/core/browser/cookie_settings.h

@@ -14,6 +14,7 @@
 #include "base/threading/thread_checker.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
+#include "components/content_settings/core/common/cookie_settings_base.h"
 #include "components/keyed_service/core/refcounted_keyed_service.h"
 #include "components/prefs/pref_change_registrar.h"
 
@@ -28,7 +29,8 @@ const char kDummyExtensionScheme[] = ":no-extension-scheme:";
 // A frontend to the cookie settings of |HostContentSettingsMap|. Handles
 // cookie-specific logic such as blocking third-party cookies. Written on the UI
 // thread and read on any thread.
-class CookieSettings : public RefcountedKeyedService {
+class CookieSettings : public CookieSettingsBase,
+                       public RefcountedKeyedService {
  public:
   // Creates a new CookieSettings instance.
   // The caller is responsible for ensuring that |extension_scheme| is valid for
@@ -59,20 +61,6 @@ class CookieSettings : public RefcountedKeyedService {
   // This may be called on any thread.
   bool IsCookieSessionOnly(const GURL& url) const;
 
-  // Returns true if the cookie associated with |domain| should be deleted
-  // on exit.
-  // This uses domain matching as described in section 5.1.3 of RFC 6265 to
-  // identify content setting rules that could have influenced the cookie
-  // when it was created.
-  // As |cookie_settings| can be expensive to create, it should be cached if
-  // multiple calls to ShouldDeleteCookieOnExit() are made.
-  //
-  // This may be called on any thread.
-  bool ShouldDeleteCookieOnExit(
-      const ContentSettingsForOneType& cookie_settings,
-      const std::string& domain,
-      bool is_https) const;
-
   // Returns all patterns with a non-default cookie setting, mapped to their
   // actual settings, in the precedence order of the setting rules. |settings|
   // must be a non-nullptr outparam.
@@ -103,11 +91,11 @@ class CookieSettings : public RefcountedKeyedService {
   // called.
   void ShutdownOnUIThread() override;
 
-  // A helper for applying third party cookie blocking rules.
+  // content_settings::CookieSettingsBase:
   void GetCookieSetting(const GURL& url,
                         const GURL& first_party_url,
                         content_settings::SettingSource* source,
-                        ContentSetting* cookie_setting) const;
+                        ContentSetting* cookie_setting) const override;
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 

components/content_settings/core/common/BUILD.gn

@@ -15,6 +15,8 @@ static_library("common") {
     "content_settings_types.h",
     "content_settings_utils.cc",
     "content_settings_utils.h",
+    "cookie_settings_base.cc",
+    "cookie_settings_base.h",
     "pref_names.cc",
     "pref_names.h",
   ]
@@ -35,10 +37,12 @@ source_set("unit_tests") {
   sources = [
     "content_settings_pattern_parser_unittest.cc",
     "content_settings_pattern_unittest.cc",
+    "cookie_settings_base_unittest.cc",
   ]
 
   deps = [
     ":common",
+    "//base",
     "//net",
     "//testing/gmock",
     "//testing/gtest",

components/content_settings/core/common/DEPS

@@ -2,6 +2,7 @@ include_rules = [
   "+mojo/public/cpp/base",
   "+mojo/public/cpp/bindings",
   "+net/base",
+  "+net/cookies/cookie_util.h",
   "+testing",
   "+url",
 ]