Certificate Transparency: Add observer for Signed Certificate Timestamps

net/cert/multi_log_ct_verifier_unittest.cc

Index: net/cert/multi_log_ct_verifier_unittest.cc
diff --git a/net/cert/multi_log_ct_verifier_unittest.cc b/net/cert/multi_log_ct_verifier_unittest.cc
index 104335a35c425d1acc19cc04c43daf831ab4c8b5..0cf188364461adbdb0a7938bd15ab654bdcaa412 100644
--- a/net/cert/multi_log_ct_verifier_unittest.cc
+++ b/net/cert/multi_log_ct_verifier_unittest.cc
@@ -26,8 +26,12 @@
 #include "net/log/test_net_log_entry.h"
 #include "net/test/cert_test_util.h"
 #include "net/test/ct_test_util.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using testing::_;
+using testing::Mock;
+
 namespace net {
 
 namespace {
@@ -36,6 +40,13 @@ const char kLogDescription[] = "somelog";
 const char kSCTCountHistogram[] =
     "Net.CertificateTransparency.SCTsPerConnection";
 
+class MockSCTObserver : public CTVerifier::Observer {
+ public:
+  MOCK_METHOD2(OnSCTVerified,
+               void(X509Certificate* cert,
+                    const ct::SignedCertificateTimestamp* sct));
+};
+
 class MultiLogCTVerifierTest : public ::testing::Test {
  public:
   void SetUp() override {
@@ -301,6 +312,29 @@ TEST_F(MultiLogCTVerifierTest, CountsZeroSCTsCorrectly) {
             GetValueFromHistogram(kSCTCountHistogram, 0));
 }
 
+TEST_F(MultiLogCTVerifierTest, NotifiesOfValidSCT) {
+  MockSCTObserver observer;
+  verifier_->SetObserver(&observer);
+
+  EXPECT_CALL(observer, OnSCTVerified(embedded_sct_chain_.get(), _));
+  ASSERT_TRUE(VerifySinglePrecertificateChain(embedded_sct_chain_));
+  Mock::VerifyAndClearExpectations(&observer);

[Ryan Sleevi, 2015/07/02 14:25:46]

This isn't needed if you remove 322/323, since this is implicitly handled as the
mock object goes out of scope.

[Eran Messeri, 2015/07/02 15:04:38]

Done.

+
+  verifier_->SetObserver(nullptr);

[Ryan Sleevi, 2015/07/02 14:25:46]

This isn't necessary - is it? verifier_ gets deleted as soon as this test
completed.

[Eran Messeri, 2015/07/02 15:04:39]

Done.

+}
+
+TEST_F(MultiLogCTVerifierTest, StopsNotifyingCorrectly) {
+  MockSCTObserver observer;
+  verifier_->SetObserver(&observer);
+
+  EXPECT_CALL(observer, OnSCTVerified(embedded_sct_chain_.get(), _)).Times(1);
+  ASSERT_TRUE(VerifySinglePrecertificateChain(embedded_sct_chain_));
+  verifier_->SetObserver(nullptr);

[Ryan Sleevi, 2015/07/02 14:25:46]

335 isn't strictly necessary, but it seems like you're not testing the ordering
of the calls (namely, that the delivery of notifications happens prior to the
SetObserver(nullptr)

Suggested restructuring:

331 ASSERT_TRUE(...);
332 Mock::VerifyAndClearExpectations(&observer);
333
334 verifier_->SetObserver(nullptr)
335 ASSERT_TRUE(...);
336 }

[Eran Messeri, 2015/07/02 15:04:38]

Done, though I had to add:
EXPECT_CALL(observer, OnSCTVerified(...)).Times(0);
after VerifyAndClearExpectations otherwise the call would be allowed (and a
warning would be emitted about an uninteresting mock function call)

+  ASSERT_TRUE(VerifySinglePrecertificateChain(embedded_sct_chain_));
+
+  Mock::VerifyAndClearExpectations(&observer);
+}
+
 }  // namespace
 
 }  // namespace net