Finished implementing AttestationPolicyObserver.

chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc

Index: chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
diff --git a/chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc b/chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
index 3ba9c9a6b383864c30f32ad902e3a89d692b09da..c58b684fd68bc779fdc4cae6ef42be8dd609bf91 100644
--- a/chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
+++ b/chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
@@ -2,9 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>
+
 #include "base/bind.h"
 #include "base/message_loop.h"
 #include "base/run_loop.h"
+#include "chrome/browser/chromeos/attestation/attestation_key_payload.pb.h"
 #include "chrome/browser/chromeos/attestation/attestation_policy_observer.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/cros_settings_names.h"
@@ -13,8 +16,12 @@
 #include "chromeos/attestation/mock_attestation_flow.h"
 #include "chromeos/dbus/mock_cryptohome_client.h"
 #include "content/public/test/test_browser_thread.h"
+#include "crypto/rsa_private_key.h"
+#include "net/cert/x509_certificate.h"
+#include "net/cert/x509_util_nss.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using std::string;

[Mattias Nissler (ping if slow), 2013/04/22 10:25:42]

We usually don't do this given the little benefit it provides.

[dkrahn, 2013/04/22 22:33:31]

Done.

 using testing::_;
 using testing::Invoke;
 using testing::StrictMock;
@@ -35,16 +42,34 @@ void DBusCallbackTrue(const BoolDBusMethodCallback& callback) {
       FROM_HERE, base::Bind(callback, DBUS_METHOD_CALL_SUCCESS, true));
 }
 
-void DBusDataCallback(const CryptohomeClient::DataMethodCallback& callback) {
+void CertCallbackSuccess(const AttestationFlow::CertificateCallback& callback) {
   MessageLoop::current()->PostTask(
-      FROM_HERE, base::Bind(callback, DBUS_METHOD_CALL_SUCCESS, true, "fake"));
+      FROM_HERE, base::Bind(callback, true, "fake_cert"));
 }
 
-void CertCallbackSuccess(const AttestationFlow::CertificateCallback& callback) {
+void StatusCallbackSuccess(
+    const policy::CloudPolicyClient::StatusCallback& callback) {
   MessageLoop::current()->PostTask(
-      FROM_HERE, base::Bind(callback, true, "fake_cert"));
+      FROM_HERE, base::Bind(callback, true));
 }
 
+class FakeDBusData {
+ public:
+  explicit FakeDBusData(const string& data) : data_(data) {}
+
+  void operator() (const CryptohomeClient::DataMethodCallback& callback) {
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, DBUS_METHOD_CALL_SUCCESS, true, data_));
+  }
+
+ private:
+  string data_;
+};
+
+// A cached private key so we only need to generate once.
+scoped_ptr<crypto::RSAPrivateKey> g_private_key;

[Mattias Nissler (ping if slow), 2013/04/22 10:25:42]

No non-POD globals please.

[dkrahn, 2013/04/22 22:33:31]

Done.

