Skip to content
This repository was archived by the owner on Feb 24, 2025. It is now read-only.

Commit f8b408d

Browse files
diegoreymendezdiegoreymendez
authored
Improve error logging (#24)
Task/Issue URL: https://app.asana.com/0/1203512625915051/1203969517643013/f and https://app.asana.com/0/1203512625915051/1203971625280790/f Tech Design URL: BSK PR: more-duckduckgo-org/browserserviceskit-network-protection#17
1 parent ad21e35 commit f8b408d

20 files changed

+798
-361
lines changed

‎DuckDuckGo.xcodeproj/project.pbxproj

+59-1
Large diffs are not rendered by default.

‎DuckDuckGo/API/APIHeaders.swift

+1-1
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ final class APIHeaders {
3030
static let ifNoneMatch = "If-None-Match"
3131
static let moreInfo = "X-DuckDuckGo-MoreInfo"
3232
}
33-
33+
3434
private let appVersion: AppVersion
3535

3636
init(appVersion: AppVersion = AppVersion.shared) {

‎DuckDuckGo/Common/Extensions/URLExtension.swift

-14
Original file line numberDiff line numberDiff line change
@@ -95,18 +95,6 @@ extension URL {
9595
return Self.preferences.appendingPathComponent(pane.rawValue)
9696
}
9797

98-
// MARK: Pixel
99-
100-
static let pixelBase = ProcessInfo.processInfo.environment["PIXEL_BASE_URL", default: "https://improving.duckduckgo.com"]
101-
102-
static func pixelUrl(forPixelNamed pixelName: String) -> URL {
103-
let urlString = "\(Self.pixelBase)/t/\(pixelName)"
104-
let url = URL(string: urlString)!
105-
// url = url.addParameter(name: \"atb\", value: statisticsStore.atbWithVariant ?? \"\")")
106-
// https://app.asana.com/0/1177771139624306/1199951074455863/f
107-
return url
108-
}
109-
11098
// MARK: ATB
11199

112100
static var devMode: String {
@@ -273,8 +261,6 @@ extension URL {
273261

274262
static var duckDuckGoEmail = URL(string: "https://duckduckgo.com/email-protection")!
275263

276-
static var duckDuckGoMorePrivacyInfo = URL(string: "https://help.duckduckgo.com/duckduckgo-help-pages/privacy/atb/")!
277-
278264
var isDuckDuckGo: Bool {
279265
absoluteString.starts(with: Self.duckDuckGo.absoluteString)
280266
}

‎DuckDuckGo/Main/View/MainWindowController.swift

-6
Original file line numberDiff line numberDiff line change
@@ -77,12 +77,6 @@ final class MainWindowController: NSWindowController {
7777
if shouldShowOnboarding {
7878
mainViewController.tabCollectionViewModel.selectedTabViewModel?.tab.startOnboarding()
7979
}
80-
81-
DispatchQueue.main.asyncAfter(deadline: .now() + 1) {
82-
#if NETP_SYSTEM_EXTENSION
83-
SystemExtensionManager.shared.activate()
84-
#endif
85-
}
8680
}
8781

8882
private func subscribeToResolutionChange() {

‎DuckDuckGo/NetworkProtection/NetworkProtectionControllerErrorStore.swift

+53
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
//
2+
// NetworkProtectionControllerIssuesStore.swift
3+
//
4+
// Copyright © 2023 DuckDuckGo. All rights reserved.
5+
//
6+
// Licensed under the Apache License, Version 2.0 (the "License");
7+
// you may not use this file except in compliance with the License.
8+
// You may obtain a copy of the License at
9+
//
10+
// http://www.apache.org/licenses/LICENSE-2.0
11+
//
12+
// Unless required by applicable law or agreed to in writing, software
13+
// distributed under the License is distributed on an "AS IS" BASIS,
14+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
// See the License for the specific language governing permissions and
16+
// limitations under the License.
17+
//
18+
19+
import Foundation
20+
21+
/// This class provides a mechanism to store and announce issues when interacting with the tunnel.
22+
/// The reason this lass is necessry is because we need to store and share failures across different UI elements. As an example
23+
/// we may need to show these errors in the status menu (which will eventually be run in its own agent), and in the status view within
24+
/// the app.
25+
///
26+
final class NetworkProtectionControllerErrorStore {
27+
private static let lastErrorMessageKey = "com.duckduckgo.NetworkProtectionControllerErrorStore.lastErrorMessage"
28+
private let userDefaults: UserDefaults
29+
private let distributedNotificationCenter: DistributedNotificationCenter
30+
31+
init(userDefaults: UserDefaults = .standard,
32+
distributedNotificationCenter: DistributedNotificationCenter = .forType(.networkProtection)) {
33+
self.userDefaults = userDefaults
34+
self.distributedNotificationCenter = distributedNotificationCenter
35+
}
36+
37+
var lastErrorMessage: String? {
38+
get {
39+
userDefaults.string(forKey: Self.lastErrorMessageKey)
40+
}
41+
42+
set {
43+
userDefaults.set(newValue, forKey: Self.lastErrorMessageKey)
44+
postLastErrorMessageChangedNotification()
45+
}
46+
}
47+
48+
// MARK: - Posting Notifications
49+
50+
private func postLastErrorMessageChangedNotification() {
51+
distributedNotificationCenter.postNotificationName(.NetPControllerErrorStatusChanged, object: nil, userInfo: nil, options: [.deliverImmediately, .postToAllSessions])
52+
}
53+
}

‎DuckDuckGo/NetworkProtection/NetworkProtectionLogger.swift

-13
Original file line numberDiff line numberDiff line change
@@ -24,22 +24,9 @@ protocol NetworkProtectionLogger {
2424
}
2525

2626
final class DefaultNetworkProtectionLogger: NetworkProtectionLogger {
27-
2827
func log(_ error: Error) {
2928
let format = StaticString(stringLiteral: "🔴 %{public}@")
3029
os_log(format, type: .error, error.localizedDescription)
31-
32-
let nsError = error as NSError
33-
34-
/// Note: `configurationReadWriteFailed` is raised when the user does not grant permission to access the system's VPN info (which we should ignore),
35-
/// but the error code's description makes it sound like it could signal other issues with reading and writing the VPN configuration, which we don't want to ignore by default.
36-
/// For this reason we're keeping the log but disabling the assertion for this error code.
37-
///
38-
let skipAssertion = nsError.domain == NEVPNErrorDomain && nsError.code == NEVPNError.configurationReadWriteFailed.rawValue
39-
40-
if !skipAssertion {
41-
assertionFailure(error.localizedDescription)
42-
}
4330
}
4431

4532
}

‎DuckDuckGo/NetworkProtection/NetworkProtectionProvider.swift

+49-92
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import BrowserServicesKit
2424
import NetworkExtension
2525
import NetworkProtection
2626
import SystemExtensions
27-
import Common
2827

2928
enum NetworkProtectionConnectionStatus {
3029
case notConfigured
@@ -75,11 +74,10 @@ final class DefaultNetworkProtectionProvider: NetworkProtectionProvider {
7574
/// The logger that this object will use for errors that are handled by this class.
7675
///
7776
private let logger: NetworkProtectionLogger
78-
79-
/// Handles registration of the current device with the Network Protection backend.
80-
/// The manager is also responsible to maintaining the current known list of backend servers, and allowing the user to pick which one they connect to.
81-
///
82-
private let deviceManager: NetworkProtectionDeviceManagement
77+
78+
/// Stores the last controller error for the purpose of updating the UI as needed..
79+
///
80+
private let controllerErrorStore = NetworkProtectionControllerErrorStore()
8381

8482
// MARK: - Notifications & Observers
8583

@@ -92,12 +90,6 @@ final class DefaultNetworkProtectionProvider: NetworkProtectionProvider {
9290
private var configChangeObserverToken: NSObjectProtocol?
9391

9492
let configChangePublisher = CurrentValueSubject<Void, Never>(())
95-
96-
// MARK: - Bundle Identifiers
97-
98-
var extensionBundleIdentifier: String {
99-
NetworkProtectionBundle.extensionBundle().bundleIdentifier!
100-
}
10193

10294
// MARK: - VPN Tunnel & Configuration
10395

@@ -192,11 +184,8 @@ final class DefaultNetworkProtectionProvider: NetworkProtectionProvider {
192184

193185
convenience init() {
194186
let keychainStore = NetworkProtectionKeychainStore(useSystemKeychain: NetworkProtectionBundle.usesSystemKeychain())
195-
let deviceManager = NetworkProtectionDeviceManager(keyStore: keychainStore,
196-
errorEvents: Self.networkProtectionDebugEvents)
197187

198188
self.init(notificationCenter: .default,
199-
deviceManager: deviceManager,
200189
logger: DefaultNetworkProtectionLogger())
201190
}
202191

@@ -207,11 +196,9 @@ final class DefaultNetworkProtectionProvider: NetworkProtectionProvider {
207196
/// - logger: (meant for testing) the logger that this object will use.
208197
///
209198
init(notificationCenter: NotificationCenter,
210-
deviceManager: NetworkProtectionDeviceManagement,
211199
logger: NetworkProtectionLogger) {
212200

213201
self.logger = logger
214-
self.deviceManager = deviceManager
215202
self.notificationCenter = notificationCenter
216203

217204
startObservingNotifications()
@@ -261,65 +248,6 @@ final class DefaultNetworkProtectionProvider: NetworkProtectionProvider {
261248
configChangeObserverToken = nil
262249
}
263250

264-
// MARK: - Error Reporting
265-
266-
static let networkProtectionDebugEvents: EventMapping<NetworkProtectionError>? = .init { event, _, _, _ in
267-
let domainEvent: Pixel.Event
268-
269-
switch event {
270-
case .noServerRegistrationInfo:
271-
domainEvent = .networkProtectionTunnelConfigurationNoServerRegistrationInfo
272-
case .couldNotSelectClosestServer:
273-
domainEvent = .networkProtectionTunnelConfigurationCouldNotSelectClosestServer
274-
case .couldNotGetPeerPublicKey:
275-
domainEvent = .networkProtectionTunnelConfigurationCouldNotGetPeerPublicKey
276-
case .couldNotGetPeerHostName:
277-
domainEvent = .networkProtectionTunnelConfigurationCouldNotGetPeerHostName
278-
case .couldNotGetInterfaceAddressRange:
279-
domainEvent = .networkProtectionTunnelConfigurationCouldNotGetInterfaceAddressRange
280-
281-
case .failedToFetchServerList:
282-
return
283-
case .failedToParseServerListResponse:
284-
domainEvent = .networkProtectionClientFailedToParseServerListResponse
285-
case .failedToEncodeRegisterKeyRequest:
286-
domainEvent = .networkProtectionClientFailedToEncodeRegisterKeyRequest
287-
case .failedToFetchRegisteredServers:
288-
return
289-
case .failedToParseRegisteredServersResponse:
290-
domainEvent = .networkProtectionClientFailedToParseRegisteredServersResponse
291-
case .serverListInconsistency:
292-
// - TODO: not sure what to do here
293-
return
294-
295-
case .failedToEncodeServerList:
296-
domainEvent = .networkProtectionServerListStoreFailedToEncodeServerList
297-
case .failedToWriteServerList(let eventError):
298-
domainEvent = .networkProtectionServerListStoreFailedToWriteServerList(error: eventError)
299-
case .noServerListFound:
300-
return
301-
case .couldNotCreateServerListDirectory:
302-
return
303-
304-
case .failedToReadServerList(let eventError):
305-
domainEvent = .networkProtectionServerListStoreFailedToReadServerList(error: eventError)
306-
307-
case .failedToCastKeychainValueToData(let field):
308-
domainEvent = .networkProtectionKeychainErrorFailedToCastKeychainValueToData(field: field)
309-
case .keychainReadError(let field, let status):
310-
domainEvent = .networkProtectionKeychainReadError(field: field, status: status)
311-
case .keychainWriteError(let field, let status):
312-
domainEvent = .networkProtectionKeychainWriteError(field: field, status: status)
313-
case .keychainDeleteError(let field, let status):
314-
domainEvent = .networkProtectionKeychainDeleteError(field: field, status: status)
315-
316-
case .unhandledError(function: let function, line: let line, error: let error):
317-
domainEvent = .networkProtectionUnhandledError(function: function, line: line, error: error)
318-
}
319-
320-
Pixel.fire(domainEvent, includeAppVersionParameter: true)
321-
}
322-
323251
// MARK: - Notifications: Handling
324252

325253
static let statusChangeQueue: OperationQueue = {
@@ -371,9 +299,6 @@ final class DefaultNetworkProtectionProvider: NetworkProtectionProvider {
371299
/// Setups the tunnel manager if it's not set up already.
372300
///
373301
private func setup(_ tunnelManager: NETunnelProviderManager) async throws {
374-
// 1 - If configuration exists... let it through (but what if it's broken? the service will take care of it)
375-
// 2 - If a configuration doesn't exist, fill in a dummy one (can this cause issues?)
376-
377302
if tunnelManager.localizedDescription == nil {
378303
tunnelManager.localizedDescription = UserText.networkProtectionTunnelName
379304
}
@@ -382,12 +307,9 @@ final class DefaultNetworkProtectionProvider: NetworkProtectionProvider {
382307
tunnelManager.isEnabled = true
383308
}
384309

385-
guard tunnelManager.protocolConfiguration == nil else {
386-
return
387-
}
388-
389310
let protocolConfiguration = NETunnelProviderProtocol()
390311
protocolConfiguration.serverAddress = "127.0.0.1" // Dummy address... the NetP service will take care of grabbing a real server
312+
protocolConfiguration.providerBundleIdentifier = NetworkProtectionBundle.extensionBundle().bundleIdentifier
391313
tunnelManager.protocolConfiguration = protocolConfiguration
392314
}
393315

@@ -409,30 +331,65 @@ final class DefaultNetworkProtectionProvider: NetworkProtectionProvider {
409331
return false
410332
}
411333
}
334+
335+
// MARK: - Ensure things are working
336+
337+
/// - Returns: `true` if the system extension and the background agent were activated successfully
338+
///
339+
private func ensureSystemExtensionAndAgentAreActivated() async throws -> Bool {
340+
NetworkProtectionAgentManager.current.enable()
341+
342+
#if NETP_SYSTEM_EXTENSION
343+
if case .willActivateAfterReboot = try await SystemExtensionManager.shared.activate(waitingForUserApprovalHandler: { [weak self] in
344+
self?.controllerErrorStore.lastErrorMessage = "Go to Security & Privacy in System Settings to allow Network Protection to activate"
345+
}) {
346+
controllerErrorStore.lastErrorMessage = "Please reboot to activate Network Protection"
347+
return false
348+
}
349+
#endif
350+
351+
return true
352+
}
412353

413354
// MARK: - Starting & Stopping the VPN
414355

415356
/// Starts the VPN connection used for Network Protection
416357
///
417358
func start() async throws {
418-
NetworkProtectionAgentManager.current.enable()
419-
let tunnelManager = try await loadOrMakeTunnelManager()
359+
guard try await ensureSystemExtensionAndAgentAreActivated() else {
360+
return
361+
}
362+
363+
controllerErrorStore.lastErrorMessage = nil
364+
let tunnelManager: NETunnelProviderManager
365+
366+
do {
367+
tunnelManager = try await loadOrMakeTunnelManager()
368+
} catch {
369+
controllerErrorStore.lastErrorMessage = error.localizedDescription
370+
throw error
371+
}
420372

421373
switch tunnelManager.connection.status {
422374
case .invalid:
423375
reloadTunnelManager()
424376
try await start()
425-
case .disconnected, .disconnecting:
377+
case .connected:
378+
// Intentional no-op
379+
break
380+
default:
426381
var options = [String: NSObject]()
427-
382+
428383
if let selectedServerName = NetworkProtectionSelectedServerUserDefaultsStore().selectedServer.stringValue {
429384
options["selectedServer"] = selectedServerName as NSString
430385
}
431-
432-
try tunnelManager.connection.startVPNTunnel(options: options)
433-
default:
434-
// Intentional no-op
435-
break
386+
387+
do {
388+
try tunnelManager.connection.startVPNTunnel(options: options)
389+
} catch {
390+
controllerErrorStore.lastErrorMessage = error.localizedDescription
391+
throw error
392+
}
436393
}
437394
}
438395

0 commit comments

Comments
 (0)