Use different type of CookieAccessDelegate for Android WebView

This change adds different types with which CookieAccessDelegateImpl
can be constructed. One type, USE_CONTENT_SETTINGS is the default
behavior of checking the CookieSettings to determine whether a given
cookie is Legacy or Nonlegacy. The other type, ALWAYS_LEGACY, always
returns Legacy access semantics for every cookie.

The ALWAYS_LEGACY type of CookieAccessDelegate is used for Android
WebView, for compatibility reasons. (The SameSite cookie changes would
break too much stuff, so we treat every cookie as Legacy to maintain
compatibility.)

Bug: 986319
Change-Id: I68384da121b025525da5fd2ef6f9dff25a8a2ff5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1872782Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Martin Barbella <mbarbella@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Commit-Queue: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708674}

android_webview/browser/DEPS

@@ -59,6 +59,8 @@ include_rules = [
   "+printing",
 
   "+services/network/public",
+  # This is needed for the CookieManager to configure the CookieStore directly.
+  "+services/network/cookie_access_delegate_impl.h",
   "+services/resource_coordinator/public/cpp",
   "+services/service_manager/public/cpp",
 

android_webview/browser/aw_browser_context.cc

@@ -454,6 +454,9 @@ AwBrowserContext::GetNetworkContextParams(
       network::mojom::CookieManagerParams::New();
   context_params->cookie_manager_params->allow_file_scheme_cookies =
       GetCookieManager()->AllowFileSchemeCookies();
+  // Set all cookies to Legacy on Android WebView, for compatibility reasons.
+  context_params->cookie_manager_params->cookie_access_delegate_type =
+      network::mojom::CookieAccessDelegateType::ALWAYS_LEGACY;
 
   context_params->initial_ssl_config = network::mojom::SSLConfig::New();
   // Allow SHA-1 to be used for locally-installed trust anchors, as WebView

android_webview/browser/cookie_manager.cc

@@ -44,6 +44,7 @@
 #include "net/cookies/cookie_util.h"
 #include "net/cookies/parsed_cookie.h"
 #include "net/url_request/url_request_context.h"
+#include "services/network/cookie_access_delegate_impl.h"
 #include "services/network/network_service.h"
 #include "services/network/public/mojom/cookie_manager.mojom.h"
 #include "url/url_constants.h"
@@ -324,6 +325,11 @@ net::CookieStore* CookieManager::GetCookieStore() {
     }
 
     cookie_store_ = content::CreateCookieStore(cookie_config, nullptr);
+    // Use a CookieAccessDelegate that always returns Legacy mode, for
+    // compatibility reasons.
+    cookie_store_->SetCookieAccessDelegate(
+        std::make_unique<network::CookieAccessDelegateImpl>(
+            network::mojom::CookieAccessDelegateType::ALWAYS_LEGACY));
   }
 
   return cookie_store_.get();

chrome/browser/net/profile_network_context_service.cc

@@ -417,6 +417,8 @@ ProfileNetworkContextService::CreateCookieManagerParams(
       &settings_for_legacy_cookie_access);
   out->settings_for_legacy_cookie_access =
       std::move(settings_for_legacy_cookie_access);
+  out->cookie_access_delegate_type =
+      network::mojom::CookieAccessDelegateType::USE_CONTENT_SETTINGS;
   return out;
 }
 

services/network/cookie_access_delegate_impl.cc

