network_time_tracker: add temporary time protocol.

components/network_time/network_time_tracker_unittest.cc

Index: components/network_time/network_time_tracker_unittest.cc
diff --git a/components/network_time/network_time_tracker_unittest.cc b/components/network_time/network_time_tracker_unittest.cc
index b42196cf0183b723eee4eacfd22071c2ca25ff5a..e7234af368de560b820ac0e6ac60b9aefbefbec8 100644
--- a/components/network_time/network_time_tracker_unittest.cc
+++ b/components/network_time/network_time_tracker_unittest.cc
@@ -5,12 +5,21 @@
 #include "components/network_time/network_time_tracker.h"
 
 #include <memory>
+#include <utility>
 
 #include "base/compiler_specific.h"
+#include "base/message_loop/message_loop.h"
+#include "base/strings/stringprintf.h"
 #include "base/test/simple_test_clock.h"
 #include "base/test/simple_test_tick_clock.h"
+#include "components/client_update_protocol/ecdsa.h"
 #include "components/network_time/network_time_pref_names.h"
 #include "components/prefs/testing_pref_service.h"
+#include "net/http/http_response_headers.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_response.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace network_time {
@@ -19,25 +28,46 @@ class NetworkTimeTrackerTest : public testing::Test {
  public:
   ~NetworkTimeTrackerTest() override {}
 
-  void SetUp() override {
+  NetworkTimeTrackerTest()
+      : io_thread_("IO thread"),
+        clock_(new base::SimpleTestClock),
+        tick_clock_(new base::SimpleTestTickClock),
+        test_server_(new net::EmbeddedTestServer) {
+    base::Thread::Options thread_options;
+    thread_options.message_loop_type = base::MessageLoop::TYPE_IO;
+    EXPECT_TRUE(io_thread_.StartWithOptions(thread_options));
     NetworkTimeTracker::RegisterPrefs(pref_service_.registry());
 
-    clock_ = new base::SimpleTestClock();
-    tick_clock_ = new base::SimpleTestTickClock();
+    tracker_.reset(new NetworkTimeTracker(
+        std::unique_ptr<base::Clock>(clock_),
+        std::unique_ptr<base::TickClock>(tick_clock_), &pref_service_,
+        new net::TestURLRequestContextGetter(io_thread_.task_runner())));
+
     // Do this to be sure that |is_null| returns false.
     clock_->Advance(base::TimeDelta::FromDays(111));
     tick_clock_->Advance(base::TimeDelta::FromDays(222));
 
-    tracker_.reset(new NetworkTimeTracker(
-        std::unique_ptr<base::Clock>(clock_),
-        std::unique_ptr<base::TickClock>(tick_clock_), &pref_service_));
-
     // Can not be smaller than 15, it's the NowFromSystemTime() resolution.
     resolution_ = base::TimeDelta::FromMilliseconds(17);
     latency_ = base::TimeDelta::FromMilliseconds(50);
     adjustment_ = 7 * base::TimeDelta::FromMilliseconds(kTicksResolutionMs);
   }
 
+  void TearDown() override {
+    if (test_server_->Started()) {
+      ASSERT_TRUE(test_server_->ShutdownAndWaitUntilComplete());
+    }

[mmenke, 2016/05/02 17:47:28]

This isn't needed - the test server will shut itself down on destruction.

[mab, 2016/05/05 19:57:42]

Done.

+    io_thread_.Stop();
+  }
+
+  void WaitForFetch(uint32_t nonce) {
+    tracker_->query_signer_->OverrideNonceForTesting(1 /* key version */,
+                                                     nonce);
+    while (tracker_->time_fetcher_) {
+      base::MessageLoop::current()->RunUntilIdle();

[mmenke, 2016/05/02 17:47:28]

This works, but repeatedly spinning up a runloop while waiting for one thing to
happen is considered a bit of an anti-pattern.  Can we just watch the pref
service's prefs::kNetworkTimeMapping, and when it's modified, quite the run
loop?

Or if there's some other way to watch the service I'm unaware of, you could use
that instead.

[mab, 2016/05/05 19:57:42]

Modification of prefs is an incidental effect of a *successful* fetch, so I'd
rather not depend upon it.

If it's important to not busy-wait here, then I'd rather block in the most
straightforward way possible, e.g. with a condition variable.  Would that work?

[mmenke, 2016/05/06 15:00:37]

SGTM

[mab, 2016/05/06 18:12:37]

I ended up doing this by simply passing in the MessageLoop, to be Quit when
complete.  It seemed most straightforward.  LMK if this is horrible.  :-)

+    }
+  }
+
   // Replaces |tracker_| with a new object, while preserving the
   // testing clocks.
   void Reset() {
@@ -49,7 +79,50 @@ class NetworkTimeTrackerTest : public testing::Test {
     tick_clock_= new_tick_clock;
     tracker_.reset(new NetworkTimeTracker(
         std::unique_ptr<base::Clock>(clock_),
-        std::unique_ptr<base::TickClock>(tick_clock_), &pref_service_));
+        std::unique_ptr<base::TickClock>(tick_clock_), &pref_service_,
+        new net::TestURLRequestContextGetter(io_thread_.task_runner())));
+  }
+
+  // Returns a valid time response.  Update as follows:
+  //
+  // curl http://clients2.google.com/time/1/current?cup2key=1:123123123
+  //
+  // where 1 is the key version and 123123123 is the nonce.  Copy the nonce, the
+  // response, and the x-cup-server-proof header into the test.
+  static std::unique_ptr<net::test_server::HttpResponse>
+  GoodTimeResponseHandler(const net::test_server::HttpRequest& request) {
+    net::test_server::BasicHttpResponse* response =
+        new net::test_server::BasicHttpResponse();
+    response->set_code(net::HTTP_OK);
+    response->set_content(
+        ")]}'\n"
+        "{\"current_time_millis\":1461621971825,\"server_nonce\":-6."
+        "006853099049523E85}");
+    response->AddCustomHeader(
+        "x-cup-server-proof",
+        "304402202e0f24db1ea69f1bbe81da4108f381fcf7a2781c53cf7663cb47083cb5fe8e"
+        "fd"
+        "022009d2b67c0deceaaf849f7c529be96701ed5f15d5efcaf401a94e0801accc9832:"
+        "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
+    return std::unique_ptr<net::test_server::HttpResponse>(response);
+  }
+
+  static std::unique_ptr<net::test_server::HttpResponse>
+  BadSignatureResponseHandler(const net::test_server::HttpRequest& request) {
+    net::test_server::BasicHttpResponse* response =
+        new net::test_server::BasicHttpResponse();
+    response->set_code(net::HTTP_OK);
+    response->set_content("foo");
+    response->AddCustomHeader("x-cup-server-proof", "dead:beef");
+    return std::unique_ptr<net::test_server::HttpResponse>(response);
+  }
+
+  static std::unique_ptr<net::test_server::HttpResponse> ErrorResponseHandler(
+      const net::test_server::HttpRequest& request) {
+    net::test_server::BasicHttpResponse* response =
+        new net::test_server::BasicHttpResponse();
+    response->set_code(net::HTTP_INTERNAL_SERVER_ERROR);
+    return std::unique_ptr<net::test_server::HttpResponse>(response);
   }

[mmenke, 2016/05/02 17:47:28]

You should also test a network error (Response has just an error code, no
headers or body).  Can do with with net::test_server::RawHttpResponse("", "");

[mab, 2016/05/05 19:57:42]

Done.

 
   // Updates the notifier's time with the specified parameters.
@@ -69,6 +142,8 @@ class NetworkTimeTrackerTest : public testing::Test {
   }
 
  protected:
+  base::Thread io_thread_;
+  base::MessageLoop message_loop_;
   base::TimeDelta resolution_;
   base::TimeDelta latency_;
   base::TimeDelta adjustment_;
@@ -76,6 +151,7 @@ class NetworkTimeTrackerTest : public testing::Test {
   base::SimpleTestTickClock* tick_clock_;
   TestingPrefServiceSimple pref_service_;
   std::unique_ptr<NetworkTimeTracker> tracker_;
+  std::unique_ptr<net::EmbeddedTestServer> test_server_;
 };
 
 TEST_F(NetworkTimeTrackerTest, Uninitialized) {
@@ -131,7 +207,7 @@ TEST_F(NetworkTimeTrackerTest, ClockIsWack) {
                     tick_clock_->NowTicks());
 
   base::Time out_network_time;
-  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, nullptr));
   EXPECT_EQ(in_network_time, out_network_time);
 }
 
