Surface specific XPC & login item errors (#2773)

Task/Issue URL: https://app.asana.com/0/72649045549333/1207013105069620/f Tech Design URL: CC: BSK PR: duckduckgo/BrowserServicesKit#819 **Description**: **Steps to test this PR**: 1. For the UI, go to Debug > VPN > Simulate known failure and check the warning banner in the VPN popover 2. To check if XPC errors are surfaced, go to `NetworkProtectionIPCTunnelController.start()`, and add a simulated error like `handleFailure(NSError(domain: "SMAppServiceErrorDomain", code: 1))` inside `handleFailure(_:)` 3. To check if login item version mismatch is handled, go to `TunnelControllerIPCService.register(version:bundlePath:completion)` and change the `DefaultIPCMetadataCollector.version != version` to `DefaultIPCMetadataCollector.version == version` 4. To check if ClientError code 5 is handled, block access to NetP endpoint (at router level or using Proxyman), then try to start the VPN. 5. Use Debug > VPN > Log metadata to console to check if lastKnownFailureDescription is recorded
duckduckgo · Jun 4, 2024 · a390dfd · a390dfd
1 parent 447a20d
commit a390dfd
Show file tree

Hide file tree

Showing 28 changed files with 412 additions and 69 deletions.
diff --git a/DuckDuckGo.xcodeproj/project.pbxproj b/DuckDuckGo.xcodeproj/project.pbxproj
@@ -12993,7 +12993,7 @@
 			repositoryURL = "https://github.com/duckduckgo/BrowserServicesKit";
 			requirement = {
 				kind = exactVersion;
-				version = 150.0.0;
+				version = 150.1.0;
 			};
 		};
 		9FF521422BAA8FF300B9819B /* XCRemoteSwiftPackageReference "lottie-spm" */ = {

diff --git a/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
@@ -32,8 +32,8 @@
       "kind" : "remoteSourceControl",
       "location" : "https://github.com/duckduckgo/BrowserServicesKit",
       "state" : {
-        "revision" : "03e6b719671c5baaa2afa474d447b707bf595820",
-        "version" : "150.0.0"
+        "revision" : "79fe0c99e43c6c1bf2c0a4d397368033fd37eae9",
+        "version" : "150.1.0"
       }
     },
     {

diff --git a/DuckDuckGo/Common/Localizables/UserText+NetworkProtection.swift b/DuckDuckGo/Common/Localizables/UserText+NetworkProtection.swift
@@ -65,7 +65,7 @@ extension UserText {
     }
 
     // "network.protection.system.extension.unknown.activation.error" - Message shown to users when they try to enable NetP and there is an unexpected activation error.
-    static let networkProtectionUnknownActivationError = "There as an unexpected error. Please try again."
+    static let networkProtectionUnknownActivationError = "There was an unexpected error. Please try again."
     // "network.protection.system.extension.please.reboot" - Message shown to users when they try to enable NetP and they need to reboot the computer to complete the installation
     static let networkProtectionPleaseReboot = "VPN update available. Restart your Mac to reconnect."
 }

