Add iOS test for connecting to API via bridges

[niklasberglund]

This PR introduces the test testAPIConnectionViaBridges which blocks API access and makes sure the app can still successfully function. It also contains some more or less related improvements:

• The connectivity check for ad blocking test now checks whether host is connectable on port 80 instead of attempting to send HTTP requests
• Waiting for successful connection to relay in testAdBlockingViaDNS - sometimes the connectivity check happened before connected to relay
• Dropped MULLVAD_ and Mullvad prefix before configuration and plist entries.

To test running testAPIConnectionViaBridges introduced in this PR you have to be connected to the app-team-ios-tests wifi at the office. It's a wifi running on the NUC where the firewall API is also running. The tests use the wifi and firewall API to block traffic.

You also need to configure UITests.xcconfig. Note that some variable names have changed(removed MULLVAD_ prefix). TEST_DEVICE_IDENTIFIER_UUID´ needs to have a value set. Can be generated with uuidgen. FIREWALL_API_BASE_URL` is also introduced, but it's value should not be changed.

---

This change is 

[linear]

IOS-438 Test API connection via alternative means

[buggmagnet]

Reviewed 17 of 18 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @niklasberglund)

---

ios/Configurations/UITests.xcconfig.template line 19 at r2 (raw file):

// Mullvad accounts used by UI tests
NO_TIME_ACCOUNT_NUMBER = 

nit
Let's make sure we don't rename those too often

---

ios/Configurations/UITests.xcconfig.template line 24 at r2 (raw file):

 
// Base URL for the firewall API, Note that // will be treated as a comment, therefor you need to insert a ${} between the slashes for example http:/${}/8.8.8.8
FIREWALL_API_BASE_URL = http:/${}/8.8.8.8

Why not have it as part of the Info.plist file then ?
It seems hacky to have to do this

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 3367 at r2 (raw file):

			children = (
				8518F6392B601910009EB113 /* Test base classes */,
				85557B0C2B591B0F00795FE1 /* Networking */,

Don't forget to sort by name in the organiser

---

ios/MullvadVPNUITests/Networking/FirewallAPIClient.swift line 26 at r2 (raw file):

    /// Get IP address of the iOS device under test
    static func getIPAddress() -> String? {

This doesn't have to be static

---

ios/MullvadVPNUITests/Networking/FirewallAPIClient.swift line 34 at r2 (raw file):

        // For each interface
        for ptr in sequence(first: firstAddr, next: { $0.pointee.ifa_next }) {

nit
Pretty cool use of sequence to deal with linked lists ! 👍

---

ios/MullvadVPNUITests/Networking/FirewallAPIClient.swift line 36 at r2 (raw file):

        for ptr in sequence(first: firstAddr, next: { $0.pointee.ifa_next }) {
            let flags = Int32(ptr.pointee.ifa_flags)
            let addr = ptr.pointee.ifa_addr.pointee

Let's use reasonable names for readability's sake, also we try not to use abbreviations as per the Swift API Design Guidelines

---

ios/MullvadVPNUITests/Networking/FirewallAPIClient.swift line 109 at r2 (raw file):

            if waitResult != .completed {
                fatalError("Failed to create firewall rule - timeout")

Please avoid using fatalError
Even if the firewall is down, some tests can still run, it doesn't make sense to abort an entire test run because it was not accessible.
Instead, we could make this a throwing function and throw an error here.

The same comment applies for other uses of fatalError

---

ios/MullvadVPNUITests/Networking/FirewallRule.swift line 12 at r2 (raw file):

import XCTest

class FirewallRule {

This can be a struct instead, it has no logic, therefore it doesn't need to be a class

---

ios/MullvadVPNUITests/Networking/FirewallRule.swift line 21 at r2 (raw file):

    ///     - toIPAddress: Block traffic to this destination IP address.
    ///     - protocols: Protocols which should be blocked. Valid values: `tcp`, `udp`, `icmp`. If none is specified all will be blocked.
    private init(fromIPAddress: String, toIPAddress: String, protocols: [String]) {

For a nicer API use, we could define an enum for the protocols backed by a String, also because strong typing FTW

---

ios/MullvadVPNUITests/Networking/FirewallRule.swift line 28 at r2 (raw file):

    /// Make a firewall rule blocking API access for the current device under test
    public static func makeBlockAPIAccessFirewallRule() -> FirewallRule {

This also doesn't need to be static

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 11 at r2 (raw file):

import Foundation

class MullvadAPIWrapper {

This can be a struct

[niklasberglund]

Reviewable status: 12 of 18 files reviewed, 11 unresolved discussions (waiting on @buggmagnet)

---

ios/Configurations/UITests.xcconfig.template line 19 at r2 (raw file):

Previously, buggmagnet wrote…

nit
Let's make sure we don't rename those too often

Agree. At first for some reason I thought other attributes had a MULLVAD_ prefix. But later realised they don't so I corrected my mistake and removed all MULLVAD_ prefixes.

---

ios/Configurations/UITests.xcconfig.template line 24 at r2 (raw file):

Previously, buggmagnet wrote…

Why not have it as part of the Info.plist file then ?
It seems hacky to have to do this

It does look hacky. But if some configuration values are in .xcconfig files and some in Info.plist it might be difficult to keep track of where to modify what configuration values? Another way to get rid of the hackiness could be to not include http:// in the configuration value, so it'd be something like FIREWALL_API_HOST rather than FIREWALL_API_BASE_URL.

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 3367 at r2 (raw file):

Previously, buggmagnet wrote…

Don't forget to sort by name in the organiser

Sorted by name now 👍

---

ios/MullvadVPNUITests/Networking/FirewallAPIClient.swift line 26 at r2 (raw file):

Previously, buggmagnet wrote…

This doesn't have to be static

Discussed on Slack but not sure if we reached any conclusion on this. getIPAddress() is very standalone and not tied to an instance of the firewall API client.

---

ios/MullvadVPNUITests/Networking/FirewallAPIClient.swift line 34 at r2 (raw file):

Previously, buggmagnet wrote…

nit
Pretty cool use of sequence to deal with linked lists ! 👍

Credit goes to ChatGPT-4

---

ios/MullvadVPNUITests/Networking/FirewallAPIClient.swift line 36 at r2 (raw file):

Previously, buggmagnet wrote…

Let's use reasonable names for readability's sake, also we try not to use abbreviations as per the Swift API Design Guidelines

Ah yes agree. ChatGPT helped with this function, I just made some small modifications but totally fortgot to fix the variable names. Have updated to more descriptive names.

---

ios/MullvadVPNUITests/Networking/FirewallAPIClient.swift line 109 at r2 (raw file):

Previously, buggmagnet wrote…

Please avoid using fatalError
Even if the firewall is down, some tests can still run, it doesn't make sense to abort an entire test run because it was not accessible.
Instead, we could make this a throwing function and throw an error here.

The same comment applies for other uses of fatalError

Yes you're right. The current test should be aborted but not the entire test run. fatalError is way too harsh, have updated the approach - using XCTFail() for descriptive messages and force unwrapping which will fail if something crucial for the test is missing.

---

ios/MullvadVPNUITests/Networking/FirewallRule.swift line 12 at r2 (raw file):

Previously, buggmagnet wrote…

This can be a struct instead, it has no logic, therefore it doesn't need to be a class

Very true. Converted to struct.

---

ios/MullvadVPNUITests/Networking/FirewallRule.swift line 21 at r2 (raw file):

Previously, buggmagnet wrote…

For a nicer API use, we could define an enum for the protocols backed by a String, also because strong typing FTW

enumified it 👍

---

ios/MullvadVPNUITests/Networking/FirewallRule.swift line 28 at r2 (raw file):

Previously, buggmagnet wrote…

This also doesn't need to be static

Discussed on Slack

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 11 at r2 (raw file):

Previously, buggmagnet wrote…

This can be a struct

Discussed on Slack

[buggmagnet]

Reviewed 5 of 6 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @niklasberglund)

---

ios/MullvadVPNUITests/Networking/FirewallAPIClient.swift line 26 at r2 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Discussed on Slack but not sure if we reached any conclusion on this. getIPAddress() is very standalone and not tied to an instance of the firewall API client.

Static functions tend to be vulnerable to thread safety if they have to store a buffer locally, so usually it's bad practice to use static functions (and it makes code harder to refactor)

This one should be fine however.

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 26 at r4 (raw file):

    }

    public static func getAPIIPAddress() -> String? {

Instead of returning an optional, it's a better developper experience if this throws instead

If the function is to fail, the encompassign tests should abort because the thrown error.

    enum APIWrapperError: Error {
        case invalidIPFormat
    }
    
    public static func getAPIIPAddress2() throws -> String {
        guard let ipAddress = endpoint.components(separatedBy: ":").first else {
            throw APIWrapperError.invalidIPFormat
        }
        return ipAddress
    }

...

    public static func verifyCanAccessAPI2() throws {
        let apiIPAddress = try MullvadAPIWrapper.getAPIIPAddress2()
        /// .. the rest of things
    }


// In the test

func testMyTest() throws {
    try NetworkTester.verifyCanAccessAPI2()

The call site doesn't have to manually to do if checks, and automatically fails if an error is thrown.

This would also avoid all the force unwraps we are trying to avoid in the first place

[niklasberglund]

Reviewable status: 11 of 20 files reviewed, 5 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPN.xcodeproj/xcshareddata/xcschemes/MullvadVPNUITests.xcscheme line 68 at r5 (raw file):

            ReferencedContainer = "container:MullvadVPN.xcodeproj">
         </BuildableReference>
      </MacroExpansion>

Snuck this small scheme change in to make the MullvadVPNUITests target runnable, this way it can be built without running tests.

---

ios/MullvadVPNUITests/Networking/MullvadAPIWrapper.swift line 26 at r4 (raw file):

Previously, buggmagnet wrote…

Instead of returning an optional, it's a better developper experience if this throws instead

If the function is to fail, the encompassign tests should abort because the thrown error.

    enum APIWrapperError: Error {
        case invalidIPFormat
    }
    
    public static func getAPIIPAddress2() throws -> String {
        guard let ipAddress = endpoint.components(separatedBy: ":").first else {
            throw APIWrapperError.invalidIPFormat
        }
        return ipAddress
    }

...

    public static func verifyCanAccessAPI2() throws {
        let apiIPAddress = try MullvadAPIWrapper.getAPIIPAddress2()
        /// .. the rest of things
    }


// In the test

func testMyTest() throws {
    try NetworkTester.verifyCanAccessAPI2()

The call site doesn't have to manually to do if checks, and automatically fails if an error is thrown.

This would also avoid all the force unwraps we are trying to avoid in the first place

Throwing and not handling looks a bit more intentional 👍 Have updated to throw errors instead. The error throwing also highlighted the awkwardness of getIPAddress() in FirewallAPIClient so I did a small refactor - moved it to a class Networking where now the methods from NetworkTester also reside.

[buggmagnet]

Reviewable status: 11 of 20 files reviewed, 6 unresolved discussions (waiting on @niklasberglund)

---

ios/Configurations/UITests.xcconfig.template line 8 at r5 (raw file):

//  Copyright © 2024 Mullvad VPN AB. All rights reserved.
//

This template is missing the includes for the base config

---

ios/MullvadVPNUITests/Networking/Networking.swift line 60 at r5 (raw file):

                        NI_NUMERICHOST
                    )
                    ipAddress = String(cString: hostname)

As we have seen during debug, this will output an iPv6 address that include the interface the IP address is scoped to (i.e. fe80::1%ne0)

[niklasberglund]

Reviewable status: 10 of 20 files reviewed, 6 unresolved discussions (waiting on @buggmagnet)

---

ios/Configurations/UITests.xcconfig.template line 8 at r5 (raw file):

Previously, buggmagnet wrote…

This template is missing the includes for the base config

Added include to the template file. Also removed the header because other xcconfig files don't have it, and reordered attributes a bit so the attributes that do not need to be changed are in the bottom of the file.

---

ios/MullvadVPNUITests/Networking/Networking.swift line 60 at r5 (raw file):

Previously, buggmagnet wrote…

As we have seen during debug, this will output an iPv6 address that include the interface the IP address is scoped to (i.e. fe80::1%ne0)

The issue was that both sa_famil AF_INET(IPv4) and AF_INET6(IPv6) was accepted. Have pushed a fix.

[buggmagnet]

Reviewed 8 of 9 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @niklasberglund)

---

ios/MullvadVPN.xcodeproj/xcshareddata/xcschemes/MullvadVPNUITests.xcscheme line 68 at r5 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

Snuck this small scheme change in to make the MullvadVPNUITests target runnable, this way it can be built without running tests.

neat !

---

ios/MullvadVPNUITests/Networking/Networking.swift line 89 at r6 (raw file):

    private static func canConnectSocket(host: String, port: String) -> Bool {
        let socketHost = NWEndpoint.Host(host)
        let socketPort = NWEndpoint.Port(port)!

Let's make this not crashing at runtime

guard let socketPort = NWEndpoint.Port(port) else { return false }

---

ios/MullvadVPNUITests/Networking/Networking.swift line 142 at r6 (raw file):

    }

    /// Verify that an ad serving domain is NOT reachable by making sure a connection can be established on port 80

nit
can not be established

[niklasberglund]

Reviewable status: 19 of 20 files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPNUITests/Networking/Networking.swift line 89 at r6 (raw file):

Previously, buggmagnet wrote…

Let's make this not crashing at runtime

guard let socketPort = NWEndpoint.Port(port) else { return false }

A silent fail could lead to false test results. The crashing is intentional. How about make it more descriptive by using XCTFail?

guard let socketPort \= NWEndpoint.Port(port) else { XCTFail("Failed to set port") }

---

ios/MullvadVPNUITests/Networking/Networking.swift line 142 at r6 (raw file):

Previously, buggmagnet wrote…

nit
can not be established

Ohh I flipped it. Have flipped it back now.

[niklasberglund]

Reviewable status: 18 of 20 files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPNUITests/Networking/Networking.swift line 89 at r6 (raw file):

Previously, niklasberglund (Niklas Berglund) wrote…

A silent fail could lead to false test results. The crashing is intentional. How about make it more descriptive by using XCTFail?

guard let socketPort \= NWEndpoint.Port(port) else { XCTFail("Failed to set port") }

Updated code to use XCTUnwrap

[buggmagnet]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @niklasberglund)