+
 }  // namespace
 
 class AttestationPolicyObserverTest : public ::testing::Test {
@@ -70,6 +95,12 @@ class AttestationPolicyObserverTest : public ::testing::Test {
   }
 
  protected:
+  enum CertExpiryOptions {
+    CERT_VALID,
+    CERT_EXPIRING_SOON,
+    CERT_EXPIRED
+  };
+
   void Run() {
     AttestationPolicyObserver observer(&policy_client_,
                                        &cryptohome_client_,
@@ -77,6 +108,49 @@ class AttestationPolicyObserverTest : public ::testing::Test {
     base::RunLoop().RunUntilIdle();
   }
 
+  string CreatePayload() {
+    AttestationKeyPayload proto;
+    proto.set_is_certificate_uploaded(true);
+    string serialized;
+    proto.SerializeToString(&serialized);
+    return serialized;
+  }
+
+  bool CreateCertificate(CertExpiryOptions options, string* certificate) {
+    base::Time valid_start = base::Time::Now() - base::TimeDelta::FromDays(90);
+    base::Time valid_expiry;
+    switch (options) {
+      case CERT_VALID:
+        valid_expiry = base::Time::Now() + base::TimeDelta::FromDays(90);
+        break;
+      case CERT_EXPIRING_SOON:
+        valid_expiry = base::Time::Now() + base::TimeDelta::FromDays(20);
+        break;
+      case CERT_EXPIRED:
+        valid_expiry = base::Time::Now() - base::TimeDelta::FromDays(20);
+        break;
+      default:
+        NOTREACHED();
+    }
+    if (!g_private_key.get())
+      g_private_key.reset(crypto::RSAPrivateKey::Create(2048));

[Mattias Nissler (ping if slow), 2013/04/22 10:25:42]

How costly is this? policy_builder.cc has pre-created keys embedded which you
could potentially use here as well?

[dkrahn, 2013/04/22 22:33:31]

Good idea -- used a key from policy_builder.cc.

+    if (!g_private_key.get())
+      return false;
+    net::X509Certificate::OSCertHandle handle =
+        net::x509_util::CreateSelfSignedCert(g_private_key->public_key(),
+                                             g_private_key->key(),
+                                             "CN=subject",
+                                             12345,
+                                             valid_start,
+                                             valid_expiry);

[Mattias Nissler (ping if slow), 2013/04/22 10:25:42]

Do we have any suitable certs in chrome/test/data already?

[dkrahn, 2013/04/22 22:33:31]

The reason I'm generating dynamically is to fiddle with the X.509 expiry fields
which must be relative to the current time.

[Mattias Nissler (ping if slow), 2013/04/23 10:53:01]

I see.

+
+    if (!handle)
+      return false;
+    bool result = net::X509Certificate::GetDEREncoded(handle, certificate);
+    net::X509Certificate::FreeOSCertHandle(handle);
+    return result;
+  }
+
   MessageLoop message_loop_;
   content::TestBrowserThread ui_thread_;
   ScopedTestCrosSettings test_cros_settings_;
@@ -101,16 +175,92 @@ TEST_F(AttestationPolicyObserverTest, UnregisteredPolicyClient) {
 TEST_F(AttestationPolicyObserverTest, NewCertificate) {
   EXPECT_CALL(cryptohome_client_, TpmAttestationDoesKeyExist(_, _, _))
       .WillOnce(WithArgs<2>(Invoke(DBusCallbackFalse)));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationGetKeyPayload(_, _, _))
+      .WillOnce(WithArgs<2>(Invoke(FakeDBusData(""))));
+  EXPECT_CALL(cryptohome_client_,
+              TpmAttestationSetKeyPayload(_, _, CreatePayload(), _))
+      .WillOnce(WithArgs<3>(Invoke(DBusCallbackTrue)));
+  EXPECT_CALL(attestation_flow_, GetCertificate(_, _))
+      .WillOnce(WithArgs<1>(Invoke(CertCallbackSuccess)));
+  EXPECT_CALL(policy_client_, UploadCertificate("fake_cert", _))
+      .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
+  Run();
+}
+
+TEST_F(AttestationPolicyObserverTest, KeyExistsNotUploaded) {
+  string certificate;
+  ASSERT_TRUE(CreateCertificate(CERT_VALID, &certificate));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationDoesKeyExist(_, _, _))
+      .WillOnce(WithArgs<2>(Invoke(DBusCallbackTrue)));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationGetCertificate(_, _, _))
+      .WillOnce(WithArgs<2>(Invoke(FakeDBusData(certificate))));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationGetKeyPayload(_, _, _))
+      .WillRepeatedly(WithArgs<2>(Invoke(FakeDBusData(""))));
+  EXPECT_CALL(cryptohome_client_,
+              TpmAttestationSetKeyPayload(_, _, CreatePayload(), _))
+      .WillOnce(WithArgs<3>(Invoke(DBusCallbackTrue)));
+  EXPECT_CALL(policy_client_, UploadCertificate(certificate, _))
+      .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
+  Run();
+}
+
+TEST_F(AttestationPolicyObserverTest, KeyExistsAlreadyUploaded) {
+  string certificate;
+  ASSERT_TRUE(CreateCertificate(CERT_VALID, &certificate));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationDoesKeyExist(_, _, _))
+      .WillOnce(WithArgs<2>(Invoke(DBusCallbackTrue)));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationGetCertificate(_, _, _))
+      .WillOnce(WithArgs<2>(Invoke(FakeDBusData(certificate))));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationGetKeyPayload(_, _, _))
+      .WillOnce(WithArgs<2>(Invoke(FakeDBusData(CreatePayload()))));
+  Run();
+}
+
+TEST_F(AttestationPolicyObserverTest, KeyExistsCertExpiringSoon) {
+  string certificate;
+  ASSERT_TRUE(CreateCertificate(CERT_EXPIRING_SOON, &certificate));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationDoesKeyExist(_, _, _))
+      .WillOnce(WithArgs<2>(Invoke(DBusCallbackTrue)));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationGetCertificate(_, _, _))
+      .WillOnce(WithArgs<2>(Invoke(FakeDBusData(certificate))));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationGetKeyPayload(_, _, _))
+      .WillRepeatedly(WithArgs<2>(Invoke(FakeDBusData(""))));
+  EXPECT_CALL(cryptohome_client_,
+              TpmAttestationSetKeyPayload(_, _, CreatePayload(), _))
+      .WillOnce(WithArgs<3>(Invoke(DBusCallbackTrue)));
+  EXPECT_CALL(attestation_flow_, GetCertificate(_, _))
+      .WillOnce(WithArgs<1>(Invoke(CertCallbackSuccess)));
+  EXPECT_CALL(policy_client_, UploadCertificate("fake_cert", _))
+      .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
+  Run();
+}
+
+TEST_F(AttestationPolicyObserverTest, KeyExistsCertExpired) {
+  string certificate;
+  ASSERT_TRUE(CreateCertificate(CERT_EXPIRED, &certificate));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationDoesKeyExist(_, _, _))
+      .WillOnce(WithArgs<2>(Invoke(DBusCallbackTrue)));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationGetCertificate(_, _, _))
+      .WillOnce(WithArgs<2>(Invoke(FakeDBusData(certificate))));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationGetKeyPayload(_, _, _))
+      .WillRepeatedly(WithArgs<2>(Invoke(FakeDBusData(""))));
+  EXPECT_CALL(cryptohome_client_,
+              TpmAttestationSetKeyPayload(_, _, CreatePayload(), _))
+      .WillOnce(WithArgs<3>(Invoke(DBusCallbackTrue)));
   EXPECT_CALL(attestation_flow_, GetCertificate(_, _))
       .WillOnce(WithArgs<1>(Invoke(CertCallbackSuccess)));