diff --git a/DuckDuckGo/MainWindow/MainViewController.swift b/DuckDuckGo/MainWindow/MainViewController.swift
@@ -71,7 +71,9 @@ final class MainViewController: NSViewController {
 #endif
 
             let ipcClient = TunnelControllerIPCClient()
-            ipcClient.register()
+            ipcClient.register { error in
+                NetworkProtectionKnownFailureStore().lastKnownFailure = KnownFailure(error)
+            }
             let vpnUninstaller = VPNUninstaller(ipcClient: ipcClient)
 
             return NetworkProtectionNavBarPopoverManager(
@@ -97,7 +99,8 @@ final class MainViewController: NSViewController {
                 connectionErrorObserver: ipcClient.ipcConnectionErrorObserver,
                 connectivityIssuesObserver: connectivityIssuesObserver,
                 controllerErrorMessageObserver: controllerErrorMessageObserver,
-                dataVolumeObserver: ipcClient.ipcDataVolumeObserver
+                dataVolumeObserver: ipcClient.ipcDataVolumeObserver,
+                knownFailureObserver: KnownFailureObserverThroughDistributedNotifications()
             )
         }()
 

diff --git a/DuckDuckGo/NavigationBar/View/NetPPopoverManagerMock.swift b/DuckDuckGo/NavigationBar/View/NetPPopoverManagerMock.swift
@@ -74,6 +74,12 @@ final class IPCClientMock: NetworkProtectionIPCClient {
     }
     var ipcDataVolumeObserver: any NetworkProtection.DataVolumeObserver = DataVolumeObserverMock()
 
+    final class KnownFailureObserverMock: NetworkProtection.KnownFailureObserver {
+        var publisher: AnyPublisher<KnownFailure?, Never> = PassthroughSubject().eraseToAnyPublisher()
+        var recentValue: KnownFailure?
+    }
+    var ipcKnownFailureObserver: any NetworkProtection.KnownFailureObserver = KnownFailureObserverMock()
+
     func start(completion: @escaping (Error?) -> Void) {
         completion(nil)
     }

diff --git a/DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenu.swift b/DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionDebugMenu.swift
@@ -405,6 +405,13 @@ final class NetworkProtectionDebugMenu: NSMenu {
 
     }
 
+    func menuItem(title: String, action: Selector, representedObject: Any?) -> NSMenuItem {
+        let menuItem = NSMenuItem(title: title, action: action, keyEquivalent: "")
+        menuItem.target = self
+        menuItem.representedObject = representedObject
+        return menuItem
+    }
+
     // MARK: - Menu State Update
 
     override func update() {

diff --git a/...o/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarPopoverManager.swift b/...o/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionNavBarPopoverManager.swift
@@ -69,7 +69,8 @@ final class NetworkProtectionNavBarPopoverManager: NetPPopoverManager {
                 connectionErrorObserver: ipcClient.ipcConnectionErrorObserver,
                 connectivityIssuesObserver: ConnectivityIssueObserverThroughDistributedNotifications(),
                 controllerErrorMessageObserver: ControllerErrorMesssageObserverThroughDistributedNotifications(),
-                dataVolumeObserver: ipcClient.ipcDataVolumeObserver
+                dataVolumeObserver: ipcClient.ipcDataVolumeObserver,
+                knownFailureObserver: KnownFailureObserverThroughDistributedNotifications()
             )
 
             let onboardingStatusPublisher = UserDefaults.netP.networkProtectionOnboardingStatusPublisher

diff --git a/...uckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionTunnelController.swift b/...uckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionTunnelController.swift
@@ -68,6 +68,8 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr
     ///
     private let controllerErrorStore = NetworkProtectionControllerErrorStore()
 
+    private let knownFailureStore = NetworkProtectionKnownFailureStore()
+
     // MARK: - VPN Tunnel & Configuration
 
     /// Auth token store
@@ -597,6 +599,7 @@ final class NetworkProtectionTunnelController: TunnelController, TunnelSessionPr
             }
         } catch {
             VPNOperationErrorRecorder().recordControllerStartFailure(error)
+            knownFailureStore.lastKnownFailure = KnownFailure(error)
 
             if case StartError.cancelled = error {
                 PixelKit.fire(

diff --git a/DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/TunnelControllerProvider.swift b/DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/TunnelControllerProvider.swift
@@ -17,6 +17,7 @@
 //
 
 import Foundation
+import NetworkProtection
 import NetworkProtectionIPC
 
 final class TunnelControllerProvider {
@@ -26,7 +27,9 @@ final class TunnelControllerProvider {
 
     private init() {
         let ipcClient = TunnelControllerIPCClient()
-        ipcClient.register()
+        ipcClient.register { error in
+            NetworkProtectionKnownFailureStore().lastKnownFailure = KnownFailure(error)
+        }
         tunnelController = NetworkProtectionIPCTunnelController(ipcClient: ipcClient)
     }
 

diff --git a/...NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift b/...NetworkProtection/AppTargets/DeveloperIDTarget/NetworkProtectionIPCTunnelController.swift
@@ -52,18 +52,21 @@ final class NetworkProtectionIPCTunnelController {
     private let ipcClient: NetworkProtectionIPCClient
     private let pixelKit: PixelFiring?
     private let errorRecorder: VPNOperationErrorRecorder
+    private let knownFailureStore: NetworkProtectionKnownFailureStore
 
     init(featureVisibility: NetworkProtectionFeatureVisibility = DefaultNetworkProtectionVisibility(subscriptionManager: Application.appDelegate.subscriptionManager),
          loginItemsManager: LoginItemsManaging = LoginItemsManager(),
          ipcClient: NetworkProtectionIPCClient,
          pixelKit: PixelFiring? = PixelKit.shared,
-         errorRecorder: VPNOperationErrorRecorder = VPNOperationErrorRecorder()) {
+         errorRecorder: VPNOperationErrorRecorder = VPNOperationErrorRecorder(),
+         knownFailureStore: NetworkProtectionKnownFailureStore = NetworkProtectionKnownFailureStore()) {
 
         self.featureVisibility = featureVisibility
         self.loginItemsManager = loginItemsManager
         self.ipcClient = ipcClient
         self.pixelKit = pixelKit
         self.errorRecorder = errorRecorder
+        self.knownFailureStore = knownFailureStore
     }
 
     // MARK: - Login Items Manager
@@ -91,6 +94,7 @@ extension NetworkProtectionIPCTunnelController: TunnelController {
         pixelKit?.fire(StartAttempt.begin)
 
         func handleFailure(_ error: Error) {
+            knownFailureStore.lastKnownFailure = KnownFailure(error)
             errorRecorder.recordIPCStartFailure(error)
             log(error)
             pixelKit?.fire(StartAttempt.failure(error), frequency: .dailyAndCount)
@@ -99,6 +103,8 @@ extension NetworkProtectionIPCTunnelController: TunnelController {
         do {
             try await enableLoginItems()
 
+            knownFailureStore.reset()
+
             ipcClient.start { [pixelKit] error in
                 if let error {
                     handleFailure(error)

diff --git a/DuckDuckGo/VPNFeedbackForm/VPNMetadataCollector.swift b/DuckDuckGo/VPNFeedbackForm/VPNMetadataCollector.swift
@@ -52,6 +52,7 @@ struct VPNMetadata: Encodable {
         let connectionState: String
         let lastStartErrorDescription: String
         let lastTunnelErrorDescription: String
+        let lastKnownFailureDescription: String
         let connectedServer: String
         let connectedServerIP: String
     }
@@ -127,7 +128,7 @@ final class DefaultVPNMetadataCollector: VPNMetadataCollector {
     init(defaults: UserDefaults = .netP,
          accountManager: AccountManaging) {
         let ipcClient = TunnelControllerIPCClient()
-        ipcClient.register()
+        ipcClient.register { _ in }
         self.accountManager = accountManager
         self.ipcClient = ipcClient
         self.defaults = defaults
@@ -138,7 +139,8 @@ final class DefaultVPNMetadataCollector: VPNMetadataCollector {
             connectionErrorObserver: ipcClient.connectionErrorObserver,
             connectivityIssuesObserver: ConnectivityIssueObserverThroughDistributedNotifications(),
             controllerErrorMessageObserver: ControllerErrorMesssageObserverThroughDistributedNotifications(),
-            dataVolumeObserver: ipcClient.dataVolumeObserver
+            dataVolumeObserver: ipcClient.dataVolumeObserver,
+            knownFailureObserver: KnownFailureObserverThroughDistributedNotifications()
         )
 
         // Force refresh just in case. A refresh is requested when the IPC client is created, but distributed notifications don't guarantee delivery
@@ -266,12 +268,14 @@ final class DefaultVPNMetadataCollector: VPNMetadataCollector {
 
         let connectionState = String(describing: statusReporter.statusObserver.recentValue)
         let lastTunnelErrorDescription = await errorHistory.lastTunnelErrorDescription
+        let lastKnownFailureDescription = NetworkProtectionKnownFailureStore().lastKnownFailure?.description ?? "none"
         let connectedServer = statusReporter.serverInfoObserver.recentValue.serverLocation?.serverLocation ?? "none"
         let connectedServerIP = statusReporter.serverInfoObserver.recentValue.serverAddress ?? "none"
         return .init(onboardingState: onboardingState,
                      connectionState: connectionState,
                      lastStartErrorDescription: errorHistory.lastStartErrorDescription,
                      lastTunnelErrorDescription: lastTunnelErrorDescription,
+                     lastKnownFailureDescription: lastKnownFailureDescription,
                      connectedServer: connectedServer,
                      connectedServerIP: connectedServerIP)
     }

diff --git a/DuckDuckGoVPN/DuckDuckGoVPNAppDelegate.swift b/DuckDuckGoVPN/DuckDuckGoVPNAppDelegate.swift
@@ -266,7 +266,8 @@ final class DuckDuckGoVPNAppDelegate: NSObject, NSApplicationDelegate {
             connectionErrorObserver: errorObserver,
             connectivityIssuesObserver: DisabledConnectivityIssueObserver(),
             controllerErrorMessageObserver: ControllerErrorMesssageObserverThroughDistributedNotifications(),
-            dataVolumeObserver: dataVolumeObserver
+            dataVolumeObserver: dataVolumeObserver,
+            knownFailureObserver: KnownFailureObserverThroughDistributedNotifications()
         )
     }()
 

diff --git a/DuckDuckGoVPN/TunnelControllerIPCService.swift b/DuckDuckGoVPN/TunnelControllerIPCService.swift
@@ -37,6 +37,16 @@ final class TunnelControllerIPCService {
     private var cancellables = Set<AnyCancellable>()
     private let defaults: UserDefaults
 
+    enum IPCError: SilentErrorConvertible {
+        case versionMismatched
+
+        var asSilentError: KnownFailure.SilentError? {
+            switch self {
+            case .versionMismatched: return .loginItemVersionMismatched
+            }
+        }
+    }
+
     init(tunnelController: NetworkProtectionTunnelController,
          uninstaller: VPNUninstalling,
          networkExtensionController: NetworkExtensionController,
@@ -53,6 +63,7 @@ final class TunnelControllerIPCService {
         subscribeToErrorChanges()
         subscribeToStatusUpdates()
         subscribeToServerChanges()
+        subscribeToKnownFailureUpdates()
         subscribeToDataVolumeUpdates()
 
         server.serverDelegate = self
@@ -89,6 +100,15 @@ final class TunnelControllerIPCService {
             .store(in: &cancellables)
     }
 
+    private func subscribeToKnownFailureUpdates() {
+        statusReporter.knownFailureObserver.publisher
+            .subscribe(on: DispatchQueue.main)
+            .sink { [weak self] failure in
+                self?.server.knownFailureUpdated(failure)
+            }
+            .store(in: &cancellables)
+    }
+
     private func subscribeToDataVolumeUpdates() {
         statusReporter.dataVolumeObserver.publisher
             .subscribe(on: DispatchQueue.main)
@@ -103,9 +123,20 @@ final class TunnelControllerIPCService {
 
 extension TunnelControllerIPCService: IPCServerInterface {
 
-    func register() {
+    func register(completion: @escaping (Error?) -> Void) {
+        register(version: version, bundlePath: bundlePath, completion: completion)
+    }
+
+    func register(version: String, bundlePath: String, completion: @escaping (Error?) -> Void) {
         server.serverInfoChanged(statusReporter.serverInfoObserver.recentValue)
         server.statusChanged(statusReporter.statusObserver.recentValue)
+        if self.version != version {
+            let error = TunnelControllerIPCService.IPCError.versionMismatched
+            NetworkProtectionKnownFailureStore().lastKnownFailure = KnownFailure(error)
+            completion(error)
+        } else {
+            completion(nil)
+        }
     }
 
     func start(completion: @escaping (Error?) -> Void) {
@@ -169,3 +200,25 @@ extension TunnelControllerIPCService: IPCServerInterface {
         }
     }
 }
+
+// MARK: - Error Handling
+
+extension TunnelControllerIPCService.IPCError: LocalizedError, CustomNSError {
+    var errorDescription: String? {
+        switch self {
+        case .versionMismatched: return "Login item version mismatched"
+        }
+    }
+
+    var errorCode: Int {
+        switch self {
+        case .versionMismatched: return 0
+        }
+    }
+
+    var errorUserInfo: [String: Any] {
+        switch self {
+        case .versionMismatched: return [:]
+        }
+    }
+}
diff --git a/LocalPackages/DataBrokerProtection/Package.swift b/LocalPackages/DataBrokerProtection/Package.swift
@@ -29,7 +29,7 @@ let package = Package(
             targets: ["DataBrokerProtection"])
     ],
     dependencies: [
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "150.0.0"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "150.1.0"),
         .package(path: "../SwiftUIExtensions"),
         .package(path: "../XPCHelper"),
     ],

diff --git a/LocalPackages/NetworkProtectionMac/Package.swift b/LocalPackages/NetworkProtectionMac/Package.swift
@@ -31,7 +31,7 @@ let package = Package(
         .library(name: "NetworkProtectionUI", targets: ["NetworkProtectionUI"]),
     ],
     dependencies: [
-        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "150.0.0"),
+        .package(url: "https://github.com/duckduckgo/BrowserServicesKit", exact: "150.1.0"),
         .package(url: "https://github.com/airbnb/lottie-spm", exact: "4.4.1"),
         .package(path: "../XPCHelper"),
         .package(path: "../SwiftUIExtensions"),

diff --git a/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionIPC/IPCMetadataCollector.swift b/LocalPackages/NetworkProtectionMac/Sources/NetworkProtectionIPC/IPCMetadataCollector.swift
@@ -0,0 +1,44 @@
+//
+//  IPCMetadataCollector.swift
+//
+//  Copyright © 2024 DuckDuckGo. All rights reserved.
+//
+//  Licensed under the Apache License, Version 2.0 (the "License");
+//  you may not use this file except in compliance with the License.
+//  You may obtain a copy of the License at
+//
+//  http://www.apache.org/licenses/LICENSE-2.0
+//
+//  Unless required by applicable law or agreed to in writing, software
+//  distributed under the License is distributed on an "AS IS" BASIS,
+//  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//  See the License for the specific language governing permissions and
+//  limitations under the License.
+//
+
+import Foundation
+
+public protocol IPCMetadataCollector {
+    static var version: String { get }
+    static var bundlePath: String { get }
+}
+
+final public class DefaultIPCMetadataCollector: IPCMetadataCollector {
+    public static var version: String {
+        shortVersion + "/" + buildNumber
+    }
+
+    public static var bundlePath: String {
+        Bundle.main.bundlePath
+    }
+
+    // swiftlint:disable force_cast
+    private static var shortVersion: String {
+        Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString") as! String
+    }
+
+    private static var buildNumber: String {
+        Bundle.main.object(forInfoDictionaryKey: "CFBundleVersion") as! String
+    }
+    // swiftlint:enable force_cast
+}