Support the Clear-Site-Data header on resource requests

content/browser/browsing_data/clear_site_data_throttle_unittest.cc

Index: content/browser/browsing_data/clear_site_data_throttle_unittest.cc
diff --git a/content/browser/browsing_data/clear_site_data_throttle_unittest.cc b/content/browser/browsing_data/clear_site_data_throttle_unittest.cc
index bdfa8fad3d519996800691f6526b3e3ad320f0f1..4e799c5f27ec4d93b37d20df66d6ccc5a5f169a5 100644
--- a/content/browser/browsing_data/clear_site_data_throttle_unittest.cc
+++ b/content/browser/browsing_data/clear_site_data_throttle_unittest.cc
@@ -7,28 +7,128 @@
 #include <memory>
 
 #include "base/command_line.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/message_loop/message_loop.h"
+#include "base/strings/stringprintf.h"
+#include "base/test/scoped_command_line.h"
+#include "content/common/net/url_request_service_worker_data.h"
 #include "content/public/common/content_switches.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_util.h"
+#include "net/url_request/redirect_info.h"
+#include "net/url_request/url_request_job.h"
+#include "net/url_request/url_request_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using ::testing::_;
+
 namespace content {
 
-class ClearSiteDataThrottleTest : public testing::Test {
+namespace {
+
+static const char* kClearCookiesHeader =

[mmenke, 2017/05/22 19:36:09]

nit:  static not needed, "const char kClearCookiesHeader[]" is preferred, I
believe (Since that means neither the pointer nor what it's pointing at can be
modified)

[msramek, 2017/05/24 22:59:52]

Done. Changed here as well as in clear_site_data_throttle.cc.

+    "Clear-Site-Data: { \"types\": [ \"cookies\" ] }";
+
+// Used to verify that resource throttle delegate calls are made.
+class MockResourceThrottleDelegate : public ResourceThrottle::Delegate {
+ public:
+  MOCK_METHOD0(Cancel, void());
+  MOCK_METHOD0(CancelAndIgnore, void());
+  MOCK_METHOD1(CancelWithError, void(int));
+  MOCK_METHOD0(Resume, void());
+};
+
+// A testing ConsoleMessagesDelegate that does not require valid WebContents
+// or thread jumping.
+class TestConsoleMessagesDelegate
+    : public ClearSiteDataThrottle::ConsoleMessagesDelegate {

[mmenke, 2017/05/22 19:36:09]

Is this class needed?  The production one already handles a WebContentsGetter
that returns nullptr (Since it's possible the WebContents was destroyed)

[msramek, 2017/05/24 22:59:52]

Removed. Thanks for noticing.

  public:
-  void SetUp() override {
-    base::CommandLine::ForCurrentProcess()->AppendSwitch(
-        switches::kEnableExperimentalWebPlatformFeatures);
-    throttle_ = ClearSiteDataThrottle::CreateThrottleForNavigation(nullptr);
+  TestConsoleMessagesDelegate() {}
+  ~TestConsoleMessagesDelegate() override {}
+
+  void OutputMessages(const ResourceRequestInfo::WebContentsGetter&
+                          web_contents_getter) override {
+    // No valid WebContents in this unittest.
   }
+};
 
-  ClearSiteDataThrottle* GetThrottle() {
-    return static_cast<ClearSiteDataThrottle*>(throttle_.get());
+// A slightly modified ClearSiteDataThrottle for testing with unconditional
+// construction, injectable response headers, and dummy clearing functionality.
+class TestThrottle : public ClearSiteDataThrottle {
+ public:
+  TestThrottle(net::URLRequest* request,
+               std::unique_ptr<TestConsoleMessagesDelegate> delegate)
+      : ClearSiteDataThrottle(request, std::move(delegate)) {}
+  ~TestThrottle() override {}
+
+  void SetResponseHeaders(std::string headers) {

[mmenke, 2017/05/22 19:36:09]

nit:  const std::string&

[msramek, 2017/05/24 22:59:52]

Done.

+    headers = "HTTP/1.1 200\n" + headers;
+    headers_ = new net::HttpResponseHeaders(
+        net::HttpUtil::AssembleRawHeaders(headers.c_str(), headers.size()));
   }
 
+  MOCK_METHOD4(ClearSiteData,
+               void(const url::Origin& origin,
+                    bool clear_cookies,
+                    bool clear_storage,
+                    bool clear_cache));
+
+ protected:
+  const net::HttpResponseHeaders* GetResponseHeaders() const override {
+    return headers_.get();
+  }
+
+  void ExecuteClearingTask(const url::Origin& origin,
+                           bool clear_cookies,
+                           bool clear_storage,
+                           bool clear_cache,
+                           const base::Closure& callback) override {
+    ClearSiteData(origin, clear_cookies, clear_storage, clear_cache);
+
+    callback.Run();
+  }
+
+ private:
+  scoped_refptr<net::HttpResponseHeaders> headers_;
+};
+
+}  // namespace
+
+class ClearSiteDataThrottleTest : public testing::Test {
  private:
-  std::unique_ptr<NavigationThrottle> throttle_;
+  base::MessageLoop message_loop_;

[mmenke, 2017/05/22 19:36:09]

I think there's now a more task-runner friendly way of doing this? 
base::test::ScopedTaskEnvironment?

[msramek, 2017/05/24 22:59:52]

Done.

 };
 
+TEST_F(ClearSiteDataThrottleTest, CreateThrottleForRequest) {
+  // Enable experimental features.
+  std::unique_ptr<base::test::ScopedCommandLine> command_line(
+      new base::test::ScopedCommandLine());
+  command_line->GetProcessCommandLine()->AppendSwitch(
+      switches::kEnableExperimentalWebPlatformFeatures);
+
+  // Create a URL request.
+  GURL url("https://www.example.com");
+  net::TestURLRequestContext context;
+  std::unique_ptr<net::URLRequest> request(
+      context.CreateRequest(url, net::DEFAULT_PRIORITY, nullptr));
+
+  // We will not create the throttle for an empty ResourceRequestInfo.
+  EXPECT_FALSE(ClearSiteDataThrottle::CreateThrottleForRequest(request.get()));
+
+  // We can create the throttle for a valid ResourceRequestInfo.
+  ResourceRequestInfo::AllocateForTesting(request.get(), RESOURCE_TYPE_IMAGE,
+                                          nullptr, 0, 0, 0, false, true, true,
+                                          true, false);
+  EXPECT_TRUE(ClearSiteDataThrottle::CreateThrottleForRequest(request.get()));
+
+  // But not if experimental web features are disabled again.
+  request->SetLoadFlags(net::LOAD_NORMAL);
+  command_line.reset();
+  EXPECT_FALSE(ClearSiteDataThrottle::CreateThrottleForRequest(request.get()));
+}
+
 TEST_F(ClearSiteDataThrottleTest, ParseHeader) {
   struct TestCase {
     const char* header;
@@ -76,11 +176,11 @@ TEST_F(ClearSiteDataThrottleTest, ParseHeader) {
     bool actual_storage;
     bool actual_cache;
 
-    std::vector<ClearSiteDataThrottle::ConsoleMessage> messages;
+    TestConsoleMessagesDelegate console_delegate;
 
-    EXPECT_TRUE(GetThrottle()->ParseHeader(test_case.header, &actual_cookies,
-                                           &actual_storage, &actual_cache,
-                                           &messages));
+    EXPECT_TRUE(ClearSiteDataThrottle::ParseHeader(
+        test_case.header, &actual_cookies, &actual_storage, &actual_cache,
+        &console_delegate, GURL()));
 
     EXPECT_EQ(test_case.cookies, actual_cookies);
     EXPECT_EQ(test_case.storage, actual_storage);
@@ -99,11 +199,11 @@ TEST_F(ClearSiteDataThrottleTest, InvalidHeader) {
       {"{ \"field\" : {} }",
        "Expecting a JSON dictionary with a 'types' field.\n"},
       {"{ \"types\" : [ \"passwords\" ] }",
-       "Invalid type: \"passwords\".\n"
-       "No valid types specified in the 'types' field.\n"},
+       "Unrecognized type: \"passwords\".\n"
+       "No recognized types specified in the 'types' field.\n"},
       {"{ \"types\" : [ [ \"list in a list\" ] ] }",
-       "Invalid type: [\"list in a list\"].\n"
-       "No valid types specified in the 'types' field.\n"},
+       "Unrecognized type: [\"list in a list\"].\n"
+       "No recognized types specified in the 'types' field.\n"},
       {"{ \"types\" : [ \"кукис\", \"сторидж\", \"кэш\" ]",
        "Must only contain ASCII characters.\n"}};
 
@@ -114,14 +214,14 @@ TEST_F(ClearSiteDataThrottleTest, InvalidHeader) {
     bool actual_storage;
     bool actual_cache;
 
-    std::vector<ClearSiteDataThrottle::ConsoleMessage> messages;
+    TestConsoleMessagesDelegate console_delegate;
 
-    EXPECT_FALSE(GetThrottle()->ParseHeader(test_case.header, &actual_cookies,
-                                            &actual_storage, &actual_cache,
-                                            &messages));
+    EXPECT_FALSE(ClearSiteDataThrottle::ParseHeader(
+        test_case.header, &actual_cookies, &actual_storage, &actual_cache,
+        &console_delegate, GURL()));
 
     std::string multiline_message;
-    for (const auto& message : messages) {
+    for (const auto& message : console_delegate.messages()) {
       EXPECT_EQ(CONSOLE_MESSAGE_LEVEL_ERROR, message.level);
       multiline_message += message.text + "\n";
     }

[mmenke, 2017/05/22 19:36:09]

optional:  Hrm...There's no testing of the output of ClearSiteDataOnUIThread. 
Would it make sense to have the Delegate thing create the full sctrings, then
pass them over to the UI thread, just for better testing?  Not going to insist
on it.

[msramek, 2017/05/24 22:59:52]

Do you mean OutputMessagesOnUIThread()? I must admit I don't fully remember why
(as I wrote this test long ago), but I found unittesting the actual console
output to be nontrivial. I can try again, but I'd like to not add much more code
in this CL.

A mitigation is that a bug where the strings don't make it to the console is
visually discoverable.

[mmenke, 2017/05/25 15:19:01]

Sorry, I did indeed mean OutputMessagesOnUIThread.  But what I mean we should
test is not whether it outputs to console, but the whole last_seen_url thing.  I
don't think a single test relies on that behavior working.
It's really not.  Suppose the last_seen_url logic was broken in the case of
multiple different URLs resetting cookies.  What do you think are the odds
someone would actually notice that?

[msramek, 2017/05/30 21:58:44]

Oh, that's what you mean. OK, I added a unittest for the formatting of the
console messages,

This required changes:

--> ConsoleMessagesDelegate requires the output method to be mockable, so that
we can read it in the tests but not pass it to a real WebContents. However,
since the class lives on the IO thread, the output method must be on the UI
thread, so to avoid lifetime problems I want to keep it static. Therefore, I now
pass the output method as a base::Callback. Unittests supply a different
base::Callback.

--> In the unittest, I need to simulate the Throttle redirecting over different
URLs. Again, changing the URL of a URLRequest for testing purposes seems to be
nontrivial, so instead, I defined virtual GetCurrentURL() which is overriden in
the tests. An alternative would be to have the throttle keep track of the URL
itself by not calling URLRequest::url(), but instead watching RedirectInfo
instances passed in WillRedirectRequest(). I liked the virtual method approach a
bit better.

@@ -130,4 +230,211 @@ TEST_F(ClearSiteDataThrottleTest, InvalidHeader) {
   }
 }
 
+TEST_F(ClearSiteDataThrottleTest, LoadDoNotSaveCookies) {
+  net::TestURLRequestContext context;
+  std::unique_ptr<net::URLRequest> request(context.CreateRequest(
+      GURL("https://www.example.com"), net::DEFAULT_PRIORITY, nullptr));
+  std::unique_ptr<TestConsoleMessagesDelegate> scoped_console_delegate(
+      new TestConsoleMessagesDelegate());
+  const TestConsoleMessagesDelegate* console_delegate =
+      scoped_console_delegate.get();
+  TestThrottle throttle(request.get(), std::move(scoped_console_delegate));
+  MockResourceThrottleDelegate delegate;
+  throttle.set_delegate_for_testing(&delegate);
+  throttle.SetResponseHeaders(kClearCookiesHeader);
+
+  bool defer;
+  throttle.WillProcessResponse(&defer);
+  EXPECT_TRUE(defer);
+  EXPECT_EQ(1u, console_delegate->messages().size());
+  EXPECT_EQ("Clearing cookies.", console_delegate->messages().front().text);
+  EXPECT_EQ(console_delegate->messages().front().level,
+            CONSOLE_MESSAGE_LEVEL_INFO);
+
+  request->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES);
+  throttle.WillProcessResponse(&defer);
+  EXPECT_FALSE(defer);
+  EXPECT_EQ(2u, console_delegate->messages().size());
+  EXPECT_EQ(
+      "The request's credentials mode prohibits modifying cookies "
+      "and other local data.",
+      console_delegate->messages().rbegin()->text);
+  EXPECT_EQ(CONSOLE_MESSAGE_LEVEL_ERROR,
+            console_delegate->messages().rbegin()->level);
+}
+
+TEST_F(ClearSiteDataThrottleTest, InvalidOrigin) {
+  struct TestCase {
+    const char* origin;
+    std::string error_message;
+  } kTestCases[] = {
+      // The throttle only works on secure origins.
+      {"https://secure-origin.com", ""},
+      {"filesystem:https://secure-origin.com/temporary/", ""},
+
+      // That includes localhost.
+      {"http://localhost", ""},
+
+      // Not on insecure origins.
+      {"http://insecure-origin.com", "Not supported for insecure origins."},
+      {"filesystem:http://insecure-origin.com/temporary/",
+       "Not supported for insecure origins."},
+
+      // Not on unique origins.
+      {"file:///foo/bar.txt", "Not supported for unique origins."},
+  };
+
+  net::TestURLRequestContext context;
+
+  for (const TestCase& test_case : kTestCases) {
+    std::unique_ptr<net::URLRequest> request(context.CreateRequest(
+        GURL(test_case.origin), net::DEFAULT_PRIORITY, nullptr));
+    std::unique_ptr<TestConsoleMessagesDelegate> scoped_console_delegate(
+        new TestConsoleMessagesDelegate());
+    const TestConsoleMessagesDelegate* console_delegate =
+        scoped_console_delegate.get();
+    TestThrottle throttle(request.get(), std::move(scoped_console_delegate));
+    MockResourceThrottleDelegate delegate;
+    throttle.set_delegate_for_testing(&delegate);
+    throttle.SetResponseHeaders(kClearCookiesHeader);
+
+    bool defer;
+    throttle.WillProcessResponse(&defer);
+
+    EXPECT_EQ(console_delegate->messages().size(), 1u);

[mmenke, 2017/05/22 19:36:09]

Why is the size 1 on secure origins, and not 0?

[msramek, 2017/05/24 22:59:52]

The throttle also outputs a message on successful deletion. This is encouraged
by the spec: https://www.w3.org/TR/clear-site-data/#clear-response

I added that expectation to this test explicitly.

+    if (!defer) {
+      EXPECT_EQ(test_case.error_message,
+                console_delegate->messages().front().text);
+      EXPECT_EQ(CONSOLE_MESSAGE_LEVEL_ERROR,
+                console_delegate->messages().front().level);
+    }
+  }
+}
+
+TEST_F(ClearSiteDataThrottleTest, DeferAndResume) {
+  enum Stage { START, REDIRECT, RESPONSE };
+
+  struct TestCase {
+    Stage stage;
+    std::string response_headers;
+    bool should_defer;
+  } kTestCases[] = {
+      // The throttle never interferes while the request is starting. Response
+      // headers are ignored, because URLRequest is not supposed to have any
+      // at this stage in the first place.
+      {START, "", false},
+      {START, kClearCookiesHeader, false},
+
+      // The throttle does not defer redirects if there are no interesting
+      // response headers.
+      {REDIRECT, "", false},
+      {REDIRECT, "Set-Cookie: abc=123;", false},
+      {REDIRECT, "Content-Type: image/png;", false},
+
+      // That includes malformed Clear-Site-Data headers or header values
+      // that do not lead to deletion.
+      {REDIRECT, "Clear-Site-Data: { types: cookies } ", false},
+      {REDIRECT, "Clear-Site-Data: { \"types\": [ \"unknown type\" ] }", false},
+
+      // However, redirects are deferred for valid Clear-Site-Data headers.
+      {REDIRECT,
+       "Clear-Site-Data: { \"types\": [ \"cookies\", \"unknown type\" ] }",
+       true},
+      {REDIRECT,
+       base::StringPrintf("Content-Type: image/png;\n%s", kClearCookiesHeader),
+       true},
+      {REDIRECT,
+       base::StringPrintf("%s\nContent-Type: image/png;", kClearCookiesHeader),
+       true},
+
+      // We expect at most one instance of the header. Multiple instances
+      // will not be parsed currently. This is not an inherent property of
+      // Clear-Site-Data, just a documentation of the current behavior.
+      {REDIRECT,
+       base::StringPrintf("%s\n%s", kClearCookiesHeader, kClearCookiesHeader),
+       false},
+
+      // Final response headers are treated the same way as in the case
+      // of redirect.
+      {REDIRECT, "Set-Cookie: abc=123;", false},
+      {REDIRECT, "Clear-Site-Data: { types: cookies } ", false},
+      {REDIRECT, kClearCookiesHeader, true},
+  };
+
+  struct TestOrigin {
+    const char* origin;
+    bool valid;
+  } kTestOrigins[] = {
+      // The throttle only works on secure origins.
+      {"https://secure-origin.com", true},
+      {"filesystem:https://secure-origin.com/temporary/", true},
+
+      // That includes localhost.
+      {"http://localhost", true},
+
+      // Not on insecure origins.
+      {"http://insecure-origin.com", false},
+      {"filesystem:http://insecure-origin.com/temporary/", false},
+
+      // Not on unique origins.
+      {"data:unique-origin;", false},
+  };
+
+  net::TestURLRequestContext context;
+
+  for (const TestOrigin& test_origin : kTestOrigins) {
+    for (const TestCase& test_case : kTestCases) {
+      SCOPED_TRACE(base::StringPrintf("Origin=%s\nStage=%d\nHeaders:\n%s",
+                                      test_origin.origin, test_case.stage,
+                                      test_case.response_headers.c_str()));
+
+      std::unique_ptr<net::URLRequest> request(context.CreateRequest(
+          GURL(test_origin.origin), net::DEFAULT_PRIORITY, nullptr));
+      TestThrottle throttle(request.get(),
+                            base::MakeUnique<TestConsoleMessagesDelegate>());
+      throttle.SetResponseHeaders(test_case.response_headers);
+
+      MockResourceThrottleDelegate delegate;
+      throttle.set_delegate_for_testing(&delegate);
+
+      // Whether we should defer is always conditional on the origin
+      // being valid.
+      bool expected_defer = test_case.should_defer && test_origin.valid;
+
+      // If we expect loading to be deferred, then we also expect data to be
+      // cleared and the load to eventually resume.
+      if (expected_defer) {
+        testing::Expectation e = EXPECT_CALL(

[mmenke, 2017/05/22 19:36:09]

This name violates the google style guide - should give some information about
the name.

[msramek, 2017/05/24 22:59:52]

Fixed. Sorry for that.

+            throttle,
+            ClearSiteData(url::Origin(GURL(test_origin.origin)), _, _, _));

[mmenke, 2017/05/22 19:36:09]

I'm not going to block on this, but GMOCK syntax is very convoluted and unclear
- the network stack team, at least, generally avoids any use of GMOCK for this
reason, and instead implements our own mock classes.  This also makes it easier
for third parties to start hacking on Chrome, and more generally makes tests
more debuggable.

[msramek, 2017/05/24 22:59:52]

In that case I'd prefer to keep it as is - as usual, in order not to expand this
already large CL.

I agree with you that GMOCK errors are hard to debug, but custom mock classes
typically cost a lot of boilerplate code if you want them to support things like
testing::_, Times(0), verbose outputs etc., so I'm actually surprised to hear
that.

[mmenke, 2017/05/25 15:19:01]

Not going to advocate any more for getting rid of gmock after this statement,
but....

MockResourceThrottleDelegate gets nothing from gmock, since you never use it in
any EXPECT_CALLs, so that just leaves TestThrottle.  TestThrottle only has one
mock method, which just needs to track number of times called, and last values
passed in to it, so does seem like not a whole lot of extra code or complexity,
here.

+        EXPECT_CALL(delegate, Resume()).After(e);
+      } else {
+        EXPECT_CALL(throttle, ClearSiteData(_, _, _, _)).Times(0);

[mmenke, 2017/05/22 19:36:09]

I don't think we ever check that ClearSiteData is called with the right
parameters in any of these tests?  Suggest doing it in this one, or one like it,
and setting each one individually (ParseHeader makes sure we're parsing them
correctly, but I don't think anything is making sure we're passing them
correctly to the ClearSiteData call, other than the browser_tests).

[msramek, 2017/05/24 22:59:52]

I expanded the ParseHeader test, since that one tests different datatypes, so it
seemed to fit there the best.

Although I don't think it's super valuable, since even if we verify that we get
that far, we still don't see if BrowsingDataRemover is called correctly.

That's exactly why we now have these new integration-y browsertests, which you
asked me to write a few months ago :)

[mmenke, 2017/05/25 15:19:01]

True.  My feeling is just that we should test individual classes as much as
possible in unit tests.  If something breaks, we'd really prefer a unit test
failure to debug, instead of a browser test one.  Of course, we also need to
test things are plugged in correctly, and for other things, unit tests just
don't work that well, but I feel that unit tests should be as comprehensive as
we can manage, though that's certainly a debatable point.

[msramek, 2017/05/30 21:58:44]

Acknowledged. Fair enough.

+        EXPECT_CALL(delegate, Resume()).Times(0);
+      }
+
+      bool actual_defer = false;
+
+      switch (test_case.stage) {
+        case START: {
+          throttle.WillStartRequest(&actual_defer);
+          break;
+        }
+        case REDIRECT: {
+          net::RedirectInfo redirect_info;
+          throttle.WillRedirectRequest(redirect_info, &actual_defer);
+          break;
+        }
+        case RESPONSE: {
+          throttle.WillProcessResponse(&actual_defer);
+          break;
+        }
+      }
+
+      EXPECT_EQ(expected_defer, actual_defer);
+      testing::Mock::VerifyAndClearExpectations(&delegate);
+    }
+  }
+}
+
 }  // namespace content