Skip to content
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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
106 changes: 106 additions & 0 deletions Sources/NetworkProtection/Models/VPNServerSelectionSanitizer.swift
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments here:

  1. Let's add unit tests to cover this class in its entirety.
  2. The name of this class sounds off to me. This does more than just sanitize the selection - it effectively manages the entire selection logic (locations vs servers in settings). Additionally when referencing this in the init parameter below (for PacketTunnelProvider) the instance is named serverSelection, which makes me think you agree. Why not name this something more generic like VPNServerSelector or VPNServerSelectionResolver.
  3. To further expand on point 2, I really think that to the PacketTunnelProvider whether this does sanitization or not is an implementation detail - what matters is that this class provides a server / location to connect to.

Copy link
Contributor Author

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.

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
}
}
67 changes: 8 additions & 59 deletions Sources/NetworkProtection/PacketTunnelProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@

// MARK: - Server Selection

private let serverSelection: VPNServerSelectionSanitizing

public var lastSelectedServerInfo: NetworkProtectionServerInfo? {
didSet {
lastSelectedServerInfoPublisher.send(lastSelectedServerInfo)
Expand Down Expand Up @@ -342,6 +344,7 @@
debugEvents: EventMapping<NetworkProtectionError>?,
providerEvents: EventMapping<Event>,
settings: VPNSettings,
serverSelection: VPNServerSelectionSanitizing,
defaults: UserDefaults,
isSubscriptionEnabled: Bool,
entitlementCheck: (() async -> Result<Bool, Error>)?) {
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {

Expand All @@ -757,7 +735,6 @@

do {
let tunnelConfiguration = try await generateTunnelConfiguration(environment: environment,
serverSelectionMethod: serverSelectionMethod,
includedRoutes: includedRoutes ?? [],
excludedRoutes: settings.excludedRanges,
regenerateKey: regenerateKey)
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -910,7 +887,7 @@
}

// swiftlint:disable:next cyclomatic_complexity
private func handleSettingsChange(_ change: VPNSettings.Change, completionHandler: ((Data?) -> Void)? = nil) {

Check failure on line 890 in Sources/NetworkProtection/PacketTunnelProvider.swift

View workflow job for this annotation

GitHub Actions / Run SwiftLint

SwiftLint rule 'cyclomatic_complexity' did not trigger a violation in the disabled region; remove the disable command (superfluous_disable_command)
switch change {
case .setExcludeLocalNetworks:
Task { @MainActor in
Expand All @@ -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)
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ServerSelector. I’ve been trying to avoid adding too much more logic to the PacketTunnelProvider, but this selection logic seemed closely related to the responsibilities of the ServerSelector. The extraction should allow it to all be unit tested too which I’ll do before moving this PR out of draft.


case .setSelectedServer, .setSelectedLocation:
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
}
Expand Down Expand Up @@ -1041,9 +993,6 @@
}

settings.selectedServer = .endpoint(serverName)
if case .connected = connectionStatus {
try? await updateTunnelConfiguration(environment: settings.selectedEnvironment, serverSelectionMethod: .preferredServer(serverName: serverName), reassert: true)
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ import Common

public protocol NetworkProtectionLocationListRepository {
func fetchLocationList() async throws -> [NetworkProtectionLocation]
func fetchLocationListIgnoringCache() async throws -> [NetworkProtectionLocation]
}

final public class NetworkProtectionLocationListCompositeRepository: NetworkProtectionLocationListRepository {

@MainActor private static var locationList: [NetworkProtectionLocation] = []
@MainActor private static var cacheTimestamp = Date()
private static let cacheValidity = TimeInterval(60) // Refreshes at most once per minute
Expand Down Expand Up @@ -59,6 +61,11 @@ final public class NetworkProtectionLocationListCompositeRepository: NetworkProt
guard !canUseCache else {
return Self.locationList
}
return try await fetchLocationListIgnoringCache()
}

@MainActor
public func fetchLocationListIgnoringCache() async throws -> [NetworkProtectionLocation] {
do {
guard let authToken = try tokenStore.fetchToken() else {
throw NetworkProtectionError.noAuthTokenFound
Expand Down