Add controller VPN uninstall pixels #2781

diegoreymendez · 2024-05-15T11:45:54Z

Task/Issue URL: https://app.asana.com/0/1206580121312550/1207324076451291/f

iOS PR: duckduckgo/iOS#2857
BSK PR: duckduckgo/BrowserServicesKit#822

Description

Adds VPN uninstall pixels to the controller.

Testing

Test Success Case

Install the VPN
Uninstall the VPN

Make sure uninstallation works well, and that you see these pixels.

👾[Standard-Fired] m_mac_vpn_controller_uninstall_attempt ["pixelSource": "vpnAgent", "appVersion": "1.88.0"]
👾[Daily and Count-Fired] m_mac_vpn_controller_uninstall_success_d ["pixelSource": "vpnAgent", "appVersion": "1.88.0"]
👾[Daily and Count-Fired] m_mac_vpn_controller_uninstall_success_c ["appVersion": "1.88.0", "pixelSource": "vpnAgent"]

Test Cancel Case

Install the VPN
Uninstall the VPN
When the dialog comes up asking touch ID permission or admin pwd, cancel.

Uninstallation will be cancelled and you should see these pixels:

👾[Standard-Fired] m_mac_vpn_controller_uninstall_attempt ["pixelSource": "vpnAgent", "appVersion": "1.88.0"]
👾[Daily and Count-Fired] m_mac_vpn_controller_uninstall_cancelled_d ["pixelSource": "vpnAgent", "appVersion": "1.88.0", "reason": "sysexInstallationRequiresAuthorization"]
👾[Daily and Count-Fired] m_mac_vpn_controller_uninstall_cancelled_c ["reason": "sysexInstallationRequiresAuthorization", "appVersion": "1.88.0", "pixelSource": "vpnAgent"]

Test Failure Case

Throw an error in this line
Install the VPN
Uninstall the VPN

Uninstallation will fail and you should see these pixels:

👾[Standard-Fired] m_mac_vpn_controller_uninstall_attempt ["appVersion": "1.88.0", "pixelSource": "vpnAgent"]
👾[Daily and Count-Fired] m_mac_vpn_controller_uninstall_failure_d ["d": "Testing", "pixelSource": "vpnAgent", "e": "1", "appVersion": "1.88.0"]
👾[Daily and Count-Fired] m_mac_vpn_controller_uninstall_failure_c ["d": "Testing", "pixelSource": "vpnAgent", "e": "1", "appVersion": "1.88.0"]

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

…t the controller

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugUtilities.swift

diegoreymendez · 2024-05-15T12:30:19Z

DuckDuckGoVPN/DuckDuckGoVPNAppDelegate.swift

