From a9fa6a2f62fb5f504297f885778d501325c14541 Mon Sep 17 00:00:00 2001 From: Davide Date: Mon, 30 Sep 2024 14:56:20 +0200 Subject: [PATCH] Maintain one configuration per profile (#636) Helps with automation. Install the VPN configuration before persisting a profile, so that the 1:1 reference with OS settings is maintained. Likewise, uninstall the VPN configuration after removing a profile. This before-save hook also resolves a problem with multiple imports, where multiple VPN permission alerts coalesce if no VPN configuration is installed. Now the first import waits for the permission synchronously. Fixes #618 --- .../xcshareddata/swiftpm/Package.resolved | 2 +- Passepartout/Library/Package.swift | 2 +- .../AppLibrary/Business/ProfileManager.swift | 116 +++++++++++------- .../Library/Sources/AppUI/AppUI.swift | 4 - .../Sources/AppUI/Business/AppContext.swift | 99 +++++++++------ .../AppUI/Business/ConnectionObserver.swift | 6 +- .../AppUI/Business/Tunnel+Extensions.swift | 31 +---- .../TunnelInstallationProviding.swift | 12 +- .../Sources/AppUI/L10n/SwiftGen+Strings.swift | 2 - .../Resources/en.lproj/Localizable.strings | 1 - .../AppUI/Views/App/ProfileGridView.swift | 6 +- .../AppUI/Views/App/ProfileListView.swift | 4 +- .../AppUI/Views/UI/ProfileContextMenu.swift | 9 -- .../AppUI/Views/UI/ProfileRowView.swift | 2 +- .../AppUI/Views/UI/TunnelRestartButton.swift | 2 +- .../AppUI/Views/UI/TunnelToggleButton.swift | 4 +- .../Views/UI/TunnelUninstallButton.swift | 50 -------- .../AppUITests/ConnectionObserverTests.swift | 9 +- .../Tests/AppUITests/ProfileEditorTests.swift | 12 +- .../AppUITests/ProfileImporterTests.swift | 32 +++-- 20 files changed, 193 insertions(+), 212 deletions(-) delete mode 100644 Passepartout/Library/Sources/AppUI/Views/UI/TunnelUninstallButton.swift diff --git a/Passepartout.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/Passepartout.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved index 02a0753c..635f4c24 100644 --- a/Passepartout.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved +++ b/Passepartout.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved @@ -32,7 +32,7 @@ "kind" : "remoteSourceControl", "location" : "git@github.com:passepartoutvpn/passepartoutkit", "state" : { - "revision" : "ed3f54281b672af0f1127f00033579a36a9afed5" + "revision" : "263bedc756d07eb107d7bfe3b50dbc5db28675d4" } }, { diff --git a/Passepartout/Library/Package.swift b/Passepartout/Library/Package.swift index b1f7d8a8..7409427f 100644 --- a/Passepartout/Library/Package.swift +++ b/Passepartout/Library/Package.swift @@ -31,7 +31,7 @@ let package = Package( ], dependencies: [ // .package(url: "git@github.com:passepartoutvpn/passepartoutkit", from: "0.7.0"), - .package(url: "git@github.com:passepartoutvpn/passepartoutkit", revision: "ed3f54281b672af0f1127f00033579a36a9afed5"), + .package(url: "git@github.com:passepartoutvpn/passepartoutkit", revision: "263bedc756d07eb107d7bfe3b50dbc5db28675d4"), // .package(path: "../../../passepartoutkit"), .package(url: "git@github.com:passepartoutvpn/passepartoutkit-openvpn-openssl", from: "0.6.0"), // .package(path: "../../../passepartoutkit-openvpn-openssl"), diff --git a/Passepartout/Library/Sources/AppLibrary/Business/ProfileManager.swift b/Passepartout/Library/Sources/AppLibrary/Business/ProfileManager.swift index 44e39bf0..fd308596 100644 --- a/Passepartout/Library/Sources/AppLibrary/Business/ProfileManager.swift +++ b/Passepartout/Library/Sources/AppLibrary/Business/ProfileManager.swift @@ -27,17 +27,26 @@ import AppData import Combine import Foundation import PassepartoutKit +import UtilsLibrary @MainActor public final class ProfileManager: ObservableObject { - public let didSave: PassthroughSubject + public enum Event { + case save(Profile) - public var didUpdate: AnyPublisher<[Profile], Never> { - $profiles.eraseToAnyPublisher() + case remove([Profile.ID]) + + case update([Profile]) } + public var beforeSave: ((Profile) async throws -> Void)? + + public var afterRemove: (([Profile.ID]) async -> Void)? + + public let didChange: PassthroughSubject + @Published - var profiles: [Profile] + private var profiles: [Profile] private var allProfileIds: Set @@ -49,7 +58,7 @@ public final class ProfileManager: ObservableObject { // for testing/previews public init(profiles: [Profile]) { - didSave = PassthroughSubject() + didChange = PassthroughSubject() self.profiles = profiles.sorted { $0.name.lowercased() < $1.name.lowercased() } @@ -57,21 +66,21 @@ public final class ProfileManager: ObservableObject { repository = MockProfileRepository(profiles: profiles) searchSubject = CurrentValueSubject("") subscriptions = [] - - observeObjects(searchDebounce: 0) } - public init(repository: any ProfileRepository, searchDebounce: Int = 200) { - didSave = PassthroughSubject() + public init(repository: any ProfileRepository) { + didChange = PassthroughSubject() profiles = [] allProfileIds = [] self.repository = repository searchSubject = CurrentValueSubject("") subscriptions = [] - - observeObjects(searchDebounce: searchDebounce) } +} +// MARK: - CRUD + +extension ProfileManager { public var hasProfiles: Bool { !profiles.isEmpty } @@ -98,8 +107,9 @@ public final class ProfileManager: ObservableObject { public func save(_ profile: Profile) async throws { do { + try await beforeSave?(profile) try await repository.saveEntities([profile]) - didSave.send(profile) + didChange.send(.save(profile)) } catch { pp_log(.app, .fault, "Unable to save profile \(profile.id): \(error)") throw error @@ -114,6 +124,8 @@ public final class ProfileManager: ObservableObject { do { allProfileIds.subtract(profileIds) try await repository.removeEntities(withIds: profileIds) + await afterRemove?(profileIds) + didChange.send(.remove(profileIds)) } catch { pp_log(.app, .fault, "Unable to remove profiles \(profileIds): \(error)") } @@ -124,6 +136,8 @@ public final class ProfileManager: ObservableObject { } } +// MARK: - Shortcuts + extension ProfileManager { public func new(withName name: String) -> Profile { var builder = Profile.Builder() @@ -149,35 +163,6 @@ extension ProfileManager { } private extension ProfileManager { - func observeObjects(searchDebounce: Int) { - repository - .entitiesPublisher - .receive(on: DispatchQueue.main) - .sink { [weak self] in - guard let self else { - return - } - self.profiles = $0.entities - if !$0.isFiltering { - allProfileIds = Set($0.entities.map(\.id)) - } - } - .store(in: &subscriptions) - - searchSubject - .debounce(for: .milliseconds(searchDebounce), scheduler: DispatchQueue.main) - .sink { [weak self] search in - Task { - guard !search.isEmpty else { - try await self?.repository.resetFilter() - return - } - try await self?.repository.filter(byName: search) - } - } - .store(in: &subscriptions) - } - func firstUniqueName(from name: String) -> String { let allNames = profiles.map(\.name) var newName = name @@ -191,3 +176,52 @@ private extension ProfileManager { } } } + +// MARK: - Observation + +extension ProfileManager { + public func observeObjects(searchDebounce: Int = 200) { + repository + .entitiesPublisher + .receive(on: DispatchQueue.main) + .sink { [weak self] in + self?.notifyUpdatedEntities($0) + } + .store(in: &subscriptions) + + searchSubject + .debounce(for: .milliseconds(searchDebounce), scheduler: DispatchQueue.main) + .sink { [weak self] in + self?.performSearch($0) + } + .store(in: &subscriptions) + } +} + +private extension ProfileManager { + func notifyUpdatedEntities(_ result: EntitiesResult) { + let oldProfiles = profiles.reduce(into: [:]) { + $0[$1.id] = $1 + } + let newProfiles = result.entities + let updatedProfiles = newProfiles.filter { + $0 != oldProfiles[$0.id] // includes new profiles + } + + if !result.isFiltering { + allProfileIds = Set(newProfiles.map(\.id)) + } + profiles = newProfiles + didChange.send(.update(updatedProfiles)) + } + + func performSearch(_ search: String) { + Task { + guard !search.isEmpty else { + try await repository.resetFilter() + return + } + try await repository.filter(byName: search) + } + } +} diff --git a/Passepartout/Library/Sources/AppUI/AppUI.swift b/Passepartout/Library/Sources/AppUI/AppUI.swift index 1aba0fd4..e0e51483 100644 --- a/Passepartout/Library/Sources/AppUI/AppUI.swift +++ b/Passepartout/Library/Sources/AppUI/AppUI.swift @@ -32,10 +32,6 @@ public struct AppUI { public static func configure(with context: AppContext) { assertMissingModuleImplementations() - Task { - await context.iapManager.reloadReceipt() - try await context.tunnel.prepare() - } } } diff --git a/Passepartout/Library/Sources/AppUI/Business/AppContext.swift b/Passepartout/Library/Sources/AppUI/Business/AppContext.swift index 2c0c0c42..c2b42109 100644 --- a/Passepartout/Library/Sources/AppUI/Business/AppContext.swift +++ b/Passepartout/Library/Sources/AppUI/Business/AppContext.swift @@ -60,61 +60,80 @@ public final class AppContext: ObservableObject { self.profileManager = profileManager self.tunnel = tunnel self.tunnelEnvironment = tunnelEnvironment - self.registry = registry - self.constants = constants - subscriptions = [] - connectionObserver = ConnectionObserver( tunnel: tunnel, environment: tunnelEnvironment, interval: constants.connection.refreshInterval ) + self.registry = registry + self.constants = constants + subscriptions = [] - observeObjects() + profileManager.beforeSave = { [weak self] in + try await self?.installSavedProfile($0) + } + profileManager.afterRemove = { [weak self] in + self?.uninstallRemovedProfiles(withIds: $0) + } + + Task { + try await tunnel.prepare() + await iapManager.reloadReceipt() + connectionObserver.observeObjects() + profileManager.observeObjects() + observeObjects() + } } } +// MARK: - Observation + private extension AppContext { func observeObjects() { profileManager - .didSave - .sink { [weak self] profile in - guard let self else { - return - } - guard profile.id == tunnel.installedProfile?.id else { - return - } - Task { - if profile.isInteractive { - try await self.tunnel.disconnect() - return - } - if self.tunnel.status == .active { - try await self.tunnel.reconnect(with: profile, processor: self.iapManager) - } else { - try await self.tunnel.reinstate(profile, processor: self.iapManager) - } - } - } - .store(in: &subscriptions) + .didChange + .sink { [weak self] event in + switch event { + case .save(let profile): + self?.syncTunnelIfCurrentProfile(profile) - profileManager - .didUpdate - .sink { [weak self] _ in - guard let self else { - return - } - guard let installedProfile = tunnel.installedProfile else { - return - } - guard profileManager.exists(withId: installedProfile.id) else { - Task { - try await self.tunnel.disconnect() - } - return + default: + break } } .store(in: &subscriptions) } } + +private extension AppContext { + func installSavedProfile(_ profile: Profile) async throws { + try await tunnel.install(profile, processor: iapManager) + } + + func uninstallRemovedProfiles(withIds profileIds: [Profile.ID]) { + Task { + for id in profileIds { + do { + try await tunnel.uninstall(profileId: id) + } catch { + pp_log(.app, .error, "Unable to uninstall profile \(id): \(error)") + } + } + } + } + + func syncTunnelIfCurrentProfile(_ profile: Profile) { + guard profile.id == tunnel.currentProfile?.id else { + return + } + Task { + if profile.isInteractive { + try await tunnel.disconnect() + return + } + if tunnel.status == .active { + try await tunnel.connect(with: profile, processor: iapManager) + } + } + } +} diff --git a/Passepartout/Library/Sources/AppUI/Business/ConnectionObserver.swift b/Passepartout/Library/Sources/AppUI/Business/ConnectionObserver.swift index 27e68350..c0b90d46 100644 --- a/Passepartout/Library/Sources/AppUI/Business/ConnectionObserver.swift +++ b/Passepartout/Library/Sources/AppUI/Business/ConnectionObserver.swift @@ -65,13 +65,9 @@ public final class ConnectionObserver: ObservableObject { self.environment = environment self.interval = interval subscriptions = [] - - observeObjects() } -} -private extension ConnectionObserver { - func observeObjects() { + public func observeObjects() { tunnel .$status .receive(on: DispatchQueue.main) diff --git a/Passepartout/Library/Sources/AppUI/Business/Tunnel+Extensions.swift b/Passepartout/Library/Sources/AppUI/Business/Tunnel+Extensions.swift index 3254f1df..fe99182c 100644 --- a/Passepartout/Library/Sources/AppUI/Business/Tunnel+Extensions.swift +++ b/Passepartout/Library/Sources/AppUI/Business/Tunnel+Extensions.swift @@ -32,28 +32,14 @@ protocol ProfileProcessor { } extension Tunnel { - func reinstate(_ profile: Profile, processor: ProfileProcessor) async throws { - try await install(profile, processor: processor) + func install(_ profile: Profile, processor: ProfileProcessor) async throws { + let newProfile = try processor.processedProfile(profile) + try await install(newProfile, connect: false, title: \.name) } func connect(with profile: Profile, processor: ProfileProcessor) async throws { - try await install(profile, processor: processor) - guard !Task.isCancelled else { - return - } - try await connect() - } - - func reconnect(with profile: Profile, processor: ProfileProcessor) async throws { - try await disconnect() - guard !Task.isCancelled else { - return - } - try await install(profile, processor: processor) - guard !Task.isCancelled else { - return - } - try await connect() + let newProfile = try processor.processedProfile(profile) + try await install(newProfile, connect: true, title: \.name) } func currentLog(parameters: Constants.Log) async -> [String] { @@ -70,10 +56,3 @@ extension Tunnel { } } } - -private extension Tunnel { - func install(_ profile: Profile, processor: ProfileProcessor) async throws { - let newProfile = try processor.processedProfile(profile) - try await install(profile: newProfile, title: \.name) - } -} diff --git a/Passepartout/Library/Sources/AppUI/Business/TunnelInstallationProviding.swift b/Passepartout/Library/Sources/AppUI/Business/TunnelInstallationProviding.swift index 3898fa04..aa1db65a 100644 --- a/Passepartout/Library/Sources/AppUI/Business/TunnelInstallationProviding.swift +++ b/Passepartout/Library/Sources/AppUI/Business/TunnelInstallationProviding.swift @@ -36,25 +36,25 @@ protocol TunnelInstallationProviding { struct TunnelInstallation { let header: ProfileHeader - let isEnabled: Bool + let onDemand: Bool } @MainActor extension TunnelInstallationProviding { var installation: TunnelInstallation? { - guard let installedProfile = tunnel.installedProfile else { + guard let currentProfile = tunnel.currentProfile else { return nil } guard let header = profileManager.headers.first(where: { - $0.id == installedProfile.id + $0.id == currentProfile.id }) else { return nil } - return TunnelInstallation(header: header, isEnabled: installedProfile.isEnabled) + return TunnelInstallation(header: header, onDemand: currentProfile.onDemand) } - var installedProfile: Profile? { - guard let id = tunnel.installedProfile?.id else { + var currentProfile: Profile? { + guard let id = tunnel.currentProfile?.id else { return nil } return profileManager.profile(withId: id) diff --git a/Passepartout/Library/Sources/AppUI/L10n/SwiftGen+Strings.swift b/Passepartout/Library/Sources/AppUI/L10n/SwiftGen+Strings.swift index d95c6862..7d85e82e 100644 --- a/Passepartout/Library/Sources/AppUI/L10n/SwiftGen+Strings.swift +++ b/Passepartout/Library/Sources/AppUI/L10n/SwiftGen+Strings.swift @@ -265,8 +265,6 @@ public enum Strings { public static let storage = Strings.tr("Localizable", "global.storage", fallback: "Storage") /// Subnet public static let subnet = Strings.tr("Localizable", "global.subnet", fallback: "Subnet") - /// Uninstall - public static let uninstall = Strings.tr("Localizable", "global.uninstall", fallback: "Uninstall") /// Unknown public static let unknown = Strings.tr("Localizable", "global.unknown", fallback: "Unknown") /// Username diff --git a/Passepartout/Library/Sources/AppUI/Resources/en.lproj/Localizable.strings b/Passepartout/Library/Sources/AppUI/Resources/en.lproj/Localizable.strings index cd99e3be..aefd56f7 100644 --- a/Passepartout/Library/Sources/AppUI/Resources/en.lproj/Localizable.strings +++ b/Passepartout/Library/Sources/AppUI/Resources/en.lproj/Localizable.strings @@ -58,7 +58,6 @@ "global.status" = "Status"; "global.storage" = "Storage"; "global.subnet" = "Subnet"; -"global.uninstall" = "Uninstall"; "global.unknown" = "Unknown"; "global.username" = "Username"; "global.version" = "Version"; diff --git a/Passepartout/Library/Sources/AppUI/Views/App/ProfileGridView.swift b/Passepartout/Library/Sources/AppUI/Views/App/ProfileGridView.swift index 1c844e23..3b01d0d5 100644 --- a/Passepartout/Library/Sources/AppUI/Views/App/ProfileGridView.swift +++ b/Passepartout/Library/Sources/AppUI/Views/App/ProfileGridView.swift @@ -85,7 +85,7 @@ private extension ProfileGridView { InstalledProfileView( layout: .grid, profileManager: profileManager, - profile: installedProfile, + profile: currentProfile, tunnel: tunnel, interactiveManager: interactiveManager, errorHandler: errorHandler, @@ -95,7 +95,7 @@ private extension ProfileGridView { ) ) .contextMenu { - installedProfile.map { + currentProfile.map { ProfileContextMenu( profileManager: profileManager, tunnel: tunnel, @@ -121,7 +121,7 @@ private extension ProfileGridView { withMarker: true, onEdit: onEdit ) - .themeGridCell(isSelected: header.id == nextProfileId ?? tunnel.installedProfile?.id) + .themeGridCell(isSelected: header.id == nextProfileId ?? tunnel.currentProfile?.id) .contextMenu { ProfileContextMenu( profileManager: profileManager, diff --git a/Passepartout/Library/Sources/AppUI/Views/App/ProfileListView.swift b/Passepartout/Library/Sources/AppUI/Views/App/ProfileListView.swift index 526b5960..f296e0fa 100644 --- a/Passepartout/Library/Sources/AppUI/Views/App/ProfileListView.swift +++ b/Passepartout/Library/Sources/AppUI/Views/App/ProfileListView.swift @@ -76,7 +76,7 @@ private extension ProfileListView { InstalledProfileView( layout: .list, profileManager: profileManager, - profile: installedProfile, + profile: currentProfile, tunnel: tunnel, interactiveManager: interactiveManager, errorHandler: errorHandler, @@ -86,7 +86,7 @@ private extension ProfileListView { ) ) .contextMenu { - installedProfile.map { + currentProfile.map { ProfileContextMenu( profileManager: profileManager, tunnel: tunnel, diff --git a/Passepartout/Library/Sources/AppUI/Views/UI/ProfileContextMenu.swift b/Passepartout/Library/Sources/AppUI/Views/UI/ProfileContextMenu.swift index f036bbc9..152cfa37 100644 --- a/Passepartout/Library/Sources/AppUI/Views/UI/ProfileContextMenu.swift +++ b/Passepartout/Library/Sources/AppUI/Views/UI/ProfileContextMenu.swift @@ -52,9 +52,6 @@ struct ProfileContextMenu: View { profileEditButton profileDuplicateButton Divider() - if isInstalledProfile { - tunnelUninstallButton - } profileRemoveButton } } @@ -86,12 +83,6 @@ private extension ProfileContextMenu { } } - var tunnelUninstallButton: some View { - TunnelUninstallButton(tunnel: tunnel) { - ThemeImageLabel(Strings.Global.uninstall, .tunnelUninstall) - } - } - var profileEditButton: some View { Button { onEdit(header) diff --git a/Passepartout/Library/Sources/AppUI/Views/UI/ProfileRowView.swift b/Passepartout/Library/Sources/AppUI/Views/UI/ProfileRowView.swift index 0f782ca4..9a7b92e4 100644 --- a/Passepartout/Library/Sources/AppUI/Views/UI/ProfileRowView.swift +++ b/Passepartout/Library/Sources/AppUI/Views/UI/ProfileRowView.swift @@ -71,7 +71,7 @@ struct ProfileRowView: View, TunnelContextProviding { private extension ProfileRowView { var markerView: some View { ThemeImage(header.id == nextProfileId ? .pending : statusImage) - .opacity(header.id == nextProfileId || header.id == tunnel.installedProfile?.id ? 1.0 : 0.0) + .opacity(header.id == nextProfileId || header.id == tunnel.currentProfile?.id ? 1.0 : 0.0) .frame(width: 24.0) } diff --git a/Passepartout/Library/Sources/AppUI/Views/UI/TunnelRestartButton.swift b/Passepartout/Library/Sources/AppUI/Views/UI/TunnelRestartButton.swift index 32e5d174..d65d13be 100644 --- a/Passepartout/Library/Sources/AppUI/Views/UI/TunnelRestartButton.swift +++ b/Passepartout/Library/Sources/AppUI/Views/UI/TunnelRestartButton.swift @@ -55,7 +55,7 @@ struct TunnelRestartButton