@@ -9,13 +9,20 @@
 namespace network {
 
 CookieAccessDelegateImpl::CookieAccessDelegateImpl(
+    mojom::CookieAccessDelegateType type,
     const CookieSettings* cookie_settings)
-    : cookie_settings_(cookie_settings) {}
+    : type_(type), cookie_settings_(cookie_settings) {
+  if (type == mojom::CookieAccessDelegateType::USE_CONTENT_SETTINGS) {
+    DCHECK(cookie_settings);
+  }
+}
 
 CookieAccessDelegateImpl::~CookieAccessDelegateImpl() = default;
 
 net::CookieAccessSemantics CookieAccessDelegateImpl::GetAccessSemantics(
     const net::CanonicalCookie& cookie) const {
+  if (type_ == mojom::CookieAccessDelegateType::ALWAYS_LEGACY)
+    return net::CookieAccessSemantics::LEGACY;
   return cookie_settings_->GetCookieAccessSemanticsForDomain(cookie.Domain());
 }
 

services/network/cookie_access_delegate_impl.h

@@ -8,16 +8,19 @@
 #include "base/component_export.h"
 #include "net/cookies/cookie_access_delegate.h"
 #include "services/network/cookie_settings.h"
+#include "services/network/public/mojom/cookie_manager.mojom.h"
 
 namespace network {
 
 class COMPONENT_EXPORT(NETWORK_SERVICE) CookieAccessDelegateImpl
     : public net::CookieAccessDelegate {
  public:
-  // |cookie_settings| contains the set of content settings that describes which
-  // cookies should be subject to legacy access rules.
-  // |cookie_settings| is expected to outlive this class.
-  explicit CookieAccessDelegateImpl(const CookieSettings* cookie_settings);
+  // If |type| is USE_CONTENT_SETTINGS, a non-null |cookie_settings| is
+  // expected. |cookie_settings| contains the set of content settings that
+  // describes which cookies should be subject to legacy access rules.
+  // If non-null, |cookie_settings| is expected to outlive this class.
+  CookieAccessDelegateImpl(mojom::CookieAccessDelegateType type,
+                           const CookieSettings* cookie_settings = nullptr);
 
   ~CookieAccessDelegateImpl() override;
 
@@ -26,6 +29,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CookieAccessDelegateImpl
       const net::CanonicalCookie& cookie) const override;
 
  private:
+  const mojom::CookieAccessDelegateType type_;
   const CookieSettings* const cookie_settings_;
 };
 

services/network/cookie_manager.cc

@@ -18,6 +18,7 @@
 #include "net/cookies/cookie_options.h"
 #include "net/cookies/cookie_store.h"
 #include "net/cookies/cookie_util.h"
+#include "services/network/cookie_access_delegate_impl.h"
 #include "services/network/session_cleanup_cookie_store.h"
 #include "url/gurl.h"
 
@@ -47,12 +48,18 @@ CookieManager::CookieManager(
     mojom::CookieManagerParamsPtr params)
     : cookie_store_(cookie_store),
       session_cleanup_cookie_store_(std::move(session_cleanup_cookie_store)) {
+  mojom::CookieAccessDelegateType cookie_access_delegate_type =
+      mojom::CookieAccessDelegateType::USE_CONTENT_SETTINGS;
   if (params) {
     ConfigureCookieSettings(*params, &cookie_settings_);
+    cookie_access_delegate_type = params->cookie_access_delegate_type;
     // Don't wait for callback, the work happens synchronously.
     AllowFileSchemeCookies(params->allow_file_scheme_cookies,
                            base::DoNothing());
   }
+  cookie_store_->SetCookieAccessDelegate(
+      std::make_unique<CookieAccessDelegateImpl>(cookie_access_delegate_type,
+                                                 &cookie_settings_));
 }
 
 CookieManager::~CookieManager() {
@@ -60,6 +67,10 @@ CookieManager::~CookieManager() {
     session_cleanup_cookie_store_->DeleteSessionCookies(
         cookie_settings_.CreateDeleteCookieOnExitPredicate());
   }
+  // Make sure we destroy the CookieStore's CookieAccessDelegate, because it
+  // holds a pointer to this CookieManager's CookieSettings, which is about to
+  // be destroyed.
+  cookie_store_->SetCookieAccessDelegate(nullptr);
 }
 
 void CookieManager::AddReceiver(

services/network/network_context.cc

@@ -64,7 +64,6 @@
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_builder.h"
-#include "services/network/cookie_access_delegate_impl.h"
 #include "services/network/cookie_manager.h"
 #include "services/network/cors/cors_url_loader_factory.h"
 #include "services/network/host_resolver.h"
@@ -429,12 +428,6 @@ NetworkContext::~NetworkContext() {
   // shutdown would be unnecessary as the reports would be thrown out.
   domain_reliability_monitor_.reset();
 
-  // The cookie store's CookieAccessDelegate holds a pointer to the
-  // CookieManager's CookieSettings. Since the CookieManager is being destroyed,
-  // first destroy the CookieAccessDelegate.
-  if (url_request_context_ && url_request_context_->cookie_store())
-    url_request_context_->cookie_store()->SetCookieAccessDelegate(nullptr);
-
   if (url_request_context_ &&
       url_request_context_->transport_security_state()) {
     if (certificate_report_sender_) {
@@ -1984,10 +1977,6 @@ URLRequestContextOwner NetworkContext::MakeURLRequestContext() {
       std::move(session_cleanup_cookie_store),
       std::move(params_->cookie_manager_params));
 
-  result.url_request_context->cookie_store()->SetCookieAccessDelegate(
-      std::make_unique<CookieAccessDelegateImpl>(
-          &cookie_manager_->cookie_settings()));
-
   if (cert_net_fetcher_)
     cert_net_fetcher_->SetURLRequestContext(result.url_request_context.get());
 

services/network/public/mojom/cookie_manager.mojom

@@ -33,6 +33,19 @@ struct CookieManagerParams {
   // access rules.
   array<content_settings.mojom.ContentSettingPatternSource>
       settings_for_legacy_cookie_access;
+
+  // The type of CookieAccessDelegate to pass to the underlying CookieStore.
+  // If these params are not present, CookieManager defaults to using
+  // USE_CONTENT_SETTINGS.
+  CookieAccessDelegateType cookie_access_delegate_type = USE_CONTENT_SETTINGS;
+};
+
+enum CookieAccessDelegateType {
+  // Decides access semantics based on the content settings it was constructed
+  // with.
+  USE_CONTENT_SETTINGS,
+  // Always returns Legacy access semantics.
+  ALWAYS_LEGACY,
 };
 
 enum CookiePriority {