Allow remote fingerprint update during a call

talk/app/webrtc/peerconnection_unittest.cc

Index: talk/app/webrtc/peerconnection_unittest.cc
diff --git a/talk/app/webrtc/peerconnection_unittest.cc b/talk/app/webrtc/peerconnection_unittest.cc
index 3193ffd898e71c1cdf23ccb3049c477c6ecd9811..3367484605e39a59e1df0e58109ccc0a035d1f8d 100644
--- a/talk/app/webrtc/peerconnection_unittest.cc
+++ b/talk/app/webrtc/peerconnection_unittest.cc
@@ -658,6 +658,10 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver,
     return pc()->ice_gathering_state();
   }
 
+  static void set_use_alternate_key(bool use_alternate_key) {
+    use_alternate_key_ = use_alternate_key;

[pthatcher1, 2015/11/18 20:42:43]

Why is this (and use_alternate_key_) static?

[guoweis_webrtc, 2015/11/25 21:03:13]

Done.

+  }
+
  private:
   class DummyDtmfObserver : public DtmfSenderObserverInterface {
    public:
@@ -740,9 +744,15 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver,
     ice_server.uri = "stun:stun.l.google.com:19302";
     ice_servers.push_back(ice_server);
 
-    rtc::scoped_ptr<webrtc::DtlsIdentityStoreInterface> dtls_identity_store(
+    rtc::scoped_ptr<FakeDtlsIdentityStore> dtls_identity_store(
         rtc::SSLStreamAdapter::HaveDtlsSrtp() ? new FakeDtlsIdentityStore()
                                               : nullptr);
+
+    if (use_alternate_key_) {
+      dtls_identity_store->use_alternate_key();
+    } else {
+      dtls_identity_store->use_original_key();
+    }
     return peer_connection_factory_->CreatePeerConnection(
         ice_servers, constraints, factory, dtls_identity_store.Pass(), this);
   }
@@ -890,8 +900,14 @@ class PeerConnectionTestClient : public webrtc::PeerConnectionObserver,
 
   rtc::scoped_refptr<DataChannelInterface> data_channel_;
   rtc::scoped_ptr<MockDataChannelObserver> data_observer_;
+
+  static bool use_alternate_key_;
 };
 
+// FakeDtlsIdentityStore supports 2 keys. We'll use the original key to start
+// with.
+bool PeerConnectionTestClient::use_alternate_key_ = false;
+
 // TODO(deadbeef): Rename this to P2PTestConductor once the Linux memcheck and
 // Windows DrMemory Full bots' blacklists are updated.
 class JsepPeerConnectionP2PTestClient : public testing::Test {
@@ -968,6 +984,11 @@ class JsepPeerConnectionP2PTestClient : public testing::Test {
                              nullptr);
   }
 
+  void MutuallySetSignalingReceiver() {

[pthatcher1, 2015/11/18 20:42:43]

How about "SetSignalingReceivers" (an "s" on the end and no "Mutually")?

[guoweis_webrtc, 2015/11/25 21:03:13]

Done.

+    initiating_client_->set_signaling_message_receiver(receiving_client_.get());
+    receiving_client_->set_signaling_message_receiver(initiating_client_.get());
+  }
+
   bool CreateTestClients(MediaConstraintsInterface* init_constraints,
                          PeerConnectionFactory::Options* init_options,
                          MediaConstraintsInterface* recv_constraints,
@@ -979,8 +1000,7 @@ class JsepPeerConnectionP2PTestClient : public testing::Test {
     if (!initiating_client_ || !receiving_client_) {
       return false;
     }
-    initiating_client_->set_signaling_message_receiver(receiving_client_.get());
-    receiving_client_->set_signaling_message_receiver(initiating_client_.get());
+    MutuallySetSignalingReceiver();
     return true;
   }
 
@@ -1058,6 +1078,47 @@ class JsepPeerConnectionP2PTestClient : public testing::Test {
                      kMaxWaitForFramesMs);
   }
 
+  // This test sets up a successful call then do the call transfer to another

[pthatcher1, 2015/11/18 20:42:42]

do => does
the call transfer => a call transfer

[guoweis_webrtc, 2015/11/25 21:03:13]

Done.

+  // caller or callee. It uses the alternate identity in FakeDtlsIdentityStore
+  // to make sure a different certificate/fingerprint is used.
+  void TransferCall(bool test_callee) {
+    MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp);
+    FakeConstraints setup_constraints;
+    setup_constraints.AddMandatory(MediaConstraintsInterface::kEnableDtlsSrtp,
+                                   true);

[pthatcher1, 2015/11/18 20:42:42]

Isn't this the default by now?

[guoweis_webrtc, 2015/11/25 21:03:13]

