From 68b289326859efe95bfe183d517900c47dac4385 Mon Sep 17 00:00:00 2001 From: Bug Magnet Date: Fri, 13 Oct 2023 16:47:51 +0200 Subject: [PATCH] PR Feedback part 3 --- ios/MullvadVPN.xcodeproj/project.pbxproj | 20 +++++-- .../TunnelManager/StartTunnelOperation.swift | 2 +- ios/MullvadVPN/TunnelManager/Tunnel.swift | 14 ++--- .../TunnelManager/TunnelManager.swift | 4 +- .../TunnelManager/TunnelStore.swift | 27 +++++----- ios/MullvadVPNTests/AddressCacheTests.swift | 4 +- ios/MullvadVPNTests/TunnelStore+Stubs.swift | 11 ++-- .../Actor/State+Extensions.swift | 2 +- .../AppMessageHandlerTests.swift | 10 +++- .../PacketTunnelActorTests.swift | 53 ++++++++++++------- 10 files changed, 96 insertions(+), 51 deletions(-) diff --git a/ios/MullvadVPN.xcodeproj/project.pbxproj b/ios/MullvadVPN.xcodeproj/project.pbxproj index 07659ec5850f..3f6ab488ff17 100644 --- a/ios/MullvadVPN.xcodeproj/project.pbxproj +++ b/ios/MullvadVPN.xcodeproj/project.pbxproj @@ -494,6 +494,8 @@ A97F1F472A1F4E1A00ECEFDE /* MullvadTransport.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = A97F1F412A1F4E1A00ECEFDE /* MullvadTransport.framework */; }; A97F1F482A1F4E1A00ECEFDE /* MullvadTransport.framework in Embed Frameworks */ = {isa = PBXBuildFile; fileRef = A97F1F412A1F4E1A00ECEFDE /* MullvadTransport.framework */; settings = {ATTRIBUTES = (CodeSignOnCopy, RemoveHeadersOnCopy, ); }; }; A97FF5502A0D2FFC00900996 /* NSFileCoordinator+Extensions.swift in Sources */ = {isa = PBXBuildFile; fileRef = A97FF54F2A0D2FFC00900996 /* NSFileCoordinator+Extensions.swift */; }; + A988DF212ADD293D00D807EF /* RESTTransportStrategy.swift in Sources */ = {isa = PBXBuildFile; fileRef = A9A1DE782AD5708E0073F689 /* RESTTransportStrategy.swift */; }; + A988DF242ADD307200D807EF /* libRelaySelector.a in Frameworks */ = {isa = PBXBuildFile; fileRef = 5898D29829017DAC00EB5EBA /* libRelaySelector.a */; }; A9A1DE792AD5708E0073F689 /* RESTTransportStrategy.swift in Sources */ = {isa = PBXBuildFile; fileRef = A9A1DE782AD5708E0073F689 /* RESTTransportStrategy.swift */; }; A9A5F9E12ACB05160083449F /* AddressCacheTracker.swift in Sources */ = {isa = PBXBuildFile; fileRef = 06AC114028F841390037AF9A /* AddressCacheTracker.swift */; }; A9A5F9E22ACB05160083449F /* BackgroundTask.swift in Sources */ = {isa = PBXBuildFile; fileRef = 58C76A0A2A338E4300100D75 /* BackgroundTask.swift */; }; @@ -948,6 +950,13 @@ remoteGlobalIDString = A97F1F402A1F4E1A00ECEFDE; remoteInfo = MullvadTransport; }; + A988DF222ADD305300D807EF /* PBXContainerItemProxy */ = { + isa = PBXContainerItemProxy; + containerPortal = 58CE5E58224146200008646E /* Project object */; + proxyType = 1; + remoteGlobalIDString = 5898D29729017DAC00EB5EBA; + remoteInfo = RelaySelector; + }; A9B2CF702A1F64B20013CC6C /* PBXContainerItemProxy */ = { isa = PBXContainerItemProxy; containerPortal = 58CE5E58224146200008646E /* Project object */; @@ -1540,7 +1549,6 @@ A900E9BB2ACC609200C95F67 /* DevicesProxy+Stubs.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "DevicesProxy+Stubs.swift"; sourceTree = ""; }; A900E9BD2ACC654100C95F67 /* APIProxy+Stubs.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "APIProxy+Stubs.swift"; sourceTree = ""; }; A900E9BF2ACC661900C95F67 /* AccessTokenManager+Stubs.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "AccessTokenManager+Stubs.swift"; sourceTree = ""; }; - A917351E29FAA9C400D5DCFD /* RESTTransportStrategy.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RESTTransportStrategy.swift; sourceTree = ""; }; A917352029FAAA5200D5DCFD /* TransportStrategyTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TransportStrategyTests.swift; sourceTree = ""; }; A92ECC202A77FFAF0052F1B1 /* TunnelSettings.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TunnelSettings.swift; sourceTree = ""; }; A92ECC232A7802520052F1B1 /* StoredAccountData.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StoredAccountData.swift; sourceTree = ""; }; @@ -1691,6 +1699,7 @@ isa = PBXFrameworksBuildPhase; buildActionMask = 2147483647; files = ( + A988DF242ADD307200D807EF /* libRelaySelector.a in Frameworks */, A94D691B2ABAD66700413DD4 /* WireGuardKitTypes in Frameworks */, 58C7A43E2A863F470060C66F /* PacketTunnelCore.framework in Frameworks */, ); @@ -3283,6 +3292,7 @@ buildRules = ( ); dependencies = ( + A988DF232ADD305300D807EF /* PBXTargetDependency */, 58C7A4402A863F470060C66F /* PBXTargetDependency */, 58C7A4722A864B860060C66F /* PBXTargetDependency */, ); @@ -4118,7 +4128,6 @@ 58C7A4522A863FB50060C66F /* Pinger.swift in Sources */, 580D6B8C2AB3369300B2D6E0 /* BlockedStateErrorMapperProtocol.swift in Sources */, 58C7AF172ABD84AA007EDD7A /* ProxyURLRequest.swift in Sources */, - 58C7AF142ABD8480007EDD7A /* RelaySelectorResult+PacketTunnelRelay.swift in Sources */, 5838321F2AC3160A00EA2071 /* PacketTunnelActor+KeyPolicy.swift in Sources */, 58C7AF122ABD8480007EDD7A /* TunnelProviderReply.swift in Sources */, 58C7A4572A863FB90060C66F /* TunnelDeviceInfoProtocol.swift in Sources */, @@ -4531,7 +4540,7 @@ buildActionMask = 2147483647; files = ( 58B465702A98C53300467203 /* RequestExecutorTests.swift in Sources */, - A9A1DE792AD5708E0073F689 /* RESTTransportStrategy.swift in Sources */, + A988DF212ADD293D00D807EF /* RESTTransportStrategy.swift in Sources */, A917352129FAAA5200D5DCFD /* TransportStrategyTests.swift in Sources */, 58FBFBE9291622580020E046 /* ExponentialBackoffTests.swift in Sources */, 58BDEB9D2A98F69E00F578F2 /* MemoryCache.swift in Sources */, @@ -4802,6 +4811,11 @@ target = A97F1F402A1F4E1A00ECEFDE /* MullvadTransport */; targetProxy = A97F1F452A1F4E1A00ECEFDE /* PBXContainerItemProxy */; }; + A988DF232ADD305300D807EF /* PBXTargetDependency */ = { + isa = PBXTargetDependency; + target = 5898D29729017DAC00EB5EBA /* RelaySelector */; + targetProxy = A988DF222ADD305300D807EF /* PBXContainerItemProxy */; + }; A9B2CF712A1F64B20013CC6C /* PBXTargetDependency */ = { isa = PBXTargetDependency; target = 06799ABB28F98E1D00ACD94E /* MullvadREST */; diff --git a/ios/MullvadVPN/TunnelManager/StartTunnelOperation.swift b/ios/MullvadVPN/TunnelManager/StartTunnelOperation.swift index da7b14d7bbcb..86dce49a3a9e 100644 --- a/ios/MullvadVPN/TunnelManager/StartTunnelOperation.swift +++ b/ios/MullvadVPN/TunnelManager/StartTunnelOperation.swift @@ -85,7 +85,7 @@ class StartTunnelOperation: ResultOperation { } } - private func startTunnel(tunnel: Tunnel, selectedRelay: SelectedRelay) throws { + private func startTunnel(tunnel: any TunnelProtocol, selectedRelay: SelectedRelay) throws { var tunnelOptions = PacketTunnelOptions() do { diff --git a/ios/MullvadVPN/TunnelManager/Tunnel.swift b/ios/MullvadVPN/TunnelManager/Tunnel.swift index 9edb3f1bc700..da4359c3c5d9 100644 --- a/ios/MullvadVPN/TunnelManager/Tunnel.swift +++ b/ios/MullvadVPN/TunnelManager/Tunnel.swift @@ -25,8 +25,10 @@ protocol TunnelProtocol: AnyObject { var isOnDemandEnabled: Bool { get set } var startDate: Date? { get } - func addObserver(_ observer: TunnelStatusObserver) - func removeObserver(_ observer: TunnelStatusObserver) + init(tunnelProvider: TunnelProviderManagerType) + + func addObserver(_ observer: any TunnelStatusObserver) + func removeObserver(_ observer: any TunnelStatusObserver) func addBlockObserver( queue: DispatchQueue?, handler: @escaping (any TunnelProtocol, NEVPNStatus) -> Void @@ -105,7 +107,7 @@ final class Tunnel: TunnelProtocol, Equatable { } private let lock = NSLock() - private var observerList = ObserverList() + private var observerList = ObserverList() private var _startDate: Date? internal let tunnelProvider: TunnelProviderManagerType @@ -169,11 +171,11 @@ final class Tunnel: TunnelProtocol, Equatable { return observer } - func addObserver(_ observer: TunnelStatusObserver) { + func addObserver(_ observer: any TunnelStatusObserver) { observerList.append(observer) } - func removeObserver(_ observer: TunnelStatusObserver) { + func removeObserver(_ observer: any TunnelStatusObserver) { observerList.remove(observer) } @@ -185,7 +187,7 @@ final class Tunnel: TunnelProtocol, Equatable { handleVPNStatus(newStatus) observerList.forEach { observer in - observer.tunnel(self, didReceiveStatus: newStatus) +// observer.tunnel(self, didReceiveStatus: newStatus) } } diff --git a/ios/MullvadVPN/TunnelManager/TunnelManager.swift b/ios/MullvadVPN/TunnelManager/TunnelManager.swift index 4a2dd07a3df9..ab178dba380d 100644 --- a/ios/MullvadVPN/TunnelManager/TunnelManager.swift +++ b/ios/MullvadVPN/TunnelManager/TunnelManager.swift @@ -45,7 +45,7 @@ final class TunnelManager: StorePaymentObserver { // MARK: - Internal variables private let application: BackgroundTaskProvider - fileprivate let tunnelStore: TunnelStoreProtocol + fileprivate let tunnelStore: any TunnelStoreProtocol private let relayCacheTracker: RelayCacheTrackerProtocol private let accountsProxy: RESTAccountHandling private let devicesProxy: DeviceHandling @@ -82,7 +82,7 @@ final class TunnelManager: StorePaymentObserver { init( application: BackgroundTaskProvider, - tunnelStore: TunnelStoreProtocol, + tunnelStore: any TunnelStoreProtocol, relayCacheTracker: RelayCacheTrackerProtocol, accountsProxy: RESTAccountHandling, devicesProxy: DeviceHandling, diff --git a/ios/MullvadVPN/TunnelManager/TunnelStore.swift b/ios/MullvadVPN/TunnelManager/TunnelStore.swift index f750ea9c4f8e..b93c33ac45c2 100644 --- a/ios/MullvadVPN/TunnelManager/TunnelStore.swift +++ b/ios/MullvadVPN/TunnelManager/TunnelStore.swift @@ -12,20 +12,22 @@ import NetworkExtension import UIKit protocol TunnelStoreProtocol { - func getPersistentTunnels() -> [any TunnelProtocol] - func createNewTunnel() -> any TunnelProtocol + associatedtype TunnelType: TunnelProtocol, Equatable + func getPersistentTunnels() -> [TunnelType] + func createNewTunnel() -> TunnelType } /// Wrapper around system VPN tunnels. final class TunnelStore: TunnelStoreProtocol, TunnelStatusObserver { + typealias TunnelType = Tunnel private let logger = Logger(label: "TunnelStore") private let lock = NSLock() /// Persistent tunnels registered with the system. - private var persistentTunnels: [any TunnelProtocol] = [] + private var persistentTunnels: [TunnelType] = [] /// Newly created tunnels, stored as collection of weak boxes. - private var newTunnels: [WeakBox] = [] + private var newTunnels: [WeakBox] = [] init(application: UIApplication) { NotificationCenter.default.addObserver( @@ -36,7 +38,7 @@ final class TunnelStore: TunnelStoreProtocol, TunnelStatusObserver { ) } - func getPersistentTunnels() -> [any TunnelProtocol] { + func getPersistentTunnels() -> [TunnelType] { lock.lock() defer { lock.unlock() } @@ -71,12 +73,12 @@ final class TunnelStore: TunnelStoreProtocol, TunnelStatusObserver { } } - func createNewTunnel() -> any TunnelProtocol { + func createNewTunnel() -> TunnelType { lock.lock() defer { lock.unlock() } let tunnelProviderManager = TunnelProviderManagerType() - let tunnel = Tunnel(tunnelProvider: tunnelProviderManager) + let tunnel = TunnelType(tunnelProvider: tunnelProviderManager) tunnel.addObserver(self) newTunnels = newTunnels.filter { $0.value != nil } @@ -91,20 +93,19 @@ final class TunnelStore: TunnelStoreProtocol, TunnelStatusObserver { lock.lock() defer { lock.unlock() } - handleTunnelStatus(tunnel: tunnel, status: status) + // swiftlint:disable:next force_cast + handleTunnelStatus(tunnel: tunnel as! TunnelType, status: status) } - private func handleTunnelStatus(tunnel: any TunnelProtocol, status: NEVPNStatus) { - guard let tunnel = tunnel as? Tunnel else { return } - + private func handleTunnelStatus(tunnel: TunnelType, status: NEVPNStatus) { if status == .invalid, - let index = persistentTunnels.map({ $0 as? Tunnel }).firstIndex(of: tunnel) { + let index = persistentTunnels.firstIndex(of: tunnel) { persistentTunnels.remove(at: index) logger.debug("Persistent tunnel was removed: \(tunnel.logFormat()).") } if status != .invalid, - let index = newTunnels.map({ $0.value as? Tunnel }).firstIndex(where: { $0 == tunnel }) { + let index = newTunnels.compactMap({ $0.value }).firstIndex(where: { $0 == tunnel }) { newTunnels.remove(at: index) persistentTunnels.append(tunnel) logger.debug("New tunnel became persistent: \(tunnel.logFormat()).") diff --git a/ios/MullvadVPNTests/AddressCacheTests.swift b/ios/MullvadVPNTests/AddressCacheTests.swift index 5a5dedfe1acb..898feb085069 100644 --- a/ios/MullvadVPNTests/AddressCacheTests.swift +++ b/ios/MullvadVPNTests/AddressCacheTests.swift @@ -45,7 +45,9 @@ final class AddressCacheTests: XCTestCase { addressCache.setEndpoints([apiEndpoint]) let dateAfterUpdate = addressCache.getLastUpdateDate() - XCTAssertNotEqual(dateBeforeUpdate, dateAfterUpdate) + let timeDifference = dateAfterUpdate.timeIntervalSince(dateBeforeUpdate) + + XCTAssertNotEqual(0.0, timeDifference) } func testSetEndpointsDoesNotDoAnythingIfSettingEmptyEndpoints() throws { diff --git a/ios/MullvadVPNTests/TunnelStore+Stubs.swift b/ios/MullvadVPNTests/TunnelStore+Stubs.swift index d9a15f68146b..546583942628 100644 --- a/ios/MullvadVPNTests/TunnelStore+Stubs.swift +++ b/ios/MullvadVPNTests/TunnelStore+Stubs.swift @@ -10,11 +10,12 @@ import Foundation import NetworkExtension struct TunnelStoreStub: TunnelStoreProtocol { - func getPersistentTunnels() -> [any TunnelProtocol] { + typealias TunnelType = TunnelStub + func getPersistentTunnels() -> [TunnelType] { [] } - func createNewTunnel() -> any TunnelProtocol { + func createNewTunnel() -> TunnelType { TunnelStub(status: .invalid, isOnDemandEnabled: false) } } @@ -23,7 +24,11 @@ class DummyTunnelStatusObserver: TunnelStatusObserver { func tunnel(_ tunnel: any TunnelProtocol, didReceiveStatus status: NEVPNStatus) {} } -class TunnelStub: TunnelProtocol { +final class TunnelStub: TunnelProtocol, Equatable { + convenience init(tunnelProvider: TunnelProviderManagerType) { + self.init(status: .invalid, isOnDemandEnabled: false) + } + static func == (lhs: TunnelStub, rhs: TunnelStub) -> Bool { ObjectIdentifier(lhs) == ObjectIdentifier(rhs) } diff --git a/ios/PacketTunnelCore/Actor/State+Extensions.swift b/ios/PacketTunnelCore/Actor/State+Extensions.swift index 8e0e715dd23e..6e6d40bfb760 100644 --- a/ios/PacketTunnelCore/Actor/State+Extensions.swift +++ b/ios/PacketTunnelCore/Actor/State+Extensions.swift @@ -1,5 +1,5 @@ // -// State+.swift +// State+Extensions.swift // PacketTunnelCore // // Created by pronebird on 08/09/2023. diff --git a/ios/PacketTunnelCoreTests/AppMessageHandlerTests.swift b/ios/PacketTunnelCoreTests/AppMessageHandlerTests.swift index 59550a183010..bea5a83e8b02 100644 --- a/ios/PacketTunnelCoreTests/AppMessageHandlerTests.swift +++ b/ios/PacketTunnelCoreTests/AppMessageHandlerTests.swift @@ -81,14 +81,20 @@ final class AppMessageHandlerTests: XCTestCase { let appMessageHandler = createAppMessageHandler(actor: actor) let relayConstraints = RelayConstraints(location: .only(.hostname("se", "sto", "se6-wireguard"))) - let selectorResult = try? RelaySelector.evaluate( + let selectorResult = try XCTUnwrap(try? RelaySelector.evaluate( relays: ServerRelaysResponseStubs.sampleRelays, constraints: relayConstraints, numberOfFailedAttempts: 0 + )) + + let selectedRelay = SelectedRelay( + endpoint: selectorResult.endpoint, + hostname: selectorResult.relay.hostname, + location: selectorResult.location ) _ = try? await appMessageHandler.handleAppMessage( - TunnelProviderMessage.reconnectTunnel(selectorResult).encode() + TunnelProviderMessage.reconnectTunnel(.preSelected(selectedRelay)).encode() ) await fulfillment(of: [reconnectExpectation], timeout: 1) diff --git a/ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift b/ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift index 39b6078f2b58..969d9c70df95 100644 --- a/ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift +++ b/ios/PacketTunnelCoreTests/PacketTunnelActorTests.swift @@ -185,14 +185,18 @@ final class PacketTunnelActorTests: XCTestCase { func testStopIsNoopBeforeStart() async throws { let actor = PacketTunnelActor.mock() + let disconnectedExpectation = expectation(description: "Disconnected state") + disconnectedExpectation.isInverted = true + + await expect(.disconnected, on: actor) { + disconnectedExpectation.fulfill() + } + actor.stop() actor.stop() actor.stop() - switch await actor.state { - case .initial: break - default: XCTFail("Actor did not start, should be in .initial state") - } + await fulfillment(of: [disconnectedExpectation], timeout: Duration.milliseconds(100).timeInterval) } func testStopCancelsDefaultPathObserver() async throws { @@ -226,6 +230,8 @@ final class PacketTunnelActorTests: XCTestCase { let actor = PacketTunnelActor.mock() let connectingStateExpectation = expectation(description: "Connecting state") let disconnectedStateExpectation = expectation(description: "Disconnected state") + let errorStateExpectation = expectation(description: "Should not enter error state") + errorStateExpectation.isInverted = true stateSink = await actor.$state .receive(on: DispatchQueue.main) @@ -235,7 +241,7 @@ final class PacketTunnelActorTests: XCTestCase { actor.setErrorState(reason: .readSettings) connectingStateExpectation.fulfill() case .error: - XCTFail("Should not go to error state") + errorStateExpectation.fulfill() case .disconnected: disconnectedStateExpectation.fulfill() default: @@ -245,24 +251,28 @@ final class PacketTunnelActorTests: XCTestCase { actor.start(options: launchOptions) actor.stop() + await fulfillment(of: [connectingStateExpectation, disconnectedStateExpectation], timeout: 1) + await fulfillment(of: [errorStateExpectation], timeout: Duration.milliseconds(100).timeInterval) } func testReconnectIsNoopBeforeConnecting() async throws { let actor = PacketTunnelActor.mock() - let initialStateExpectation = expectation(description: "Expect initial state") + let reconnectingStateExpectation = expectation(description: "Expect initial state") + reconnectingStateExpectation.isInverted = true - stateSink = await actor.$state.receive(on: DispatchQueue.main).sink { newState in - if case .initial = newState { - initialStateExpectation.fulfill() - return - } - XCTFail("Should not change states before starting the actor") + let expression: (State) -> Bool = { if case .reconnecting = $0 { true } else { false } } + + await expect(expression, on: actor) { + reconnectingStateExpectation.fulfill() } actor.reconnect(to: .random) - await fulfillment(of: [initialStateExpectation], timeout: 1) + await fulfillment( + of: [reconnectingStateExpectation], + timeout: Duration.milliseconds(100).timeInterval + ) } func testCannotReconnectAfterStopping() async throws { @@ -270,19 +280,24 @@ final class PacketTunnelActorTests: XCTestCase { let disconnectedStateExpectation = expectation(description: "Expect disconnected state") - await expect(.disconnected, on: actor) { - disconnectedStateExpectation.fulfill() - } + await expect(.disconnected, on: actor) { disconnectedStateExpectation.fulfill() } actor.start(options: launchOptions) actor.stop() await fulfillment(of: [disconnectedStateExpectation], timeout: 1) - await expect(.initial, on: actor) { - XCTFail("Should not be trying to reconnect after stopping") - } + let reconnectingStateExpectation = expectation(description: "Expect initial state") + reconnectingStateExpectation.isInverted = true + let expression: (State) -> Bool = { if case .reconnecting = $0 { true } else { false } } + + await expect(expression, on: actor) { reconnectingStateExpectation.fulfill() } + actor.reconnect(to: .random) + await fulfillment( + of: [reconnectingStateExpectation], + timeout: Duration.milliseconds(100).timeInterval + ) } func testReconnectionStopsTunnelMonitor() async throws {