Refactor/clean shouldUseProxy (#722)

make code more readable by split code into smaller functions

add CandidateTypePeerReflexive check

Add shouldUseProxy tests

client/internal/peer/conn.go

@@ -310,38 +310,44 @@ func (conn *Conn) Open() error {
 }
 
 // useProxy determines whether a direct connection (without a go proxy) is possible
-// There are 3 cases: one of the peers has a public IP or both peers are in the same private network
+//
+// There are 2 cases:
+//
+// * When neither candidate is from hard nat and one of the peers has a public IP
+//
+// * both peers are in the same private network
+//
 // Please note, that this check happens when peers were already able to ping each other using ICE layer.
 func shouldUseProxy(pair *ice.CandidatePair) bool {
-	remoteIP := net.ParseIP(pair.Remote.Address())
-	myIp := net.ParseIP(pair.Local.Address())
-	remoteIsPublic := IsPublicIP(remoteIP)
-	myIsPublic := IsPublicIP(myIp)
-
-	if pair.Local.Type() == ice.CandidateTypeRelay || pair.Remote.Type() == ice.CandidateTypeRelay {
-		return true
-	}
-
-	//one of the hosts has a public IP
-	if remoteIsPublic && pair.Remote.Type() == ice.CandidateTypeHost {
-		return false
-	}
-	if myIsPublic && pair.Local.Type() == ice.CandidateTypeHost {
+	if !isHardNATCandidate(pair.Local) && isHostCandidateWithPublicIP(pair.Remote) {
 		return false
 	}
 
-	if pair.Local.Type() == ice.CandidateTypeHost && pair.Remote.Type() == ice.CandidateTypeHost {
-		if !remoteIsPublic && !myIsPublic {
-			//both hosts are in the same private network
-			return false
-		}
+	if !isHardNATCandidate(pair.Remote) && isHostCandidateWithPublicIP(pair.Local) {
+		return false
+	}
+
+	if isHostCandidateWithPrivateIP(pair.Local) && isHostCandidateWithPrivateIP(pair.Remote) {
+		return false
 	}
 
 	return true
 }
 
-// IsPublicIP indicates whether IP is public or not.
-func IsPublicIP(ip net.IP) bool {
+func isHardNATCandidate(candidate ice.Candidate) bool {
+	return candidate.Type() == ice.CandidateTypeRelay || candidate.Type() == ice.CandidateTypePeerReflexive
+}
+
+func isHostCandidateWithPublicIP(candidate ice.Candidate) bool {
+	return candidate.Type() == ice.CandidateTypeHost && isPublicIP(candidate.Address())
+}
+
+func isHostCandidateWithPrivateIP(candidate ice.Candidate) bool {
+	return candidate.Type() == ice.CandidateTypeHost && !isPublicIP(candidate.Address())
+}
+
+func isPublicIP(address string) bool {
+	ip := net.ParseIP(address)
 	if ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsPrivate() {
 		return false
 	}

client/internal/peer/conn_test.go

@@ -166,3 +166,166 @@ func TestConn_Close(t *testing.T) {
 
 	wg.Wait()
 }
+
+type mockICECandidate struct {
+	ice.Candidate
+	AddressFunc func() string
+	TypeFunc    func() ice.CandidateType
+}
+
+// Address mocks and overwrite ice.Candidate Address method
+func (m *mockICECandidate) Address() string {
+	if m.AddressFunc != nil {
+		return m.AddressFunc()
+	}
+	return ""
+}
+
+// Type mocks and overwrite ice.Candidate Type method
+func (m *mockICECandidate) Type() ice.CandidateType {
+	if m.TypeFunc != nil {
+		return m.TypeFunc()
+	}
+	return ice.CandidateTypeUnspecified
+}
+
+func TestConn_ShouldUseProxy(t *testing.T) {
+	publicHostCandidate := &mockICECandidate{
+		AddressFunc: func() string {
+			return "8.8.8.8"
+		},
+		TypeFunc: func() ice.CandidateType {
+			return ice.CandidateTypeHost
+		},
+	}
+	privateHostCandidate := &mockICECandidate{
+		AddressFunc: func() string {
+			return "10.0.0.1"
+		},
+		TypeFunc: func() ice.CandidateType {
+			return ice.CandidateTypeHost
+		},
+	}
+	srflxCandidate := &mockICECandidate{
+		AddressFunc: func() string {
+			return "1.1.1.1"
+		},
+		TypeFunc: func() ice.CandidateType {
+			return ice.CandidateTypeServerReflexive
+		},
+	}
+
+	prflxCandidate := &mockICECandidate{
+		AddressFunc: func() string {
+			return "1.1.1.1"
+		},
+		TypeFunc: func() ice.CandidateType {
+			return ice.CandidateTypePeerReflexive
+		},
+	}
+
+	relayCandidate := &mockICECandidate{
+		AddressFunc: func() string {
+			return "1.1.1.1"
+		},
+		TypeFunc: func() ice.CandidateType {
+			return ice.CandidateTypeRelay
+		},
+	}
+
+	testCases := []struct {
+		name        string
+		candatePair *ice.CandidatePair
+		expected    bool
+	}{
+		{
+			name: "Use Proxy When Local Candidate Is Relay",
+			candatePair: &ice.CandidatePair{
+				Local:  relayCandidate,
+				Remote: privateHostCandidate,
+			},
+			expected: true,
+		},
+		{
+			name: "Use Proxy When Remote Candidate Is Relay",
+			candatePair: &ice.CandidatePair{
+				Local:  privateHostCandidate,
+				Remote: relayCandidate,
+			},
+			expected: true,
+		},
+		{
+			name: "Use Proxy When Local Candidate Is Peer Reflexive",
+			candatePair: &ice.CandidatePair{
+				Local:  prflxCandidate,
+				Remote: privateHostCandidate,
+			},
+			expected: true,
+		},
+		{
+			name: "Use Proxy When Remote Candidate Is Peer Reflexive",
+			candatePair: &ice.CandidatePair{
+				Local:  privateHostCandidate,
+				Remote: prflxCandidate,
+			},
+			expected: true,
+		},
+		{
+			name: "Don't Use Proxy When Local Candidate Is Public And Remote Is Private",
+			candatePair: &ice.CandidatePair{
+				Local:  publicHostCandidate,
+				Remote: privateHostCandidate,
+			},
+			expected: false,
+		},
+		{
+			name: "Don't Use Proxy When Remote Candidate Is Public And Local Is Private",
+			candatePair: &ice.CandidatePair{
+				Local:  privateHostCandidate,
+				Remote: publicHostCandidate,
+			},
+			expected: false,
+		},
+		{
+			name: "Don't Use Proxy When Local Candidate is Public And Remote Is Server Reflexive",
+			candatePair: &ice.CandidatePair{
+				Local:  publicHostCandidate,
+				Remote: srflxCandidate,
+			},
+			expected: false,
+		},
+		{
+			name: "Don't Use Proxy When Remote Candidate is Public And Local Is Server Reflexive",
+			candatePair: &ice.CandidatePair{
+				Local:  srflxCandidate,
+				Remote: publicHostCandidate,
+			},
+			expected: false,
+		},
+		{
+			name: "Don't Use Proxy When Both Candidates Are Public",
+			candatePair: &ice.CandidatePair{
+				Local:  publicHostCandidate,
+				Remote: publicHostCandidate,
+			},
+			expected: false,
+		},
+		{
+			name: "Don't Use Proxy When Both Candidates Are Private",
+			candatePair: &ice.CandidatePair{
+				Local:  privateHostCandidate,
+				Remote: privateHostCandidate,
+			},
+			expected: false,
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			result := shouldUseProxy(testCase.candatePair)
+			if result != testCase.expected {
+				t.Errorf("got a different result. Expected %t Got %t", testCase.expected, result)
+			}
+		})
+	}
+}