Add tests for SSL role and datachannel ID assignment.

Document with a comment the suspected place that could cause a bug.
Also fix an error in previous role observation code.

Bug: webrtc:13668
Change-Id: Id7f6af6905d90f7974b5570145c201c8339aaf72
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/251388
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35973}

pc/data_channel_integrationtest.cc

@@ -86,6 +86,13 @@ class DataChannelIntegrationTestUnifiedPlan
       : PeerConnectionIntegrationBaseTest(SdpSemantics::kUnifiedPlan) {}
 };
 
+void MakeActiveSctpOffer(cricket::SessionDescription* desc) {
+  auto& transport_infos = desc->transport_infos();
+  for (auto& transport_info : transport_infos) {
+    transport_info.description.connection_role = cricket::CONNECTIONROLE_ACTIVE;
+  }
+}
+
 // This test causes a PeerConnection to enter Disconnected state, and
 // sends data on a DataChannel while disconnected.
 // The data should be surfaced when the connection reestablishes.
@@ -577,6 +584,85 @@ TEST_P(DataChannelIntegrationTest, ClosingConnectionStopsPacketFlow) {
   EXPECT_EQ(sent_packets_a, sent_packets_b);
 }
 
+TEST_P(DataChannelIntegrationTest, DtlsRoleIsSetNormally) {
+  ASSERT_TRUE(CreatePeerConnectionWrappers());
+  ConnectFakeSignaling();
+  caller()->CreateDataChannel();
+  ASSERT_FALSE(caller()->pc()->GetSctpTransport());
+  caller()->CreateAndSetAndSignalOffer();
+  ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout);
+  ASSERT_TRUE_WAIT(caller()->data_observer()->IsOpen(), kDefaultTimeout);
+  ASSERT_TRUE(caller()->pc()->GetSctpTransport());
+  ASSERT_TRUE(
+      caller()->pc()->GetSctpTransport()->Information().dtls_transport());
+  EXPECT_TRUE(caller()
+                  ->pc()
+                  ->GetSctpTransport()
+                  ->Information()
+                  .dtls_transport()
+                  ->Information()
+                  .role());
+  EXPECT_EQ(caller()
+                ->pc()
+                ->GetSctpTransport()
+                ->Information()
+                .dtls_transport()
+                ->Information()
+                .role(),
+            DtlsTransportTlsRole::kServer);
+  EXPECT_EQ(callee()
+                ->pc()
+                ->GetSctpTransport()
+                ->Information()
+                .dtls_transport()
+                ->Information()
+                .role(),
+            DtlsTransportTlsRole::kClient);
+  // ID should be assigned according to the odd/even rule based on role; client
+  // gets even numbers, server gets odd ones.
+  // RFC 8832 section 6.
+  // TODO(hta): Test multiple channels.
+  EXPECT_EQ(caller()->data_channel()->id(), 1);
+}
+
+TEST_P(DataChannelIntegrationTest, DtlsRoleIsSetWhenReversed) {
+  ASSERT_TRUE(CreatePeerConnectionWrappers());
+  ConnectFakeSignaling();
+  caller()->CreateDataChannel();
+  callee()->SetReceivedSdpMunger(MakeActiveSctpOffer);
+  caller()->CreateAndSetAndSignalOffer();
+  ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout);
+  ASSERT_TRUE_WAIT(caller()->data_observer()->IsOpen(), kDefaultTimeout);
+  EXPECT_TRUE(caller()
+                  ->pc()
+                  ->GetSctpTransport()
+                  ->Information()
+                  .dtls_transport()
+                  ->Information()
+                  .role());
+  EXPECT_EQ(caller()
+                ->pc()
+                ->GetSctpTransport()
+                ->Information()
+                .dtls_transport()
+                ->Information()
+                .role(),
+            DtlsTransportTlsRole::kClient);
+  EXPECT_EQ(callee()
+                ->pc()
+                ->GetSctpTransport()
+                ->Information()
+                .dtls_transport()
+                ->Information()
+                .role(),
+            DtlsTransportTlsRole::kServer);
+  // ID should be assigned according to the odd/even rule based on role; client
+  // gets even numbers, server gets odd ones.
+  // RFC 8832 section 6.
+  // TODO(hta): Test multiple channels.
+  EXPECT_EQ(caller()->data_channel()->id(), 0);
+}
+
 // Test that transport stats are generated by the RTCStatsCollector for a
 // connection that only involves data channels. This is a regression test for
 // crbug.com/826972.

pc/dtls_transport.cc

@@ -114,10 +114,10 @@ void DtlsTransport::UpdateInformation() {
       if (success) {
         switch (internal_role) {
           case rtc::SSL_CLIENT:
-            role = DtlsTransportTlsRole::kServer;
+            role = DtlsTransportTlsRole::kClient;
             break;
           case rtc::SSL_SERVER:
-            role = DtlsTransportTlsRole::kClient;
+            role = DtlsTransportTlsRole::kServer;
             break;
         }
       }

pc/dtls_transport_unittest.cc

@@ -130,6 +130,7 @@ TEST_F(DtlsTransportTest, RoleAppearsOnConnect) {
                    kDefaultTimeout);
   EXPECT_TRUE(observer_.info_.role());
   EXPECT_TRUE(transport()->Information().role());
+  EXPECT_EQ(transport()->Information().role(), DtlsTransportTlsRole::kClient);
 }
 
 TEST_F(DtlsTransportTest, CertificateAppearsOnConnect) {

pc/peer_connection.cc

@@ -2168,11 +2168,18 @@ bool PeerConnection::GetSctpSslRole(rtc::SSLRole* role) {
           return transport_controller_->GetDtlsRole(*sctp_mid_n_);
         });
     if (!dtls_role && sdp_handler_->is_caller().has_value()) {
+      // This works fine if we are the offerer, but can be a mistake if
+      // we are the answerer and the remote offer is PASSIVE. In that
+      // case, we will guess the role wrong.
+      // TODO(bugs.webrtc.org/13668): Check if this actually happens.
+      RTC_LOG(LS_ERROR) << "Possible risk: DTLS role guesser is active";
       dtls_role =
           *sdp_handler_->is_caller() ? rtc::SSL_SERVER : rtc::SSL_CLIENT;
     }
-    *role = *dtls_role;
-    return true;
+    if (dtls_role) {
+      *role = *dtls_role;
+      return true;
+    }
   }
   return false;
 }