From e1ac4965452ec9162989c7b0d4f906e0a032fd2c Mon Sep 17 00:00:00 2001 From: Roopesh Chander Date: Tue, 22 Aug 2023 20:55:16 +0530 Subject: [PATCH] apple: macOS app: Some bugfixes (#1937) Fixes the following issues with the macOS app: 1. Every alternate launch of the app caused the tunnel to go down 2. When signing out and signing back in, resources were not getting updated 3. While the tunnel is up, when we quit the app and restart the app, the tunnel was brought down --- .../FirezoneKit/AuthClient/AuthClient.swift | 8 +--- .../FirezoneKit/Features/MainView.swift | 5 +- .../FirezoneKit/Features/WelcomeView.swift | 9 ++-- .../FirezoneKit/Models/AuthResponse.swift | 6 +-- .../Sources/FirezoneKit/Stores/AppStore.swift | 15 +++--- .../FirezoneKit/Stores/AuthStore.swift | 47 +++++++++++-------- .../FirezoneKit/Stores/TunnelStore.swift | 42 ++++++++++++++--- .../Sources/FirezoneKit/Views/MenuBar.swift | 44 +++++++---------- .../FirezoneNetworkExtension/Adapter.swift | 12 ++--- .../NetworkSettings.swift | 4 +- 10 files changed, 111 insertions(+), 81 deletions(-) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/AuthClient/AuthClient.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/AuthClient/AuthClient.swift index 572a44b81..69b56c6fb 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/AuthClient/AuthClient.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/AuthClient/AuthClient.swift @@ -81,12 +81,8 @@ private final class WebAuthenticationSession: NSObject, return } - do { - let authResponse = try AuthResponse(portalURL: host, token: token, actorName: actorName) - continuation.resume(returning: authResponse) - } catch { - continuation.resume(throwing: AuthClientError.authResponseError(error)) - } + let authResponse = AuthResponse(portalURL: host, token: token, actorName: actorName) + continuation.resume(returning: authResponse) } session.presentationContextProvider = self diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift index 1c19e2ffc..c854ae907 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift @@ -18,7 +18,10 @@ final class MainViewModel: ObservableObject { private let appStore: AppStore var authResponse: AuthResponse? { - appStore.auth.authResponse + switch appStore.auth.loginStatus { + case .signedIn(let authResponse): return authResponse + default: return nil + } } var status: NEVPNStatus { diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/WelcomeView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/WelcomeView.swift index 67456d138..a9f376b34 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/WelcomeView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/WelcomeView.swift @@ -59,16 +59,17 @@ final class WelcomeViewModel: ObservableObject { destination = .undefinedSettingsAlert(.undefinedSettings) } - appStore.auth.$authResponse + appStore.auth.$loginStatus .receive(on: mainQueue) - .sink(receiveValue: { [weak self] authResponse in + .sink(receiveValue: { [weak self] loginStatus in guard let self else { return } - if authResponse != nil { + switch loginStatus { + case .signedIn: self.state = .authenticated(MainViewModel(appStore: self.appStore)) - } else { + default: self.state = .unauthenticated(AuthViewModel()) } }) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/AuthResponse.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/AuthResponse.swift index 576d3613e..9effea3c1 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/AuthResponse.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/AuthResponse.swift @@ -16,7 +16,7 @@ struct AuthResponse { // The opaque auth token let token: String - init(portalURL: URL, token: String, actorName: String?) throws { + init(portalURL: URL, token: String, actorName: String?) { self.portalURL = portalURL self.actorName = actorName self.token = token @@ -26,14 +26,14 @@ struct AuthResponse { #if DEBUG extension AuthResponse { static let invalid = - try! AuthResponse( + AuthResponse( portalURL: URL(string: "http://localhost:4568")!, token: "", actorName: nil ) static let valid = - try! AuthResponse( + AuthResponse( portalURL: URL(string: "http://localhost:4568")!, token: "b1zwwwAdf=", actorName: "foobar" diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift index f9740df70..e869ebb11 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift @@ -31,26 +31,29 @@ final class AppStore: ObservableObject { } .store(in: &cancellables) - auth.$authResponse + auth.$loginStatus .receive(on: mainQueue) - .sink { [weak self] authResponse in + .sink { [weak self] loginStatus in Task { [weak self] in - await self?.handleAuthResponseChanged(authResponse) + await self?.handleLoginStatusChanged(loginStatus) } } .store(in: &cancellables) } - private func handleAuthResponseChanged(_ authResponse: AuthResponse?) async { - if let authResponse = authResponse { + private func handleLoginStatusChanged(_ loginStatus: AuthStore.LoginStatus) async { + switch loginStatus { + case .signedIn(let authResponse): do { try await tunnel.start(authResponse: authResponse) } catch { logger.error("Error starting tunnel: \(String(describing: error)) -- signing out") auth.signOut() } - } else { + case .signedOut: tunnel.stop() + case .uninitialized: + break } } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift index 0a34d6a20..e11e799e8 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift @@ -26,6 +26,12 @@ final class AuthStore: ObservableObject { static let shared = AuthStore() + enum LoginStatus { + case uninitialized + case signedOut + case signedIn(AuthResponse) + } + @Dependency(\.keychain) private var keychain @Dependency(\.auth) private var auth @Dependency(\.settingsClient) private var settingsClient @@ -33,43 +39,44 @@ final class AuthStore: ObservableObject { private let authBaseURL: URL private var cancellables = Set() - @Published private(set) var authResponse: AuthResponse? + @Published private(set) var loginStatus: LoginStatus private init() { self.authBaseURL = Self.getAuthBaseURLFromInfoPlist() + self.loginStatus = .uninitialized Task { - self.authResponse = await { () -> AuthResponse? in + self.loginStatus = await { () -> LoginStatus in guard let teamId = settingsClient.fetchSettings()?.teamId else { logger.debug("No team-id found in settings") - return nil + return .signedOut } guard let token = try? await keychain.token() else { logger.debug("Token not found in keychain") - return nil + return .signedOut } guard let actorName = try? await keychain.actorName() else { logger.debug("Actor not found in keychain") - return nil + return .signedOut } let portalURL = self.authURL(teamId: teamId) - guard let authResponse = try? AuthResponse(portalURL: portalURL, token: token, actorName: actorName) else { - logger.debug("Token or Actor recovered from keychain is invalid") - return nil - } + let authResponse = AuthResponse(portalURL: portalURL, token: token, actorName: actorName) logger.debug("Token recovered from keychain.") - return authResponse + return .signedIn(authResponse) }() } - $authResponse.dropFirst() - .sink { [weak self] authResponse in + $loginStatus + .sink { [weak self] loginStatus in Task { [weak self] in - if let authResponse { - try? await self?.keychain.save(token: authResponse.token, actorName: authResponse.actorName) - self?.logger.debug("authResponse saved on keychain.") - } else { - try? await self?.keychain.deleteAuthResponse() - self?.logger.debug("token deleted from keychain.") + switch loginStatus { + case .signedIn(let authResponse): + try? await self?.keychain.save(token: authResponse.token, actorName: authResponse.actorName) + self?.logger.debug("authResponse saved on keychain.") + case .signedOut: + try? await self?.keychain.deleteAuthResponse() + self?.logger.debug("token deleted from keychain.") + case .uninitialized: + break } } } @@ -81,7 +88,7 @@ final class AuthStore: ObservableObject { let portalURL = authURL(teamId: teamId) let authResponse = try await auth.signIn(portalURL) - self.authResponse = authResponse + self.loginStatus = .signedIn(authResponse) } func signIn() async throws { @@ -98,7 +105,7 @@ final class AuthStore: ObservableObject { func signOut() { logger.trace("\(#function)") - authResponse = nil + loginStatus = .signedOut } static func getAuthBaseURLFromInfoPlist() -> URL { diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift index 4938dc882..2d695d65d 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift @@ -20,7 +20,7 @@ final class TunnelStore: ObservableObject { didSet { setupTunnelObservers() } } - @Published private(set) var status: NEVPNStatus = .invalid { + @Published private(set) var status: NEVPNStatus { didSet { TunnelStore.logger.info("status changed: \(self.status.description)") } } @@ -41,6 +41,7 @@ final class TunnelStore: ObservableObject { init(tunnel: NETunnelProviderManager) { self.controlPlaneURL = Self.getControlPlaneURLFromInfoPlist() self.tunnel = tunnel + self.status = tunnel.connection.status tunnel.isEnabled = true setupTunnelObservers() } @@ -67,6 +68,16 @@ final class TunnelStore: ObservableObject { // make sure we have latest preferences before starting try await tunnel.loadFromPreferences() + if tunnel.connection.status == .connected || tunnel.connection.status == .connecting { + if let (tunnelControlPlaneURLString, tunnelToken) = Self.getTunnelConfigurationParameters(of: tunnel) { + if tunnelControlPlaneURLString == self.controlPlaneURL.absoluteString && tunnelToken == authResponse.token { + // Already connected / connecting with the required configuration + TunnelStore.logger.debug("\(#function): Already connected / connecting. Nothing to do.") + return + } + } + } + tunnel.protocolConfiguration = Self.makeProtocolConfiguration( controlPlaneURL: self.controlPlaneURL, token: authResponse.token @@ -104,6 +115,10 @@ final class TunnelStore: ObservableObject { private func updateResources() { let session = tunnel.connection as! NETunnelProviderSession + guard session.status == .connected else { + self.resources = DisplayableResources() + return + } let resourcesQuery = resources.versionStringToData() do { try session.sendProviderMessage(resourcesQuery) { [weak self] reply in @@ -162,6 +177,19 @@ final class TunnelStore: ObservableObject { return proto } + private static func getTunnelConfigurationParameters(of tunnelProvider: NETunnelProviderManager) -> (String, String)? { + guard let tunnelProtocol = tunnelProvider.protocolConfiguration as? NETunnelProviderProtocol else { + return nil + } + guard let controlPlaneURLString = tunnelProtocol.providerConfiguration?["controlPlaneURL"] as? String else { + return nil + } + guard let token = tunnelProtocol.providerConfiguration?["token"] as? String else { + return nil + } + return (controlPlaneURLString, token) + } + private func setupTunnelObservers() { TunnelStore.logger.trace("\(#function)") @@ -174,14 +202,13 @@ final class TunnelStore: ObservableObject { named: .NEVPNStatusDidChange, object: nil ) { - guard let session = notification.object as? NETunnelProviderSession, - let tunnelProvider = session.manager as? NETunnelProviderManager - else { + guard let session = notification.object as? NETunnelProviderSession else { return } - self.status = tunnelProvider.connection.status + let status = session.status + self.status = status if let startTunnelContinuation = self.startTunnelContinuation { - switch self.status { + switch status { case .connected: startTunnelContinuation.resume(returning: ()) self.startTunnelContinuation = nil @@ -192,6 +219,9 @@ final class TunnelStore: ObservableObject { break } } + if status != .connected { + self.resources = DisplayableResources() + } } } ) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift index b41d8b876..d540fe9a5 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift @@ -53,17 +53,19 @@ public final class MenuBar: NSObject { Task { let tunnel = try await TunnelStore.loadOrCreate() self.appStore = AppStore(tunnelStore: TunnelStore(tunnel: tunnel)) + updateStatusItemIcon() } } private func setupObservers() { - appStore?.auth.$authResponse + appStore?.auth.$loginStatus .receive(on: mainQueue) - .sink { [weak self] authResponse in - if let authResponse { - self?.showSignedIn(authResponse.actorName) - } else { - self?.showSignedOut() + .sink { [weak self] loginStatus in + switch loginStatus { + case .signedIn(let authResponse): + self?.showSignedIn(authResponse.actorName) + default: + self?.showSignedOut() } } .store(in: &cancellables) @@ -71,17 +73,8 @@ public final class MenuBar: NSObject { appStore?.tunnel.$status .receive(on: mainQueue) .sink { [weak self] status in - if status == .connected { - self?.connectionMenuItem.title = "Disconnect" - self?.statusItem.button?.image = self?.connectedIcon - } else { - self?.connectionMenuItem.title = "Connect" - self?.statusItem.button?.image = self?.disconnectedIcon - } + self?.updateStatusItemIcon() self?.handleMenuVisibilityOrStatusChanged() - if status != .connected { - self?.setOrderedResources([]) - } } .store(in: &cancellables) @@ -96,14 +89,6 @@ public final class MenuBar: NSObject { private lazy var menu = NSMenu() - private lazy var connectionMenuItem = createMenuItem( - menu, - title: "Connect", - action: #selector(connectButtonTapped), - isHidden: true, - target: self - ) - private lazy var signInMenuItem = createMenuItem( menu, title: "Sign in", @@ -152,7 +137,6 @@ public final class MenuBar: NSObject { }() private func createMenu() { - menu.addItem(connectionMenuItem) menu.addItem(signInMenuItem) menu.addItem(signOutMenuItem) menu.addItem(NSMenuItem.separator()) @@ -208,7 +192,7 @@ public final class MenuBar: NSObject { appStore?.tunnel.stop() } else { Task { - if let authResponse = appStore?.auth.authResponse { + if case .signedIn(let authResponse) = appStore?.auth.loginStatus { do { try await appStore?.tunnel.start(authResponse: authResponse) } catch { @@ -249,6 +233,14 @@ public final class MenuBar: NSObject { NSWorkspace.shared.open(URL(string: "firezone://settings")!) } + private func updateStatusItemIcon() { + if self.appStore?.tunnel.status == .connected { + self.statusItem.button?.image = self.connectedIcon + } else { + self.statusItem.button?.image = self.disconnectedIcon + } + } + private func handleMenuVisibilityOrStatusChanged() { guard let appStore = appStore else { return } let status = appStore.tunnel.status diff --git a/swift/apple/FirezoneNetworkExtension/Adapter.swift b/swift/apple/FirezoneNetworkExtension/Adapter.swift index 86aeaab42..ad3863184 100644 --- a/swift/apple/FirezoneNetworkExtension/Adapter.swift +++ b/swift/apple/FirezoneNetworkExtension/Adapter.swift @@ -101,7 +101,7 @@ public class Adapter { // Shutdown the tunnel if case .tunnelReady(let wrappedSession) = self.state { logger.debug("Adapter.deinit: Shutting down connlib") - _ = wrappedSession.disconnect() + wrappedSession.disconnect() } } @@ -148,15 +148,14 @@ public class Adapter { case .tunnelReady(let session): self.logger.debug("Adapter.stop: Shutting down connlib") self.state = .stoppingTunnel(session: session, onStopped: completionHandler) - _ = session.disconnect() + session.disconnect() case .startingTunnel(let session, let onStarted): self.logger.debug("Adapter.stop: Shutting down connlib before tunnel ready") self.state = .stoppingTunnel(session: session, onStopped: { onStarted?(AdapterError.stoppedByRequestWhileStarting) completionHandler() }) - // FIXME: Is it ok to call disconnect() while we haven't got onTunnelReady? - _ = session.disconnect() + session.disconnect() case .stoppingTunnelTemporarily(let session, let onStopped): self.state = .stoppingTunnel(session: session, onStopped: { onStopped?() @@ -210,8 +209,7 @@ extension Adapter { onStarted?(nil) self.packetTunnelProvider?.reasserting = true self.state = .stoppingTunnelTemporarily(session: session, onStopped: nil) - // FIXME: Is it ok to call disconnect() while we haven't got onTunnelReady? - _ = session.disconnect() + session.disconnect() } case .tunnelReady(let session): @@ -225,7 +223,7 @@ extension Adapter { self.logger.debug("Adapter.didReceivePathUpdate: Offline. Shutting down connlib.") self.packetTunnelProvider?.reasserting = true self.state = .stoppingTunnelTemporarily(session: session, onStopped: nil) - _ = session.disconnect() + session.disconnect() } case .stoppingTunnelTemporarily: diff --git a/swift/apple/FirezoneNetworkExtension/NetworkSettings.swift b/swift/apple/FirezoneNetworkExtension/NetworkSettings.swift index 1ffea3647..5c575e4f5 100644 --- a/swift/apple/FirezoneNetworkExtension/NetworkSettings.swift +++ b/swift/apple/FirezoneNetworkExtension/NetworkSettings.swift @@ -110,7 +110,7 @@ class NetworkSettings { } let validNetworkPrefixLength = Self.validNetworkPrefixLength(fromString: networkPrefixLengthString, maximumValue: 32) let ipv4SubnetMask = Self.ipv4SubnetMask(networkPrefixLength: validNetworkPrefixLength) - logger.debug("NetworkSettings.apply: Adding IPv4 route: \(address) (subnet mask: \(ipv4SubnetMask))") + logger.debug("NetworkSettings.apply: Adding IPv4 route: \(address, privacy: .public) (subnet mask: \(ipv4SubnetMask, privacy: .public))") tunnelIPv4Routes.append(NEIPv4Route(destinationAddress: address, subnetMask: ipv4SubnetMask)) } if groupSeparator == ":" { // IPv6 address @@ -119,7 +119,7 @@ class NetworkSettings { continue } let validNetworkPrefixLength = Self.validNetworkPrefixLength(fromString: networkPrefixLengthString, maximumValue: 128) - logger.debug("NetworkSettings.apply: Adding IPv6 route: \(address) (prefix length: \(validNetworkPrefixLength))") + logger.debug("NetworkSettings.apply: Adding IPv6 route: \(address, privacy: .public) (prefix length: \(validNetworkPrefixLength, privacy: .public))") tunnelIPv6Routes.append(NEIPv6Route(destinationAddress: address, networkPrefixLength: NSNumber(integerLiteral: validNetworkPrefixLength))) } } else {