VPN: Remove server cache (#749)

Task/Issue URL: https://app.asana.com/0/0/1206931344651423/f iOS PR: duckduckgo/iOS#2642 macOS PR: duckduckgo/macos-browser#2504 What kind of version bump will this require?: Major/Minor/Patch ## Description Remove unnecessary server list caching.
duckduckgo · Mar 26, 2024 · fa15af7 · fa15af7
1 parent c2ae796
commit fa15af7
Show file tree

Hide file tree

Showing 10 changed files with 51 additions and 476 deletions.
diff --git a/Sources/NetworkProtection/Diagnostics/NetworkProtectionError.swift b/Sources/NetworkProtection/Diagnostics/NetworkProtectionError.swift
@@ -46,14 +46,6 @@ public enum NetworkProtectionError: LocalizedError, CustomNSError {
     case invalidAuthToken
     case serverListInconsistency
 
-    // Server list store errors
-    case failedToEncodeServerList(Error)
-    case failedToDecodeServerList(Error)
-    case failedToWriteServerList(Error)
-    case noServerListFound
-    case couldNotCreateServerListDirectory(Error)
-    case failedToReadServerList(Error)
-
     // Keychain errors
     case failedToCastKeychainValueToData(field: String)
     case keychainReadError(field: String, status: Int32)
@@ -102,31 +94,24 @@ public enum NetworkProtectionError: LocalizedError, CustomNSError {
         case .failedToParseRedeemResponse: return 111
         case .invalidAuthToken: return 112
         case .serverListInconsistency: return 113
-            // 200+ - Server list store errors
-        case .failedToEncodeServerList: return 200
-        case .failedToDecodeServerList: return 201
-        case .failedToWriteServerList: return 202
-        case .noServerListFound: return 203
-        case .couldNotCreateServerListDirectory: return 204
-        case .failedToReadServerList: return 205
-            // 300+ - Keychain errors
+            // 200+ - Keychain errors
         case .failedToCastKeychainValueToData: return 300
-        case .keychainReadError: return 301
-        case .keychainWriteError: return 302
-        case .keychainUpdateError: return 303
-        case .keychainDeleteError: return 304
-            // 400+ - Wireguard errors
-        case .wireGuardCannotLocateTunnelFileDescriptor: return 400
-        case .wireGuardInvalidState: return 401
-        case .wireGuardDnsResolution: return 402
-        case .wireGuardSetNetworkSettings: return 403
-        case .startWireGuardBackend: return 404
-            // 500+ Auth errors
-        case .noAuthTokenFound: return 500
-            // 600+ Subscription errors
-        case .vpnAccessRevoked: return 600
-            // 700+ Unhandled errors
-        case .unhandledError: return 700
+        case .keychainReadError: return 201
+        case .keychainWriteError: return 202
+        case .keychainUpdateError: return 203
+        case .keychainDeleteError: return 204
+            // 300+ - Wireguard errors
+        case .wireGuardCannotLocateTunnelFileDescriptor: return 300
+        case .wireGuardInvalidState: return 301
+        case .wireGuardDnsResolution: return 302
+        case .wireGuardSetNetworkSettings: return 303
+        case .startWireGuardBackend: return 304
+            // 400+ Auth errors
+        case .noAuthTokenFound: return 400
+            // 500+ Subscription errors
+        case .vpnAccessRevoked: return 500
+            // 600+ Unhandled errors
+        case .unhandledError: return 600
         }
     }
 
@@ -143,7 +128,6 @@ public enum NetworkProtectionError: LocalizedError, CustomNSError {
                 .failedToRetrieveAuthToken,
                 .invalidAuthToken,
                 .serverListInconsistency,
-                .noServerListFound,
                 .failedToCastKeychainValueToData,
                 .keychainReadError,
                 .keychainWriteError,
@@ -171,11 +155,6 @@ public enum NetworkProtectionError: LocalizedError, CustomNSError {
                 .failedToParseLocationListResponse(let error),
                 .failedToParseRegisteredServersResponse(let error),
                 .failedToParseRedeemResponse(let error),
-                .failedToEncodeServerList(let error),
-                .failedToDecodeServerList(let error),
-                .failedToWriteServerList(let error),
-                .couldNotCreateServerListDirectory(let error),
-                .failedToReadServerList(let error),
                 .wireGuardSetNetworkSettings(let error),
                 .unhandledError(_, _, let error):
             return [

diff --git a/Sources/NetworkProtection/NetworkProtectionDeviceManager.swift b/Sources/NetworkProtection/NetworkProtectionDeviceManager.swift
@@ -54,7 +54,6 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {
     private let networkClient: NetworkProtectionClient
     private let tokenStore: NetworkProtectionTokenStore
     private let keyStore: NetworkProtectionKeyStore
-    private let serverListStore: NetworkProtectionServerListStore
 
     private let errorEvents: EventMapping<NetworkProtectionError>?
 
@@ -63,27 +62,23 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {
     public init(environment: VPNSettings.SelectedEnvironment,
                 tokenStore: NetworkProtectionTokenStore,
                 keyStore: NetworkProtectionKeyStore,
-                serverListStore: NetworkProtectionServerListStore? = nil,
                 errorEvents: EventMapping<NetworkProtectionError>?,
                 isSubscriptionEnabled: Bool) {
         self.init(networkClient: NetworkProtectionBackendClient(environment: environment, isSubscriptionEnabled: isSubscriptionEnabled),
                   tokenStore: tokenStore,
                   keyStore: keyStore,
-                  serverListStore: serverListStore,
                   errorEvents: errorEvents,
                   isSubscriptionEnabled: isSubscriptionEnabled)
     }
 
     init(networkClient: NetworkProtectionClient,
          tokenStore: NetworkProtectionTokenStore,
          keyStore: NetworkProtectionKeyStore,
-         serverListStore: NetworkProtectionServerListStore? = nil,
          errorEvents: EventMapping<NetworkProtectionError>?,
          isSubscriptionEnabled: Bool) {
         self.networkClient = networkClient
         self.tokenStore = tokenStore
         self.keyStore = keyStore
-        self.serverListStore = serverListStore ?? NetworkProtectionServerListFileSystemStore(errorEvents: errorEvents)
         self.errorEvents = errorEvents
         self.isSubscriptionEnabled = isSubscriptionEnabled
     }
@@ -95,27 +90,15 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {
         guard let token = try? tokenStore.fetchToken() else {
             throw NetworkProtectionError.noAuthTokenFound
         }
-        let servers = await networkClient.getServers(authToken: token)
+        let result = await networkClient.getServers(authToken: token)
         let completeServerList: [NetworkProtectionServer]
 
-        switch servers {
+        switch result {
         case .success(let serverList):
             completeServerList = serverList
         case .failure(let failure):
             handle(clientError: failure)
-            return try serverListStore.storedNetworkProtectionServerList()
-        }
-
-        do {
-            try serverListStore.store(serverList: completeServerList)
-        } catch let error as NetworkProtectionServerListStoreError {
-            errorEvents?.fire(error.networkProtectionError)
-            // Intentionally not rethrowing as the failing call is not critical to provide
-            // a working UX.
-        } catch {
-            errorEvents?.fire(.unhandledError(function: #function, line: #line, error: error))
-            // Intentionally not rethrowing as the failing call is not critical to provide
-            // a working UX.
+            throw failure
         }
 
         return completeServerList
@@ -189,7 +172,6 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {
     //
     // - Throws:`NetworkProtectionError`
     //
-    // swiftlint:disable:next cyclomatic_complexity
     private func register(keyPair: KeyPair,
                           selectionMethod: NetworkProtectionServerSelectionMethod) async throws -> (server: NetworkProtectionServer,
                                                                                                     newExpiration: Date?) {
@@ -223,16 +205,6 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {
 
         switch registeredServersResult {
         case .success(let registeredServers):
-            do {
-                try serverListStore.store(serverList: registeredServers)
-            } catch let error as NetworkProtectionServerListStoreError {
-                errorEvents?.fire(error.networkProtectionError)
-                // Intentionally not rethrowing, as this failure is not critical for this method
-            } catch {
-                errorEvents?.fire(.unhandledError(function: #function, line: #line, error: error))
-                // Intentionally not rethrowing, as this failure is not critical for this method
-            }
-
             guard let registeredServer = registeredServers.first(where: { $0.serverName != excludedServerName }) else {
                 // If we're looking to exclude a server we should have a few other options available.  If we can't find any
                 // then it means theres an inconsistency in the server list that was returned.
@@ -253,27 +225,6 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {
         }
     }
 
-    /// Retrieves the first cached server that's registered with the specified key pair.
-    ///
-    private func cachedServer(registeredWith keyPair: KeyPair) throws -> NetworkProtectionServer {
-        do {
-            guard let server = try serverListStore.storedNetworkProtectionServerList().first(where: {
-                $0.isRegistered(with: keyPair.publicKey)
-            }) else {
-                errorEvents?.fire(NetworkProtectionError.noServerListFound)
-                throw NetworkProtectionError.noServerListFound
-            }
-
-            return server
-        } catch let error as NetworkProtectionError {
-            errorEvents?.fire(error)
-            throw error
-        } catch {
-            errorEvents?.fire(NetworkProtectionError.unhandledError(function: #function, line: #line, error: error))
-            throw NetworkProtectionError.unhandledError(function: #function, line: #line, error: error)
-        }
-    }
-
     // MARK: - Internal
 
     func server(in servers: [NetworkProtectionServer], matching name: String?) -> NetworkProtectionServer? {

diff --git a/Sources/NetworkProtection/Networking/NetworkProtectionClient.swift b/Sources/NetworkProtection/Networking/NetworkProtectionClient.swift
@@ -31,7 +31,7 @@ protocol NetworkProtectionClient {
                   requestBody: RegisterKeyRequestBody) async -> Result<[NetworkProtectionServer], NetworkProtectionClientError>
 }
 
-public enum NetworkProtectionClientError: Error, NetworkProtectionErrorConvertible {
+public enum NetworkProtectionClientError: CustomNSError, NetworkProtectionErrorConvertible {
     case failedToFetchLocationList(Error)
     case failedToParseLocationListResponse(Error)
     case failedToFetchServerList(Error)
@@ -65,6 +65,25 @@ public enum NetworkProtectionClientError: Error, NetworkProtectionErrorConvertib
         case .accessDenied: return .vpnAccessRevoked
         }
     }
+
+    public var errorCode: Int {
+        switch self {
+        case .failedToFetchLocationList: return 0
+        case .failedToParseLocationListResponse: return 1
+        case .failedToFetchServerList: return 2
+        case .failedToParseServerListResponse: return 3
+        case .failedToEncodeRegisterKeyRequest: return 4
+        case .failedToFetchRegisteredServers: return 5
+        case .failedToParseRegisteredServersResponse: return 6
+        case .failedToEncodeRedeemRequest: return 7
+        case .invalidInviteCode: return 8
+        case .failedToRedeemInviteCode: return 9
+        case .failedToRetrieveAuthToken: return 10
+        case .failedToParseRedeemResponse: return 11
+        case .invalidAuthToken: return 12
+        case .accessDenied: return 13
+        }
+    }
 }
 
 struct RegisterKeyRequestBody: Encodable {

diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift
@@ -997,9 +997,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
     private func handleResetAllState(completionHandler: ((Data?) -> Void)? = nil) {
         resetRegistrationKey()
 
-        let serverCache = NetworkProtectionServerListFileSystemStore(errorEvents: nil)
-        serverCache.removeServerList()
-
         try? tokenStore.deleteToken()
 
         // This is not really an error, we received a command to reset the connection

diff --git a/Sources/NetworkProtection/Repositories/NetworkProtectionLocationListRepository.swift b/Sources/NetworkProtection/Repositories/NetworkProtectionLocationListRepository.swift
@@ -25,6 +25,8 @@ public protocol NetworkProtectionLocationListRepository {
 
 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
     private let client: NetworkProtectionClient
     private let tokenStore: NetworkProtectionTokenStore
     private let errorEvents: EventMapping<NetworkProtectionError>
@@ -54,14 +56,15 @@ final public class NetworkProtectionLocationListCompositeRepository: NetworkProt
 
     @MainActor
     public func fetchLocationList() async throws -> [NetworkProtectionLocation] {
-        guard Self.locationList.isEmpty else {
+        guard !canUseCache else {
             return Self.locationList
         }
         do {
             guard let authToken = try tokenStore.fetchToken() else {
                 throw NetworkProtectionError.noAuthTokenFound
             }
             Self.locationList = try await client.getLocations(authToken: authToken).get()
+            Self.cacheTimestamp = Date()
         } catch let error as NetworkProtectionErrorConvertible {
             errorEvents.fire(error.networkProtectionError)
             throw error.networkProtectionError
@@ -77,7 +80,12 @@ final public class NetworkProtectionLocationListCompositeRepository: NetworkProt
     }
 
     @MainActor
-    static func clearCache() {
+    private var canUseCache: Bool {
+        !Self.locationList.isEmpty && Date().timeIntervalSince(Self.cacheTimestamp) < Self.cacheValidity
+    }
+
+    @MainActor
+    public static func clearCache() {
         locationList = []
     }
 }