@@ -185,7 +261,7 @@ TEST_F(NetworkTimeTrackerTest, SpringForward) {
   tick_clock_->Advance(base::TimeDelta::FromSeconds(1));
   clock_->Advance(base::TimeDelta::FromDays(1));
   base::Time out_network_time;
-  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, nullptr));
 }
 
 TEST_F(NetworkTimeTrackerTest, FallBack) {
@@ -195,7 +271,7 @@ TEST_F(NetworkTimeTrackerTest, FallBack) {
   tick_clock_->Advance(base::TimeDelta::FromSeconds(1));
   clock_->Advance(base::TimeDelta::FromDays(-1));
   base::Time out_network_time;
-  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, nullptr));
 }
 
 TEST_F(NetworkTimeTrackerTest, SuspendAndResume) {
@@ -205,7 +281,7 @@ TEST_F(NetworkTimeTrackerTest, SuspendAndResume) {
                     tick_clock_->NowTicks());
   clock_->Advance(base::TimeDelta::FromHours(1));
   base::Time out_network_time;
-  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, nullptr));
 }
 
 TEST_F(NetworkTimeTrackerTest, Serialize) {
@@ -237,7 +313,7 @@ TEST_F(NetworkTimeTrackerTest, DeserializeOldFormat) {
                     tick_clock_->NowTicks());
 
   base::Time out_network_time;
-  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, nullptr));
   double local, network;
   const base::DictionaryValue* saved_prefs =
       pref_service_.GetDictionary(prefs::kNetworkTimeMapping);
