Skip to content

Commit

Permalink
First-Party Cookies: Wire it up as an experimental web platform feature
Browse files Browse the repository at this point in the history
This patch adds a flag to the NetworkDelegate to control the first-party
cookies experiment, and implements the flag in the ChromeNetworkDelegate
and ShellNetworkDelegate by checking the value of the experimental web
platform features command-line flag.

Once we decide whether or not to ship this feature, we can revert
everything in this patch other than the tests and the change to
URLRequestHttpJob::DoLoadCookies.

BUG=459154

Review URL: https://codereview.chromium.org/940373002

Cr-Commit-Position: refs/heads/master@{#318295}
  • Loading branch information
mikewest authored and Commit bot committed Feb 26, 2015
1 parent 5b3d93b commit 3f3daac
Show file tree
Hide file tree
Showing 15 changed files with 236 additions and 13 deletions.
10 changes: 9 additions & 1 deletion chrome/browser/net/chrome_network_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <vector>

#include "base/base_paths.h"
#include "base/command_line.h"
#include "base/debug/alias.h"
#include "base/debug/dump_without_crashing.h"
#include "base/debug/stack_trace.h"
Expand Down Expand Up @@ -39,6 +40,7 @@
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/render_view_host.h"
#include "content/public/browser/resource_request_info.h"
#include "content/public/common/content_switches.h"
#include "content/public/common/process_type.h"
#include "net/base/host_port_pair.h"
#include "net/base/load_flags.h"
Expand All @@ -59,7 +61,6 @@
#endif

#if defined(OS_CHROMEOS)
#include "base/command_line.h"
#include "base/sys_info.h"
#include "chrome/common/chrome_switches.h"
#endif
Expand Down Expand Up @@ -293,6 +294,9 @@ ChromeNetworkDelegate::ChromeNetworkDelegate(
url_blacklist_manager_(NULL),
#endif
domain_reliability_monitor_(NULL),
experimental_web_platform_features_enabled_(
base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kEnableExperimentalWebPlatformFeatures)),
first_request_(true),
prerender_tracker_(NULL) {
DCHECK(enable_referrers);
Expand Down Expand Up @@ -724,6 +728,10 @@ bool ChromeNetworkDelegate::OnCanEnablePrivacyMode(
return privacy_mode;
}

bool ChromeNetworkDelegate::OnFirstPartyOnlyCookieExperimentEnabled() const {
return experimental_web_platform_features_enabled_;
}

bool ChromeNetworkDelegate::OnCancelURLRequestWithPolicyViolatingReferrerHeader(
const net::URLRequest& request,
const GURL& target_url,
Expand Down
5 changes: 4 additions & 1 deletion chrome/browser/net/chrome_network_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class ChromeNetworkDelegate : public net::NetworkDelegateImpl {
static void AllowAccessToAllFiles();

private:
friend class ChromeNetworkDelegateTest;
friend class ChromeNetworkDelegateThrottlingTest;

// NetworkDelegate implementation.
int OnBeforeURLRequest(net::URLRequest* request,
Expand Down Expand Up @@ -184,6 +184,7 @@ class ChromeNetworkDelegate : public net::NetworkDelegateImpl {
bool OnCanEnablePrivacyMode(
const GURL& url,
const GURL& first_party_for_cookies) const override;
bool OnFirstPartyOnlyCookieExperimentEnabled() const override;
bool OnCancelURLRequestWithPolicyViolatingReferrerHeader(
const net::URLRequest& request,
const GURL& target_url,
Expand Down Expand Up @@ -225,6 +226,8 @@ class ChromeNetworkDelegate : public net::NetworkDelegateImpl {
// static anyway since it is based on a command-line flag.
static bool g_never_throttle_requests_;

bool experimental_web_platform_features_enabled_;

bool first_request_;

prerender::PrerenderTracker* prerender_tracker_;
Expand Down
46 changes: 39 additions & 7 deletions chrome/browser/net/chrome_network_delegate_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "chrome/browser/net/chrome_network_delegate.h"

#include "base/command_line.h"
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
#include "base/message_loop/message_loop.h"
Expand All @@ -13,6 +14,7 @@
#include "chrome/common/pref_names.h"
#include "chrome/test/base/testing_pref_service_syncable.h"
#include "chrome/test/base/testing_profile.h"
#include "content/public/common/content_switches.h"
#include "content/public/test/test_browser_thread_bundle.h"
#include "net/base/request_priority.h"
#include "net/url_request/url_request.h"
Expand All @@ -23,12 +25,41 @@
#include "chrome/browser/extensions/event_router_forwarder.h"
#endif

TEST(ChromeNetworkDelegateTest, DisableFirstPartyOnlyCookiesIffFlagDisabled) {
BooleanPrefMember pref_member_;
scoped_ptr<ChromeNetworkDelegate> delegate;

#if defined(ENABLE_EXTENSIONS)
scoped_refptr<extensions::EventRouterForwarder> forwarder =
new extensions::EventRouterForwarder();
delegate.reset(new ChromeNetworkDelegate(forwarder.get(), &pref_member_));
#else
delegate.reset(new ChromeNetworkDelegate(nullptr, &pref_member_));
#endif
EXPECT_FALSE(delegate->FirstPartyOnlyCookieExperimentEnabled());
}

TEST(ChromeNetworkDelegateTest, EnableFirstPartyOnlyCookiesIffFlagEnabled) {
base::CommandLine::ForCurrentProcess()->AppendSwitch(
switches::kEnableExperimentalWebPlatformFeatures);
BooleanPrefMember pref_member_;
scoped_ptr<ChromeNetworkDelegate> delegate;

#if defined(ENABLE_EXTENSIONS)
class ChromeNetworkDelegateTest : public testing::Test {
scoped_refptr<extensions::EventRouterForwarder> forwarder =
new extensions::EventRouterForwarder();
delegate.reset(new ChromeNetworkDelegate(forwarder.get(), &pref_member_));
#else
delegate.reset(new ChromeNetworkDelegate(nullptr, &pref_member_));
#endif
EXPECT_TRUE(delegate->FirstPartyOnlyCookieExperimentEnabled());
}

#if defined(ENABLE_EXTENSIONS)
class ChromeNetworkDelegateThrottlingTest : public testing::Test {
protected:
ChromeNetworkDelegateTest()
: forwarder_(new extensions::EventRouterForwarder()) {
}
ChromeNetworkDelegateThrottlingTest()
: forwarder_(new extensions::EventRouterForwarder()) {}

void SetUp() override {
never_throttle_requests_original_value_ =
Expand All @@ -43,7 +74,7 @@ class ChromeNetworkDelegateTest : public testing::Test {

scoped_ptr<ChromeNetworkDelegate> CreateNetworkDelegate() {
return make_scoped_ptr(
new ChromeNetworkDelegate(forwarder_.get(), &pref_member_));
new ChromeNetworkDelegate(forwarder(), &pref_member_));
}

// Implementation moved here for access to private bits.
Expand Down Expand Up @@ -81,14 +112,16 @@ class ChromeNetworkDelegateTest : public testing::Test {
}

private:
extensions::EventRouterForwarder* forwarder() { return forwarder_.get(); }

bool never_throttle_requests_original_value_;
base::MessageLoopForIO message_loop_;

scoped_refptr<extensions::EventRouterForwarder> forwarder_;
BooleanPrefMember pref_member_;
};

TEST_F(ChromeNetworkDelegateTest, NeverThrottleLogic) {
TEST_F(ChromeNetworkDelegateThrottlingTest, NeverThrottleLogic) {
NeverThrottleLogicImpl();
}
#endif // defined(ENABLE_EXTENSIONS)
Expand Down Expand Up @@ -313,4 +346,3 @@ TEST_F(ChromeNetworkDelegatePrivacyModeTest,
EXPECT_FALSE(network_delegate_->CanEnablePrivacyMode(kAllowedSite,
kBlockedFirstPartySite));
}

7 changes: 7 additions & 0 deletions content/shell/browser/shell_network_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "content/shell/browser/shell_network_delegate.h"

#include "base/command_line.h"
#include "content/public/common/content_switches.h"
#include "net/base/net_errors.h"
#include "net/base/static_cookie_policy.h"
#include "net/url_request/url_request.h"
Expand Down Expand Up @@ -114,4 +116,9 @@ bool ShellNetworkDelegate::OnCanThrottleRequest(
return false;
}

bool ShellNetworkDelegate::OnFirstPartyOnlyCookieExperimentEnabled() const {
return base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kEnableExperimentalWebPlatformFeatures);
}

} // namespace content
1 change: 1 addition & 0 deletions content/shell/browser/shell_network_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class ShellNetworkDelegate : public net::NetworkDelegateImpl {
bool OnCanAccessFile(const net::URLRequest& request,
const base::FilePath& path) const override;
bool OnCanThrottleRequest(const net::URLRequest& request) const override;
bool OnFirstPartyOnlyCookieExperimentEnabled() const override;

DISALLOW_COPY_AND_ASSIGN(ShellNetworkDelegate);
};
Expand Down
9 changes: 9 additions & 0 deletions net/base/layered_network_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,15 @@ void LayeredNetworkDelegate::OnCanEnablePrivacyModeInternal(
const GURL& first_party_for_cookies) const {
}

bool LayeredNetworkDelegate::OnFirstPartyOnlyCookieExperimentEnabled() const {
OnFirstPartyOnlyCookieExperimentEnabledInternal();
return nested_network_delegate_->FirstPartyOnlyCookieExperimentEnabled();
}

void LayeredNetworkDelegate::OnFirstPartyOnlyCookieExperimentEnabledInternal()
const {
}

bool LayeredNetworkDelegate::
OnCancelURLRequestWithPolicyViolatingReferrerHeader(
const URLRequest& request,
Expand Down
3 changes: 3 additions & 0 deletions net/base/layered_network_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class NET_EXPORT LayeredNetworkDelegate : public NetworkDelegate {
bool OnCanThrottleRequest(const URLRequest& request) const final;
bool OnCanEnablePrivacyMode(const GURL& url,
const GURL& first_party_for_cookies) const final;
bool OnFirstPartyOnlyCookieExperimentEnabled() const final;
bool OnCancelURLRequestWithPolicyViolatingReferrerHeader(
const URLRequest& request,
const GURL& target_url,
Expand Down Expand Up @@ -152,6 +153,8 @@ class NET_EXPORT LayeredNetworkDelegate : public NetworkDelegate {
const GURL& url,
const GURL& first_party_for_cookies) const;

virtual void OnFirstPartyOnlyCookieExperimentEnabledInternal() const;

virtual void OnCancelURLRequestWithPolicyViolatingReferrerHeaderInternal(
const URLRequest& request,
const GURL& target_url,
Expand Down
4 changes: 4 additions & 0 deletions net/base/network_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ bool NetworkDelegate::CanEnablePrivacyMode(
return OnCanEnablePrivacyMode(url, first_party_for_cookies);
}

bool NetworkDelegate::FirstPartyOnlyCookieExperimentEnabled() const {
return OnFirstPartyOnlyCookieExperimentEnabled();
}

bool NetworkDelegate::CancelURLRequestWithPolicyViolatingReferrerHeader(
const URLRequest& request,
const GURL& target_url,
Expand Down
11 changes: 11 additions & 0 deletions net/base/network_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ class NET_EXPORT NetworkDelegate : public base::NonThreadSafe {
bool CanEnablePrivacyMode(const GURL& url,
const GURL& first_party_for_cookies) const;

// TODO(mkwst): Remove this once we decide whether or not we wish to ship
// first-party cookies. https://crbug.com/459154
bool FirstPartyOnlyCookieExperimentEnabled() const;

bool CancelURLRequestWithPolicyViolatingReferrerHeader(
const URLRequest& request,
const GURL& target_url,
Expand Down Expand Up @@ -266,6 +270,13 @@ class NET_EXPORT NetworkDelegate : public base::NonThreadSafe {
const GURL& url,
const GURL& first_party_for_cookies) const = 0;

// Returns true if the embedder has enabled the "first-party" cookie
// experiment, and false otherwise.
//
// TODO(mkwst): Remove this once we decide whether or not we wish to ship
// first-party cookies. https://crbug.com/459154
virtual bool OnFirstPartyOnlyCookieExperimentEnabled() const = 0;

// Called when the |referrer_url| for requesting |target_url| during handling
// of the |request| is does not comply with the referrer policy (e.g. a
// secure referrer for an insecure initial target).
Expand Down
4 changes: 4 additions & 0 deletions net/base/network_delegate_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ bool NetworkDelegateImpl::OnCanEnablePrivacyMode(
return false;
}

bool NetworkDelegateImpl::OnFirstPartyOnlyCookieExperimentEnabled() const {
return false;
}

bool NetworkDelegateImpl::OnCancelURLRequestWithPolicyViolatingReferrerHeader(
const URLRequest& request,
const GURL& target_url,
Expand Down
7 changes: 7 additions & 0 deletions net/base/network_delegate_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,13 @@ class NET_EXPORT NetworkDelegateImpl : public NetworkDelegate {
const GURL& url,
const GURL& first_party_for_cookies) const override;

// Returns true if the embedder has enabled the "first-party" cookie
// experiment, and false otherwise.
//
// TODO(mkwst): Remove this once we decide whether or not we wish to ship
// first-party cookies. https://crbug.com/459154
bool OnFirstPartyOnlyCookieExperimentEnabled() const override;

// Called when the |referrer_url| for requesting |target_url| during handling
// of the |request| is does not comply with the referrer policy (e.g. a
// secure referrer for an insecure initial target).
Expand Down
11 changes: 7 additions & 4 deletions net/url_request/url_request_http_job.cc
Original file line number Diff line number Diff line change
Expand Up @@ -629,11 +629,14 @@ void URLRequestHttpJob::AddCookieHeaderAndStart() {
void URLRequestHttpJob::DoLoadCookies() {
CookieOptions options;
options.set_include_httponly();
options.set_include_first_party_only();

// TODO(mkwst): Pipe a switch down here to allow us to decide whether we
// should enforce "first-party only" cookies or not (by setting |options|'s
// first-party-url to the first-party-for-cookies value. crbug.com/459154
// TODO(mkwst): Drop this `if` once we decide whether or not to ship
// first-party cookies: https://crbug.com/459154
if (network_delegate() &&
network_delegate()->FirstPartyOnlyCookieExperimentEnabled())
options.set_first_party_url(request_->first_party_for_cookies());
else
options.set_include_first_party_only();

GetCookieStore()->GetCookiesWithOptionsAsync(
request_->url(), options,
Expand Down
5 changes: 5 additions & 0 deletions net/url_request/url_request_test_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ TestNetworkDelegate::TestNetworkDelegate()
has_load_timing_info_before_auth_(false),
can_access_files_(true),
can_throttle_requests_(true),
first_party_only_cookies_enabled_(false),
cancel_request_with_policy_violating_referrer_(false),
will_be_intercepted_on_next_error_(false) {
}
Expand Down Expand Up @@ -603,6 +604,10 @@ bool TestNetworkDelegate::OnCanThrottleRequest(
return can_throttle_requests_;
}

bool TestNetworkDelegate::OnFirstPartyOnlyCookieExperimentEnabled() const {
return first_party_only_cookies_enabled_;
}

bool TestNetworkDelegate::OnCancelURLRequestWithPolicyViolatingReferrerHeader(
const URLRequest& request,
const GURL& target_url,
Expand Down
6 changes: 6 additions & 0 deletions net/url_request/url_request_test_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,10 @@ class TestNetworkDelegate : public NetworkDelegateImpl {
void set_can_access_files(bool val) { can_access_files_ = val; }
bool can_access_files() const { return can_access_files_; }

void set_first_party_only_cookies_enabled(bool val) {
first_party_only_cookies_enabled_ = val;
}

void set_can_throttle_requests(bool val) { can_throttle_requests_ = val; }
bool can_throttle_requests() const { return can_throttle_requests_; }

Expand Down Expand Up @@ -325,6 +329,7 @@ class TestNetworkDelegate : public NetworkDelegateImpl {
bool OnCanAccessFile(const URLRequest& request,
const base::FilePath& path) const override;
bool OnCanThrottleRequest(const URLRequest& request) const override;
bool OnFirstPartyOnlyCookieExperimentEnabled() const override;
bool OnCancelURLRequestWithPolicyViolatingReferrerHeader(
const URLRequest& request,
const GURL& target_url,
Expand Down Expand Up @@ -370,6 +375,7 @@ class TestNetworkDelegate : public NetworkDelegateImpl {

bool can_access_files_; // true by default
bool can_throttle_requests_; // true by default
bool first_party_only_cookies_enabled_; // false by default
bool cancel_request_with_policy_violating_referrer_; // false by default
bool will_be_intercepted_on_next_error_;
};
Expand Down
Loading

0 comments on commit 3f3daac

Please sign in to comment.