From e9cd4623fde605d1d3fae846dd5f6147322154f9 Mon Sep 17 00:00:00 2001 From: Roopesh Chander Date: Thu, 7 Dec 2023 18:45:42 +0530 Subject: [PATCH] Remove handling account ID in app, prevent changing settings when signed in (#2804) This PR - Addresses the Apple clients part of #2791 - Fixes #2668 The Settings UI change of separating Logs and Advanced settings is not part of this PR. --- .../FirezoneKit/Features/AuthView.swift | 9 +- .../FirezoneKit/Features/MainView.swift | 6 +- .../FirezoneKit/Features/SettingsView.swift | 230 +++++++----------- .../FirezoneKit/Features/WelcomeView.swift | 4 - .../FirezoneKit/Keychain/Keychain.swift | 14 +- .../Keychain/KeychainStorage.swift | 6 +- .../FirezoneKit/Models/FirezoneError.swift | 1 - .../Sources/FirezoneKit/Models/Settings.swift | 25 +- .../Sources/FirezoneKit/Stores/AppStore.swift | 2 +- .../FirezoneKit/Stores/AuthStore.swift | 135 ++++------ .../FirezoneKit/Stores/TunnelStore.swift | 97 ++++---- .../Sources/FirezoneKit/Views/MenuBar.swift | 10 +- 12 files changed, 223 insertions(+), 316 deletions(-) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/AuthView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/AuthView.swift index c74234a3e..9336a888f 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/AuthView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/AuthView.swift @@ -19,15 +19,8 @@ final class AuthViewModel: ObservableObject { private var cancellables = Set() func signInButtonTapped() async { - guard let accountId = authStore.tunnelStore.tunnelAuthStatus.accountId(), - !accountId.isEmpty - else { - settingsUndefined() - return - } - do { - try await authStore.signIn(accountId: accountId) + try await authStore.signIn() } catch { dump(error) } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift index 3b601877c..a20aab6d0 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/MainView.swift @@ -60,7 +60,9 @@ import SwiftUI } func signOutButtonTapped() { - appStore.auth.signOut() + Task { + await appStore.auth.signOut() + } } func startTunnel() async { @@ -88,7 +90,7 @@ import SwiftUI Section(header: Text("Authentication")) { Group { switch self.model.loginStatus { - case .signedIn(_, let actorName): + case .signedIn(let actorName): HStack { Text(actorName.isEmpty ? "Signed in" : "Signed in as") Spacer() diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/SettingsView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/SettingsView.swift index ce048eb75..6f0d3f6f7 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/SettingsView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/SettingsView.swift @@ -20,14 +20,16 @@ public final class SettingsViewModel: ObservableObject { @Dependency(\.authStore) private var authStore - @Published var accountSettings: AccountSettings + var tunnelAuthStatus: TunnelAuthStatus { + authStore.tunnelStore.tunnelAuthStatus + } + @Published var advancedSettings: AdvancedSettings public var onSettingsSaved: () -> Void = unimplemented() private var cancellables = Set() public init() { - accountSettings = AccountSettings() advancedSettings = AdvancedSettings.defaultValue loadSettings() } @@ -35,11 +37,10 @@ public final class SettingsViewModel: ObservableObject { func loadSettings() { Task { authStore.tunnelStore.$tunnelAuthStatus - .filter { $0.isInitialized } + .first { $0.isInitialized } .receive(on: RunLoop.main) .sink { [weak self] tunnelAuthStatus in guard let self = self else { return } - self.accountSettings = AccountSettings(accountId: tunnelAuthStatus.accountId() ?? "") self.advancedSettings = authStore.tunnelStore.advancedSettings() ?? AdvancedSettings.defaultValue } @@ -47,27 +48,22 @@ public final class SettingsViewModel: ObservableObject { } } - func saveAccountSettings() { - Task { - let accountId = await authStore.loginStatus.accountId - if accountId == accountSettings.accountId { - // Not changed - await MainActor.run { - accountSettings.isSavedToDisk = true - } - return - } - try await updateTunnelAuthStatus(accountId: accountSettings.accountId) - await MainActor.run { - accountSettings.isSavedToDisk = true - } - } - } - func saveAdvancedSettings() { + let isChanged = (authStore.tunnelStore.advancedSettings() != advancedSettings) + guard isChanged else { + advancedSettings.isSavedToDisk = true + return + } Task { - guard let authBaseURL = URL(string: advancedSettings.authBaseURLString) else { - fatalError("Saved authBaseURL is invalid") + if case .signedIn = self.tunnelAuthStatus { + await authStore.signOut() + } + let authBaseURLString = advancedSettings.authBaseURLString + guard URL(string: authBaseURLString) != nil else { + logger.error( + "Not saving advanced settings because authBaseURL '\(authBaseURLString, privacy: .public)' is invalid" + ) + return } do { try await authStore.tunnelStore.saveAdvancedSettings(advancedSettings) @@ -77,30 +73,6 @@ public final class SettingsViewModel: ObservableObject { await MainActor.run { advancedSettings.isSavedToDisk = true } - var isChanged = false - if isChanged { - try await updateTunnelAuthStatus( - accountId: authStore.tunnelStore.tunnelAuthStatus.accountId() ?? "") - } - } - } - - // updateTunnelAuthStatus: - // When the authBaseURL or the accountId changes, we should update the signed-in-ness. - // This is done by searching the keychain for an entry with the authBaseURL+accountId - // combination. If an entry was found, we consider that entry to mean we're logged in. - func updateTunnelAuthStatus(accountId: String) async throws { - let tunnelAuthStatus: TunnelAuthStatus = await { - if accountId.isEmpty { - return .accountNotSetup - } else { - return await authStore.tunnelAuthStatusForAccount(accountId: accountId) - } - }() - do { - try await authStore.tunnelStore.saveAuthStatus(tunnelAuthStatus) - } catch { - logger.error("Error saving auth status to tunnel store: \(error, privacy: .public)") } } } @@ -111,7 +83,26 @@ public struct SettingsView: View { @ObservedObject var model: SettingsViewModel @Environment(\.dismiss) var dismiss + enum ConfirmationAlertContinueAction: Int { + case none + case saveAdvancedSettings + case saveAllSettingsAndDismiss + + func performAction(on view: SettingsView) { + switch self { + case .none: + break + case .saveAdvancedSettings: + view.saveAdvancedSettings() + case .saveAllSettingsAndDismiss: + view.saveAllSettingsAndDismiss() + } + } + } + @State private var isExportingLogs = false + @State private var isShowingConfirmationAlert = false + @State private var confirmationAlertContinueAction: ConfirmationAlertContinueAction = .none #if os(iOS) @State private var logTempZipFileURL: URL? @@ -142,13 +133,6 @@ public struct SettingsView: View { #if os(iOS) NavigationView { TabView { - accountTab - .tabItem { - Image(systemName: "person.3.fill") - Text("Account") - } - .badge(model.accountSettings.isValid ? nil : "!") - advancedTab .tabItem { Image(systemName: "slider.horizontal.3") @@ -159,12 +143,16 @@ public struct SettingsView: View { .toolbar { ToolbarItem(placement: .navigationBarTrailing) { Button("Save") { - self.saveSettings() + let action = ConfirmationAlertContinueAction.saveAllSettingsAndDismiss + if case .signedIn = model.tunnelAuthStatus { + self.confirmationAlertContinueAction = action + self.isShowingConfirmationAlert = true + } else { + action.performAction(on: self) + } } .disabled( - (model.accountSettings.isSavedToDisk && model.advancedSettings.isSavedToDisk) - || !model.accountSettings.isValid - || !model.advancedSettings.isValid + (model.advancedSettings.isSavedToDisk || !model.advancedSettings.isValid) ) } ToolbarItem(placement: .navigationBarLeading) { @@ -176,13 +164,26 @@ public struct SettingsView: View { .navigationTitle("Settings") .navigationBarTitleDisplayMode(.inline) } + .alert( + "Saving settings will sign you out", + isPresented: $isShowingConfirmationAlert, + presenting: confirmationAlertContinueAction, + actions: { confirmationAlertContinueAction in + Button("Cancel", role: .cancel) { + // Nothing to do + } + Button("Continue") { + confirmationAlertContinueAction.performAction(on: self) + } + }, + message: { _ in + Text("Changing settings will sign you out and disconnect you from resources") + } + ) + #elseif os(macOS) VStack { TabView { - accountTab - .tabItem { - Text("Account") - } advancedTab .tabItem { Text("Advanced") @@ -190,86 +191,28 @@ public struct SettingsView: View { } .padding(20) } + .alert( + "Saving settings will sign you out", + isPresented: $isShowingConfirmationAlert, + presenting: confirmationAlertContinueAction, + actions: { confirmationAlertContinueAction in + Button("Cancel", role: .cancel) { + // Nothing to do + } + Button("Continue", role: .destructive) { + confirmationAlertContinueAction.performAction(on: self) + } + }, + message: { _ in + Text("Changing settings will sign you out and disconnect you from resources") + } + ) .onDisappear(perform: { self.loadSettings() }) #else #error("Unsupported platform") #endif } - private var accountTab: some View { - #if os(macOS) - VStack { - Spacer() - Form { - Section( - content: { - HStack(spacing: 15) { - Spacer() - Text("Account ID:") - TextField( - "", - text: Binding( - get: { model.accountSettings.accountId }, - set: { model.accountSettings.accountId = $0 } - ), - prompt: Text(PlaceholderText.accountId) - ) - .frame(maxWidth: 240) - .onSubmit { - self.model.saveAccountSettings() - } - Spacer() - } - }, - footer: { - Text(FootnoteText.forAccount) - .foregroundStyle(.secondary) - } - ) - Button( - "Apply", - action: { - self.model.saveAccountSettings() - } - ) - .disabled( - model.accountSettings.isSavedToDisk - || !model.accountSettings.isValid - ) - .padding(.top, 5) - } - Spacer() - } - #elseif os(iOS) - VStack { - Form { - Section( - content: { - HStack(spacing: 15) { - Text("Account ID") - .foregroundStyle(.secondary) - TextField( - PlaceholderText.accountId, - text: Binding( - get: { model.accountSettings.accountId }, - set: { model.accountSettings.accountId = $0 } - ) - ) - .autocorrectionDisabled() - .textInputAutocapitalization(.never) - .submitLabel(.done) - } - }, - header: { Text("Account") }, - footer: { Text(FootnoteText.forAccount) } - ) - } - } - #else - #error("Unsupported platform") - #endif - } - private var advancedTab: some View { #if os(macOS) VStack { @@ -311,7 +254,13 @@ public struct SettingsView: View { Button( "Apply", action: { - self.model.saveAdvancedSettings() + let action = ConfirmationAlertContinueAction.saveAdvancedSettings + if case .signedIn = model.tunnelAuthStatus { + self.confirmationAlertContinueAction = action + self.isShowingConfirmationAlert = true + } else { + action.performAction(on: self) + } } ) .disabled(model.advancedSettings.isSavedToDisk || !model.advancedSettings.isValid) @@ -429,8 +378,11 @@ public struct SettingsView: View { #endif } - func saveSettings() { - model.saveAccountSettings() + func saveAdvancedSettings() { + model.saveAdvancedSettings() + } + + func saveAllSettingsAndDismiss() { model.saveAdvancedSettings() dismiss() } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/WelcomeView.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/WelcomeView.swift index f939887dd..508c5e3ec 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/WelcomeView.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Features/WelcomeView.swift @@ -55,10 +55,6 @@ import SwiftUINavigationCore defer { bindDestination() } - if case .accountNotSetup = appStore.tunnel.tunnelAuthStatus { - destination = .undefinedSettingsAlert(.undefinedSettings) - } - appStore.auth.$loginStatus .receive(on: mainQueue) .sink(receiveValue: { [weak self] loginStatus in diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/Keychain.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/Keychain.swift index e84ac3cc8..20a6e6471 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/Keychain.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/Keychain.swift @@ -25,7 +25,7 @@ public actor Keychain { public typealias PersistentRef = Data public struct TokenAttributes { - let authURLString: String + let authBaseURLString: String let actorName: String } @@ -56,7 +56,7 @@ public actor Keychain { kSecClass: kSecClassGenericPassword, kSecAttrLabel: "Firezone access token (\(tokenAttributes.actorName))", kSecAttrDescription: "Firezone access token", - kSecAttrService: tokenAttributes.authURLString, + kSecAttrService: tokenAttributes.authBaseURLString, // The UUID uniquifies this item in the keychain kSecAttrAccount: "\(tokenAttributes.actorName): \(UUID().uuidString)", kSecValueData: token.data(using: .utf8) as Any, @@ -72,7 +72,7 @@ public actor Keychain { kSecClass: kSecClassGenericPassword, kSecAttrLabel: "Firezone access token (\(tokenAttributes.actorName))", kSecAttrDescription: "Firezone access token", - kSecAttrService: tokenAttributes.authURLString, + kSecAttrService: tokenAttributes.authBaseURLString, // The UUID uniquifies this item in the keychain kSecAttrAccount: "\(tokenAttributes.actorName): \(UUID().uuidString)", kSecValueData: token.data(using: .utf8) as Any, @@ -100,7 +100,7 @@ public actor Keychain { let checkForStaleItemsQuery = [ kSecClass: kSecClassGenericPassword, - kSecAttrService: tokenAttributes.authURLString, + kSecAttrService: tokenAttributes.authBaseURLString, kSecMatchLimit: kSecMatchLimitAll, kSecReturnPersistentRef: true, ] as [CFString: Any] @@ -217,7 +217,7 @@ public actor Keychain { .startIndex..<(account.lastIndex(of: ":") ?? account.endIndex)]) let attributes = TokenAttributes( - authURLString: service, + authBaseURLString: service, actorName: actorName) continuation.resume(returning: attributes) return @@ -244,14 +244,14 @@ public actor Keychain { } } - func search(authURLString: String) async -> PersistentRef? { + func search(authBaseURLString: String) async -> PersistentRef? { return await withCheckedContinuation { [weak self] continuation in self?.workQueue.async { let query = [ kSecClass: kSecClassGenericPassword, kSecAttrDescription: "Firezone access token", - kSecAttrService: authURLString, + kSecAttrService: authBaseURLString, kSecReturnPersistentRef: true, ] as [CFString: Any] var result: CFTypeRef? diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/KeychainStorage.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/KeychainStorage.swift index 6bac90ff6..47f534c4b 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/KeychainStorage.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Keychain/KeychainStorage.swift @@ -12,7 +12,7 @@ struct KeychainStorage: Sendable { @Sendable (Keychain.Token, Keychain.TokenAttributes) async throws -> Keychain.PersistentRef var delete: @Sendable (Keychain.PersistentRef) async throws -> Void var loadAttributes: @Sendable (Keychain.PersistentRef) async -> Keychain.TokenAttributes? - var searchByAuthURL: @Sendable (URL) async -> Keychain.PersistentRef? + var searchByAuthBaseURL: @Sendable (URL) async -> Keychain.PersistentRef? } extension KeychainStorage: DependencyKey { @@ -23,7 +23,7 @@ extension KeychainStorage: DependencyKey { store: { try await keychain.store(token: $0, tokenAttributes: $1) }, delete: { try await keychain.delete(persistentRef: $0) }, loadAttributes: { await keychain.loadAttributes(persistentRef: $0) }, - searchByAuthURL: { await keychain.search(authURLString: $0.absoluteString) } + searchByAuthBaseURL: { await keychain.search(authBaseURLString: $0.absoluteString) } ) } @@ -45,7 +45,7 @@ extension KeychainStorage: DependencyKey { loadAttributes: { ref in storage.value[ref]?.1 }, - searchByAuthURL: { _ in + searchByAuthBaseURL: { _ in nil } ) diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/FirezoneError.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/FirezoneError.swift index 8da241ac4..5c9670d88 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/FirezoneError.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/FirezoneError.swift @@ -7,5 +7,4 @@ import Foundation enum FirezoneError: Error { - case missingTeamId } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/Settings.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/Settings.swift index eafda603d..691cf1196 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/Settings.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Models/Settings.swift @@ -6,25 +6,6 @@ import Foundation -struct AccountSettings { - var accountId: String = "" { - didSet { if oldValue != accountId { isSavedToDisk = false } } - } - - var isSavedToDisk = true - - var isValid: Bool { - !accountId.isEmpty - && accountId.unicodeScalars.allSatisfy { Self.teamIdAllowedCharacterSet.contains($0) } - } - - static let teamIdAllowedCharacterSet: CharacterSet = { - var pathAllowed = CharacterSet.urlPathAllowed - pathAllowed.remove("/") - return pathAllowed - }() -} - struct AdvancedSettings: Equatable { var authBaseURLString: String { didSet { if oldValue != authBaseURLString { isSavedToDisk = false } } @@ -65,3 +46,9 @@ struct AdvancedSettings: Equatable { // Note: To see what the connlibLogFilterString values mean, see: // https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.EnvFilter.html } + +extension AdvancedSettings: CustomStringConvertible { + var description: String { + "(\(authBaseURLString), \(apiURLString), \(connlibLogFilterString))" + } +} diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift index d78526e01..a7410e9fa 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AppStore.swift @@ -36,7 +36,7 @@ final class AppStore: ObservableObject { Task { do { try await tunnel.stop() - auth.signOut() + await auth.signOut() } catch { logger.error("\(#function): Error stopping tunnel: \(String(describing: error))") } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift index 0bd5f0a23..c4fe8ef61 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/AuthStore.swift @@ -29,25 +29,17 @@ final class AuthStore: ObservableObject { enum LoginStatus: CustomStringConvertible { case uninitialized - case signedOut(accountId: String?) - case signedIn(accountId: String, actorName: String) - - var accountId: String? { - switch self { - case .uninitialized: return nil - case .signedOut(let accountId): return accountId - case .signedIn(let accountId, _): return accountId - } - } + case signedOut + case signedIn(actorName: String) var description: String { switch self { case .uninitialized: return "uninitialized" - case .signedOut(let accountId): - return "signedOut(accountId: \(accountId ?? "nil"))" - case .signedIn(let accountId, let actorName): - return "signedIn(accountId: \(accountId), actorName: \(actorName))" + case .signedOut: + return "signedOut" + case .signedIn(let actorName): + return "signedIn(actorName: \(actorName))" } } } @@ -96,22 +88,6 @@ final class AuthStore: ObservableObject { .sink { [weak self] status in guard let self = self else { return } Task { - if status == .disconnected { - self.logger.log("\(#function): Disconnected") - if let tsEvent = TunnelShutdownEvent.loadFromDisk() { - self.logger.log( - "\(#function): Tunnel shutdown event: \(tsEvent, privacy: .public)" - ) - switch tsEvent.action { - case .signoutImmediately: - self.signOut() - case .retryThenSignout: - self.retryStartTunnel() - } - } else { - self.logger.log("\(#function): Tunnel shutdown event not found") - } - } if status == .connected { self.resetReconnectionAttemptsRemaining() } @@ -134,51 +110,35 @@ final class AuthStore: ObservableObject { switch tunnelAuthStatus { case .tunnelUninitialized: return .uninitialized - case .accountNotSetup: - return .signedOut(accountId: nil) - case .signedOut(_, let tunnelAccountId): - return .signedOut(accountId: tunnelAccountId) - case .signedIn(let tunnelAuthBaseURL, let tunnelAccountId, let tokenReference): + case .signedOut: + return .signedOut + case .signedIn(let tunnelAuthBaseURL, let tokenReference): guard self.authBaseURL == tunnelAuthBaseURL else { - return .signedOut(accountId: tunnelAccountId) + return .signedOut } - let tunnelPortalURLString = self.authURL(accountId: tunnelAccountId).absoluteString + let tunnelBaseURLString = self.authBaseURL.absoluteString guard let tokenAttributes = await keychain.loadAttributes(tokenReference), - tunnelPortalURLString == tokenAttributes.authURLString + tunnelBaseURLString == tokenAttributes.authBaseURLString else { - return .signedOut(accountId: tunnelAccountId) + return .signedOut } - return .signedIn(accountId: tunnelAccountId, actorName: tokenAttributes.actorName) + return .signedIn(actorName: tokenAttributes.actorName) } } - func signIn(accountId: String) async throws { - logger.trace("\(#function)") - - let portalURL = authURL(accountId: accountId) - let authResponse = try await auth.signIn(portalURL) - let attributes = Keychain.TokenAttributes( - authURLString: portalURL.absoluteString, actorName: authResponse.actorName ?? "") - let tokenRef = try await keychain.store(authResponse.token, attributes) - - try await tunnelStore.saveAuthStatus( - .signedIn(authBaseURL: self.authBaseURL, accountId: accountId, tokenReference: tokenRef)) - } - func signIn() async throws { logger.trace("\(#function)") - guard case .signedOut(let accountId) = self.loginStatus, let accountId = accountId, - !accountId.isEmpty - else { - logger.log("No account-id found in tunnel") - throw FirezoneError.missingTeamId - } + let authResponse = try await auth.signIn(self.authBaseURL) + let attributes = Keychain.TokenAttributes( + authBaseURLString: self.authBaseURL.absoluteString, actorName: authResponse.actorName ?? "") + let tokenRef = try await keychain.store(authResponse.token, attributes) - try await signIn(accountId: accountId) + try await tunnelStore.saveAuthStatus( + .signedIn(authBaseURL: self.authBaseURL, tokenReference: tokenRef)) } - func signOut() { + func signOut() async { logger.trace("\(#function)") guard case .signedIn = self.tunnelStore.tunnelAuthStatus else { @@ -186,10 +146,13 @@ final class AuthStore: ObservableObject { return } - Task { - if let tokenRef = try await tunnelStore.stopAndSignOut() { + do { + try await tunnelStore.stop() + if let tokenRef = try await tunnelStore.signOut() { try await keychain.delete(tokenRef) } + } catch { + logger.error("\(#function): Error signing out: \(error, privacy: .public)") } resetReconnectionAttemptsRemaining() @@ -208,7 +171,21 @@ final class AuthStore: ObservableObject { try await tunnelStore.start() } catch { logger.error("\(#function): Error starting tunnel: \(String(describing: error))") - self.retryStartTunnel() + if let tsEvent = TunnelShutdownEvent.loadFromDisk() { + self.logger.log( + "\(#function): Tunnel shutdown event: \(tsEvent, privacy: .public)" + ) + switch tsEvent.action { + case .signoutImmediately: + Task { + await self.signOut() + } + case .retryThenSignout: + self.retryStartTunnel() + } + } else { + self.logger.log("\(#function): Tunnel shutdown event not found") + } } } } @@ -227,7 +204,9 @@ final class AuthStore: ObservableObject { self.startTunnel() } } else { - self.signOut() + Task { + await self.signOut() + } } } @@ -235,14 +214,7 @@ final class AuthStore: ObservableObject { 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() - } - } + self.startTunnel() case .signedOut: Task { do { @@ -250,6 +222,10 @@ final class AuthStore: ObservableObject { } catch { logger.error("\(#function): Error stopping tunnel: \(String(describing: error))") } + if tunnelStore.tunnelAuthStatus != .signedOut { + // Bring tunnelAuthStatus in line, in case it's out of touch with the login status + try await tunnelStore.saveAuthStatus(.signedOut) + } } case .uninitialized: break @@ -260,16 +236,11 @@ final class AuthStore: ObservableObject { self.reconnectionAttemptsRemaining = Self.maxReconnectionAttemptCount } - func tunnelAuthStatusForAccount(accountId: String) async -> TunnelAuthStatus { - let portalURL = authURL(accountId: accountId) - if let tokenRef = await keychain.searchByAuthURL(portalURL) { - return .signedIn(authBaseURL: authBaseURL, accountId: accountId, tokenReference: tokenRef) + func tunnelAuthStatus(for authBaseURL: URL) async -> TunnelAuthStatus { + if let tokenRef = await keychain.searchByAuthBaseURL(authBaseURL) { + return .signedIn(authBaseURL: authBaseURL, tokenReference: tokenRef) } else { - return .signedOut(authBaseURL: authBaseURL, accountId: accountId) + return .signedOut } } - - func authURL(accountId: String) -> URL { - self.authBaseURL.appendingPathComponent(accountId) - } } diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift index 31bd27c7b..6323009b0 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Stores/TunnelStore.swift @@ -12,12 +12,21 @@ import OSLog enum TunnelStoreError: Error { case tunnelCouldNotBeStarted case tunnelCouldNotBeStopped + case cannotSaveToTunnelWhenConnected + case cannotSignOutWhenConnected + case stopAlreadyBeingAttempted } public struct TunnelProviderKeys { static let keyAuthBaseURLString = "authBaseURLString" - static let keyAccountId = "accountId" public static let keyConnlibLogFilter = "connlibLogFilter" + + // The following key is not added to the tunnel provider. + // The key is defined so that the key can be removed if it exists + // in a tunnel provider configuration. A tunnel configuration + // created by an earlier version of the app could have this + // key, and we use this key definition to remove it. + static let keyAccountId = "accountId" } final class TunnelStore: ObservableObject { @@ -80,7 +89,7 @@ final class TunnelStore: ObservableObject { try await tunnel.saveToPreferences() Self.logger.log("\(#function): Tunnel created") self.tunnel = tunnel - self.tunnelAuthStatus = .accountNotSetup + self.tunnelAuthStatus = .signedOut } setupTunnelObservers() Self.logger.log("\(#function): TunnelStore initialized") @@ -90,24 +99,35 @@ final class TunnelStore: ObservableObject { } func saveAuthStatus(_ tunnelAuthStatus: TunnelAuthStatus) async throws { + Self.logger.log("TunnelStore.\(#function) \(tunnelAuthStatus, privacy: .public)") guard let tunnel = tunnel else { fatalError("Tunnel not initialized yet") } - try await stop() + let tunnelStatus = tunnel.connection.status + if tunnelStatus == .connected || tunnelStatus == .connecting { + throw TunnelStoreError.cannotSaveToTunnelWhenConnected + } + try await tunnel.loadFromPreferences() try await tunnel.saveAuthStatus(tunnelAuthStatus) self.tunnelAuthStatus = tunnelAuthStatus } func saveAdvancedSettings(_ advancedSettings: AdvancedSettings) async throws { + Self.logger.log("TunnelStore.\(#function) \(advancedSettings, privacy: .public)") guard let tunnel = tunnel else { fatalError("Tunnel not initialized yet") } - try await stop() + let tunnelStatus = tunnel.connection.status + if tunnelStatus == .connected || tunnelStatus == .connecting { + throw TunnelStoreError.cannotSaveToTunnelWhenConnected + } + try await tunnel.loadFromPreferences() try await tunnel.saveAdvancedSettings(advancedSettings) + self.tunnelAuthStatus = tunnel.authStatus() } func advancedSettings() -> AdvancedSettings? { @@ -165,6 +185,10 @@ final class TunnelStore: ObservableObject { return } + guard self.stopTunnelContinuation == nil else { + throw TunnelStoreError.stopAlreadyBeingAttempted + } + TunnelStore.logger.trace("\(#function)") let status = tunnel.connection.status @@ -177,23 +201,20 @@ final class TunnelStore: ObservableObject { } } - func stopAndSignOut() async throws -> Keychain.PersistentRef? { + func signOut() async throws -> Keychain.PersistentRef? { guard let tunnel = tunnel else { Self.logger.log("\(#function): TunnelStore is not initialized") return nil } - TunnelStore.logger.trace("\(#function)") - - let status = tunnel.connection.status - if status == .connected || status == .connecting { - let session = castToSession(tunnel.connection) - session.stopTunnel() + let tunnelStatus = tunnel.connection.status + if tunnelStatus == .connected || tunnelStatus == .connecting { + throw TunnelStoreError.cannotSignOutWhenConnected } - if case .signedIn(let authBaseURL, let accountId, let tokenReference) = self.tunnelAuthStatus { + if case .signedIn(_, let tokenReference) = self.tunnelAuthStatus { do { - try await saveAuthStatus(.signedOut(authBaseURL: authBaseURL, accountId: accountId)) + try await saveAuthStatus(.signedOut) } catch { TunnelStore.logger.trace( "\(#function): Error saving signed out auth status: \(error)" @@ -324,9 +345,8 @@ final class TunnelStore: ObservableObject { enum TunnelAuthStatus: Equatable, CustomStringConvertible { case tunnelUninitialized - case accountNotSetup - case signedOut(authBaseURL: URL, accountId: String) - case signedIn(authBaseURL: URL, accountId: String, tokenReference: Data) + case signedOut + case signedIn(authBaseURL: URL, tokenReference: Data) var isInitialized: Bool { switch self { @@ -335,27 +355,14 @@ enum TunnelAuthStatus: Equatable, CustomStringConvertible { } } - func accountId() -> String? { - switch self { - case .tunnelUninitialized, .accountNotSetup: - return nil - case .signedOut(_, let accountId): - return accountId - case .signedIn(_, let accountId, _): - return accountId - } - } - var description: String { switch self { case .tunnelUninitialized: return "tunnel uninitialized" - case .accountNotSetup: - return "account not setup" - case .signedOut(let authBaseURL, let accountId): - return "signedOut(authBaseURL: \(authBaseURL), accountId: \(accountId))" - case .signedIn(let authBaseURL, let accountId, _): - return "signedIn(authBaseURL: \(authBaseURL), accountId: \(accountId))" + case .signedOut: + return "signedOut" + case .signedIn(let authBaseURL, _): + return "signedIn(authBaseURL: \(authBaseURL))" } } } @@ -389,19 +396,18 @@ extension NETunnelProviderManager { } return URL(string: urlString) }() - let accountId = providerConfig[TunnelProviderKeys.keyAccountId] as? String let tokenRef = protocolConfiguration.passwordReference - if let authBaseURL = authBaseURL, let accountId = accountId { + if let authBaseURL = authBaseURL { if let tokenRef = tokenRef { - return .signedIn(authBaseURL: authBaseURL, accountId: accountId, tokenReference: tokenRef) + return .signedIn(authBaseURL: authBaseURL, tokenReference: tokenRef) } else { - return .signedOut(authBaseURL: authBaseURL, accountId: accountId) + return .signedOut } } else { - return .accountNotSetup + return .signedOut } } - return .accountNotSetup + return .signedOut } func saveAuthStatus(_ authStatus: TunnelAuthStatus) async throws { @@ -409,17 +415,18 @@ extension NETunnelProviderManager { var providerConfig: [String: Any] = protocolConfiguration.providerConfiguration ?? [:] switch authStatus { - case .tunnelUninitialized, .accountNotSetup: + case .tunnelUninitialized: + protocolConfiguration.passwordReference = nil break - case .signedOut(let authBaseURL, let accountId): + case .signedOut: + protocolConfiguration.passwordReference = nil + break + case .signedIn(let authBaseURL, let tokenReference): providerConfig[TunnelProviderKeys.keyAuthBaseURLString] = authBaseURL.absoluteString - providerConfig[TunnelProviderKeys.keyAccountId] = accountId - case .signedIn(let authBaseURL, let accountId, let tokenReference): - providerConfig[TunnelProviderKeys.keyAuthBaseURLString] = authBaseURL.absoluteString - providerConfig[TunnelProviderKeys.keyAccountId] = accountId protocolConfiguration.passwordReference = tokenReference } + providerConfig.removeValue(forKey: TunnelProviderKeys.keyAccountId) protocolConfiguration.providerConfiguration = providerConfig ensureTunnelConfigurationIsValid() diff --git a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift index 04247f488..99b9f38d8 100644 --- a/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift +++ b/swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift @@ -41,7 +41,7 @@ private var connectingAnimationTimer: Timer? let settingsViewModel: SettingsViewModel - private var loginStatus: AuthStore.LoginStatus = .signedOut(accountId: nil) + private var loginStatus: AuthStore.LoginStatus = .signedOut private var tunnelStatus: NEVPNStatus = .invalid public init(settingsViewModel: SettingsViewModel) { @@ -210,8 +210,6 @@ Task { do { try await appStore?.auth.signIn() - } catch FirezoneError.missingTeamId { - openSettingsWindow() } catch { logger.error("Error signing in: \(String(describing: error))") } @@ -219,7 +217,9 @@ } @objc private func signOutButtonTapped() { - appStore?.auth.signOut() + Task { + await appStore?.auth.signOut() + } } @objc private func settingsButtonTapped() { @@ -316,7 +316,7 @@ signInMenuItem.target = self signInMenuItem.isEnabled = true signOutMenuItem.isHidden = true - case .signedIn(_, let actorName): + case .signedIn(let actorName): signInMenuItem.title = actorName.isEmpty ? "Signed in" : "Signed in as \(actorName)" signInMenuItem.target = nil signOutMenuItem.isHidden = false