QA(webconnectivity): make sure output matches MK

QA/webconnectivity.py

@@ -80,9 +80,9 @@ def webconnectivity_transparent_http_proxy(ooni_exe, outfile):
     assert tk["body_length_match"] == True
     assert tk["body_proportion"] == 1
     assert tk["status_code_match"] == True
-    assert tk["header_match"] == True
+    assert tk["headers_match"] == True
     assert tk["title_match"] == True
-    assert tk["blocking"] == None
+    assert tk["blocking"] == False
     assert tk["accessible"] == True
 
 

experiment/webconnectivity/httpanalysis.go

@@ -16,7 +16,7 @@ type HTTPAnalysisResult struct {
 	BodyLengthMatch *bool    `json:"body_length_match"`
 	BodyProportion  *float64 `json:"body_proportion"`
 	StatusCodeMatch *bool    `json:"status_code_match"`
-	HeadersMatch    *bool    `json:"header_match"`
+	HeadersMatch    *bool    `json:"headers_match"`
 	TitleMatch      *bool    `json:"title_match"`
 }
 

experiment/webconnectivity/summary.go

@@ -10,8 +10,48 @@ import (
 
 // Summary contains the Web Connectivity summary.
 type Summary struct {
-	Blocking   *string `json:"blocking"`
-	Accessible *bool   `json:"accessible"`
+	// Accessible is nil when the measurement failed, true if we do
+	// not think there was blocking, false in case of blocking.
+	Accessible *bool `json:"accessible"`
+
+	// BlockingReason indicates the cause of blocking when the Accessible
+	// variable is false. BlockingReason is meaningless otherwise.
+	//
+	// This is an intermediate variable used to compute Blocking, which
+	// is what OONI data consumers expect to see.
+	BlockingReason *string `json:"-"`
+
+	// Blocking implements the blocking variable as expected by OONI
+	// data consumers. See DetermineBlocking's docs.
+	Blocking interface{} `json:"blocking"`
+}
+
+// DetermineBlocking returns the value of Summary.Blocking according to
+// the expectations of OONI data consumers (nil|false|string).
+//
+// Measurement Kit sets blocking to false when accessible is true. The spec
+// doesn't mention this possibility, as of 2019-08-20-001. Yet we implemented
+// it back in 2016, with little explanation <https://git.io/JJHOl>.
+//
+// We eventually managed to link such a change with the 0.3.4 release of
+// Measurement Kit <https://git.io/JJHOS>. This led us to find out the
+// related issue #867 <https://git.io/JJHOH>. From this issue it become
+// clear that the change on Measurement Kit was applied to mirror a change
+// implemented in OONI Probe Legacy <https://git.io/JJH3T>. In such a
+// change, determine_blocking() was modified to return False in case no
+// blocking was detected, to distinguish this case from the case where
+// there was an early failure in the experiment.
+//
+// Indeed, the OONI Android app uses the case where `blocking` is `null`
+// to flag failed tests. Instead, success is identified by `blocking` being
+// false and all other cases indicate anomaly <https://git.io/JJH3C>.
+//
+// Because of that, we must preserve the original behaviour.
+func DetermineBlocking(s Summary) interface{} {
+	if s.Accessible != nil && *s.Accessible == true {
+		return false
+	}
+	return s.BlockingReason
 }
 
 func stringPointerToString(v *string) (out string) {
@@ -24,12 +64,16 @@ func stringPointerToString(v *string) (out string) {
 
 // Log logs the summary using the provided logger.
 func (s Summary) Log(logger model.Logger) {
-	logger.Infof("Blocking %+v", stringPointerToString(s.Blocking))
+	logger.Infof("Blocking %+v", stringPointerToString(s.BlockingReason))
 	logger.Infof("Accessible %+v", boolPointerToString(s.Accessible))
 }
 
 // Summarize computes the summary from the TestKeys.
 func Summarize(tk *TestKeys) (out Summary) {
+	// Make sure we correctly set out.Blocking's value.
+	defer func() {
+		out.Blocking = DetermineBlocking(out)
+	}()
 	var (
 		accessible   = true
 		inaccessible = false
@@ -65,10 +109,10 @@ func Summarize(tk *TestKeys) (out Summary) {
 	if tk.TCPConnectAttempts > 0 && tk.TCPConnectSuccesses <= 0 {
 		switch tk.DNSConsistency {
 		case DNSConsistent:
-			out.Blocking = &tcpIP
+			out.BlockingReason = &tcpIP
 			out.Accessible = &inaccessible
 		case DNSInconsistent:
-			out.Blocking = &dns
+			out.BlockingReason = &dns
 			out.Accessible = &inaccessible
 		default:
 			// this case should not happen with this implementation
@@ -93,37 +137,37 @@ func Summarize(tk *TestKeys) (out Summary) {
 		case modelx.FailureConnectionRefused:
 			// This is possibly because a subsequent connection to some
 			// other endpoint has been blocked. So tcp-ip.
-			out.Blocking = &tcpIP
+			out.BlockingReason = &tcpIP
 			out.Accessible = &inaccessible
 		case modelx.FailureConnectionReset:
 			// We don't currently support TLS failures and we don't have a
 			// way to know if it was during TLS or later. So, for now we are
 			// going to call this error condition an http-failure.
-			out.Blocking = &httpFailure
+			out.BlockingReason = &httpFailure
 			out.Accessible = &inaccessible
 		case modelx.FailureDNSNXDOMAINError:
 			// This is possibly because a subsequent resolution to
 			// some other domain name has been blocked.
-			out.Blocking = &dns
+			out.BlockingReason = &dns
 			out.Accessible = &inaccessible
 		case modelx.FailureEOFError:
 			// We have seen this happening with TLS handshakes as well as
 			// sometimes with HTTP blocking. So http-failure.
-			out.Blocking = &httpFailure
+			out.BlockingReason = &httpFailure
 			out.Accessible = &inaccessible
 		case modelx.FailureGenericTimeoutError:
 			// Alas, here we don't know whether it's connect or whether it's
 			// perhaps the TLS handshake. Yet, there is some common ground here
 			// that suddenly all our packets are discared at TCP/IP level.
-			out.Blocking = &tcpIP
+			out.BlockingReason = &tcpIP
 			out.Accessible = &inaccessible
 		case modelx.FailureSSLInvalidHostname,
 			modelx.FailureSSLInvalidCertificate,
 			modelx.FailureSSLUnknownAuthority:
 			// We treat these three cases equally. Misconfiguration is a bit
 			// less likely since we also checked with the control. Since there
 			// is no TLS, for now we're going to call this http-failure.
-			out.Blocking = &httpFailure
+			out.BlockingReason = &httpFailure
 			out.Accessible = &inaccessible
 		default:
 			// We have not been able to classify the error. Could this perhaps be
@@ -134,9 +178,9 @@ func Summarize(tk *TestKeys) (out Summary) {
 		// should not trust the resolver, then let's bet on the DNS. If the
 		// chain is longer, for now better to be conservative. (I would argue
 		// that with a lying DNS that's likely the culprit, honestly.)
-		if out.Blocking != nil && len(tk.Requests) == 1 &&
+		if out.BlockingReason != nil && len(tk.Requests) == 1 &&
 			tk.DNSConsistency == DNSInconsistent {
-			out.Blocking = &dns
+			out.BlockingReason = &dns
 		}
 		return
 	}
@@ -161,13 +205,13 @@ func Summarize(tk *TestKeys) (out Summary) {
 	// It seems we didn't get the expected web page. What now? Well, if
 	// the DNS does not seem trustworthy, let us blame it.
 	if tk.DNSConsistency == DNSInconsistent {
-		out.Blocking = &dns
+		out.BlockingReason = &dns
 		out.Accessible = &inaccessible
 		return
 	}
 	// The only remaining conclusion seems that the web page we have got
 	// doesn't match what we were expecting.
-	out.Blocking = &httpDiff
+	out.BlockingReason = &httpDiff
 	out.Accessible = &inaccessible
 	return
 }