RTCCertificates added to RTCConfiguration, used by WebRtcSession/-DescriptionFactory

talk/app/webrtc/webrtcsession_unittest.cc

Index: talk/app/webrtc/webrtcsession_unittest.cc
diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc
index 1e1785fe5752f7ba4886d71b75eaa0e99d83f55f..62ac7e2de7e8b3a9ff5b1987155e1d50a1707e01 100644
--- a/talk/app/webrtc/webrtcsession_unittest.cc
+++ b/talk/app/webrtc/webrtcsession_unittest.cc
@@ -156,6 +156,8 @@ static const char kSdpWithRtx[] =
     "a=rtpmap:96 rtx/90000\r\n"
     "a=fmtp:96 apt=0\r\n";
 
+enum RTCCertificateGenerationMethod { ALREADY_GENERATED, DTLS_IDENTITY_STORE };
+
 // Add some extra |newlines| to the |message| after |line|.
 static void InjectAfter(const std::string& line,
                         const std::string& newlines,
@@ -310,7 +312,8 @@ class FakeAudioRenderer : public cricket::AudioRenderer {
   cricket::AudioRenderer::Sink* sink_;
 };
 
-class WebRtcSessionTest : public testing::Test {
+class WebRtcSessionTest
+    : public testing::TestWithParam<RTCCertificateGenerationMethod> {
  protected:
   // TODO Investigate why ChannelManager crashes, if it's created
   // after stun_server.
@@ -352,6 +355,11 @@ class WebRtcSessionTest : public testing::Test {
     network_manager_.AddInterface(addr);
   }
 
+  // If |dtls_identity_store| != null or |rtc_configuration| contains
+  // |certificates| then DTLS will be enabled unless explicitly disabled by
+  // |rtc_configuration| options. When DTLS is enabled a certificate will be
+  // used if provided, otherwise one will be generated using the
+  // |dtls_identity_store|.
   void Init(
       rtc::scoped_ptr<webrtc::DtlsIdentityStoreInterface> dtls_identity_store,
       const PeerConnectionInterface::RTCConfiguration& rtc_configuration) {
@@ -399,10 +407,27 @@ class WebRtcSessionTest : public testing::Test {
     Init(nullptr, configuration);
   }
 
-  void InitWithDtls(bool identity_request_should_fail = false) {
+  void InitWithDtls(RTCCertificateGenerationMethod cert_gen_method) {

[torbjorng (webrtc), 2015/08/21 12:39:19]

Argument splits method into disjoints chunks. Consider splitting into two
methods, or else if possible parameterize and share code between then/else.

[hbos, 2015/08/24 10:16:44]

Done.

+    if (cert_gen_method == ALREADY_GENERATED) {
+      PeerConnectionInterface::RTCConfiguration configuration;
+      configuration.certificates.push_back(
+          FakeDtlsIdentityStore::GenerateCertificate());
+      Init(nullptr, configuration);
+    } else if (cert_gen_method == DTLS_IDENTITY_STORE) {
+      rtc::scoped_ptr<FakeDtlsIdentityStore> dtls_identity_store(
+          new FakeDtlsIdentityStore());
+      dtls_identity_store->set_should_fail(false);
+      PeerConnectionInterface::RTCConfiguration configuration;
+      Init(dtls_identity_store.Pass(), configuration);
+    } else {
+      CHECK(false);
+    }
+  }
+
+  void InitWithDtlsIdentityGenFail() {
     rtc::scoped_ptr<FakeDtlsIdentityStore> dtls_identity_store(
         new FakeDtlsIdentityStore());
-    dtls_identity_store->set_should_fail(identity_request_should_fail);
+    dtls_identity_store->set_should_fail(true);
     PeerConnectionInterface::RTCConfiguration configuration;
     Init(dtls_identity_store.Pass(), configuration);
   }
@@ -1182,7 +1207,10 @@ class WebRtcSessionTest : public testing::Test {
 
   void VerifyMultipleAsyncCreateDescription(
       bool success, CreateSessionDescriptionRequest::Type type) {
-    InitWithDtls(!success);
+    if (success)
+      InitWithDtls(DTLS_IDENTITY_STORE);

[torbjorng (webrtc), 2015/08/21 12:39:19]

Should we use both cert sources here (as provided by TEST_P)?

[hbos, 2015/08/24 10:16:44]

Yeah why not! Done.

+    else
+      InitWithDtlsIdentityGenFail();
     SetFactoryDtlsSrtp();
     if (type == CreateSessionDescriptionRequest::kAnswer) {
       cricket::MediaSessionOptions options;
@@ -1256,8 +1284,8 @@ class WebRtcSessionTest : public testing::Test {
   rtc::scoped_refptr<FakeMetricsObserver> metrics_observer_;
 };
 
-TEST_F(WebRtcSessionTest, TestInitializeWithDtls) {
-  InitWithDtls();
+TEST_P(WebRtcSessionTest, TestInitializeWithDtls) {
+  InitWithDtls(GetParam());
   // SDES is disabled when DTLS is on.
   EXPECT_EQ(cricket::SEC_DISABLED, session_->SdesPolicy());
 }
@@ -1564,10 +1592,10 @@ TEST_F(WebRtcSessionTest, TestSetRemoteNonSdesAnswerWhenSdesOn) {
 
 // Test that we accept an offer with a DTLS fingerprint when DTLS is on
 // and that we return an answer with a DTLS fingerprint.
-TEST_F(WebRtcSessionTest, TestReceiveDtlsOfferCreateDtlsAnswer) {
+TEST_P(WebRtcSessionTest, TestReceiveDtlsOfferCreateDtlsAnswer) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
   mediastream_signaling_.SendAudioVideoStream1();
-  InitWithDtls();
+  InitWithDtls(GetParam());
   SetFactoryDtlsSrtp();
   cricket::MediaSessionOptions options;
   options.recv_video = true;
@@ -1593,10 +1621,10 @@ TEST_F(WebRtcSessionTest, TestReceiveDtlsOfferCreateDtlsAnswer) {
 
 // Test that we set a local offer with a DTLS fingerprint when DTLS is on
 // and then we accept a remote answer with a DTLS fingerprint successfully.
-TEST_F(WebRtcSessionTest, TestCreateDtlsOfferReceiveDtlsAnswer) {
+TEST_P(WebRtcSessionTest, TestCreateDtlsOfferReceiveDtlsAnswer) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
   mediastream_signaling_.SendAudioVideoStream1();
-  InitWithDtls();
+  InitWithDtls(GetParam());
   SetFactoryDtlsSrtp();
 
   // Verify that we get a crypto fingerprint in the answer.
@@ -1623,9 +1651,9 @@ TEST_F(WebRtcSessionTest, TestCreateDtlsOfferReceiveDtlsAnswer) {
 
 // Test that if we support DTLS and the other side didn't offer a fingerprint,
 // we will fail to set the remote description.
-TEST_F(WebRtcSessionTest, TestReceiveNonDtlsOfferWhenDtlsOn) {
+TEST_P(WebRtcSessionTest, TestReceiveNonDtlsOfferWhenDtlsOn) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
-  InitWithDtls();
+  InitWithDtls(GetParam());
   cricket::MediaSessionOptions options;
   options.recv_video = true;
   options.bundle_enabled = true;
@@ -1647,9 +1675,9 @@ TEST_F(WebRtcSessionTest, TestReceiveNonDtlsOfferWhenDtlsOn) {
 
 // Test that we return a failure when applying a local answer that doesn't have
 // a DTLS fingerprint when DTLS is required.
-TEST_F(WebRtcSessionTest, TestSetLocalNonDtlsAnswerWhenDtlsOn) {
+TEST_P(WebRtcSessionTest, TestSetLocalNonDtlsAnswerWhenDtlsOn) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
-  InitWithDtls();
+  InitWithDtls(GetParam());
   SessionDescriptionInterface* offer = NULL;
   SessionDescriptionInterface* answer = NULL;
   CreateDtlsOfferAndNonDtlsAnswer(&offer, &answer);
@@ -1663,11 +1691,11 @@ TEST_F(WebRtcSessionTest, TestSetLocalNonDtlsAnswerWhenDtlsOn) {
 
 // Test that we return a failure when applying a remote answer that doesn't have
 // a DTLS fingerprint when DTLS is required.
-TEST_F(WebRtcSessionTest, TestSetRemoteNonDtlsAnswerWhenDtlsOn) {
+TEST_P(WebRtcSessionTest, TestSetRemoteNonDtlsAnswerWhenDtlsOn) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
   // Enable both SDES and DTLS, so that offer won't be outright rejected as a
   // result of using the "UDP/TLS/RTP/SAVPF" profile.
-  InitWithDtls();
+  InitWithDtls(GetParam());
   session_->SetSdesPolicy(cricket::SEC_ENABLED);
   SessionDescriptionInterface* offer = CreateOffer();
   cricket::MediaSessionOptions options;
@@ -1684,10 +1712,10 @@ TEST_F(WebRtcSessionTest, TestSetRemoteNonDtlsAnswerWhenDtlsOn) {
 
 // Test that we create a local offer without SDES or DTLS and accept a remote
 // answer without SDES or DTLS when encryption is disabled.
-TEST_F(WebRtcSessionTest, TestCreateOfferReceiveAnswerWithoutEncryption) {
+TEST_P(WebRtcSessionTest, TestCreateOfferReceiveAnswerWithoutEncryption) {
   mediastream_signaling_.SendAudioVideoStream1();
   options_.disable_encryption = true;
-  InitWithDtls();
+  InitWithDtls(GetParam());
 
   // Verify that we get a crypto fingerprint in the answer.
   SessionDescriptionInterface* offer = CreateOffer();
@@ -1713,9 +1741,9 @@ TEST_F(WebRtcSessionTest, TestCreateOfferReceiveAnswerWithoutEncryption) {
 
 // Test that we create a local answer without SDES or DTLS and accept a remote
 // offer without SDES or DTLS when encryption is disabled.
-TEST_F(WebRtcSessionTest, TestCreateAnswerReceiveOfferWithoutEncryption) {
+TEST_P(WebRtcSessionTest, TestCreateAnswerReceiveOfferWithoutEncryption) {
   options_.disable_encryption = true;
-  InitWithDtls();
+  InitWithDtls(GetParam());
 
   cricket::MediaSessionOptions options;
   options.recv_video = true;
@@ -3411,7 +3439,7 @@ TEST_F(WebRtcSessionTest, TestRtpDataChannel) {
   EXPECT_EQ(cricket::DCT_RTP, data_engine_->last_channel_type());
 }
 
-TEST_F(WebRtcSessionTest, TestRtpDataChannelConstraintTakesPrecedence) {
+TEST_P(WebRtcSessionTest, TestRtpDataChannelConstraintTakesPrecedence) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
 
   constraints_.reset(new FakeConstraints());
@@ -3419,26 +3447,26 @@ TEST_F(WebRtcSessionTest, TestRtpDataChannelConstraintTakesPrecedence) {
       webrtc::MediaConstraintsInterface::kEnableRtpDataChannels, true);
   options_.disable_sctp_data_channels = false;
 
-  InitWithDtls();
+  InitWithDtls(GetParam());
 
   SetLocalDescriptionWithDataChannel();
   EXPECT_EQ(cricket::DCT_RTP, data_engine_->last_channel_type());
 }
 
-TEST_F(WebRtcSessionTest, TestCreateOfferWithSctpEnabledWithoutStreams) {
+TEST_P(WebRtcSessionTest, TestCreateOfferWithSctpEnabledWithoutStreams) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
 
-  InitWithDtls();
+  InitWithDtls(GetParam());
 
   rtc::scoped_ptr<SessionDescriptionInterface> offer(CreateOffer());
   EXPECT_TRUE(offer->description()->GetContentByName("data") == NULL);
   EXPECT_TRUE(offer->description()->GetTransportInfoByName("data") == NULL);
 }
 
-TEST_F(WebRtcSessionTest, TestCreateAnswerWithSctpInOfferAndNoStreams) {
+TEST_P(WebRtcSessionTest, TestCreateAnswerWithSctpInOfferAndNoStreams) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
   SetFactoryDtlsSrtp();
-  InitWithDtls();
+  InitWithDtls(GetParam());
 
   // Create remote offer with SCTP.
   cricket::MediaSessionOptions options;
@@ -3454,40 +3482,40 @@ TEST_F(WebRtcSessionTest, TestCreateAnswerWithSctpInOfferAndNoStreams) {
   EXPECT_TRUE(answer->description()->GetTransportInfoByName("data") != NULL);
 }
 
-TEST_F(WebRtcSessionTest, TestSctpDataChannelWithoutDtls) {
+TEST_P(WebRtcSessionTest, TestSctpDataChannelWithoutDtls) {
   constraints_.reset(new FakeConstraints());
   constraints_->AddOptional(
       webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, false);
-  InitWithDtls();
+  InitWithDtls(GetParam());
 
   SetLocalDescriptionWithDataChannel();
   EXPECT_EQ(cricket::DCT_NONE, data_engine_->last_channel_type());
 }
 
-TEST_F(WebRtcSessionTest, TestSctpDataChannelWithDtls) {
+TEST_P(WebRtcSessionTest, TestSctpDataChannelWithDtls) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
 
-  InitWithDtls();
+  InitWithDtls(GetParam());
 
   SetLocalDescriptionWithDataChannel();
   EXPECT_EQ(cricket::DCT_SCTP, data_engine_->last_channel_type());
 }
 
-TEST_F(WebRtcSessionTest, TestDisableSctpDataChannels) {
+TEST_P(WebRtcSessionTest, TestDisableSctpDataChannels) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
   options_.disable_sctp_data_channels = true;
-  InitWithDtls();
+  InitWithDtls(GetParam());
 
   SetLocalDescriptionWithDataChannel();
   EXPECT_EQ(cricket::DCT_NONE, data_engine_->last_channel_type());
 }
 
-TEST_F(WebRtcSessionTest, TestSctpDataChannelSendPortParsing) {
+TEST_P(WebRtcSessionTest, TestSctpDataChannelSendPortParsing) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
   const int new_send_port = 9998;
   const int new_recv_port = 7775;
 
-  InitWithDtls();
+  InitWithDtls(GetParam());
   SetFactoryDtlsSrtp();
 
   // By default, don't actually add the codecs to desc_factory_; they don't
@@ -3536,13 +3564,29 @@ TEST_F(WebRtcSessionTest, TestSctpDataChannelSendPortParsing) {
   EXPECT_EQ(new_recv_port, portnum);
 }
 
+// TODO(hbos): Add the following test once RTCCertificate is passed around
+// outside of WebRtcSessionDescriptionFactory code and there exists a
+// WebRtcSession::certificate().
+//TEST_F(WebRtcSessionTest, TestUsesProvidedCertificate) {
+//  rtc::scoped_refptr<rtc::RTCCertificate> certificate =
+//      FakeDtlsIdentityStore::GenerateCertificate();
+//
+//  PeerConnectionInterface::RTCConfiguration configuration;
+//  configuration.certificates.push_back(certificate);
+//  Init(nullptr, configuration);
+//  EXPECT_TRUE_WAIT(!session_->waiting_for_identity_for_testing(), 1000);
+//
+//  EXPECT_EQ(session_->certificate(), certificate);
+//}
+
 // Verifies that CreateOffer succeeds when CreateOffer is called before async
-// identity generation is finished.
-TEST_F(WebRtcSessionTest, TestCreateOfferBeforeIdentityRequestReturnSuccess) {
+// identity generation is finished (even if a certificate is provided this is
+// an async op).
+TEST_P(WebRtcSessionTest, TestCreateOfferBeforeIdentityRequestReturnSuccess) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
-  InitWithDtls();
+  InitWithDtls(GetParam());
 
-  EXPECT_TRUE(session_->waiting_for_identity());
+  EXPECT_TRUE(session_->waiting_for_identity_for_testing());
   mediastream_signaling_.SendAudioVideoStream1();
   rtc::scoped_ptr<SessionDescriptionInterface> offer(CreateOffer());
 
@@ -3552,10 +3596,11 @@ TEST_F(WebRtcSessionTest, TestCreateOfferBeforeIdentityRequestReturnSuccess) {
 }
 
 // Verifies that CreateAnswer succeeds when CreateOffer is called before async
-// identity generation is finished.
-TEST_F(WebRtcSessionTest, TestCreateAnswerBeforeIdentityRequestReturnSuccess) {
+// identity generation is finished (even if a certificate is provided this is
+// an async op).
+TEST_P(WebRtcSessionTest, TestCreateAnswerBeforeIdentityRequestReturnSuccess) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
-  InitWithDtls();
+  InitWithDtls(GetParam());
   SetFactoryDtlsSrtp();
 
   cricket::MediaSessionOptions options;
@@ -3572,12 +3617,13 @@ TEST_F(WebRtcSessionTest, TestCreateAnswerBeforeIdentityRequestReturnSuccess) {
 }
 
 // Verifies that CreateOffer succeeds when CreateOffer is called after async
-// identity generation is finished.
-TEST_F(WebRtcSessionTest, TestCreateOfferAfterIdentityRequestReturnSuccess) {
+// identity generation is finished (even if a certificate is provided this is
+// an async op).
+TEST_P(WebRtcSessionTest, TestCreateOfferAfterIdentityRequestReturnSuccess) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
-  InitWithDtls();
+  InitWithDtls(GetParam());
 
-  EXPECT_TRUE_WAIT(!session_->waiting_for_identity(), 1000);
+  EXPECT_TRUE_WAIT(!session_->waiting_for_identity_for_testing(), 1000);
 
   rtc::scoped_ptr<SessionDescriptionInterface> offer(CreateOffer());
   EXPECT_TRUE(offer != NULL);
@@ -3587,9 +3633,9 @@ TEST_F(WebRtcSessionTest, TestCreateOfferAfterIdentityRequestReturnSuccess) {
 // identity generation fails.
 TEST_F(WebRtcSessionTest, TestCreateOfferAfterIdentityRequestReturnFailure) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
-  InitWithDtls(true);
+  InitWithDtlsIdentityGenFail();
 
-  EXPECT_TRUE_WAIT(!session_->waiting_for_identity(), 1000);
+  EXPECT_TRUE_WAIT(!session_->waiting_for_identity_for_testing(), 1000);
 
   rtc::scoped_ptr<SessionDescriptionInterface> offer(CreateOffer());
   EXPECT_TRUE(offer == NULL);
@@ -3729,9 +3775,9 @@ TEST_F(WebRtcSessionTest, TestCombinedAudioVideoBweConstraint) {
 
 // Tests that we can renegotiate new media content with ICE candidates in the
 // new remote SDP.
-TEST_F(WebRtcSessionTest, TestRenegotiateNewMediaWithCandidatesInSdp) {
+TEST_P(WebRtcSessionTest, TestRenegotiateNewMediaWithCandidatesInSdp) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
-  InitWithDtls();
+  InitWithDtls(GetParam());
   SetFactoryDtlsSrtp();
 
   mediastream_signaling_.UseOptionsAudioOnly();
@@ -3759,9 +3805,9 @@ TEST_F(WebRtcSessionTest, TestRenegotiateNewMediaWithCandidatesInSdp) {
 
 // Tests that we can renegotiate new media content with ICE candidates separated
 // from the remote SDP.
-TEST_F(WebRtcSessionTest, TestRenegotiateNewMediaWithCandidatesSeparated) {
+TEST_P(WebRtcSessionTest, TestRenegotiateNewMediaWithCandidatesSeparated) {
   MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
-  InitWithDtls();
+  InitWithDtls(GetParam());
   SetFactoryDtlsSrtp();
 
   mediastream_signaling_.UseOptionsAudioOnly();
@@ -3875,11 +3921,6 @@ TEST_F(WebRtcSessionTest, CreateOffersAndShutdown) {
 
   session_.reset();
 
-  // Make sure we process pending messages on the current (signaling) thread
-  // before checking we we got our callbacks.  Quit() will do this and then
-  // immediately exit.  We won't need the queue after this point anyway.
-  rtc::Thread::Current()->Quit();
-

[hbos, 2015/08/20 16:06:11]

Two things wrong with this, which is why I removed it:
- A unittest should not do things that will have consequences outside of the
test being teared down. Quitting the thread has the lasting consequence that the
thread has stopped. If other tests run after it and for example try to do Invoke
they will not succeed (and the fact that Invoke did nothing is not immediately
clear). Adding TEST_P's changed the order of some tests and exposed this issue.
(Previously this was always the last test being run).
- Quit() does not look like it is actually process pending messages(?), it just
marks it stopped and wakes the thread up (but it's already awake?).

With messages not being processed anyway I did not replace this by a message
processing loop.

   for (auto& o : observers) {
     // We expect to have received a notification now even if the session was
     // terminated.  The offer creation may or may not have succeeded, but we
@@ -3892,3 +3933,7 @@ TEST_F(WebRtcSessionTest, CreateOffersAndShutdown) {
 // TODO(bemasc): Add a TestIceStatesBundle with BUNDLE enabled.  That test
 // currently fails because upon disconnection and reconnection OnIceComplete is
 // called more than once without returning to IceGatheringGathering.
+
+INSTANTIATE_TEST_CASE_P(
+    WebRtcSessionTests, WebRtcSessionTest,
+    testing::Values(ALREADY_GENERATED, DTLS_IDENTITY_STORE));