Merge pull request #60 from measurement-kit/code_review

Changes after ooniprobe-ios code review
ooni · Jan 19, 2017 · c270fea · c270fea
2 parents 6a6c3e0 + 0ce45bb
commit c270fea
Show file tree

Hide file tree

Showing 7 changed files with 79 additions and 3,688 deletions.
diff --git a/.gitignore b/.gitignore
@@ -1,5 +1,5 @@
 .DS_Store
-ooniprobe.xcodeproj/project.xcworkspace/xcuserdata/
-ooniprobe.xcworkspace/xcuserdata/
-Podfile.lock
-Pods/
+/ooniprobe.xcodeproj/project.xcworkspace/xcuserdata/
+/ooniprobe.xcodeproj/xcuserdata
+/ooniprobe.xcworkspace/xcuserdata/
+/Pods/
diff --git a/Podfile b/Podfile
@@ -1,10 +1,8 @@
 platform :ios, '8.0'
 target 'ooniprobe' do
-#	pod 'measurement_kit', '~> 0.1.2.1'
-	pod 'measurement_kit',  
-		:git => 'https://github.com/measurement-kit/measurement-kit.git',
-		:branch => 'stable'
-	pod 'Toast'
-	pod 'sundown'
+    pod 'measurement_kit',
+        :git => 'https://github.com/measurement-kit/measurement-kit.git',
+        :branch => 'stable'
+    pod 'Toast'
+    pod 'sundown'
 end
-
diff --git a/Podfile.lock b/Podfile.lock
@@ -1,5 +1,5 @@
 PODS:
-  - measurement_kit (0.4.0-beta)
+  - measurement_kit (0.4.0-beta.1)
   - sundown (0.1.0)
   - Toast (3.1.0)
 
@@ -15,14 +15,14 @@ EXTERNAL SOURCES:
 
 CHECKOUT OPTIONS:
   measurement_kit:
-    :commit: f547d971b7a33f29cb77e0be6898b469d97d1928
+    :commit: bb12ec38315c8f8265d022fe7259b2e2db9c2b0f
     :git: https://github.com/measurement-kit/measurement-kit.git
 
 SPEC CHECKSUMS:
-  measurement_kit: 1f8ac2368a6eae1018b9ded4ae8e8525ccea4438
+  measurement_kit: a75a6a1e91b4ca61f975470ee9dbd21d027daf9f
   sundown: 240edd458462fd773a474023559622c3a8566b5e
   Toast: 14a93686d6c8bfe2727afd342414e35660a8a1f3
 
-PODFILE CHECKSUM: b1d8130a811b7b0a00f119d95a460f16367657b9
+PODFILE CHECKSUM: 841bc4f01160d0672dfa91d06217daba5129ea51
 
 COCOAPODS: 1.1.1
diff --git a/ooniprobe.xcodeproj/project.pbxproj b/ooniprobe.xcodeproj/project.pbxproj
@@ -27,7 +27,6 @@
 		ED25282D1CEA35640073A29B /* TestInfoViewController.m in Sources */ = {isa = PBXBuildFile; fileRef = ED2528271CEA35640073A29B /* TestInfoViewController.m */; };
 		ED25282E1CEA35640073A29B /* ViewController.mm in Sources */ = {isa = PBXBuildFile; fileRef = ED2528291CEA35640073A29B /* ViewController.mm */; };
 		ED2528311CEA35770073A29B /* NetworkMeasurement.mm in Sources */ = {isa = PBXBuildFile; fileRef = ED2528301CEA35770073A29B /* NetworkMeasurement.mm */; };
