Execute ClientCertResolver on network disconnection.

chromeos/network/client_cert_resolver_unittest.cc

Index: chromeos/network/client_cert_resolver_unittest.cc
diff --git a/chromeos/network/client_cert_resolver_unittest.cc b/chromeos/network/client_cert_resolver_unittest.cc
index 7f2035b32ba68359f5b1baf6b35ffd7e5d95c807..3a3f08c490f5aaf36291cf484cfa239d947cf47d 100644
--- a/chromeos/network/client_cert_resolver_unittest.cc
+++ b/chromeos/network/client_cert_resolver_unittest.cc
@@ -11,8 +11,10 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/simple_test_clock.h"
 #include "base/values.h"
 #include "chromeos/cert_loader.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
@@ -50,6 +52,7 @@ class ClientCertResolverTest : public testing::Test,
  public:
   ClientCertResolverTest()
       : network_properties_changed_count_(0),
+        test_clock_(NULL),

[stevenjb, 2015/11/17 00:45:22]

s/NULL/nullptr throughout

[emaxx, 2015/11/17 12:44:07]

Done.

         service_test_(NULL),
         profile_test_(NULL),
         cert_loader_(NULL) {}
@@ -82,6 +85,7 @@ class ClientCertResolverTest : public testing::Test,
   void TearDown() override {
     client_cert_resolver_->RemoveObserver(this);
     client_cert_resolver_.reset();
+    test_clock_ = NULL;
     managed_config_handler_.reset();
     network_config_handler_.reset();
     network_profile_handler_.reset();
@@ -138,6 +142,10 @@ class ClientCertResolverTest : public testing::Test,
     managed_config_handler_.reset(new ManagedNetworkConfigurationHandlerImpl());
     client_cert_resolver_.reset(new ClientCertResolver());
 
+    test_clock_ = new base::SimpleTestClock;
+    test_clock_->SetNow(base::Time::Now());
+    client_cert_resolver_->SetClockForTesting(make_scoped_ptr(test_clock_));

[stevenjb, 2015/11/17 00:45:22]

This is dangerous. client_cert_resolver_ now owns test_clock_, even though this
class hangs on to a pointer to it.

Either:
a) The test should own the clock, pass a raw * to ClientCertResolver (which
would also store it as a raw *), then call
client_cert_resolver_->SetClockForTesting(nullptr) when it destroys it
b) ClientCertResolver should have a test_clock() accessor and this class
shouldn't store the pointer.

a) is probably simplest.

[emaxx, 2015/11/17 12:44:07]

Implemented a) now. Though this looks pretty similar to the previous version:
basically, there's one thing having a pointer to another one with the lifetime
managed somewhere else.

[stevenjb, 2015/11/17 17:53:52]

I acknowledge it's a subtle distinction, but I think this is easier to read.
Before I had to look at ClientCertResolver to confirm that it is OK for this
class to hold onto a raw * after passing a scoped_ptr to the class. Now the
lifetime of both client_cert_resolver_ and test_clock_ is wholly controlled by
this class.

+
     network_profile_handler_->Init();
     network_config_handler_->Init(network_state_handler_.get(),
                                   nullptr /* network_device_handler */);
@@ -247,6 +255,11 @@ class ClientCertResolverTest : public testing::Test,
         base::DictionaryValue() /* no global network config */);
   }
 
+  void SetWifiState(const std::string& state) {
+    ASSERT_TRUE(service_test_->SetServiceProperty(
+        kWifiStub, shill::kStateProperty, base::StringValue(state)));
+  }
+
   void GetClientCertProperties(std::string* pkcs11_id) {
     pkcs11_id->clear();
     const base::DictionaryValue* properties =
@@ -259,6 +272,7 @@ class ClientCertResolverTest : public testing::Test,
 
   int network_properties_changed_count_;
   std::string test_cert_id_;
+  base::SimpleTestClock* test_clock_;
   scoped_ptr<ClientCertResolver> client_cert_resolver_;
 
  private:
@@ -365,4 +379,34 @@ TEST_F(ClientCertResolverTest, ResolveAfterPolicyApplication) {
   EXPECT_EQ(1, network_properties_changed_count_);
 }
 
+TEST_F(ClientCertResolverTest, ExpiringCertificate) {
+  SetupTestCerts(true /* import issuer */);
+  SetupWifi();
+  base::RunLoop().RunUntilIdle();
+
+  SetupNetworkHandlers();
+  SetupPolicyMatchingIssuerPEM();
+  base::RunLoop().RunUntilIdle();
+
+  StartCertLoader();
+  base::RunLoop().RunUntilIdle();
+
+  SetWifiState(shill::kStateOnline);
+  base::RunLoop().RunUntilIdle();
+
+  // Verify that the resolver positively matched the pattern in the policy with
+  // the test client cert and configured the network.
+  std::string pkcs11_id;
+  GetClientCertProperties(&pkcs11_id);
+  EXPECT_EQ(test_cert_id_, pkcs11_id);
+
+  // Verify that, after the certificate expired and the network disconnection
+  // happens, no client certificate was configured.
+  test_clock_->SetNow(base::Time::Max());
+  SetWifiState(shill::kStateOffline);
+  base::RunLoop().RunUntilIdle();
+  GetClientCertProperties(&pkcs11_id);
+  EXPECT_EQ(std::string(), pkcs11_id);
+}
+
 }  // namespace chromeos