From bb8fa949313189da8d79ac32b38efd21d6a9ba1d Mon Sep 17 00:00:00 2001 From: Diego Rey Mendez Date: Fri, 28 Jun 2024 13:41:15 +0200 Subject: [PATCH 1/2] Small change to prevent noise in VPN uninstall pixels. (#2919) Task/Issue URL: https://app.asana.com/0/1207603085593419/1207678831870556/f ## Description Makes some changes to prevent VPN uninstall attempts that can't succeed from even starting. --- DuckDuckGo/Waitlist/IPCServiceLauncher.swift | 9 ++ DuckDuckGo/Waitlist/VPNUninstaller.swift | 150 ++++++++++-------- .../Sources/AppLauncher/AppLauncher.swift | 16 +- 3 files changed, 109 insertions(+), 66 deletions(-) diff --git a/DuckDuckGo/Waitlist/IPCServiceLauncher.swift b/DuckDuckGo/Waitlist/IPCServiceLauncher.swift index 579793a5d3..f898790aa6 100644 --- a/DuckDuckGo/Waitlist/IPCServiceLauncher.swift +++ b/DuckDuckGo/Waitlist/IPCServiceLauncher.swift @@ -41,6 +41,15 @@ final class IPCServiceLauncher { self.launchMethod = launchMethod } + func checkPrerequisites() -> Bool { + switch launchMethod { + case .direct(_, let appLauncher): + return appLauncher.targetAppExists() + case .loginItem: + return true + } + } + /// Enables the IPC service /// func enable() async throws { diff --git a/DuckDuckGo/Waitlist/VPNUninstaller.swift b/DuckDuckGo/Waitlist/VPNUninstaller.swift index 8730852a1a..1092c230e5 100644 --- a/DuckDuckGo/Waitlist/VPNUninstaller.swift +++ b/DuckDuckGo/Waitlist/VPNUninstaller.swift @@ -74,6 +74,7 @@ final class VPNUninstaller: VPNUninstalling { } enum IPCUninstallAttempt: PixelKitEventV2 { + case prevented case begin case cancelled(_ reason: UninstallCancellationReason) case success @@ -81,6 +82,9 @@ final class VPNUninstaller: VPNUninstalling { var name: String { switch self { + case .prevented: + return "vpn_browser_uninstall_prevented_uds" + case .begin: return "vpn_browser_uninstall_attempt_uds" @@ -97,7 +101,8 @@ final class VPNUninstaller: VPNUninstalling { var parameters: [String: String]? { switch self { - case .begin, + case .prevented, + .begin, .success, .failure: return nil @@ -108,7 +113,8 @@ final class VPNUninstaller: VPNUninstalling { var error: Error? { switch self { - case .begin, + case .prevented, + .begin, .cancelled, .success: return nil @@ -165,82 +171,100 @@ final class VPNUninstaller: VPNUninstalling { /// @MainActor func uninstall(removeSystemExtension: Bool) async throws { + // We want to check service launcher pre-requisited before firing any pixel, + // because if our VPN menu app isn't available where we're expecting to find it + // we want to avoid adding noise to our uninstall attempts and instead fire + // a daily pixel telling us the app wasn't found. + guard ipcServiceLauncher.checkPrerequisites() else { + pixelKit?.fire(IPCUninstallAttempt.prevented, frequency: .daily) + return + } + pixelKit?.fire(IPCUninstallAttempt.begin, frequency: .dailyAndCount) do { - // We can do this optimistically as it has little if any impact. - unpinNetworkProtection() + try await executeUninstallSequence(removeSystemExtension: removeSystemExtension) + pixelKit?.fire(IPCUninstallAttempt.success, frequency: .dailyAndCount) + } catch UninstallError.cancelled(let reason) { + pixelKit?.fire(IPCUninstallAttempt.cancelled(reason), frequency: .dailyAndCount) + } catch { + pixelKit?.fire(IPCUninstallAttempt.failure(error), frequency: .dailyAndCount) + } + } - guard !isDisabling else { - throw UninstallError.cancelled(reason: .alreadyUninstalling) - } + /// Uninstalls the VPN + /// + /// Don't call this directly but instead call ``uninstall(removeSystemExtension:)`` as it checks preconditions + /// and fires pixels. + /// + @MainActor + private func executeUninstallSequence(removeSystemExtension: Bool) async throws { + // We can do this optimistically as it has little if any impact. + unpinNetworkProtection() - guard vpnMenuLoginItem.status.isInstalled else { - throw UninstallError.cancelled(reason: .alreadyUninstalled) - } + guard !isDisabling else { + throw UninstallError.cancelled(reason: .alreadyUninstalling) + } - isDisabling = true + guard vpnMenuLoginItem.status.isInstalled else { + throw UninstallError.cancelled(reason: .alreadyUninstalled) + } - defer { - resetUserDefaults(uninstallSystemExtension: removeSystemExtension) - } + isDisabling = true - do { - try await ipcServiceLauncher.enable() - } catch { - throw UninstallError.runAgentError(error) - } + defer { + resetUserDefaults(uninstallSystemExtension: removeSystemExtension) + } - // Allow some time for the login items to fully launch - try await Task.sleep(nanoseconds: 500 * NSEC_PER_MSEC) - - do { - if removeSystemExtension { - try await ipcClient.uninstall(.all) - } else { - try await ipcClient.uninstall(.configuration) - } - } catch { - print("Failed to uninstall VPN, with error: \(error.localizedDescription)") - - switch error { - case OSSystemExtensionError.requestCanceled: - throw UninstallError.cancelled(reason: .sysexInstallationCancelled) - case OSSystemExtensionError.authorizationRequired: - throw UninstallError.cancelled(reason: .sysexInstallationRequiresAuthorization) - default: - throw UninstallError.uninstallError(error) - } - } + do { + try await ipcServiceLauncher.enable() + } catch { + throw UninstallError.runAgentError(error) + } + + // Allow some time for the login items to fully launch + try await Task.sleep(nanoseconds: 500 * NSEC_PER_MSEC) - // We want to give some time for the login item to reset state before disabling it - try? await Task.sleep(interval: 0.5) + do { + if removeSystemExtension { + try await ipcClient.uninstall(.all) + } else { + try await ipcClient.uninstall(.configuration) + } + } catch { + print("Failed to uninstall VPN, with error: \(error.localizedDescription)") + + switch error { + case OSSystemExtensionError.requestCanceled: + throw UninstallError.cancelled(reason: .sysexInstallationCancelled) + case OSSystemExtensionError.authorizationRequired: + throw UninstallError.cancelled(reason: .sysexInstallationRequiresAuthorization) + default: + throw UninstallError.uninstallError(error) + } + } - // Workaround: since status updates are provided through XPC we want to make sure the - // VPN is marked as disconnected. We may be able to more properly resolve this by using - // UDS for all VPN status updates. - // - // Ref: https://app.asana.com/0/0/1207499177312396/1207538373572594/f - // - VPNControllerXPCClient.shared.forceStatusToDisconnected() + // We want to give some time for the login item to reset state before disabling it + try? await Task.sleep(interval: 0.5) - // When the agent is registered as a login item, we want to unregister it - // and stop it from running, which is achieved by the next call. - removeAgents() + // Workaround: since status updates are provided through XPC we want to make sure the + // VPN is marked as disconnected. We may be able to more properly resolve this by using + // UDS for all VPN status updates. + // + // Ref: https://app.asana.com/0/0/1207499177312396/1207538373572594/f + // + VPNControllerXPCClient.shared.forceStatusToDisconnected() - // When the agent was started directly (not as a login item) we want to stop it, - // as the above call won't do anything for it. - try await stopAgents() + // When the agent is registered as a login item, we want to unregister it + // and stop it from running, which is achieved by the next call. + removeAgents() - notifyVPNUninstalled() - isDisabling = false + // When the agent was started directly (not as a login item) we want to stop it, + // as the above call won't do anything for it. + try await stopAgents() - pixelKit?.fire(IPCUninstallAttempt.success, frequency: .dailyAndCount) - } catch UninstallError.cancelled(let reason) { - pixelKit?.fire(IPCUninstallAttempt.cancelled(reason), frequency: .dailyAndCount) - } catch { - pixelKit?.fire(IPCUninstallAttempt.failure(error), frequency: .dailyAndCount) - } + notifyVPNUninstalled() + isDisabling = false } // Stop the VPN agents. diff --git a/LocalPackages/AppLauncher/Sources/AppLauncher/AppLauncher.swift b/LocalPackages/AppLauncher/Sources/AppLauncher/AppLauncher.swift index 8dee99d87d..290a907387 100644 --- a/LocalPackages/AppLauncher/Sources/AppLauncher/AppLauncher.swift +++ b/LocalPackages/AppLauncher/Sources/AppLauncher/AppLauncher.swift @@ -46,9 +46,15 @@ public final class AppLauncher: AppLaunching { } private let mainBundleURL: URL + private var workspace: NSWorkspace + private var fileManager: FileManager - public init(appBundleURL: URL) { + public init(appBundleURL: URL, + workspace: NSWorkspace = .shared, + fileManager: FileManager = .default) { mainBundleURL = appBundleURL + self.workspace = workspace + self.fileManager = fileManager } public func launchApp(withCommand command: AppLaunchCommand) async throws { @@ -76,12 +82,16 @@ public final class AppLauncher: AppLaunching { do { if let launchURL = command.launchURL { - return try await NSWorkspace.shared.open([launchURL], withApplicationAt: mainBundleURL, configuration: configuration) + return try await workspace.open([launchURL], withApplicationAt: mainBundleURL, configuration: configuration) } else { - return try await NSWorkspace.shared.openApplication(at: mainBundleURL, configuration: configuration) + return try await workspace.openApplication(at: mainBundleURL, configuration: configuration) } } catch { throw AppLaunchError.workspaceOpenError(error) } } + + public func targetAppExists() -> Bool { + fileManager.fileExists(atPath: mainBundleURL.path) + } } From a9433952d1828fd56f7e3f5ad630c5a2b455f9f8 Mon Sep 17 00:00:00 2001 From: Dax the Duck Date: Fri, 28 Jun 2024 12:11:24 +0000 Subject: [PATCH 2/2] Bump version to 1.94.0 (211) --- Configuration/BuildNumber.xcconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Configuration/BuildNumber.xcconfig b/Configuration/BuildNumber.xcconfig index cf4a22fc29..1983fc37d9 100644 --- a/Configuration/BuildNumber.xcconfig +++ b/Configuration/BuildNumber.xcconfig @@ -1 +1 @@ -CURRENT_PROJECT_VERSION = 210 +CURRENT_PROJECT_VERSION = 211