RTCCertificateGenerator added.

webrtc/base/rtccertificategenerator_unittest.cc

+/*
+ *  Copyright 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/base/rtccertificategenerator.h"
+
+#include "webrtc/base/checks.h"
+#include "webrtc/base/gunit.h"
+#include "webrtc/base/logging.h"
+#include "webrtc/base/optional.h"
+#include "webrtc/base/scoped_ptr.h"
+#include "webrtc/base/thread.h"
+
+namespace rtc {
+
+// Using a class separate from |RTCCertificateGeneratorTest| because
+// |RTCCertificateGeneratorCallback| is reference counted, and |testing::Test|s

[hta-webrtc, 2016/04/14 11:54:05]

I think this comment just confuses people.

[hbos, 2016/04/14 15:31:55]

Removed it.

+// are not.
+class RTCCertificateGeneratorTester : public RTCCertificateGeneratorCallback {

[hta-webrtc, 2016/04/14 11:54:05]

Naming suggestion: Call it a "fixture" rather than a "tester". It's something
that the test uses.

[hbos, 2016/04/14 15:31:54]

Done.

+ public:
+  RTCCertificateGeneratorTester()
+      : signaling_thread_(Thread::Current()),
+        worker_thread_(new Thread()),
+        generate_completed_(false) {
+    RTC_CHECK(signaling_thread_);
+    RTC_CHECK(worker_thread_->Start());
+    generator_.reset(
+      new RTCCertificateGenerator(signaling_thread_, worker_thread_.get()));
+  }
+  ~RTCCertificateGeneratorTester() override {}
+
+  RTCCertificateGenerator* generator() const { return generator_.get(); }
+  RTCCertificate* certificate() const { return certificate_.get(); }
+
+  void OnSuccess(const scoped_refptr<RTCCertificate>& certificate) {
+    RTC_CHECK(signaling_thread_->IsCurrent());
+    RTC_CHECK(certificate);
+    certificate_ = certificate;
+    generate_completed_ = true;
+  }
+  void OnFailure() {
+    RTC_CHECK(signaling_thread_->IsCurrent());
+    certificate_ = nullptr;
+    generate_completed_ = true;
+  }
+
+  bool GenerationCompleted() {
+    RTC_CHECK(signaling_thread_->IsCurrent());
+    if (generate_completed_) {
+      // Reset flag so that future generation requests are not considered done.
+      generate_completed_ = false;
+      return true;
+    }
+    return false;
+  }
+
+ protected:
+  Thread* const signaling_thread_;
+  scoped_ptr<Thread> worker_thread_;
+  scoped_ptr<RTCCertificateGenerator> generator_;
+  scoped_refptr<RTCCertificate> certificate_;
+  bool generate_completed_;
+};
+
+class RTCCertificateGeneratorTest
+    : public testing::Test {
+ public:
+  RTCCertificateGeneratorTest() {
+    tester_ = new RefCountedObject<RTCCertificateGeneratorTester>();
+  }
+  ~RTCCertificateGeneratorTest() {}
+
+ protected:
+  static const int kGenerationTimeoutMs = 1000;
+
+  scoped_refptr<RTCCertificateGeneratorTester> tester_;
+};
+
+TEST_F(RTCCertificateGeneratorTest, GenerateECDSABlockingly) {
+  EXPECT_TRUE(RTCCertificateGenerator::GenerateCertificateBlockingly(
+      KeyParams::ECDSA(),
+      Optional<uint64_t>()));
+}
+
+TEST_F(RTCCertificateGeneratorTest, GenerateECDSA) {
+  tester_->generator()->GenerateCertificate(
+      KeyParams::ECDSA(),
+      Optional<uint64_t>(),
+      tester_);
+  EXPECT_TRUE_WAIT(tester_->GenerationCompleted(), kGenerationTimeoutMs);
+  EXPECT_TRUE(tester_->certificate());
+}
+
+TEST_F(RTCCertificateGeneratorTest, GenerateRSA) {
+  tester_->generator()->GenerateCertificate(
+      KeyParams::RSA(),
+      Optional<uint64_t>(),
+      tester_);
+  EXPECT_TRUE_WAIT(tester_->GenerationCompleted(), kGenerationTimeoutMs);
+  EXPECT_TRUE(tester_->certificate());
+}

[hta-webrtc, 2016/04/14 11:54:05]

Should you have a test that checks the timeout path - that is, something like

start to generate RSA
EXPECT_FALSE(tester_->certificate())
EXPECT_FALSE(tester_->GenerationCompleted(), 1)
EXPECT_FALSE(tester_->certificate())
EXPECT_TRUE(tester->GenerationCompleted, kGenerationTimeoutMs)
EXPECT_TRUE(tester_->certificate())

[hbos, 2016/04/14 15:31:54]

Done. I'm not sure I understand what you mean by "checks the timeout path". This
essentially tests that a certificate is not delivered immediately
(synchronously). Was that the idea, or am I not following you?
If one is not aware of thread posting and processing, checking that
GenerationCompleted() is false after GenerateCertificateAsync() might look like
a race condition (even though it isn't). Hopefully my comment clarified why this
is not the case.

+
+TEST_F(RTCCertificateGeneratorTest, GenerateECDSAWithExpires) {
+  // By generating two certificates with different expiration we can compare the
+  // two expiration times relative to each other without knowing the current
+  // time relative to epoch, 1970-01-01T00:00:00Z. This verifies that the
+  // expiration parameter is correctly used relative to the generator's clock,
+  // but does not verify that this clock is relative to epoch.

[hbos, 2016/04/13 13:40:34]

This approach was due to not finding a "get current time in milliseconds"
function in webrtc that is relative to epoch and not being able to inject time
on this layer. Time64() of timeutils.h returned a value that was less than a
day. Generating a single certificate and looking at its absolute expires value
directly would be nicer though. What do you think?

+
+  // Generate a certificate that expires immediately.
+  tester_->generator()->GenerateCertificate(
+      KeyParams::ECDSA(),
+      Optional<uint64_t>(0),
+      tester_);
+  EXPECT_TRUE_WAIT(tester_->GenerationCompleted(), kGenerationTimeoutMs);
+  EXPECT_TRUE(tester_->certificate());

[hta-webrtc, 2016/04/14 11:54:05]

Query: Why are you using the non-blocking version of the API here? It would seem
that using the blocking version would make the test look simpler (no waiting).

[hbos, 2016/04/14 15:31:54]

It is more complicated but it guarantees (with EXPECT_TRUE_WAIT) that no more
than kGenerationTimeoutMs has passed per generation request, which is relevant
for when comparing the time stamps below.

But that ECDSA can be generated in this amount of time is already tested with a
different test, so I think it makes sense to simplify this code by using the
non-async version, yeah? Updated the code.

+  scoped_refptr<RTCCertificate> cert_a = tester_->certificate();
+
+  // Generate a certificate that expires in 1 minute.
+  const uint64_t kExpiresMs = 60000;
+  tester_->generator()->GenerateCertificate(
+      KeyParams::ECDSA(),
+      Optional<uint64_t>(kExpiresMs),
+      tester_);
+  EXPECT_TRUE_WAIT(tester_->GenerationCompleted(), kGenerationTimeoutMs);
+  EXPECT_TRUE(tester_->certificate());
+  scoped_refptr<RTCCertificate> cert_b = tester_->certificate();
+
+  // Verify that |cert_b| expires approximately |kExpiresMs| after |cert_a|
+  // (allowing a +/- 1 second plus maximum generation time difference).
+  EXPECT_GT(cert_b->Expires(), cert_a->Expires());
+  uint64_t expires_diff = cert_b->Expires() - cert_a->Expires();
+  EXPECT_GE(expires_diff, kExpiresMs - 2*kGenerationTimeoutMs - 1000);
+  EXPECT_LE(expires_diff, kExpiresMs + 2*kGenerationTimeoutMs + 1000);
+}
+
+TEST_F(RTCCertificateGeneratorTest, GenerateWithInvalidParamsShouldFail) {
+  KeyParams invalid_params = KeyParams::RSA(0, 0);
+  EXPECT_FALSE(invalid_params.IsValid());
+
+  EXPECT_FALSE(RTCCertificateGenerator::GenerateCertificateBlockingly(
+      invalid_params, Optional<uint64_t>()));
+
+  tester_->generator()->GenerateCertificate(
+      invalid_params,
+      Optional<uint64_t>(),
+      tester_);
+  EXPECT_TRUE_WAIT(tester_->GenerationCompleted(), kGenerationTimeoutMs);
+  EXPECT_FALSE(tester_->certificate());
+}
+
+}  // namespace rtc