From d06ce88c77ca7ff24845a5f1430a75fbec8fe17d Mon Sep 17 00:00:00 2001 From: Bug Magnet Date: Mon, 2 Sep 2024 16:13:31 +0200 Subject: [PATCH] Do not instantiate tunnel settings in RelaySelectorWrapper --- .../MullvadREST/RelaySelectorStub.swift | 6 +- .../Relay/RelaySelectorProtocol.swift | 5 +- .../Relay/RelaySelectorWrapper.swift | 25 +---- ios/MullvadVPN/AppDelegate.swift | 3 +- .../SimulatorTunnelProviderHost.swift | 6 +- .../TunnelManager/TunnelManager.swift | 5 +- .../Relay/RelaySelectorWrapperTests.swift | 102 +++++++----------- .../PacketTunnelActorReducerTests.swift | 13 ++- .../TunnelManager/TunnelManagerTests.swift | 10 +- .../PacketTunnelProvider.swift | 3 +- .../Actor/PacketTunnelActor.swift | 8 +- 11 files changed, 87 insertions(+), 99 deletions(-) diff --git a/ios/MullvadMockData/MullvadREST/RelaySelectorStub.swift b/ios/MullvadMockData/MullvadREST/RelaySelectorStub.swift index 8fc6ba91415e..b2dbe171e1c7 100644 --- a/ios/MullvadMockData/MullvadREST/RelaySelectorStub.swift +++ b/ios/MullvadMockData/MullvadREST/RelaySelectorStub.swift @@ -7,6 +7,7 @@ // import MullvadREST +import MullvadSettings import MullvadTypes import WireGuardKitTypes @@ -18,7 +19,10 @@ public final class RelaySelectorStub: RelaySelectorProtocol { self.selectedRelaysResult = selectedRelaysResult } - public func selectRelays(connectionAttemptCount: UInt) throws -> SelectedRelays { + public func selectRelays( + tunnelSettings: LatestTunnelSettings, + connectionAttemptCount: UInt + ) throws -> SelectedRelays { return try selectedRelaysResult(connectionAttemptCount) } } diff --git a/ios/MullvadREST/Relay/RelaySelectorProtocol.swift b/ios/MullvadREST/Relay/RelaySelectorProtocol.swift index 44a01f8dc804..92d48a823938 100644 --- a/ios/MullvadREST/Relay/RelaySelectorProtocol.swift +++ b/ios/MullvadREST/Relay/RelaySelectorProtocol.swift @@ -12,7 +12,10 @@ import MullvadTypes /// Protocol describing a type that can select a relay. public protocol RelaySelectorProtocol { - func selectRelays(connectionAttemptCount: UInt) throws -> SelectedRelays + func selectRelays( + tunnelSettings: LatestTunnelSettings, + connectionAttemptCount: UInt + ) throws -> SelectedRelays } /// Struct describing the selected relay. diff --git a/ios/MullvadREST/Relay/RelaySelectorWrapper.swift b/ios/MullvadREST/Relay/RelaySelectorWrapper.swift index 38dd42e72022..32429d93cd36 100644 --- a/ios/MullvadREST/Relay/RelaySelectorWrapper.swift +++ b/ios/MullvadREST/Relay/RelaySelectorWrapper.swift @@ -12,25 +12,12 @@ import MullvadTypes public final class RelaySelectorWrapper: RelaySelectorProtocol { let relayCache: RelayCacheProtocol - let tunnelSettingsUpdater: SettingsUpdater - private var tunnelSettings = LatestTunnelSettings() - private var observer: SettingsObserverBlock! - - deinit { - self.tunnelSettingsUpdater.removeObserver(observer) - } - - public init( - relayCache: RelayCacheProtocol, - tunnelSettingsUpdater: SettingsUpdater - ) { + public init(relayCache: RelayCacheProtocol) { self.relayCache = relayCache - self.tunnelSettingsUpdater = tunnelSettingsUpdater - - self.addObserver() } public func selectRelays( + tunnelSettings: LatestTunnelSettings, connectionAttemptCount: UInt ) throws -> SelectedRelays { let relays = try relayCache.read().relays @@ -52,12 +39,4 @@ public final class RelaySelectorWrapper: RelaySelectorProtocol { ).pick() } } - - private func addObserver() { - self.observer = SettingsObserverBlock(didUpdateSettings: { [weak self] latestTunnelSettings in - self?.tunnelSettings = latestTunnelSettings - }) - - tunnelSettingsUpdater.addObserver(observer) - } } diff --git a/ios/MullvadVPN/AppDelegate.swift b/ios/MullvadVPN/AppDelegate.swift index 68b55ee9c95c..48314ee2af15 100644 --- a/ios/MullvadVPN/AppDelegate.swift +++ b/ios/MullvadVPN/AppDelegate.swift @@ -93,8 +93,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD tunnelStore = TunnelStore(application: application) let relaySelector = RelaySelectorWrapper( - relayCache: ipOverrideWrapper, - tunnelSettingsUpdater: tunnelSettingsUpdater + relayCache: ipOverrideWrapper ) tunnelManager = createTunnelManager(application: application, relaySelector: relaySelector) diff --git a/ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift b/ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift index 04ea6c13751b..c008e0903c8f 100644 --- a/ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift +++ b/ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderHost.swift @@ -160,7 +160,11 @@ final class SimulatorTunnelProviderHost: SimulatorTunnelProviderDelegate { } private func pickRelays() throws -> SelectedRelays { - return try relaySelector.selectRelays(connectionAttemptCount: 0) + let settings = try SettingsManager.readSettings() + return try relaySelector.selectRelays( + tunnelSettings: settings, + connectionAttemptCount: 0 + ) } private func setInternalStateConnected(with selectedRelays: SelectedRelays?) { diff --git a/ios/MullvadVPN/TunnelManager/TunnelManager.swift b/ios/MullvadVPN/TunnelManager/TunnelManager.swift index 2d663a048fb0..612b1c5ea6c7 100644 --- a/ios/MullvadVPN/TunnelManager/TunnelManager.swift +++ b/ios/MullvadVPN/TunnelManager/TunnelManager.swift @@ -785,7 +785,10 @@ final class TunnelManager: StorePaymentObserver { fileprivate func selectRelays() throws -> SelectedRelays { let retryAttempts = tunnelStatus.observedState.connectionState?.connectionAttemptCount ?? 0 - return try relaySelector.selectRelays(connectionAttemptCount: retryAttempts) + return try relaySelector.selectRelays( + tunnelSettings: settings, + connectionAttemptCount: retryAttempts + ) } fileprivate func prepareForVPNConfigurationDeletion() { diff --git a/ios/MullvadVPNTests/MullvadREST/Relay/RelaySelectorWrapperTests.swift b/ios/MullvadVPNTests/MullvadREST/Relay/RelaySelectorWrapperTests.swift index 95b33883e831..770e7d689426 100644 --- a/ios/MullvadVPNTests/MullvadREST/Relay/RelaySelectorWrapperTests.swift +++ b/ios/MullvadVPNTests/MullvadREST/Relay/RelaySelectorWrapperTests.swift @@ -18,124 +18,100 @@ class RelaySelectorWrapperTests: XCTestCase { updatedAt: .distantPast )) ) + let multihopWithDaitaConstraints = RelayConstraints( + entryLocations: .only(UserSelectedRelays(locations: [.country("es")])), // Relay with DAITA. + exitLocations: .only(UserSelectedRelays(locations: [.country("us")])) + ) - var relayCache: RelayCache! - var settingsUpdater: SettingsUpdater! - var settingsListener: TunnelSettingsListener! + let multihopWithoutDaitaConstraints = RelayConstraints( + entryLocations: .only(UserSelectedRelays(locations: [.country("se")])), // Relay without DAITA. + exitLocations: .only(UserSelectedRelays(locations: [.country("us")])) + ) + let singlehopConstraints = RelayConstraints( + exitLocations: .only(UserSelectedRelays(locations: [.country("se")])) // Relay without DAITA. + ) + + var relayCache: RelayCache! override func setUp() { relayCache = RelayCache(fileCache: fileCache) - settingsListener = TunnelSettingsListener() - settingsUpdater = SettingsUpdater(listener: settingsListener) } func testSelectRelayWithMultihopOff() throws { - let wrapper = RelaySelectorWrapper( - relayCache: relayCache, - tunnelSettingsUpdater: settingsUpdater - ) + let wrapper = RelaySelectorWrapper(relayCache: relayCache) - settingsListener.onNewSettings?(LatestTunnelSettings(tunnelMultihopState: .off)) + let settings = LatestTunnelSettings( + relayConstraints: singlehopConstraints, + tunnelMultihopState: .off, + daita: DAITASettings(state: .off) + ) - let selectedRelays = try wrapper.selectRelays(connectionAttemptCount: 0) + let selectedRelays = try wrapper.selectRelays(tunnelSettings: settings, connectionAttemptCount: 0) XCTAssertNil(selectedRelays.entry) } func testSelectRelayWithMultihopOn() throws { - let wrapper = RelaySelectorWrapper( - relayCache: relayCache, - tunnelSettingsUpdater: settingsUpdater - ) + let wrapper = RelaySelectorWrapper(relayCache: relayCache) - settingsListener.onNewSettings?(LatestTunnelSettings(tunnelMultihopState: .on)) + let settings = LatestTunnelSettings( + relayConstraints: multihopWithDaitaConstraints, + tunnelMultihopState: .on, + daita: DAITASettings(state: .off) + ) - let selectedRelays = try wrapper.selectRelays(connectionAttemptCount: 0) + let selectedRelays = try wrapper.selectRelays(tunnelSettings: settings, connectionAttemptCount: 0) XCTAssertNotNil(selectedRelays.entry) } func testCanSelectRelayWithMultihopOnAndDaitaOn() throws { - let wrapper = RelaySelectorWrapper( - relayCache: relayCache, - tunnelSettingsUpdater: settingsUpdater - ) - - let constraints = RelayConstraints( - entryLocations: .only(UserSelectedRelays(locations: [.country("es")])), // Relay with DAITA. - exitLocations: .only(UserSelectedRelays(locations: [.country("us")])) - ) + let wrapper = RelaySelectorWrapper(relayCache: relayCache) let settings = LatestTunnelSettings( - relayConstraints: constraints, + relayConstraints: multihopWithDaitaConstraints, tunnelMultihopState: .on, daita: DAITASettings(state: .on) ) - settingsListener.onNewSettings?(settings) - XCTAssertNoThrow(try wrapper.selectRelays(connectionAttemptCount: 0)) + XCTAssertNoThrow(try wrapper.selectRelays(tunnelSettings: settings, connectionAttemptCount: 0)) } func testCannotSelectRelayWithMultihopOnAndDaitaOn() throws { - let wrapper = RelaySelectorWrapper( - relayCache: relayCache, - tunnelSettingsUpdater: settingsUpdater - ) - - let constraints = RelayConstraints( - entryLocations: .only(UserSelectedRelays(locations: [.country("se")])), // Relay without DAITA. - exitLocations: .only(UserSelectedRelays(locations: [.country("us")])) - ) + let wrapper = RelaySelectorWrapper(relayCache: relayCache) let settings = LatestTunnelSettings( - relayConstraints: constraints, + relayConstraints: multihopWithoutDaitaConstraints, tunnelMultihopState: .on, daita: DAITASettings(state: .on) ) - settingsListener.onNewSettings?(settings) - XCTAssertThrowsError(try wrapper.selectRelays(connectionAttemptCount: 0)) + XCTAssertThrowsError(try wrapper.selectRelays(tunnelSettings: settings, connectionAttemptCount: 0)) } func testCanSelectRelayWithMultihopOffAndDaitaOn() throws { - let wrapper = RelaySelectorWrapper( - relayCache: relayCache, - tunnelSettingsUpdater: settingsUpdater - ) - - let constraints = RelayConstraints( - exitLocations: .only(UserSelectedRelays(locations: [.country("es")])) // Relay with DAITA. - ) + let wrapper = RelaySelectorWrapper(relayCache: relayCache) let settings = LatestTunnelSettings( - relayConstraints: constraints, + relayConstraints: multihopWithoutDaitaConstraints, tunnelMultihopState: .off, daita: DAITASettings(state: .on) ) - settingsListener.onNewSettings?(settings) - let selectedRelays = try wrapper.selectRelays(connectionAttemptCount: 0) + let selectedRelays = try wrapper.selectRelays(tunnelSettings: settings, connectionAttemptCount: 0) XCTAssertNil(selectedRelays.entry) } // If DAITA is enabled and no supported relays are found, we should try to find the nearest // available relay that supports DAITA and use it as entry in a multihop selection. func testCanSelectRelayWithMultihopOffAndDaitaOnThroughMultihop() throws { - let wrapper = RelaySelectorWrapper( - relayCache: relayCache, - tunnelSettingsUpdater: settingsUpdater - ) - - let constraints = RelayConstraints( - exitLocations: .only(UserSelectedRelays(locations: [.country("se")])) // Relay without DAITA. - ) + let wrapper = RelaySelectorWrapper(relayCache: relayCache) let settings = LatestTunnelSettings( - relayConstraints: constraints, + relayConstraints: singlehopConstraints, tunnelMultihopState: .off, daita: DAITASettings(state: .on) ) - settingsListener.onNewSettings?(settings) - let selectedRelays = try wrapper.selectRelays(connectionAttemptCount: 0) + let selectedRelays = try wrapper.selectRelays(tunnelSettings: settings, connectionAttemptCount: 0) XCTAssertNotNil(selectedRelays.entry) } } diff --git a/ios/MullvadVPNTests/MullvadVPN/PacketTunnelCore/PacketTunnelActorReducerTests.swift b/ios/MullvadVPNTests/MullvadVPN/PacketTunnelCore/PacketTunnelActorReducerTests.swift index 936548afd581..49cbc5766f1a 100644 --- a/ios/MullvadVPNTests/MullvadVPN/PacketTunnelCore/PacketTunnelActorReducerTests.swift +++ b/ios/MullvadVPNTests/MullvadVPN/PacketTunnelCore/PacketTunnelActorReducerTests.swift @@ -7,14 +7,23 @@ // import MullvadMockData +@testable import MullvadREST +@testable import MullvadSettings import MullvadTypes @testable import PacketTunnelCore import WireGuardKitTypes import XCTest final class PacketTunnelActorReducerTests: XCTestCase { - // swiftlint:disable:next force_try - let selectedRelays = try! RelaySelectorStub.nonFallible().selectRelays(connectionAttemptCount: 0) + var selectedRelays: SelectedRelays! + + override func setUpWithError() throws { + let settings = LatestTunnelSettings() + selectedRelays = try RelaySelectorStub.nonFallible().selectRelays( + tunnelSettings: settings, + connectionAttemptCount: 0 + ) + } func makeConnectionData(keyPolicy: State.KeyPolicy = .useCurrent) -> State.ConnectionData { State.ConnectionData( diff --git a/ios/MullvadVPNTests/MullvadVPN/TunnelManager/TunnelManagerTests.swift b/ios/MullvadVPNTests/MullvadVPN/TunnelManager/TunnelManagerTests.swift index 28b119fe44b4..8c76b5431253 100644 --- a/ios/MullvadVPNTests/MullvadVPN/TunnelManager/TunnelManagerTests.swift +++ b/ios/MullvadVPNTests/MullvadVPN/TunnelManager/TunnelManagerTests.swift @@ -122,7 +122,10 @@ class TunnelManagerTests: XCTestCase { accountProxy.createAccountResult = .success(REST.NewAccountData.mockValue()) let relaySelector = RelaySelectorStub { _ in - try RelaySelectorStub.unsatisfied().selectRelays(connectionAttemptCount: 0) + try RelaySelectorStub.unsatisfied().selectRelays( + tunnelSettings: LatestTunnelSettings(), + connectionAttemptCount: 0 + ) } let tunnelManager = TunnelManager( @@ -148,7 +151,10 @@ class TunnelManagerTests: XCTestCase { case let .error(blockedStateReason) where blockedStateReason == .noRelaysSatisfyingConstraints: blockedExpectation.fulfill() relaySelector.selectedRelaysResult = { connectionAttemptCount in - try RelaySelectorStub.nonFallible().selectRelays(connectionAttemptCount: connectionAttemptCount) + try RelaySelectorStub.nonFallible().selectRelays( + tunnelSettings: LatestTunnelSettings(), + connectionAttemptCount: connectionAttemptCount + ) } tunnelManager.reconnectTunnel(selectNewRelay: true) diff --git a/ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift b/ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift index f40d00d4f23d..d9fd9b7f2f52 100644 --- a/ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift +++ b/ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift @@ -75,8 +75,7 @@ class PacketTunnelProvider: NEPacketTunnelProvider { deviceChecker = DeviceChecker(accountsProxy: accountsProxy, devicesProxy: devicesProxy) relaySelector = RelaySelectorWrapper( - relayCache: ipOverrideWrapper, - tunnelSettingsUpdater: tunnelSettingsUpdater + relayCache: ipOverrideWrapper ) actor = PacketTunnelActor( diff --git a/ios/PacketTunnelCore/Actor/PacketTunnelActor.swift b/ios/PacketTunnelCore/Actor/PacketTunnelActor.swift index 727b16215739..9e21e8550ee7 100644 --- a/ios/PacketTunnelCore/Actor/PacketTunnelActor.swift +++ b/ios/PacketTunnelCore/Actor/PacketTunnelActor.swift @@ -10,6 +10,7 @@ import Foundation import MullvadLogging import MullvadREST import MullvadRustRuntime +import MullvadSettings import MullvadTypes import NetworkExtension import WireGuardKitTypes @@ -370,6 +371,7 @@ extension PacketTunnelActor { nextRelays: nextRelays, relayConstraints: settings.relayConstraints, currentRelays: maybeCurrentRelays, + tunnelSettings: settings.tunnelSettings, connectionAttemptCount: connectionCount ) } @@ -488,6 +490,7 @@ extension PacketTunnelActor { nextRelays: NextRelays, relayConstraints: RelayConstraints, currentRelays: SelectedRelays?, + tunnelSettings: LatestTunnelSettings, connectionAttemptCount: UInt ) throws -> SelectedRelays { switch nextRelays { @@ -500,7 +503,10 @@ extension PacketTunnelActor { } case .random: - return try relaySelector.selectRelays(connectionAttemptCount: connectionAttemptCount) + return try relaySelector.selectRelays( + tunnelSettings: tunnelSettings, + connectionAttemptCount: connectionAttemptCount + ) case let .preSelected(selectedRelays): return selectedRelays