It's not very clear. might be the case from
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
but it's a bit subtle.

Leave it here for consistency with other cases.

+    ASSERT_TRUE(CreateTestClients(&setup_constraints, &setup_constraints));
+    LocalP2PTest();
+    VerifyRenderedSize(640, 480);
+
+    LOG(LS_INFO) << "Starting call transfer";
+
+    // Make sure the new client is using a different certificate.
+    PeerConnectionTestClient::set_use_alternate_key(true);
+    rtc::scoped_ptr<PeerConnectionTestClient> new_client(
+        PeerConnectionTestClient::CreateClient("New Peer: ", &setup_constraints,
+                                               nullptr));
+
+    // Keeping the original peer around which will still send packets to the
+    // receiving client. These SRTP packets will be dropped.
+    rtc::scoped_ptr<PeerConnectionTestClient> original_peer;
+    if (test_callee) {
+      original_peer.reset(swap_initializing_client(new_client.release()));

[pthatcher1, 2015/11/18 20:42:43]

Wouldn't this work?

if (test_callee) {
  original_peer_.reset(initiating_client_.release());
  initiating_client_.reset(new_client.release());
} else {
  original_peer_.reset(receiving_client_.release());
  receiving_client_.reset(new_client.release());
}

Then you don't need the extra functions, and I think it's more readable.

In fact, does this work?  I'm not sure, but it's worth checking:


if (test_callee) {
  original_peer_ = initiating_client;
  initiating_client_ = new_client;
} else {
  original_peer_ = receiving_client_;
  receiving_client_ = new_client;
}

Because that would be even better.

[guoweis_webrtc, 2015/11/25 21:03:13]

Done.

+    } else {
+      original_peer.reset(swap_receiving_client(new_client.release()));
+    }
+
+    // Restore the original certificate to the transfee.
+    PeerConnectionTestClient::set_use_alternate_key(false);
+    MutuallySetSignalingReceiver();
+    if (test_callee) {
+      receiving_client()->SetExpectIceRestart(true);
+    } else {
+      initializing_client()->IceRestart();
+    }
+    LocalP2PTest();
+    VerifyRenderedSize(640, 480);
+  }
+
   void SendRtpData(webrtc::DataChannelInterface* dc, const std::string& data) {
     // Messages may get lost on the unreliable DataChannel, so we send multiple
     // times to avoid test flakiness.
@@ -1071,10 +1132,29 @@ class JsepPeerConnectionP2PTestClient : public testing::Test {
   PeerConnectionTestClient* initializing_client() {
     return initiating_client_.get();
   }
+
+  // Reset the |initiating_client_| to the |client| passed in and return the
+  // original |initiating_client_|.
+  PeerConnectionTestClient* swap_initializing_client(
+      PeerConnectionTestClient* client) {
+    PeerConnectionTestClient* old = initiating_client_.release();
+    initiating_client_.reset(client);
+    return old;
+  }
+
   PeerConnectionTestClient* receiving_client() {
     return receiving_client_.get();
   }
 
+  // Reset the |receiving_client_| to the |client| passed in and return the
+  // original |receiving_client_|.
+  PeerConnectionTestClient* swap_receiving_client(
+      PeerConnectionTestClient* client) {
+    PeerConnectionTestClient* old = receiving_client_.release();
+    receiving_client_.reset(client);
+    return old;
+  }
+
  private:
   rtc::scoped_ptr<rtc::PhysicalSocketServer> pss_;
   rtc::scoped_ptr<rtc::VirtualSocketServer> ss_;
@@ -1159,6 +1239,18 @@ TEST_F(JsepPeerConnectionP2PTestClient, LocalP2PTestDtlsRenegotiate) {
   receiving_client()->Negotiate();
 }
 
+// This test sets up a call transfer to a new caller with a different DTLS
+// fingerprint.
+TEST_F(JsepPeerConnectionP2PTestClient, LocalP2PTestDtlsTransferCallee) {
+  TransferCall(true);

[pthatcher1, 2015/11/18 20:42:43]

I think there are enough "if(test_callee)" checks in the shared logic to make
sense to just duplicate logic between the two tests.  Maybe just put the
duplicate setup code in a setup method.

[guoweis_webrtc, 2015/11/25 21:03:13]

Done.

+}
+
+// This test sets up a call transfer to a new callee with a different DTLS
+// fingerprint.
+TEST_F(JsepPeerConnectionP2PTestClient, LocalP2PTestDtlsTransferCaller) {
+  TransferCall(false);
+}
+
 // This test sets up a call between two endpoints that are configured to use
 // DTLS key agreement. The offerer don't support SDES. As a result, DTLS is
 // negotiated and used for transport.