@@ -248,7 +324,7 @@ TEST_F(NetworkTimeTrackerTest, DeserializeOldFormat) {
   prefs.SetDouble("network", network);
   pref_service_.Set(prefs::kNetworkTimeMapping, prefs);
   Reset();
-  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, nullptr));
 }
 
 TEST_F(NetworkTimeTrackerTest, SerializeWithLongDelay) {
@@ -258,10 +334,10 @@ TEST_F(NetworkTimeTrackerTest, SerializeWithLongDelay) {
   UpdateNetworkTime(in_network_time - latency_ / 2, resolution_, latency_,
                     tick_clock_->NowTicks());
   base::Time out_network_time;
-  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, nullptr));
   AdvanceBoth(base::TimeDelta::FromDays(8));
   Reset();
-  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, nullptr));
 }
 
 TEST_F(NetworkTimeTrackerTest, SerializeWithTickClockAdvance) {
@@ -271,10 +347,10 @@ TEST_F(NetworkTimeTrackerTest, SerializeWithTickClockAdvance) {
   UpdateNetworkTime(in_network_time - latency_ / 2, resolution_, latency_,
                     tick_clock_->NowTicks());
   base::Time out_network_time;
-  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, nullptr));
   tick_clock_->Advance(base::TimeDelta::FromDays(1));
   Reset();
-  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, nullptr));
 }
 
 TEST_F(NetworkTimeTrackerTest, SerializeWithWallClockAdvance) {
@@ -285,10 +361,87 @@ TEST_F(NetworkTimeTrackerTest, SerializeWithWallClockAdvance) {
                     tick_clock_->NowTicks());
 
   base::Time out_network_time;
-  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, nullptr));
   clock_->Advance(base::TimeDelta::FromDays(1));
   Reset();