@@ -262,7 +263,7 @@ final class DuckDuckGoVPNAppDelegate: NSObject, NSApplicationDelegate {
            locationFormatter: DefaultVPNLocationFormatter(),
            uninstallHandler: { [weak self] in
                guard let self else { return }
-                await self.vpnUninstaller.uninstall(includingSystemExtension: true)
+                try? await self.vpnUninstaller.uninstall(includingSystemExtension: true)


@quanganhdo - You'll probably want to connect this to the UI, if you haven't already.

diegoreymendez · 2024-05-15T12:31:22Z

DuckDuckGoVPN/TunnelControllerIPCService.swift

-            }
+            try await uninstaller.removeVPNConfiguration()
+        case .uninstallVPN:
+            try await uninstaller.uninstall(includingSystemExtension: true)


Tidying things up so that we can have a single uninstall call instead of calling IPC twice from the client for removing the extension and the VPN configuration.

diegoreymendez · 2024-05-15T15:42:20Z

DuckDuckGo/Waitlist/VPNUninstaller.swift

+        case sysexInstallationCancelled
+
+        /// The user was asked for login / pwd or touchID and cancelled
+        ///
+        case sysexInstallationRequiresAuthorization


I'm keeping this separation because it exists in macOS. Not sure what cancelled is precisely, but I"m accounting for it anyway since it's purpose is quite clear.

I guess this could be the user cancelling the system dialog that's presented to them during installation. If it's not that, I have no clue. 😅

This is exactly that, I'm just confused as to what the other one is.

diegoreymendez · 2024-05-15T15:43:09Z

DuckDuckGo/Waitlist/VPNUninstaller.swift

-                        throw UninstallError.vpnConfigurationError(error)
-                    }
+            do {
+                try await ipcClient.command(.uninstallVPN)


Reduced the two IPC calls we had to a single one. I'm avoiding retries because it was causing some weird issues in edge cases, but IMHO it should be fine... we have pixels to see if there are issues.

diegoreymendez · 2024-05-15T15:43:34Z

DuckDuckGo/Waitlist/VPNUninstaller.swift

+        do {
+            try await ipcClient.command(.removeSystemExtension)
+        } catch {
+            throw UninstallError.systemExtensionError(error)


Since this is no longer called individually in the uninstall call, I'm throwing the error here.

diegoreymendez · 2024-05-15T15:43:38Z

DuckDuckGo/Waitlist/VPNUninstaller.swift

+        do {
+            try await ipcClient.command(.removeVPNConfiguration)
+        } catch {
+            throw UninstallError.vpnConfigurationError(error)


Since this is no longer called individually in the uninstall call, I'm throwing the error here.

diegoreymendez · 2024-05-15T15:44:47Z

DuckDuckGoVPN/DuckDuckGoVPNAppDelegate.swift

@@ -43,6 +43,8 @@ final class DuckDuckGoVPNApplication: NSApplication {
        }

        super.init()
+
+        setupPixelKit()


I needed to move this setup to happen earlier as otherwise the lazy initializers in the delegate would just fail to fire pixels for some reason.

In any case this should always happen as soon as possible.

diegoreymendez · 2024-05-15T15:45:00Z

DuckDuckGoVPN/DuckDuckGoVPNAppDelegate.swift

+    @MainActor
+    private func setupPixelKit() {
+        let dryRun: Bool
+
+#if DEBUG
+        dryRun = true
+#else
+        dryRun = false
+#endif
+
+        let pixelSource: String
+
+#if NETP_SYSTEM_EXTENSION
+        pixelSource = "vpnAgent"
+#else
+        pixelSource = "vpnAgentAppStore"
+#endif
+
+        PixelKit.setUp(dryRun: dryRun,
+                       appVersion: AppVersion.shared.versionNumber,
+                       source: pixelSource,
+                       defaultHeaders: [:],
+                       defaults: .netP) { (pixelName: String, headers: [String: String], parameters: [String: String], _, _, onComplete: @escaping PixelKit.CompletionBlock) in
+
+            let url = URL.pixelUrl(forPixelNamed: pixelName)
+            let apiHeaders = APIRequest.Headers(additionalHeaders: headers) // workaround - Pixel class should really handle APIRequest.Headers by itself
+            let configuration = APIRequest.Configuration(url: url, method: .get, queryParameters: parameters, headers: apiHeaders)
+            let request = APIRequest(configuration: configuration)
+
+            request.fetch { _, error in
+                onComplete(error == nil, error)
+            }
+        }
+    }


Just moved, no modifications.

diegoreymendez · 2024-05-15T15:45:51Z

DuckDuckGoVPN/DuckDuckGoVPNAppDelegate.swift

+
+                do {
+                    try await self.vpnUninstaller.uninstall(includingSystemExtension: true)
+                    exit(EXIT_SUCCESS)


This was previously done in the uninstall call, but when uninstall is called from IPC we don't want this to happen, and it's truly not inherent to calling uninstall. This should be the responsibility of the App delegate or an otherwise high-level manager.

diegoreymendez · 2024-05-15T15:46:35Z

DuckDuckGoVPN/VPNUninstaller.swift

-                try await networkExtensionController.deactivateSystemExtension()
-                defaults.networkProtectionOnboardingStatus = .isOnboarding(step: .userNeedsToAllowExtension)
-            } catch {
+    func uninstall(includingSystemExtension: Bool) async throws {


This is the gist of the changes. We're doing it all in one go with proper pixel attempt logic.

Task/Issue URL: https://app.asana.com/0/1206580121312550/1207324076451291/f iOS PR: duckduckgo/iOS#2857 macOS PR: duckduckgo/macos-browser#2781 What kind of version bump will this require?: Minor ## Description Integrating the latest macOS changes in BSK. Nothing should change for iOS except for a crash being fixed.

Task/Issue URL: https://app.asana.com/0/1206580121312550/1207324076451291/f macOS PR: duckduckgo/macos-browser#2781 BSK PR: duckduckgo/BrowserServicesKit#822 ## Description Integrating the latest macOS changes in BSK. Nothing should change for iOS except for a crash being fixed.

Cleans up the code and adds improved support for VPN uninstallation a…

dae9d65

…t the controller

diegoreymendez self-assigned this May 15, 2024

diegoreymendez mentioned this pull request May 15, 2024

Add controller VPN uninstall pixels duckduckgo/BrowserServicesKit#822

Merged

diegoreymendez marked this pull request as ready for review May 15, 2024 11:51

diegoreymendez marked this pull request as draft May 15, 2024 11:51

diegoreymendez commented May 15, 2024

View reviewed changes

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugUtilities.swift Show resolved Hide resolved

diegoreymendez commented May 15, 2024

View reviewed changes

diegoreymendez added 2 commits May 15, 2024 16:23

Makes some adjustments to the pixels

5946352

Fixes some issues with the uninstallation

9f732dd

diegoreymendez mentioned this pull request May 15, 2024

Add controller VPN uninstall pixels duckduckgo/iOS#2857

Merged

diegoreymendez added 3 commits May 15, 2024 17:25

Some final changes

ced5353

Bye bye VPNConfigurationManager

443582f

Updates BSK

901a1f1

diegoreymendez requested a review from samsymons May 15, 2024 15:40

diegoreymendez marked this pull request as ready for review May 15, 2024 15:40

diegoreymendez commented May 15, 2024

View reviewed changes

samsymons approved these changes May 15, 2024

View reviewed changes

Merged the latest from main

4770d63

Updates BSK to 145.3.0

0c29eb5

diegoreymendez merged commit 7c85aa0 into main May 16, 2024
19 checks passed

diegoreymendez deleted the diego/add-uninstall-pixels-for-menu-app branch May 16, 2024 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add controller VPN uninstall pixels #2781

Add controller VPN uninstall pixels #2781

diegoreymendez commented May 15, 2024 •

edited

Loading

diegoreymendez May 15, 2024

diegoreymendez May 15, 2024

diegoreymendez May 15, 2024

samsymons May 15, 2024

diegoreymendez May 15, 2024

diegoreymendez May 15, 2024

diegoreymendez May 15, 2024

diegoreymendez May 15, 2024

diegoreymendez May 15, 2024

diegoreymendez May 15, 2024

diegoreymendez May 15, 2024

diegoreymendez May 15, 2024

Add controller VPN uninstall pixels #2781

Add controller VPN uninstall pixels #2781

Conversation

diegoreymendez commented May 15, 2024 • edited Loading

Description

Testing

Test Success Case

Test Cancel Case

Test Failure Case

Internal references:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez commented May 15, 2024 •

edited

Loading