-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolve location selection against remote locations #764
base: main
Are you sure you want to change the base?
Changes from 5 commits
59359ee
371d6c3
799caaf
a820f94
95eeb55
0c93643
e941390
a3693bb
8b0b0ed
d402aca
dbc8db6
04d390c
30e5e62
63daeee
d2200c9
cdb946c
8399a10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
// | ||
// VPNServerSelectionSanitizer.swift | ||
// | ||
// Copyright © 2024 DuckDuckGo. All rights reserved. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// | ||
|
||
import Foundation | ||
|
||
public enum VPNServerSelectionError: Error { | ||
case countryNotFound | ||
case fetchingLocationsFailed(Error) | ||
} | ||
|
||
public protocol VPNServerSelectionSanitizing { | ||
func sanitizedServerSelectionMethod() async -> NetworkProtectionServerSelectionMethod | ||
} | ||
|
||
public final class VPNServerSelectionSanitizer: VPNServerSelectionSanitizing { | ||
private let locationListRepository: NetworkProtectionLocationListRepository | ||
private let vpnSettings: VPNSettings | ||
|
||
public init(locationListRepository: NetworkProtectionLocationListRepository, vpnSettings: VPNSettings) { | ||
self.locationListRepository = locationListRepository | ||
self.vpnSettings = vpnSettings | ||
} | ||
|
||
public func sanitizedServerSelectionMethod() async -> NetworkProtectionServerSelectionMethod { | ||
switch currentServerSelectionMethod { | ||
case .automatic, .preferredServer, .avoidServer: | ||
return currentServerSelectionMethod | ||
case .preferredLocation(let networkProtectionSelectedLocation): | ||
do { | ||
let location = try await sanitizeSelectionAgainstAvailableLocations(networkProtectionSelectedLocation) | ||
return .preferredLocation(location) | ||
} catch let error as VPNServerSelectionError { | ||
switch error { | ||
case .countryNotFound: | ||
return .automatic | ||
case .fetchingLocationsFailed: | ||
return currentServerSelectionMethod | ||
} | ||
} catch { | ||
return currentServerSelectionMethod | ||
} | ||
} | ||
} | ||
|
||
private func sanitizeSelectionAgainstAvailableLocations(_ selection: NetworkProtectionSelectedLocation) async throws -> NetworkProtectionSelectedLocation { | ||
let availableLocations: [NetworkProtectionLocation] | ||
do { | ||
availableLocations = try await locationListRepository.fetchLocationListIgnoringCache() | ||
} catch { | ||
throw VPNServerSelectionError.fetchingLocationsFailed(error) | ||
} | ||
|
||
let availableCitySelections = availableLocations.flatMap { location in | ||
location.cities.map { city in NetworkProtectionSelectedLocation(country: location.country, city: city.name) } | ||
} | ||
|
||
if availableCitySelections.contains(selection) { | ||
return selection | ||
} | ||
|
||
let selectedCountry = NetworkProtectionSelectedLocation(country: selection.country) | ||
let availableCountrySelections = availableLocations.map { NetworkProtectionSelectedLocation(country: $0.country) } | ||
guard availableCountrySelections.contains(selectedCountry) else { | ||
throw VPNServerSelectionError.countryNotFound | ||
} | ||
|
||
return selectedCountry | ||
} | ||
|
||
private var currentServerSelectionMethod: NetworkProtectionServerSelectionMethod { | ||
var serverSelectionMethod: NetworkProtectionServerSelectionMethod | ||
|
||
switch vpnSettings.selectedLocation { | ||
case .nearest: | ||
serverSelectionMethod = .automatic | ||
case .location(let networkProtectionSelectedLocation): | ||
serverSelectionMethod = .preferredLocation(networkProtectionSelectedLocation) | ||
} | ||
|
||
switch vpnSettings.selectedServer { | ||
case .automatic: | ||
break | ||
case .endpoint(let string): | ||
// Selecting a specific server will override locations setting | ||
// Only available in debug | ||
serverSelectionMethod = .preferredServer(serverName: string) | ||
} | ||
|
||
return serverSelectionMethod | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -167,6 +167,8 @@ | |
|
||
// MARK: - Server Selection | ||
|
||
private let serverSelection: VPNServerSelectionSanitizing | ||
|
||
public var lastSelectedServerInfo: NetworkProtectionServerInfo? { | ||
didSet { | ||
lastSelectedServerInfoPublisher.send(lastSelectedServerInfo) | ||
|
@@ -342,6 +344,7 @@ | |
debugEvents: EventMapping<NetworkProtectionError>?, | ||
providerEvents: EventMapping<Event>, | ||
settings: VPNSettings, | ||
serverSelection: VPNServerSelectionSanitizing, | ||
defaults: UserDefaults, | ||
isSubscriptionEnabled: Bool, | ||
entitlementCheck: (() async -> Result<Bool, Error>)?) { | ||
|
@@ -355,6 +358,7 @@ | |
self.tunnelHealth = tunnelHealthStore | ||
self.controllerErrorStore = controllerErrorStore | ||
self.settings = settings | ||
self.serverSelection = serverSelection | ||
self.defaults = defaults | ||
self.isSubscriptionEnabled = isSubscriptionEnabled | ||
self.entitlementCheck = isSubscriptionEnabled ? entitlementCheck : nil | ||
|
@@ -588,36 +592,12 @@ | |
completionHandler: completionHandler) | ||
} | ||
|
||
var currentServerSelectionMethod: NetworkProtectionServerSelectionMethod { | ||
var serverSelectionMethod: NetworkProtectionServerSelectionMethod | ||
|
||
switch settings.selectedLocation { | ||
case .nearest: | ||
serverSelectionMethod = .automatic | ||
case .location(let networkProtectionSelectedLocation): | ||
serverSelectionMethod = .preferredLocation(networkProtectionSelectedLocation) | ||
} | ||
|
||
switch settings.selectedServer { | ||
case .automatic: | ||
break | ||
case .endpoint(let string): | ||
// Selecting a specific server will override locations setting | ||
// Only available in debug | ||
serverSelectionMethod = .preferredServer(serverName: string) | ||
} | ||
|
||
return serverSelectionMethod | ||
} | ||
|
||
private func startTunnel(environment: VPNSettings.SelectedEnvironment, onDemand: Bool, completionHandler: @escaping (Error?) -> Void) { | ||
Task { | ||
do { | ||
os_log("🔵 Generating tunnel config", log: .networkProtection, type: .info) | ||
os_log("🔵 Excluded ranges are: %{public}@", log: .networkProtection, type: .info, String(describing: settings.excludedRanges)) | ||
os_log("🔵 Server selection method: %{public}@", log: .networkProtection, type: .info, currentServerSelectionMethod.debugDescription) | ||
let tunnelConfiguration = try await generateTunnelConfiguration(environment: environment, | ||
serverSelectionMethod: currentServerSelectionMethod, | ||
includedRoutes: includedRoutes ?? [], | ||
excludedRoutes: settings.excludedRanges, | ||
regenerateKey: true) | ||
|
@@ -737,15 +717,13 @@ | |
public func updateTunnelConfiguration(reassert: Bool, regenerateKey: Bool = false) async throws { | ||
try await updateTunnelConfiguration( | ||
environment: settings.selectedEnvironment, | ||
serverSelectionMethod: currentServerSelectionMethod, | ||
reassert: reassert, | ||
regenerateKey: regenerateKey | ||
) | ||
} | ||
|
||
@MainActor | ||
public func updateTunnelConfiguration(environment: VPNSettings.SelectedEnvironment = .default, | ||
serverSelectionMethod: NetworkProtectionServerSelectionMethod, | ||
reassert: Bool, | ||
regenerateKey: Bool = false) async throws { | ||
|
||
|
@@ -757,7 +735,6 @@ | |
|
||
do { | ||
let tunnelConfiguration = try await generateTunnelConfiguration(environment: environment, | ||
serverSelectionMethod: serverSelectionMethod, | ||
includedRoutes: includedRoutes ?? [], | ||
excludedRoutes: settings.excludedRanges, | ||
regenerateKey: regenerateKey) | ||
|
@@ -799,22 +776,22 @@ | |
|
||
@MainActor | ||
private func generateTunnelConfiguration(environment: VPNSettings.SelectedEnvironment = .default, | ||
serverSelectionMethod: NetworkProtectionServerSelectionMethod, | ||
includedRoutes: [IPAddressRange], | ||
excludedRoutes: [IPAddressRange], | ||
regenerateKey: Bool) async throws -> TunnelConfiguration { | ||
|
||
let configurationResult: (TunnelConfiguration, NetworkProtectionServerInfo) | ||
|
||
do { | ||
let serverSelectionMethod = await serverSelection.sanitizedServerSelectionMethod() | ||
let networkClient = NetworkProtectionBackendClient(environment: environment, | ||
isSubscriptionEnabled: isSubscriptionEnabled) | ||
let deviceManager = NetworkProtectionDeviceManager(networkClient: networkClient, | ||
tokenStore: tokenStore, | ||
keyStore: keyStore, | ||
errorEvents: debugEvents, | ||
isSubscriptionEnabled: isSubscriptionEnabled) | ||
|
||
os_log("🔵 Server selection method: %{public}@", log: .networkProtection, type: .info, serverSelectionMethod.debugDescription) | ||
configurationResult = try await deviceManager.generateTunnelConfiguration( | ||
selectionMethod: serverSelectionMethod, | ||
includedRoutes: includedRoutes, | ||
|
@@ -910,7 +887,7 @@ | |
} | ||
|
||
// swiftlint:disable:next cyclomatic_complexity | ||
private func handleSettingsChange(_ change: VPNSettings.Change, completionHandler: ((Data?) -> Void)? = nil) { | ||
switch change { | ||
case .setExcludeLocalNetworks: | ||
Task { @MainActor in | ||
|
@@ -919,35 +896,10 @@ | |
} | ||
completionHandler?(nil) | ||
} | ||
case .setSelectedServer(let selectedServer): | ||
let serverSelectionMethod: NetworkProtectionServerSelectionMethod | ||
|
||
switch selectedServer { | ||
case .automatic: | ||
serverSelectionMethod = .automatic | ||
case .endpoint(let serverName): | ||
serverSelectionMethod = .preferredServer(serverName: serverName) | ||
} | ||
|
||
Task { @MainActor in | ||
if case .connected = connectionStatus { | ||
try? await updateTunnelConfiguration(environment: settings.selectedEnvironment, serverSelectionMethod: serverSelectionMethod, reassert: true) | ||
} | ||
completionHandler?(nil) | ||
} | ||
case .setSelectedLocation(let selectedLocation): | ||
let serverSelectionMethod: NetworkProtectionServerSelectionMethod | ||
|
||
switch selectedLocation { | ||
case .nearest: | ||
serverSelectionMethod = .automatic | ||
case .location(let location): | ||
serverSelectionMethod = .preferredLocation(location) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why this is all going away? Admittedly I'm not looking to deeply into this and opting to ask as I want to avoid making (mistaken) assumptions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes sure. It’s essentially extracting this logic into the… class that has yet to be named ;). Let’s go with |
||
|
||
case .setSelectedServer, .setSelectedLocation: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this is now handling both server selection and location selection cases. |
||
Task { @MainActor in | ||
if case .connected = connectionStatus { | ||
try? await updateTunnelConfiguration(environment: settings.selectedEnvironment, serverSelectionMethod: serverSelectionMethod, reassert: true) | ||
try? await updateTunnelConfiguration(environment: settings.selectedEnvironment, reassert: true) | ||
} | ||
completionHandler?(nil) | ||
} | ||
|
@@ -1041,9 +993,6 @@ | |
} | ||
|
||
settings.selectedServer = .endpoint(serverName) | ||
if case .connected = connectionStatus { | ||
try? await updateTunnelConfiguration(environment: settings.selectedEnvironment, serverSelectionMethod: .preferredServer(serverName: serverName), reassert: true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is actually changing behaviour. Why is it no longer necessary to update the tunnel configuration when the server selection changes? |
||
} | ||
completionHandler?(nil) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments here:
PacketTunnelProvider
) the instance is namedserverSelection
, which makes me think you agree. Why not name this something more generic likeVPNServerSelector
orVPNServerSelectionResolver
.PacketTunnelProvider
whether this does sanitization or not is an implementation detail - what matters is that this class provides a server / location to connect to.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the naming here gave me a lot of trouble. So just decided to open it up to get opinions. Thanks for the input.