-  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, NULL));
+  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, nullptr));
+}
+
+TEST_F(NetworkTimeTrackerTest, UpdateFromNetwork) {
+  base::Time out_network_time;
+  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, nullptr));
+
+  test_server_->RegisterRequestHandler(
+      base::Bind(&NetworkTimeTrackerTest::GoodTimeResponseHandler));
+  EXPECT_TRUE(test_server_->Start());
+  tracker_->SetTimeServerURLForTesting(test_server_->GetURL("/"));
+  tracker_->QueryTimeService();
+  WaitForFetch(123123123);
+
+  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, nullptr));
+  EXPECT_EQ(base::Time::UnixEpoch() +
+                base::TimeDelta::FromMilliseconds(1461621971825),
+            out_network_time);
+  // Should see no backoff in the success case.
+  EXPECT_TRUE(tracker_->query_timer_.IsRunning());
+  EXPECT_EQ(base::TimeDelta::FromMinutes(60),
+            tracker_->query_timer_.GetCurrentDelay());
+}
+
+TEST_F(NetworkTimeTrackerTest, NoNetworkQueryWhileSynced) {
+  base::Time in_network_time = clock_->Now();
+  UpdateNetworkTime(in_network_time, resolution_, latency_,
+                    tick_clock_->NowTicks());
+  tracker_->QueryTimeService();
+  EXPECT_EQ(nullptr, tracker_->time_fetcher_);  // No query should be started.
+}
+
+TEST_F(NetworkTimeTrackerTest, UpdateFromNetworkBadSignature) {

[mmenke, 2016/05/02 17:47:28]

Seems like there are two cases we should care about testing:  Bad signature /
good data, and good signature / bad data.  This one tests bad signature / bad
data.

[mab, 2016/05/05 19:57:42]

It's a fair point that we should test good data + bad signature, to make sure
that the bad signature alone is sufficient to cause the data to be rejected. 
I'm not sure what we would additionally learn from bad data + bad signature,
though.

[mmenke, 2016/05/05 20:23:54]

I didn't suggest bad data + bad signature, I said that's what this test was
currently doing.

I suggested bad data + good signature (Which basically protects against bugs
server side).  Admittedly, there are a lot of bad data paths, and checking them
all is probably a bit too much.

[mab, 2016/05/05 22:24:36]

Ah, I see what you're saying.  Done.

+  test_server_->RegisterRequestHandler(
+      base::Bind(&NetworkTimeTrackerTest::BadSignatureResponseHandler));
+  EXPECT_TRUE(test_server_->Start());
+  tracker_->SetTimeServerURLForTesting(test_server_->GetURL("/"));
+  tracker_->QueryTimeService();
+  WaitForFetch(123123123);
+
+  base::Time out_network_time;
+  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, nullptr));
+  EXPECT_TRUE(tracker_->query_timer_.IsRunning());
+}
+
+TEST_F(NetworkTimeTrackerTest, UpdateFromNetworkServerError) {
+  test_server_->RegisterRequestHandler(
+      base::Bind(&NetworkTimeTrackerTest::ErrorResponseHandler));
+  EXPECT_TRUE(test_server_->Start());
+  tracker_->SetTimeServerURLForTesting(test_server_->GetURL("/"));
+  tracker_->QueryTimeService();
+  WaitForFetch(123123123);
+
+  base::Time out_network_time;
+  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, nullptr));
+  // Should see backoff in the error case.
+  EXPECT_TRUE(tracker_->query_timer_.IsRunning());
+  EXPECT_EQ(base::TimeDelta::FromMinutes(120),
+            tracker_->query_timer_.GetCurrentDelay());
+}
+
+TEST_F(NetworkTimeTrackerTest, UpdateFromNetworkLargeResponse) {
+  test_server_->RegisterRequestHandler(
+      base::Bind(&NetworkTimeTrackerTest::GoodTimeResponseHandler));
+  EXPECT_TRUE(test_server_->Start());
+  tracker_->SetTimeServerURLForTesting(test_server_->GetURL("/"));
+
+  base::Time out_network_time;
+
+  tracker_->SetMaxResponseSizeForTesting(3);
+  tracker_->QueryTimeService();
+  WaitForFetch(123123123);
+  EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, nullptr));
+
+  tracker_->SetMaxResponseSizeForTesting(1024);
+  tracker_->QueryTimeService();
+  WaitForFetch(123123123);
+  EXPECT_TRUE(tracker_->GetNetworkTime(&out_network_time, nullptr));
 }
 
 }  // namespace network_time