-		ED3AB3301D2337920061F266 /* cacert.pem in Resources */ = {isa = PBXBuildFile; fileRef = ED3AB32D1D2337920061F266 /* cacert.pem */; };
 		ED3AB3311D2337920061F266 /* GeoIP.dat in Resources */ = {isa = PBXBuildFile; fileRef = ED3AB32E1D2337920061F266 /* GeoIP.dat */; };
 		ED3AB3321D2337920061F266 /* GeoIPASNum.dat in Resources */ = {isa = PBXBuildFile; fileRef = ED3AB32F1D2337920061F266 /* GeoIPASNum.dat */; };
 		ED4D60561A7162C400777275 /* Localizable.strings in Resources */ = {isa = PBXBuildFile; fileRef = ED4D60541A7162C400777275 /* Localizable.strings */; };
@@ -99,7 +98,6 @@
 		ED2528291CEA35640073A29B /* ViewController.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = ViewController.mm; sourceTree = "<group>"; };
 		ED25282F1CEA35770073A29B /* NetworkMeasurement.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = NetworkMeasurement.h; path = Model/NetworkMeasurement.h; sourceTree = "<group>"; };
 		ED2528301CEA35770073A29B /* NetworkMeasurement.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = NetworkMeasurement.mm; path = Model/NetworkMeasurement.mm; sourceTree = "<group>"; };
-		ED3AB32D1D2337920061F266 /* cacert.pem */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = cacert.pem; sourceTree = "<group>"; };
 		ED3AB32E1D2337920061F266 /* GeoIP.dat */ = {isa = PBXFileReference; lastKnownFileType = file; path = GeoIP.dat; sourceTree = "<group>"; };
 		ED3AB32F1D2337920061F266 /* GeoIPASNum.dat */ = {isa = PBXFileReference; lastKnownFileType = file; path = GeoIPASNum.dat; sourceTree = "<group>"; };
 		ED4D60551A7162C400777275 /* Base */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = Base; path = Base.lproj/Localizable.strings; sourceTree = "<group>"; };
@@ -239,7 +237,6 @@
 				EDE32B431D5CCABB00D7B4CF /* webui */,
 				EDD22D101ADD6248006EA0C8 /* html */,
 				D4A2F62E1A6C33F7001B8460 /* fixtures */,
-				ED3AB32D1D2337920061F266 /* cacert.pem */,
 				ED3AB32E1D2337920061F266 /* GeoIP.dat */,
 				ED3AB32F1D2337920061F266 /* GeoIPASNum.dat */,
 			);
@@ -436,7 +433,6 @@
 				ED3AB3311D2337920061F266 /* GeoIP.dat in Resources */,
 				EDE32B5E1D5CCABB00D7B4CF /* 1dc35d25e61d819a9c357074014867ab.ttf in Resources */,
 				EDE32B6B1D5CCABB00D7B4CF /* c8ddf1e5e5bf3682bc7bebf30f394148.woff in Resources */,
-				ED3AB3301D2337920061F266 /* cacert.pem in Resources */,
 				EDE32B731D5CCABB00D7B4CF /* index.html in Resources */,
 				EDCB9D911CB3F9ED00BBA3BC /* DefaultPreferences.plist in Resources */,
 				EDE32B631D5CCABB00D7B4CF /* 78342dfad83c591ee5e926f2ffbd0671.woff in Resources */,

diff --git a/ooniprobe/Model/NetworkMeasurement.h b/ooniprobe/Model/NetworkMeasurement.h
@@ -5,16 +5,11 @@
 #import <Foundation/Foundation.h>
 #import <UIKit/UIKit.h>
 
