Move HSTS redirection from URLRequest to URLRequestHTTPJob.

net/url_request/url_request_http_job_unittest.cc

Index: net/url_request/url_request_http_job_unittest.cc
diff --git a/net/url_request/url_request_http_job_unittest.cc b/net/url_request/url_request_http_job_unittest.cc
index c12807f85b6605d6acaf3c7455e03493a7e1cd72..809e38273299467dc058985d4c08103f90546f80 100644
--- a/net/url_request/url_request_http_job_unittest.cc
+++ b/net/url_request/url_request_http_job_unittest.cc
@@ -21,6 +21,9 @@
 #include "net/cookies/cookie_store_test_helpers.h"
 #include "net/http/http_transaction_factory.h"
 #include "net/http/http_transaction_test_util.h"
+#include "net/log/test_net_log.h"
+#include "net/log/test_net_log_entry.h"
+#include "net/log/test_net_log_util.h"
 #include "net/socket/socket_test_util.h"
 #include "net/test/cert_test_util.h"
 #include "net/url_request/url_request.h"
@@ -625,6 +628,68 @@ TEST_F(URLRequestHttpJobTest, SdchAdvertisementPost) {
   EXPECT_FALSE(TransactionAcceptsSdchEncoding());
 }
 
+TEST_F(URLRequestHttpJobTest, HSTSInternalRedirectTest) {
+  // Setup HSTS state.
+  context_.transport_security_state()->AddHSTS(
+      "upgrade.test", base::Time::Now() + base::TimeDelta::FromSeconds(10),
+      true);
+  ASSERT_TRUE(
+      context_.transport_security_state()->ShouldUpgradeToSSL("upgrade.test"));
+  ASSERT_FALSE(context_.transport_security_state()->ShouldUpgradeToSSL(
+      "no-upgrade.test"));
+
+  net::TestNetLog net_log;
+  context_.set_net_log(&net_log);

[mmenke, 2016/06/17 15:06:18]

Could you move this into the test fixture, just before the context is
initialized and add an accessor?  It's weird to set this on a
TestURLRequestContext that's already been initialized, though it doesn't
actually matter for these tests.  A number of URLRequestContext objects have
pointers to the NetLog, and that makes sure they're all set consistently.

[Mike West, 2016/06/17 16:35:17]

Sure! Done.

+
+  struct TestCase {
+    const char* url;
+    bool upgrade_expected;
+    const char* url_expected;
+  } cases[] = {
+      {"http://upgrade.test/", true, "https://upgrade.test/"},
+      {"ws://upgrade.test/", true, "wss://upgrade.test/"},
+      {"http://upgrade.test:123/", true, "https://upgrade.test:123/"},
+      {"ws://upgrade.test:123/", true, "wss://upgrade.test:123/"},
+      {"http://no-upgrade.test/", false, "http://no-upgrade.test/"},
+      {"ws://no-upgrade.test/", false, "ws://no-upgrade.test/"},
+      {"http://no-upgrade.test:123/", false, "http://no-upgrade.test:123/"},
+      {"ws://no-upgrade.test:123/", false, "ws://no-upgrade.test:123/"},

[mmenke, 2016/06/17 15:06:18]

Hrm...Websocket requests generally aren't made through URLRequest directly, but
through a wrapper...which does use URLRequest, so I think this is fine.

However, I don't think that wrapper actually allows websocket requests to be
redirected.  Not relevant for this CL, but thinking you may run into issues
there on your other CL.

[Mike West, 2016/06/17 16:35:17]

I look forward to exploring the crazy in the other CL. ;)

+  };
+
+  for (const auto& test : cases) {
+    SCOPED_TRACE(test.url);
+    TestDelegate d;
+    TestNetworkDelegate network_delegate;
+    std::unique_ptr<URLRequest> r(
+        context_.CreateRequest(GURL(test.url), DEFAULT_PRIORITY, &d));
+    std::unique_ptr<URLRequestJob> job(URLRequestHttpJob::Factory(
+        r.get(), &network_delegate, url::kHttpScheme));

[mmenke, 2016/06/17 15:06:18]

This object isn't being used anywhere, please delete it.

[Mike West, 2016/06/17 16:35:17]

Done.

+
+    net_log.Clear();
+    r->Start();
+    base::RunLoop().Run();
+
+    if (test.upgrade_expected) {
+      net::TestNetLogEntry::List entries;
+      net_log.GetEntries(&entries);
+      int redirects = 0;
+      for (const auto& entry : entries) {
+        if (entry.type == net::NetLog::TYPE_URL_REQUEST_REDIRECT_JOB) {
+          redirects++;
+          std::string value;
+          EXPECT_TRUE(entry.GetStringValue("reason", &value));
+          EXPECT_EQ("HSTS", value);
+        }
+      }
+      EXPECT_EQ(1, redirects);
+      EXPECT_EQ(1, d.received_redirect_count());

[mmenke, 2016/06/17 15:06:18]

Suggest checking r->url_chain()'s length as well, here, and below - it's
redundant, but it's the main URLRequest API, so seems more important.

[Mike West, 2016/06/17 16:35:17]

Added.

+    } else {
+      EXPECT_EQ(0, d.received_redirect_count());
+    }
+    EXPECT_EQ(GURL(test.url_expected), r->url());
+  }
+}
+
 class MockSdchObserver : public SdchObserver {
  public:
   MockSdchObserver() {}
@@ -944,6 +1009,22 @@ TEST_F(URLRequestHttpJobWebSocketTest, CreateHelperPassedThrough) {
 
 #endif  // !defined(OS_IOS)
 
+class URLRequestHttpJobInternalRedirectTest : public ::testing::Test {
+ protected:
+  URLRequestHttpJobInternalRedirectTest()
+      : context_(new TestURLRequestContext(true)) {
+    std::unique_ptr<HttpNetworkSession::Params> params(
+        new HttpNetworkSession::Params);
+    context_->set_enable_brotli(true);
+    context_->set_http_network_session_params(std::move(params));
+    context_->set_client_socket_factory(&socket_factory_);
+    context_->Init();
+  }
+
+  MockClientSocketFactory socket_factory_;
+  std::unique_ptr<TestURLRequestContext> context_;
+};
+
 }  // namespace
 
 }  // namespace net