Check cookie access when getting or setting cookies in network service

This matches the behavior in ChromeNetworkDelegate. Third party cookie handling has been moved from NetworkContext to CookieManager. Note: A lot of these files changed are removing the unnecessary CookieOptions arg from a bunch of methods. Bug: 789636, 789632 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: I241cb396e588c433cf723c1effa8ea2c64f72266 Reviewed-on: https://chromium-review.googlesource.com/1100105 Commit-Queue: Clark DuVall <cduvall@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Christian Dullweber <dullweber@chromium.org> Cr-Commit-Position: refs/heads/master@{#568325}
marcosholgado · Jun 19, 2018 · c57280b · c57280b
1 parent b7f7455
commit c57280b
Show file tree

Hide file tree

Showing 44 changed files with 399 additions and 133 deletions.
diff --git a/android_webview/browser/aw_content_browser_client.cc b/android_webview/browser/aw_content_browser_client.cc
@@ -343,11 +343,9 @@ bool AwContentBrowserClient::AllowSetCookie(const GURL& url,
                                             const net::CanonicalCookie& cookie,
                                             content::ResourceContext* context,
                                             int render_process_id,
-                                            int render_frame_id,
-                                            const net::CookieOptions& options) {
+                                            int render_frame_id) {
   return AwCookieAccessPolicy::GetInstance()->AllowSetCookie(
-      url, first_party, cookie, context, render_process_id, render_frame_id,
-      options);
+      url, first_party, cookie, context, render_process_id, render_frame_id);
 }
 
 void AwContentBrowserClient::AllowWorkerFileSystem(

diff --git a/android_webview/browser/aw_content_browser_client.h b/android_webview/browser/aw_content_browser_client.h
@@ -74,8 +74,7 @@ class AwContentBrowserClient : public content::ContentBrowserClient {
                       const net::CanonicalCookie& cookie,
                       content::ResourceContext* context,
                       int render_process_id,
-                      int render_frame_id,
-                      const net::CookieOptions& options) override;
+                      int render_frame_id) override;
   void AllowWorkerFileSystem(
       const GURL& url,
       content::ResourceContext* context,

diff --git a/android_webview/browser/aw_cookie_access_policy.cc b/android_webview/browser/aw_cookie_access_policy.cc
@@ -120,8 +120,7 @@ bool AwCookieAccessPolicy::AllowSetCookie(const GURL& url,
                                           const net::CanonicalCookie& cookie,
                                           content::ResourceContext* context,
                                           int render_process_id,
-                                          int render_frame_id,
-                                          const net::CookieOptions& options) {
+                                          int render_frame_id) {
   bool global = GetShouldAcceptCookies();
   bool thirdParty = GetShouldAcceptThirdPartyCookies(
       render_process_id, render_frame_id,

diff --git a/android_webview/browser/aw_cookie_access_policy.h b/android_webview/browser/aw_cookie_access_policy.h
@@ -16,10 +16,6 @@ namespace content {
 class ResourceContext;
 }
 
-namespace net {
-class CookieOptions;
-}
-
 class GURL;
 
 namespace android_webview {
@@ -65,8 +61,7 @@ class AwCookieAccessPolicy {
                       const net::CanonicalCookie& cookie,
                       content::ResourceContext* context,
                       int render_process_id,
-                      int render_frame_id,
-                      const net::CookieOptions& options);
+                      int render_frame_id);
 
  private:
   friend struct base::LazyInstanceTraitsBase<AwCookieAccessPolicy>;

diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
@@ -864,9 +864,12 @@ bool GetDataSaverEnabledPref(const PrefService* prefs) {
 }
 
 WebContents* GetWebContents(int render_process_id, int render_frame_id) {
-  RenderFrameHost* rfh =
-      RenderFrameHost::FromID(render_process_id, render_frame_id);
-  return WebContents::FromRenderFrameHost(rfh);
+  if (render_process_id) {
+    RenderFrameHost* rfh =
+        RenderFrameHost::FromID(render_process_id, render_frame_id);
+    return WebContents::FromRenderFrameHost(rfh);
+  }
+  return WebContents::FromFrameTreeNodeId(render_frame_id);
 }
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
@@ -2244,13 +2247,9 @@ bool ChromeContentBrowserClient::AllowGetCookie(
   ProfileIOData* io_data = ProfileIOData::FromResourceContext(context);
   bool allow =
       io_data->GetCookieSettings()->IsCookieAccessAllowed(url, first_party);
+  OnCookiesRead(render_process_id, render_frame_id, url, first_party,
+                cookie_list, !allow);
 
-  base::Callback<content::WebContents*(void)> wc_getter =
-      base::Bind(&GetWebContents, render_process_id, render_frame_id);
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::BindOnce(&TabSpecificContentSettings::CookiesRead, wc_getter, url,
-                     first_party, cookie_list, !allow));
   return allow;
 }
 
@@ -2260,21 +2259,46 @@ bool ChromeContentBrowserClient::AllowSetCookie(
     const net::CanonicalCookie& cookie,
     content::ResourceContext* context,
     int render_process_id,
-    int render_frame_id,
-    const net::CookieOptions& options) {
+    int render_frame_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   ProfileIOData* io_data = ProfileIOData::FromResourceContext(context);
   content_settings::CookieSettings* cookie_settings =
       io_data->GetCookieSettings();
   bool allow = cookie_settings->IsCookieAccessAllowed(url, first_party);
 
-  base::Callback<content::WebContents*(void)> wc_getter =
-      base::Bind(&GetWebContents, render_process_id, render_frame_id);
+  OnCookieChange(render_process_id, render_frame_id, url, first_party, cookie,
+                 !allow);
+  return allow;
+}
+
+void ChromeContentBrowserClient::OnCookiesRead(
+    int process_id,
+    int routing_id,
+    const GURL& url,
+    const GURL& first_party_url,
+    const net::CookieList& cookie_list,
+    bool blocked_by_policy) {
+  base::RepeatingCallback<content::WebContents*(void)> wc_getter =
+      base::BindRepeating(&GetWebContents, process_id, routing_id);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::BindOnce(&TabSpecificContentSettings::CookiesRead, wc_getter, url,
+                     first_party_url, cookie_list, blocked_by_policy));
+}
+
+void ChromeContentBrowserClient::OnCookieChange(
+    int process_id,
+    int routing_id,
+    const GURL& url,
+    const GURL& first_party_url,
+    const net::CanonicalCookie& cookie,
+    bool blocked_by_policy) {
+  base::RepeatingCallback<content::WebContents*(void)> wc_getter =
+      base::BindRepeating(&GetWebContents, process_id, routing_id);
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::BindOnce(&TabSpecificContentSettings::CookieChanged, wc_getter, url,
-                     first_party, cookie, options, !allow));
-  return allow;
+                     first_party_url, cookie, blocked_by_policy));
 }
 
 void ChromeContentBrowserClient::AllowWorkerFileSystem(

diff --git a/chrome/browser/chrome_content_browser_client.h b/chrome/browser/chrome_content_browser_client.h
@@ -196,8 +196,19 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
                       const net::CanonicalCookie& cookie,
                       content::ResourceContext* context,
                       int render_process_id,
-                      int render_frame_id,
-                      const net::CookieOptions& options) override;
+                      int render_frame_id) override;
+  void OnCookiesRead(int process_id,
+                     int routing_id,
+                     const GURL& url,
+                     const GURL& first_party_url,
+                     const net::CookieList& cookie_list,
+                     bool blocked_by_policy) override;
+  void OnCookieChange(int process_id,
+                      int routing_id,
+                      const GURL& url,
+                      const GURL& first_party_url,
+                      const net::CanonicalCookie& cookie,
+                      bool blocked_by_policy) override;
   void AllowWorkerFileSystem(
       const GURL& url,
       content::ResourceContext* context,

diff --git a/chrome/browser/content_settings/content_settings_browsertest.cc b/chrome/browser/content_settings/content_settings_browsertest.cc
@@ -338,9 +338,8 @@ INSTANTIATE_TEST_CASE_P(
 // website(s) only for a session when all others are blocked.
 IN_PROC_BROWSER_TEST_F(ContentSettingsTest,
                        PRE_AllowCookiesForASessionUsingExceptions) {
-  // NOTE: don't use test_server here, since we need the port to be the same
-  // across the restart.
-  GURL url = URLRequestMockHTTPJob::GetMockUrl("setcookie.html");
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url = embedded_test_server()->GetURL("/setcookie.html");
   content_settings::CookieSettings* settings =
       CookieSettingsFactory::GetForProfile(browser()->profile()).get();
   settings->SetDefaultCookieSetting(CONTENT_SETTING_BLOCK);
@@ -355,7 +354,9 @@ IN_PROC_BROWSER_TEST_F(ContentSettingsTest,
 
 IN_PROC_BROWSER_TEST_F(ContentSettingsTest,
                        AllowCookiesForASessionUsingExceptions) {
-  GURL url = URLRequestMockHTTPJob::GetMockUrl("setcookie.html");
+  ASSERT_TRUE(embedded_test_server()->Start());
+  GURL url = embedded_test_server()->GetURL("/setcookie.html");
+  // Cookies are shared between ports, so this will get cookies set in PRE.
   ASSERT_TRUE(GetCookies(browser()->profile(), url).empty());
 }
 

diff --git a/chrome/browser/content_settings/tab_specific_content_settings.cc b/chrome/browser/content_settings/tab_specific_content_settings.cc
@@ -147,8 +147,7 @@ void TabSpecificContentSettings::CookiesRead(
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   TabSpecificContentSettings* settings = GetForWCGetter(wc_getter);
   if (settings) {
-    settings->OnCookiesRead(url, frame_url, cookie_list,
-                            blocked_by_policy);
+    settings->OnCookiesRead(url, frame_url, cookie_list, blocked_by_policy);
   }
 }
 
@@ -158,13 +157,11 @@ void TabSpecificContentSettings::CookieChanged(
     const GURL& url,
     const GURL& frame_url,
     const net::CanonicalCookie& cookie,
-    const net::CookieOptions& options,
     bool blocked_by_policy) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   TabSpecificContentSettings* settings = GetForWCGetter(wc_getter);
   if (settings)
-    settings->OnCookieChange(url, frame_url, cookie, options,
-                             blocked_by_policy);
+    settings->OnCookieChange(url, frame_url, cookie, blocked_by_policy);
 }
 
 // static
@@ -425,7 +422,6 @@ void TabSpecificContentSettings::OnCookieChange(
     const GURL& url,
     const GURL& frame_url,
     const net::CanonicalCookie& cookie,
-    const net::CookieOptions& options,
     bool blocked_by_policy) {
   if (blocked_by_policy) {
     blocked_local_shared_objects_.cookies()->AddChangedCookie(frame_url, url,

diff --git a/chrome/browser/content_settings/tab_specific_content_settings.h b/chrome/browser/content_settings/tab_specific_content_settings.h
@@ -33,10 +33,6 @@ namespace content {
 class NavigationHandle;
 }
 
-namespace net {
-class CookieOptions;
-}
-
 namespace url {
 class Origin;
 }  // namespace url
@@ -121,7 +117,6 @@ class TabSpecificContentSettings
       const GURL& url,
       const GURL& first_party_url,
       const net::CanonicalCookie& cookie,
-      const net::CookieOptions& options,
       bool blocked_by_policy);
 
   // Called when a specific Web database in the current page was accessed. If
@@ -336,7 +331,6 @@ class TabSpecificContentSettings
   void OnCookieChange(const GURL& url,
                       const GURL& first_party_url,
                       const net::CanonicalCookie& cookie,
-                      const net::CookieOptions& options,
                       bool blocked_by_policy);
   void OnFileSystemAccessed(const GURL& url,
                             bool blocked_by_policy);

diff --git a/chrome/browser/content_settings/tab_specific_content_settings_unittest.cc b/chrome/browser/content_settings/tab_specific_content_settings_unittest.cc
@@ -76,7 +76,7 @@ TEST_F(TabSpecificContentSettingsTest, BlockedContent) {
   std::unique_ptr<net::CanonicalCookie> cookie1(
       net::CanonicalCookie::Create(origin, "A=B", base::Time::Now(), options));
   ASSERT_TRUE(cookie1);
-  content_settings->OnCookieChange(origin, origin, *cookie1, options, false);
+  content_settings->OnCookieChange(origin, origin, *cookie1, false);
 #if !defined(OS_ANDROID)
   content_settings->OnContentBlocked(CONTENT_SETTINGS_TYPE_IMAGES);
 #endif
@@ -110,13 +110,13 @@ TEST_F(TabSpecificContentSettingsTest, BlockedContent) {
       CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC));
   EXPECT_TRUE(content_settings->IsContentBlocked(
       CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA));
-  content_settings->OnCookieChange(origin, origin, *cookie1, options, false);
+  content_settings->OnCookieChange(origin, origin, *cookie1, false);
 
   // Block a cookie.
   std::unique_ptr<net::CanonicalCookie> cookie2(
       net::CanonicalCookie::Create(origin, "C=D", base::Time::Now(), options));
   ASSERT_TRUE(cookie2);
-  content_settings->OnCookieChange(origin, origin, *cookie2, options, true);
+  content_settings->OnCookieChange(origin, origin, *cookie2, true);
   EXPECT_TRUE(
       content_settings->IsContentBlocked(CONTENT_SETTINGS_TYPE_COOKIES));
 
@@ -202,7 +202,7 @@ TEST_F(TabSpecificContentSettingsTest, AllowedContent) {
   std::unique_ptr<net::CanonicalCookie> cookie1(
       net::CanonicalCookie::Create(origin, "A=B", base::Time::Now(), options));
   ASSERT_TRUE(cookie1);
-  content_settings->OnCookieChange(origin, origin, *cookie1, options, false);
+  content_settings->OnCookieChange(origin, origin, *cookie1, false);
   ASSERT_TRUE(
       content_settings->IsContentAllowed(CONTENT_SETTINGS_TYPE_COOKIES));
   ASSERT_FALSE(
@@ -212,7 +212,7 @@ TEST_F(TabSpecificContentSettingsTest, AllowedContent) {
   std::unique_ptr<net::CanonicalCookie> cookie2(
       net::CanonicalCookie::Create(origin, "C=D", base::Time::Now(), options));
   ASSERT_TRUE(cookie2);
-  content_settings->OnCookieChange(origin, origin, *cookie2, options, true);
+  content_settings->OnCookieChange(origin, origin, *cookie2, true);
   ASSERT_TRUE(
       content_settings->IsContentAllowed(CONTENT_SETTINGS_TYPE_COOKIES));
   ASSERT_TRUE(
@@ -228,8 +228,7 @@ TEST_F(TabSpecificContentSettingsTest, EmptyCookieList) {
   ASSERT_FALSE(
       content_settings->IsContentBlocked(CONTENT_SETTINGS_TYPE_COOKIES));
   content_settings->OnCookiesRead(GURL("http://google.com"),
-                                  GURL("http://google.com"),
-                                  net::CookieList(),
+                                  GURL("http://google.com"), net::CookieList(),
                                   true);
   ASSERT_FALSE(
       content_settings->IsContentAllowed(CONTENT_SETTINGS_TYPE_COOKIES));
@@ -249,8 +248,7 @@ TEST_F(TabSpecificContentSettingsTest, SiteDataObserver) {
   std::unique_ptr<net::CanonicalCookie> cookie(
       net::CanonicalCookie::Create(origin, "A=B", base::Time::Now(), options));
   ASSERT_TRUE(cookie);
-  content_settings->OnCookieChange(origin, origin, *cookie, options,
-                                   blocked_by_policy);
+  content_settings->OnCookieChange(origin, origin, *cookie, blocked_by_policy);
 
   net::CookieList cookie_list;
   std::unique_ptr<net::CanonicalCookie> other_cookie(
@@ -261,8 +259,7 @@ TEST_F(TabSpecificContentSettingsTest, SiteDataObserver) {
 
   cookie_list.push_back(*other_cookie);
   content_settings->OnCookiesRead(GURL("http://google.com"),
-                                  GURL("http://google.com"),
-                                  cookie_list,
+                                  GURL("http://google.com"), cookie_list,
                                   blocked_by_policy);
   content_settings->OnFileSystemAccessed(GURL("http://google.com"),
                                               blocked_by_policy);

diff --git a/chrome/browser/net/chrome_network_delegate.cc b/chrome/browser/net/chrome_network_delegate.cc
@@ -436,7 +436,7 @@ bool ChromeNetworkDelegate::OnCanSetCookie(const net::URLRequest& request,
         BrowserThread::UI, FROM_HERE,
         base::BindOnce(&TabSpecificContentSettings::CookieChanged,
                        info->GetWebContentsGetterForRequest(), request.url(),
-                       request.site_for_cookies(), cookie, *options, !allow));
+                       request.site_for_cookies(), cookie, !allow));
   }
 
   return allow;

diff --git a/chrome/browser/profiles/profile_impl.cc b/chrome/browser/profiles/profile_impl.cc
@@ -1423,8 +1423,8 @@ void ProfileImpl::UpdateBlockThirdPartyCookies() {
       this, base::Bind(
                 [](bool block_third_party_cookies,
                    content::StoragePartition* partition) {
-                  partition->GetNetworkContext()->BlockThirdPartyCookies(
-                      block_third_party_cookies);
+                  partition->GetCookieManagerForBrowserProcess()
+                      ->BlockThirdPartyCookies(block_third_party_cookies);
                 },
                 block_third_party_cookies));
 }
@@ -1443,15 +1443,14 @@ void ProfileImpl::OnContentSettingChanged(
   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)));
+      this,
+      base::BindRepeating(
+          [](const ContentSettingsForOneType& settings,
+             content::StoragePartition* partition) {
+            partition->GetCookieManagerForBrowserProcess()->SetContentSettings(
+                settings);
+          },
+          std::move(settings)));
 }
 
 // Gets the media cache parameters from the command line. |cache_path| will be

diff --git a/chrome/browser/ui/content_settings/content_setting_image_model_unittest.cc b/chrome/browser/ui/content_settings/content_setting_image_model_unittest.cc
@@ -115,7 +115,7 @@ TEST_F(ContentSettingImageModelTest, CookieAccessed) {
   std::unique_ptr<net::CanonicalCookie> cookie(
       net::CanonicalCookie::Create(origin, "A=B", base::Time::Now(), options));
   ASSERT_TRUE(cookie);
-  content_settings->OnCookieChange(origin, origin, *cookie, options, false);
+  content_settings->OnCookieChange(origin, origin, *cookie, false);
   content_setting_image_model->UpdateFromWebContents(web_contents());
   EXPECT_TRUE(content_setting_image_model->is_visible());
   EXPECT_TRUE(HasIcon(*content_setting_image_model));

diff --git a/components/content_settings/core/browser/cookie_settings.cc b/components/content_settings/core/browser/cookie_settings.cc
@@ -41,13 +41,6 @@ ContentSetting CookieSettings::GetDefaultCookieSetting(
       CONTENT_SETTINGS_TYPE_COOKIES, provider_id);
 }
 
-bool CookieSettings::IsCookieAccessAllowed(const GURL& url,
-                                           const GURL& first_party_url) const {
-  ContentSetting setting;
-  GetCookieSetting(url, first_party_url, nullptr, &setting);
-  return IsAllowed(setting);
-}
-
 bool CookieSettings::IsCookieSessionOnly(const GURL& origin) const {
   ContentSetting setting;
   GetCookieSetting(origin, origin, nullptr, &setting);

diff --git a/components/content_settings/core/browser/cookie_settings.h b/components/content_settings/core/browser/cookie_settings.h
@@ -47,13 +47,6 @@ class CookieSettings : public CookieSettingsBase,
   // This may be called on any thread.
   ContentSetting GetDefaultCookieSetting(std::string* provider_id) const;
 
-  // Returns true if the page identified by (|url|, |first_party_url|) is
-  // allowed to access (i.e., read or write) cookies.
-  //
-  // This may be called on any thread.
-  bool IsCookieAccessAllowed(const GURL& url,
-                             const GURL& first_party_url) const;
-
   // Returns true if the cookie set by a page identified by |url| should be
   // session only. Querying this only makes sense if |IsCookieAccessAllowed|
   // has returned true.