-/*Include header from test*/
-#include <measurement_kit/ooni.hpp>
-#include <measurement_kit/nettests.hpp>
-
-#include <measurement_kit/common.hpp>
+#import <measurement_kit/common.hpp>
 
 @interface NetworkMeasurement : NSObject {
     NSString *geoip_asn;
     NSString *geoip_country;
-    NSString *ca_cert;
     bool include_ip;
     bool include_asn;
     bool include_cc;

diff --git a/ooniprobe/Model/NetworkMeasurement.mm b/ooniprobe/Model/NetworkMeasurement.mm
@@ -5,8 +5,9 @@
 #import "NetworkMeasurement.h"
 #import "TestStorage.h"
 
-#import "measurement_kit/common.hpp"
-#import "measurement_kit/ndt.hpp"
+#include <measurement_kit/ooni.hpp>
+#include <measurement_kit/nettests.hpp>
+#include <measurement_kit/ndt.hpp>
 
 #include <arpa/inet.h>
 #include <ifaddrs.h>
@@ -26,7 +27,7 @@ static void setup_idempotent() {
 }
 
 static std::string get_dns_server() {
-    std::string dns_server = "8.8.4.4:53";
+    std::string dns_server = "8.8.4.4";
     res_state res = nullptr;
     res = (res_state) malloc(sizeof(struct __res_state));
     if (res == nullptr) {
@@ -50,15 +51,16 @@ static void setup_idempotent() {
     return dns_server;
 }
 
-
 @implementation NetworkMeasurement
 
 -(id) init {
     self = [super init];
+    if (!self) {
+        return nil;
+    }
     NSBundle *bundle = [NSBundle mainBundle];
     geoip_asn = [bundle pathForResource:@"GeoIPASNum" ofType:@"dat"];
     geoip_country = [bundle pathForResource:@"GeoIP" ofType:@"dat"];
-    ca_cert = [bundle pathForResource:@"cacert" ofType:@"pem"];
     include_ip = [[[NSUserDefaults standardUserDefaults] objectForKey:@"include_ip"] boolValue];
     include_asn = [[[NSUserDefaults standardUserDefaults] objectForKey:@"include_asn"] boolValue];
     include_cc = [[[NSUserDefaults standardUserDefaults] objectForKey:@"include_cc"] boolValue];
@@ -73,13 +75,12 @@ -(void) run {
     // Nothing to do here
 }
 
-- (void)showNotification
-{
+- (void)showNotification {
     UILocalNotification* localNotification = [[UILocalNotification alloc] init];
     localNotification.fireDate = [NSDate date];
     localNotification.timeZone = [NSTimeZone defaultTimeZone];
     localNotification.alertBody = [NSString stringWithFormat:NSLocalizedString(@"finished_running", nil), NSLocalizedString(self.name, nil)];
-    [localNotification setApplicationIconBadgeNumber:[[UIApplication sharedApplication] applicationIconBadgeNumber]+1];
+    [localNotification setApplicationIconBadgeNumber:[[UIApplication sharedApplication] applicationIconBadgeNumber] + 1];
     [[UIApplication sharedApplication] presentLocalNotificationNow:localNotification];
 }
 
@@ -96,25 +97,7 @@ -(NSString*) getFileName:(NSString*)ext {
     return fileName;
 }
 
--(void)writeOrAppend:(NSString*)string{
-    NSString *fileName = [self getFileName:@"log"];
-    NSFileManager *fileManager = [NSFileManager defaultManager];
-    // Note: It is not optimal to open(), close(), and seek() each time
-    // but we agreed not to touch this code because MK should soon add the
-    // code to specify the file where to save log files.
-    if(![fileManager fileExistsAtPath:fileName])
-    {
-        [string writeToFile:fileName atomically:NO encoding:NSStringEncodingConversionAllowLossy error:nil];
-    }
-    else
-    {
-        NSFileHandle *myHandle = [NSFileHandle fileHandleForWritingAtPath:fileName];
-        [myHandle seekToEndOfFile];
-        [myHandle writeData:[[NSString stringWithFormat:@"\n%@",string] dataUsingEncoding:NSUTF8StringEncoding]];
-    }
-}
-
--(void)updateProgress:(double)prog{
+-(void)updateProgress:(double)prog {
     NSString *os = [NSString stringWithFormat:@"Progress: %.1f%%", prog * 100.0];
     self.progress = prog;
     NSLog(@"%@", os);
@@ -158,7 +141,7 @@ - (id)initWithCoder:(NSCoder *)coder {
 
 @end
 
-//NOT USED
+// NOT USED
 @implementation DNSInjection : NetworkMeasurement
 
 -(id) init {
@@ -188,7 +171,6 @@ - (void) run_test {
     mk::nettests::DnsInjectionTest()
         .set_options("backend", "8.8.8.1:53")
         .set_options("dns/nameserver", get_dns_server())
-        .set_options("net/ca_bundle_path", [ca_cert UTF8String])
         .set_options("geoip_country_path", [geoip_country UTF8String])
         .set_options("geoip_asn_path", [geoip_asn UTF8String])
         .set_options("save_real_probe_ip", include_ip)
@@ -204,7 +186,7 @@ - (void) run_test {
             [self updateProgress:prog];
         })
         .on_log([self](uint32_t type, const char *s) {
-                NSLog(@"%s", s);
+            NSLog(@"%s", s);
         })
         .start([self]() {
             [self testEnded];
@@ -221,7 +203,7 @@ -(id) init {
     return self;
 }
 
--(void)run{
+-(void)run {
     self.backgroundTask = [[UIApplication sharedApplication] beginBackgroundTaskWithExpirationHandler:^{
         [[UIApplication sharedApplication] endBackgroundTask:self.backgroundTask];
         self.backgroundTask = UIBackgroundTaskInvalid;
@@ -240,7 +222,6 @@ -(void) run_test {
     mk::nettests::HttpInvalidRequestLineTest()
         .set_options("backend", [HIRL_BACKEND UTF8String])
         .set_options("dns/nameserver", get_dns_server())
-        .set_options("net/ca_bundle_path", [ca_cert UTF8String])
         .set_options("geoip_country_path", [geoip_country UTF8String])
         .set_options("geoip_asn_path", [geoip_asn UTF8String])
         .set_options("save_real_probe_ip", include_ip)
@@ -255,7 +236,7 @@ -(void) run_test {
             [self updateProgress:prog];
         })
         .on_log([self](uint32_t type, const char *s) {
-                NSLog(@"%s", s);
+            NSLog(@"%s", s);
         })
         .start([self]() {
             [self testEnded];
@@ -264,7 +245,7 @@ -(void) run_test {
 
 @end
 
-//NOT USED
+// NOT USED
 @implementation TCPConnect : NetworkMeasurement
 
 -(id) init {
@@ -294,7 +275,6 @@ -(void) run_test {
     mk::nettests::TcpConnectTest()
         .set_options("port", 80)
         .set_options("dns/nameserver", get_dns_server())
-        .set_options("net/ca_bundle_path", [ca_cert UTF8String])
         .set_options("geoip_country_path", [geoip_country UTF8String])
         .set_options("geoip_asn_path", [geoip_asn UTF8String])
         .set_options("save_real_probe_ip", include_ip)
@@ -345,32 +325,35 @@ -(void) run_test {
     NSBundle *bundle = [NSBundle mainBundle];
     NSString *path = [bundle pathForResource:@"global" ofType:@"txt"];
     mk::nettests::WebConnectivityTest()
-    .set_options("backend", [WC_BACKEND UTF8String])
-    .set_options("port", 80)
-    .set_options("dns/nameserver", get_dns_server())
-    .set_options("nameserver", get_dns_server())
-    .set_options("geoip_country_path", [geoip_country UTF8String])
-    .set_options("geoip_asn_path", [geoip_asn UTF8String])
-    .set_options("net/ca_bundle_path", [ca_cert UTF8String])
-    .set_options("save_real_probe_ip", include_ip)
-    .set_options("save_real_probe_asn", include_asn)
-    .set_options("save_real_probe_cc", include_cc)
-    .set_options("no_collector", !upload_results)
-    .set_options("collector_base_url", [collector_address UTF8String])
-    .set_options("max_runtime", [max_runtime doubleValue])
-    .set_input_filepath([path UTF8String])
-    .set_error_filepath([[self getFileName:@"log"] UTF8String])
-    .set_output_filepath([[self getFileName:@"json"] UTF8String])
-    .set_verbosity(MK_LOG_INFO)
-    .on_progress([self](double prog, const char *s) {
-        [self updateProgress:prog];
-    })
-    .on_log([self](uint32_t type, const char *s) {
-        NSLog(@"%s", s);
-    })
-    .start([self]() {
-        [self testEnded];
-    });
+        .set_options("backend", [WC_BACKEND UTF8String])
+        /*
+         * XXX nameserver is the nameserver to be used by web connectivity to
+         * perform its DNS checks. In theory it may differ from dns/nameserver
+         * but, in practice, does it make sense to have two settings?
+         */
+        .set_options("dns/nameserver", get_dns_server())
+        .set_options("nameserver", get_dns_server())
+        .set_options("geoip_country_path", [geoip_country UTF8String])
+        .set_options("geoip_asn_path", [geoip_asn UTF8String])
+        .set_options("save_real_probe_ip", include_ip)
+        .set_options("save_real_probe_asn", include_asn)
+        .set_options("save_real_probe_cc", include_cc)
+        .set_options("no_collector", !upload_results)
+        .set_options("collector_base_url", [collector_address UTF8String])
+        .set_options("max_runtime", [max_runtime doubleValue])
+        .set_input_filepath([path UTF8String])
+        .set_error_filepath([[self getFileName:@"log"] UTF8String])
+        .set_output_filepath([[self getFileName:@"json"] UTF8String])
+        .set_verbosity(MK_LOG_INFO)
+        .on_progress([self](double prog, const char *s) {
+            [self updateProgress:prog];
+        })
+        .on_log([self](uint32_t type, const char *s) {
+            NSLog(@"%s", s);
+        })
+        .start([self]() {
+            [self testEnded];
+        });
 }
 
 @end
@@ -399,28 +382,27 @@ -(void) run_test {
     self.completed = FALSE;
     [TestStorage add_test:self];
     mk::nettests::NdtTest()
-    .set_options("test_suite", MK_NDT_DOWNLOAD)
-    .set_options("net/ca_bundle_path", [ca_cert UTF8String])
-    .set_options("dns/nameserver", get_dns_server())
-    .set_options("geoip_country_path", [geoip_country UTF8String])
-    .set_options("geoip_asn_path", [geoip_asn UTF8String])
-    .set_options("save_real_probe_ip", include_ip)
-    .set_options("save_real_probe_asn", include_asn)
-    .set_options("save_real_probe_cc", include_cc)
-    .set_options("no_collector", !upload_results)
-    .set_options("collector_base_url", [collector_address UTF8String])
-    .set_verbosity(MK_LOG_INFO)
-    .set_output_filepath([[self getFileName:@"json"] UTF8String])
-    .set_error_filepath([[self getFileName:@"log"] UTF8String])
-    .on_progress([self](double prog, const char *s) {
-        [self updateProgress:prog];
-    })
-    .on_log([self](uint32_t type, const char *s) {
-        NSLog(@"%s", s);
-    })
-    .start([self]() {
-        [self testEnded];
-    });
+        .set_options("test_suite", MK_NDT_DOWNLOAD | MK_NDT_UPLOAD)
+        .set_options("dns/nameserver", get_dns_server())
+        .set_options("geoip_country_path", [geoip_country UTF8String])
+        .set_options("geoip_asn_path", [geoip_asn UTF8String])
+        .set_options("save_real_probe_ip", include_ip)
+        .set_options("save_real_probe_asn", include_asn)
+        .set_options("save_real_probe_cc", include_cc)
+        .set_options("no_collector", !upload_results)
+        .set_options("collector_base_url", [collector_address UTF8String])
+        .set_verbosity(MK_LOG_INFO)
+        .set_output_filepath([[self getFileName:@"json"] UTF8String])
+        .set_error_filepath([[self getFileName:@"log"] UTF8String])
+        .on_progress([self](double prog, const char *s) {
+            [self updateProgress:prog];
+        })
+        .on_log([self](uint32_t type, const char *s) {
+            NSLog(@"%s", s);
+        })
+        .start([self]() {
+            [self testEnded];
+        });
 }
 
 @end