AW: simplify AwCookieAccessPolicy

No change to logic.

This simplifies aw_cookie_access_policy.cc:

 - Inline AwStaticCookiePolicy's body into AwCookieAccessPolicy
 - Rename AwStaticCookiePolicyTest to AwStaticCookiePolicyTest and
   rewrite the tests
 - Consolidate AwCookieAccessPolicy's SetCookie+GetCookie methods into
   AllowCookies() overrides, since we don't distinguish between setting
   and getting
 - Add DCHECKs for threading and to mark the legacy code path
 - Update callsites to use the AllowCookies() methods

This also adds unit test coverage for WebView's special handling of
file:// scheme cookies.

Bug: 932535
Test: run_webview_instrumentation_test_apk -f CookieManagerTest#*
Test: run_android_webview_unittests -f AwStaticCookiePolicyTest.*
Cq-Include-Trybots: luci.chromium.try:android_mojo
Change-Id: I457279d089c347f286f939e2cdcec9e371803730
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1548511Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#648336}

android_webview/browser/aw_content_browser_client.cc

@@ -497,12 +497,8 @@ bool AwContentBrowserClient::AllowGetCookie(const GURL& url,
                                             content::ResourceContext* context,
                                             int render_process_id,
                                             int render_frame_id) {
-  return AwCookieAccessPolicy::GetInstance()->AllowGetCookie(url,
-                                                             first_party,
-                                                             cookie_list,
-                                                             context,
-                                                             render_process_id,
-                                                             render_frame_id);
+  return AwCookieAccessPolicy::GetInstance()->AllowCookies(
+      url, first_party, render_process_id, render_frame_id);
 }
 
 bool AwContentBrowserClient::AllowSetCookie(const GURL& url,
@@ -511,8 +507,8 @@ bool AwContentBrowserClient::AllowSetCookie(const GURL& url,
                                             content::ResourceContext* context,
                                             int render_process_id,
                                             int render_frame_id) {
-  return AwCookieAccessPolicy::GetInstance()->AllowSetCookie(
-      url, first_party, cookie, context, render_process_id, render_frame_id);
+  return AwCookieAccessPolicy::GetInstance()->AllowCookies(
+      url, first_party, render_process_id, render_frame_id);
 }
 
 void AwContentBrowserClient::AllowWorkerFileSystem(

android_webview/browser/aw_cookie_access_policy.cc

@@ -7,18 +7,22 @@
 #include <memory>
 
 #include "android_webview/browser/aw_contents_io_thread_client.h"
+#include "base/feature_list.h"
 #include "base/logging.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/websocket_handshake_request_info.h"
 #include "net/base/net_errors.h"
+#include "net/base/static_cookie_policy.h"
+#include "net/url_request/url_request.h"
+#include "services/network/public/cpp/features.h"
+#include "url/gurl.h"
 
 using base::AutoLock;
 using content::BrowserThread;
 using content::ResourceRequestInfo;
 using content::WebSocketHandshakeRequestInfo;
-using net::StaticCookiePolicy;
 
 namespace android_webview {
 
@@ -51,6 +55,7 @@ bool AwCookieAccessPolicy::GetShouldAcceptThirdPartyCookies(
     int render_process_id,
     int render_frame_id,
     int frame_tree_node_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   std::unique_ptr<AwContentsIoThreadClient> io_thread_client =
       (frame_tree_node_id != content::RenderFrameHost::kNoFrameTreeNodeId)
           ? AwContentsIoThreadClient::FromID(frame_tree_node_id)
@@ -65,6 +70,7 @@ bool AwCookieAccessPolicy::GetShouldAcceptThirdPartyCookies(
 
 bool AwCookieAccessPolicy::GetShouldAcceptThirdPartyCookies(
     const net::URLRequest& request) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   int child_id = 0;
   int render_frame_id = 0;
   int frame_tree_node_id = content::RenderFrameHost::kNoFrameTreeNodeId;
@@ -85,81 +91,45 @@ bool AwCookieAccessPolicy::GetShouldAcceptThirdPartyCookies(
                                           frame_tree_node_id);
 }
 
-bool AwCookieAccessPolicy::OnCanGetCookies(const net::URLRequest& request,
-                                           const net::CookieList& cookie_list) {
-  bool global = GetShouldAcceptCookies();
+bool AwCookieAccessPolicy::AllowCookies(const net::URLRequest& request) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(!base::FeatureList::IsEnabled(network::features::kNetworkService));
   bool third_party = GetShouldAcceptThirdPartyCookies(request);
-  return AwStaticCookiePolicy(global, third_party)
-      .AllowGet(request.url(), request.site_for_cookies());
+  return CanAccessCookies(request.url(), request.site_for_cookies(),
+                          third_party);
 }
 
-bool AwCookieAccessPolicy::OnCanSetCookie(const net::URLRequest& request,
-                                          const net::CanonicalCookie& cookie,
-                                          net::CookieOptions* options) {
-  bool global = GetShouldAcceptCookies();
-  bool third_party = GetShouldAcceptThirdPartyCookies(request);
-  return AwStaticCookiePolicy(global, third_party)
-      .AllowSet(request.url(), request.site_for_cookies());
-}
-
-bool AwCookieAccessPolicy::AllowGetCookie(const GURL& url,
-                                          const GURL& first_party,
-                                          const net::CookieList& cookie_list,
-                                          content::ResourceContext* context,
-                                          int render_process_id,
-                                          int render_frame_id) {
-  bool global = GetShouldAcceptCookies();
+bool AwCookieAccessPolicy::AllowCookies(const GURL& url,
+                                        const GURL& first_party,
+                                        int render_process_id,
+                                        int render_frame_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   bool third_party = GetShouldAcceptThirdPartyCookies(
       render_process_id, render_frame_id,
       content::RenderFrameHost::kNoFrameTreeNodeId);
-  return AwStaticCookiePolicy(global, third_party).AllowGet(url, first_party);
+  return CanAccessCookies(url, first_party, third_party);
 }
 
-bool AwCookieAccessPolicy::AllowSetCookie(const GURL& url,
-                                          const GURL& first_party,
-                                          const net::CanonicalCookie& cookie,
-                                          content::ResourceContext* context,
-                                          int render_process_id,
-                                          int render_frame_id) {
-  bool global = GetShouldAcceptCookies();
-  bool third_party = GetShouldAcceptThirdPartyCookies(
-      render_process_id, render_frame_id,
-      content::RenderFrameHost::kNoFrameTreeNodeId);
-  return AwStaticCookiePolicy(global, third_party).AllowSet(url, first_party);
-}
+bool AwCookieAccessPolicy::CanAccessCookies(const GURL& url,
+                                            const GURL& site_for_cookies,
+                                            bool accept_third_party_cookies) {
+  if (!accept_cookies_)
+    return false;
 
-AwStaticCookiePolicy::AwStaticCookiePolicy(bool accept_cookies,
-                                           bool accept_third_party_cookies)
-    : accept_cookies_(accept_cookies),
-      accept_third_party_cookies_(accept_third_party_cookies) {
-}
+  if (accept_third_party_cookies)
+    return true;
 
-StaticCookiePolicy::Type AwStaticCookiePolicy::GetPolicy(const GURL& url)
-    const {
   // File URLs are a special case. We want file URLs to be able to set cookies
   // but (for the purpose of cookies) Chrome considers different file URLs to
   // come from different origins so we use the 'allow all' cookie policy for
   // file URLs.
-  bool isFile = url.SchemeIsFile();
-  if (!accept_cookies()) {
-    return StaticCookiePolicy::BLOCK_ALL_COOKIES;
-  }
-  if (accept_third_party_cookies() || isFile) {
-    return StaticCookiePolicy::ALLOW_ALL_COOKIES;
-  }
-  return StaticCookiePolicy::BLOCK_ALL_THIRD_PARTY_COOKIES;
-}
-
-bool AwStaticCookiePolicy::AllowSet(const GURL& url,
-                                    const GURL& first_party) const {
-  return StaticCookiePolicy(GetPolicy(url))
-             .CanAccessCookies(url, first_party) == net::OK;
-}
+  if (url.SchemeIsFile())
+    return true;
 
-bool AwStaticCookiePolicy::AllowGet(const GURL& url,
-                                    const GURL& first_party) const {
-  return StaticCookiePolicy(GetPolicy(url))
-             .CanAccessCookies(url, first_party) == net::OK;
+  // Otherwise, block third-party cookies.
+  return net::StaticCookiePolicy(
+             net::StaticCookiePolicy::BLOCK_ALL_THIRD_PARTY_COOKIES)
+             .CanAccessCookies(url, site_for_cookies) == net::OK;
 }
 
 }  // namespace android_webview

android_webview/browser/aw_cookie_access_policy.h

@@ -8,16 +8,13 @@
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/synchronization/lock.h"
-#include "net/base/static_cookie_policy.h"
-#include "net/cookies/canonical_cookie.h"
-#include "net/url_request/url_request.h"
-
-namespace content {
-class ResourceContext;
-}
 
 class GURL;
 
+namespace net {
+class URLRequest;
+}  // namespace net
+
 namespace android_webview {
 
 // Manages the cookie access (both setting and getting) policy for WebView.
@@ -27,85 +24,46 @@ class AwCookieAccessPolicy {
  public:
   static AwCookieAccessPolicy* GetInstance();
 
-  // Can we read/write any cookies?
+  // Can we read/write any cookies? Can be called from any thread.
   bool GetShouldAcceptCookies();
   void SetShouldAcceptCookies(bool allow);
 
   // Can we read/write third party cookies?
   // |render_process_id| and |render_frame_id| must be valid.
   // Navigation requests are not associated with a renderer process. In this
-  // case, |frame_tree_node_id| must be valid instead.
+  // case, |frame_tree_node_id| must be valid instead. Can only be called from
+  // the IO thread.
   bool GetShouldAcceptThirdPartyCookies(int render_process_id,
                                         int render_frame_id,
                                         int frame_tree_node_id);
   bool GetShouldAcceptThirdPartyCookies(const net::URLRequest& request);
 
-  // These are the functions called when operating over cookies from the
-  // network. See NetworkDelegate for further descriptions.
-  bool OnCanGetCookies(const net::URLRequest& request,
-                       const net::CookieList& cookie_list);
-  bool OnCanSetCookie(const net::URLRequest& request,
-                      const net::CanonicalCookie& cookie,
-                      net::CookieOptions* options);
-
-  // These are the functions called when operating over cookies from the
-  // renderer. See ContentBrowserClient for further descriptions.
-  bool AllowGetCookie(const GURL& url,
-                      const GURL& first_party,
-                      const net::CookieList& cookie_list,
-                      content::ResourceContext* context,
-                      int render_process_id,
-                      int render_frame_id);
-  bool AllowSetCookie(const GURL& url,
-                      const GURL& first_party,
-                      const net::CanonicalCookie& cookie,
-                      content::ResourceContext* context,
-                      int render_process_id,
-                      int render_frame_id);
+  // Whether or not to allow cookies to bet sent or set for |request|. Can only
+  // be called from the IO thread.
+  bool AllowCookies(const net::URLRequest& request);
+
+  // Whether or not to allow cookies for requests with these parameters.
+  bool AllowCookies(const GURL& url,
+                    const GURL& first_party,
+                    int render_process_id,
+                    int render_frame_id);
 
  private:
   friend struct base::LazyInstanceTraitsBase<AwCookieAccessPolicy>;
+  friend class AwCookieAccessPolicyTest;
 
   AwCookieAccessPolicy();
   ~AwCookieAccessPolicy();
+
+  bool CanAccessCookies(const GURL& url,
+                        const GURL& site_for_cookies,
+                        bool accept_third_party_cookies);
   bool accept_cookies_;
   base::Lock lock_;
 
   DISALLOW_COPY_AND_ASSIGN(AwCookieAccessPolicy);
 };
 
-class AwStaticCookiePolicy {
- public:
-  AwStaticCookiePolicy(bool allow_global_access,
-                       bool allow_third_party_access);
-
-  bool accept_cookies() const {
-    return accept_cookies_;
-  }
-
-  bool accept_third_party_cookies() const {
-    return accept_third_party_cookies_;
-  }
-
-  bool AllowGet(const GURL& url, const GURL& first_party) const;
-  bool AllowSet(const GURL& url, const GURL& first_party) const;
-
- private:
-  const bool accept_cookies_;
-  const bool accept_third_party_cookies_;
-
-  // We have two bits of state but only three different cases:
-  // If !ShouldAcceptCookies
-  //    then reject all cookies.
-  // If ShouldAcceptCookies and !ShouldAcceptThirdPartyCookies
-  //    then reject third party.
-  // If ShouldAcceptCookies and ShouldAcceptThirdPartyCookies
-  //    then allow all cookies.
-  net::StaticCookiePolicy::Type GetPolicy(const GURL& url) const;
-
-  DISALLOW_COPY_AND_ASSIGN(AwStaticCookiePolicy);
-};
-
 }  // namespace android_webview
 
 #endif  // ANDROID_WEBVIEW_BROWSER_AW_COOKIE_ACCESS_POLICY_H_

android_webview/browser/aw_cookie_access_policy_unittest.cc

+// Copyright 2014 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 "android_webview/browser/aw_cookie_access_policy.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace android_webview {
+
+namespace {
+const GURL kUrlFirstParty("http://first.example");
+const GURL kUrlThirdParty("http://third.example");
+const GURL kFileUrl1("file:///path/to/any_file.html");
+const GURL kFileUrl2("file:///path/to/some/other_file.html");
+}  // namespace
+
+class AwCookieAccessPolicyTest : public testing::Test {
+ public:
+  AwCookieAccessPolicyTest() {}
+
+  bool CanAccessCookies(const GURL& url,
+                        const GURL& site_for_cookies,
+                        bool accept_cookies,
+                        bool accept_third_party_cookies) {
+    AwCookieAccessPolicy policy;
+    policy.SetShouldAcceptCookies(accept_cookies);
+    return policy.CanAccessCookies(url, site_for_cookies,
+                                   accept_third_party_cookies);
+  }
+};
+
+TEST_F(AwCookieAccessPolicyTest, BlockAllCookies) {
+  EXPECT_FALSE(CanAccessCookies(kUrlFirstParty, kUrlFirstParty,
+                                false /* allow_cookies */,
+                                false /* allow_third_party_cookies */));
+  EXPECT_FALSE(CanAccessCookies(kUrlFirstParty, kUrlThirdParty,
+                                false /* allow_cookies */,
+                                false /* allow_third_party_cookies */));
+  EXPECT_FALSE(CanAccessCookies(kFileUrl1, kFileUrl1, false /* allow_cookies */,
+                                false /* allow_third_party_cookies */));
+  EXPECT_FALSE(CanAccessCookies(kFileUrl1, kFileUrl2, false /* allow_cookies */,
+                                false /* allow_third_party_cookies */));
+}
+
+TEST_F(AwCookieAccessPolicyTest, BlockAllCookiesWithThirdPartySet) {
+  EXPECT_FALSE(CanAccessCookies(kUrlFirstParty, kUrlFirstParty,
+                                false /* allow_cookies */,
+                                true /* allow_third_party_cookies */));
+  EXPECT_FALSE(CanAccessCookies(kUrlFirstParty, kUrlThirdParty,
+                                false /* allow_cookies */,
+                                true /* allow_third_party_cookies */));
+  EXPECT_FALSE(CanAccessCookies(kFileUrl1, kFileUrl1, false /* allow_cookies */,
+                                true /* allow_third_party_cookies */));
+  EXPECT_FALSE(CanAccessCookies(kFileUrl1, kFileUrl2, false /* allow_cookies */,
+                                true /* allow_third_party_cookies */));
+}
+
+TEST_F(AwCookieAccessPolicyTest, FirstPartyCookiesOnly) {
+  EXPECT_TRUE(CanAccessCookies(kUrlFirstParty, kUrlFirstParty,
+                               true /* allow_cookies */,
+                               false /* allow_third_party_cookies */));
+  EXPECT_FALSE(CanAccessCookies(kUrlFirstParty, kUrlThirdParty,
+                                true /* allow_cookies */,
+                                false /* allow_third_party_cookies */));
+  EXPECT_TRUE(CanAccessCookies(kFileUrl1, kFileUrl1, true /* allow_cookies */,
+                               false /* allow_third_party_cookies */));
+  EXPECT_TRUE(CanAccessCookies(kFileUrl1, kFileUrl2, true /* allow_cookies */,
+                               false /* allow_third_party_cookies */));
+}
+
+TEST_F(AwCookieAccessPolicyTest, AllowAllCookies) {
+  EXPECT_TRUE(CanAccessCookies(kUrlFirstParty, kUrlFirstParty,
+                               true /* allow_cookies */,
+                               true /* allow_third_party_cookies */));
+  EXPECT_TRUE(CanAccessCookies(kUrlFirstParty, kUrlThirdParty,
+                               true /* allow_cookies */,
+                               true /* allow_third_party_cookies */));
+  EXPECT_TRUE(CanAccessCookies(kFileUrl1, kFileUrl1, true /* allow_cookies */,
+                               true /* allow_third_party_cookies */));
+  EXPECT_TRUE(CanAccessCookies(kFileUrl1, kFileUrl2, true /* allow_cookies */,
+                               true /* allow_third_party_cookies */));
+}
+
+}  // namespace android_webview

android_webview/browser/aw_static_cookie_policy_unittest.cc

-// Copyright 2014 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 "android_webview/browser/aw_cookie_access_policy.h"
-
-#include "testing/gtest/include/gtest/gtest.h"
-
-class GURL;
-
-using android_webview::AwStaticCookiePolicy;
-using testing::Test;
-
-class AwStaticCookiePolicyTest : public Test {
- public:
-  static const GURL kUrlFirstParty;
-  static const GURL kUrlThirdParty;
-
-  AwStaticCookiePolicyTest() {}
-
-  void expectFirstPartyAccess(const AwStaticCookiePolicy& policy,
-                              bool expectedResult) {
-    EXPECT_EQ(expectedResult, policy.AllowSet(kUrlFirstParty, kUrlFirstParty));
-    EXPECT_EQ(expectedResult, policy.AllowGet(kUrlFirstParty, kUrlFirstParty));
-  }
-
-  void expectThirdPartyAccess(const AwStaticCookiePolicy& policy,
-                              bool expectedResult) {
-    EXPECT_EQ(expectedResult, policy.AllowSet(kUrlFirstParty, kUrlThirdParty));
-    EXPECT_EQ(expectedResult, policy.AllowGet(kUrlFirstParty, kUrlThirdParty));
-  }
-};
-
-const GURL AwStaticCookiePolicyTest::kUrlFirstParty =
-    GURL("http://first.example");
-const GURL AwStaticCookiePolicyTest::kUrlThirdParty =
-    GURL("http://third.example");
-
-TEST_F(AwStaticCookiePolicyTest, BlockAllCookies) {
-  AwStaticCookiePolicy policy(false /* allow_cookies */,
-                              false /* allow_third_party_cookies */);
-  expectFirstPartyAccess(policy, false);
-  expectThirdPartyAccess(policy, false);
-}
-
-TEST_F(AwStaticCookiePolicyTest, BlockAllCookiesWithThirdPartySet) {
-  AwStaticCookiePolicy policy(false /* allow_cookies */,
-                              true  /* allow_third_party_cookies */);
-  expectFirstPartyAccess(policy, false);
-  expectThirdPartyAccess(policy, false);
-}
-
-TEST_F(AwStaticCookiePolicyTest, FirstPartyCookiesOnly) {
-  AwStaticCookiePolicy policy(true  /* allow_cookies */,
-                              false /* allow_third_party_cookies */);
-  expectFirstPartyAccess(policy, true);
-  expectThirdPartyAccess(policy, false);
-}
-
-TEST_F(AwStaticCookiePolicyTest, AllowAllCookies) {
-  AwStaticCookiePolicy policy(true /* allow_cookies */,
-                              true /* allow_third_party_cookies */);
-  expectFirstPartyAccess(policy, true);
-  expectThirdPartyAccess(policy, true);
-}
-

android_webview/browser/net/aw_network_delegate.cc

@@ -95,8 +95,7 @@ bool AwNetworkDelegate::OnCanGetCookies(const net::URLRequest& request,
                                         const net::CookieList& cookie_list,
                                         bool allow_from_caller) {
   return allow_from_caller &&
-         AwCookieAccessPolicy::GetInstance()->OnCanGetCookies(request,
-                                                              cookie_list);
+         AwCookieAccessPolicy::GetInstance()->AllowCookies(request);
 }
 
 bool AwNetworkDelegate::OnCanSetCookie(const net::URLRequest& request,
@@ -104,8 +103,7 @@ bool AwNetworkDelegate::OnCanSetCookie(const net::URLRequest& request,
                                        net::CookieOptions* options,
                                        bool allow_from_caller) {
   return allow_from_caller &&
-         AwCookieAccessPolicy::GetInstance()->OnCanSetCookie(request, cookie,
-                                                             options);
+         AwCookieAccessPolicy::GetInstance()->AllowCookies(request);
 }
 
 bool AwNetworkDelegate::OnCanAccessFile(

android_webview/test/BUILD.gn

@@ -328,11 +328,11 @@ test("android_webview_unittests") {
   sources = [
     "../browser/aw_content_browser_client_unittest.cc",
     "../browser/aw_contents_client_bridge_unittest.cc",
+    "../browser/aw_cookie_access_policy_unittest.cc",
     "../browser/aw_form_database_service_unittest.cc",
     "../browser/aw_media_url_interceptor_unittest.cc",
     "../browser/aw_metrics_service_client_unittest.cc",
     "../browser/aw_permission_manager_unittest.cc",
-    "../browser/aw_static_cookie_policy_unittest.cc",
     "../browser/gfx/browser_view_renderer_unittest.cc",
     "../browser/gfx/test/fake_window.cc",
     "../browser/gfx/test/fake_window.h",