+  EXPECT_CALL(policy_client_, UploadCertificate("fake_cert", _))
+      .WillOnce(WithArgs<1>(Invoke(StatusCallbackSuccess)));
   Run();
 }
 
-TEST_F(AttestationPolicyObserverTest, KeyExists) {
+TEST_F(AttestationPolicyObserverTest, IgnoreUnknownCertFormat) {
   EXPECT_CALL(cryptohome_client_, TpmAttestationDoesKeyExist(_, _, _))
       .WillOnce(WithArgs<2>(Invoke(DBusCallbackTrue)));
   EXPECT_CALL(cryptohome_client_, TpmAttestationGetCertificate(_, _, _))
-      .WillOnce(WithArgs<2>(Invoke(DBusDataCallback)));
+      .WillOnce(WithArgs<2>(Invoke(FakeDBusData("unsupported_cert"))));
+  EXPECT_CALL(cryptohome_client_, TpmAttestationGetKeyPayload(_, _, _))
+      .WillOnce(WithArgs<2>(Invoke(FakeDBusData(CreatePayload()))));
   Run();
 }

[Mattias Nissler (ping if slow), 2013/04/22 10:25:42]

And what you're doing here is why some people hate gmock :)

These chains of expectations are hard to read and cause maintenance overhead
(note that you had to update expectation setup in your existing test cases as
well). The usual advice for this situation is to create an actual fake
implementation of CryptohomeClient that you can configure in the test setup.
Admittedly, in case of CryptohomeClient this is a bit painful due to the
interface being so bloated (but that's another rant).

[dkrahn, 2013/04/22 22:33:31]

Each time I update the CryptohomeClient interface, I dislike the approach in
device_local_account_browsertest.cc which I have to maintain.  However, you're
right about readability and too strictly using gmock.  I've add a SetupMocks()
method which I think makes this more readable and I use strict expectations only
when I'm actually interested in them.

[Mattias Nissler (ping if slow), 2013/04/23 10:53:01]

Well, the root cause with the pain here is not gmock being superior, but
CryptohomeClient being a kitchen sink. There's no good reason why it needs to be
that way, in fact we should probably have a separate interface for the
Attestation calls and only the attestation calls, and then it'd be a breeze to
implement a fake here.