From 747d8d92ac3ac2d504a32246e71cd3b620eb9b49 Mon Sep 17 00:00:00 2001 From: Roopesh Chander Date: Mon, 27 Nov 2023 18:39:11 +0530 Subject: [PATCH] Do retry-then-signout when startTunnel() fails as well (#2723) Fixes #2718 It appears that #2687 missed the case where the tunnel failed to start for some reason, so the tunnel status never goes to "Connecting" and then back to "Disconnected" -- we were reacting to the tunnel status becoming "Disconnected". In this PR, we do the retry when startTunnel() throws an error as well. --- .../FirezoneKit/Features/MainView.swift | 8 +-- .../Sources/FirezoneKit/Stores/AppStore.swift | 29 --------- .../FirezoneKit/Stores/AuthStore.swift | 65 +++++++++++++++---- .../FirezoneKit/Stores/TunnelStore.swift | 32 ++++++++- .../Sources/FirezoneKit/Views/MenuBar.swift | 10 +-- 5 files changed, 86 insertions(+), 58 deletions(-) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift index 10955b250..3b601877c 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift @@ -64,12 +64,8 @@ import SwiftUI } func startTunnel() async { - do { - if case .signedIn = self.loginStatus { - try await appStore.tunnel.start() - } - } catch { - logger.error("Error starting tunnel: \(String(describing: error))") + if case .signedIn = self.loginStatus { + appStore.auth.startTunnel() } } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift index 1c6fbfed9..d78526e01 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift @@ -30,35 +30,6 @@ final class AppStore: ObservableObject { self?.objectWillChange.send() } .store(in: &cancellables) - - auth.$loginStatus - .receive(on: mainQueue) - .sink { [weak self] loginStatus in - Task { [weak self] in - await self?.handleLoginStatusChanged(loginStatus) - } - } - .store(in: &cancellables) - } - - private func handleLoginStatusChanged(_ loginStatus: AuthStore.LoginStatus) async { - logger.log("\(#function): login status = \(loginStatus)") - switch loginStatus { - case .signedIn: - do { - try await tunnel.start() - } catch { - logger.error("\(#function): Error starting tunnel: \(String(describing: error))") - } - case .signedOut: - do { - try await tunnel.stop() - } catch { - logger.error("\(#function): Error stopping tunnel: \(String(describing: error))") - } - case .uninitialized: - break - } } private func signOutAndStopTunnel() { diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift index a96661a90..0bd5f0a23 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift @@ -59,7 +59,12 @@ final class AuthStore: ObservableObject { private var cancellables = Set() - @Published private(set) var loginStatus: LoginStatus + @Published private(set) var loginStatus: LoginStatus { + didSet { + self.handleLoginStatusChanged() + } + } + private var status: NEVPNStatus = .invalid private static let maxReconnectionAttemptCount = 3 @@ -101,19 +106,7 @@ final class AuthStore: ObservableObject { case .signoutImmediately: self.signOut() case .retryThenSignout: - let shouldReconnect = (self.reconnectionAttemptsRemaining > 0) - self.reconnectionAttemptsRemaining = self.reconnectionAttemptsRemaining - 1 - if shouldReconnect { - self.logger.log( - "\(#function): Will try to reconnect after 1 second (\(self.reconnectionAttemptsRemaining) attempts after this)" - ) - DispatchQueue.main.asyncAfter(deadline: .now() + .seconds(1)) { - self.logger.log("\(#function): Trying to reconnect") - self.startTunnel() - } - } else { - self.signOut() - } + self.retryStartTunnel() } } else { self.logger.log("\(#function): Tunnel shutdown event not found") @@ -215,10 +208,54 @@ final class AuthStore: ObservableObject { try await tunnelStore.start() } catch { logger.error("\(#function): Error starting tunnel: \(String(describing: error))") + self.retryStartTunnel() } } } + func retryStartTunnel() { + // Try to reconnect, but don't try more than 3 times at a time. + // If this gets called the third time, sign out. + let shouldReconnect = (self.reconnectionAttemptsRemaining > 0) + self.reconnectionAttemptsRemaining = self.reconnectionAttemptsRemaining - 1 + if shouldReconnect { + self.logger.log( + "\(#function): Will try to reconnect after 1 second (\(self.reconnectionAttemptsRemaining) attempts after this)" + ) + DispatchQueue.main.asyncAfter(deadline: .now() + .seconds(1)) { + self.logger.log("\(#function): Trying to reconnect") + self.startTunnel() + } + } else { + self.signOut() + } + } + + private func handleLoginStatusChanged() { + logger.log("\(#function): Login status: \(self.loginStatus)") + switch self.loginStatus { + case .signedIn: + Task { + do { + try await tunnelStore.start() + } catch { + logger.error("\(#function): Error starting tunnel: \(String(describing: error))") + self.retryStartTunnel() + } + } + case .signedOut: + Task { + do { + try await tunnelStore.stop() + } catch { + logger.error("\(#function): Error stopping tunnel: \(String(describing: error))") + } + } + case .uninitialized: + break + } + } + func resetReconnectionAttemptsRemaining() { self.reconnectionAttemptsRemaining = Self.maxReconnectionAttemptCount } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift index aa9ffe75c..31bd27c7b 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift @@ -192,7 +192,13 @@ final class TunnelStore: ObservableObject { } if case .signedIn(let authBaseURL, let accountId, let tokenReference) = self.tunnelAuthStatus { - try await saveAuthStatus(.signedOut(authBaseURL: authBaseURL, accountId: accountId)) + do { + try await saveAuthStatus(.signedOut(authBaseURL: authBaseURL, accountId: accountId)) + } catch { + TunnelStore.logger.trace( + "\(#function): Error saving signed out auth status: \(error)" + ) + } return tokenReference } @@ -416,6 +422,7 @@ extension NETunnelProviderManager { protocolConfiguration.providerConfiguration = providerConfig + ensureTunnelConfigurationIsValid() try await saveToPreferences() } } @@ -471,7 +478,30 @@ extension NETunnelProviderManager { protocolConfiguration.providerConfiguration = providerConfig protocolConfiguration.serverAddress = advancedSettings.apiURLString + ensureTunnelConfigurationIsValid() try await saveToPreferences() } } + + private func ensureTunnelConfigurationIsValid() { + // Ensure the tunnel config has required values populated, because + // to even sign out, we need saveToPreferences() to succeed. + if let protocolConfiguration = protocolConfiguration as? NETunnelProviderProtocol { + protocolConfiguration.providerBundleIdentifier = Bundle.main.bundleIdentifier.map { + "\($0).network-extension" + } + if protocolConfiguration.serverAddress?.isEmpty ?? true { + protocolConfiguration.serverAddress = "unknown-server" + } + } else { + let protocolConfiguration = NETunnelProviderProtocol() + protocolConfiguration.providerBundleIdentifier = Bundle.main.bundleIdentifier.map { + "\($0).network-extension" + } + protocolConfiguration.serverAddress = "unknown-server" + } + if localizedDescription?.isEmpty ?? true { + localizedDescription = "Firezone" + } + } } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift index 65c466582..04247f488 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift @@ -201,14 +201,8 @@ } @objc private func reconnectButtonTapped() { - Task { - if case .signedIn = appStore?.auth.loginStatus { - do { - try await appStore?.tunnel.start() - } catch { - logger.error("error connecting to tunnel (reconnect): \(String(describing: error))") - } - } + if case .signedIn = appStore?.auth.loginStatus { + appStore?.auth.startTunnel() } }