From 8d26a3c5360a89605b2ecf6c5ad8edadd8eea590 Mon Sep 17 00:00:00 2001 From: Roopesh Chander Date: Thu, 6 Dec 2018 15:58:27 +0530 Subject: Error handling: Cleanup Tunnels Manager errors Signed-off-by: Roopesh Chander --- WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift | 49 +++++---- .../WireGuard/UI/iOS/MainViewController.swift | 10 +- .../UI/iOS/TunnelEditTableViewController.swift | 9 +- .../UI/iOS/TunnelsListTableViewController.swift | 8 +- WireGuard/WireGuard/VPN/TunnelsManager.swift | 110 ++++++++++++--------- 5 files changed, 110 insertions(+), 76 deletions(-) (limited to 'WireGuard/WireGuard') diff --git a/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift b/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift index 2ae0cf0..35bbdec 100644 --- a/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift +++ b/WireGuard/WireGuard/UI/iOS/ErrorPresenter.swift @@ -6,27 +6,12 @@ import os.log class ErrorPresenter { static func errorMessage(for error: Error) -> (String, String) { - switch (error) { - // TunnelManagementError - case TunnelManagementError.tunnelAlreadyExistsWithThatName: - return ("Name already exists", "A tunnel with that name already exists") - case TunnelManagementError.tunnelInvalidName: - return ("Name already exists", "The tunnel name is invalid") - case TunnelManagementError.vpnSystemErrorOnAddTunnel: - return ("Unable to create tunnel", "Internal error") - case TunnelManagementError.vpnSystemErrorOnModifyTunnel: - return ("Unable to modify tunnel", "Internal error") - case TunnelManagementError.vpnSystemErrorOnRemoveTunnel: - return ("Unable to remove tunnel", "Internal error") + if let tunnelsManagerError = error as? TunnelsManagerError { + return errorMessage(forTunnelsManagerError: tunnelsManagerError) + } - // TunnelActivationError - case TunnelActivationError.tunnelActivationAttemptFailed: - return ("Activation failure", "The tunnel could not be activated due to an internal error") - case TunnelActivationError.tunnelActivationFailedInternalError: - return ("Activation failure", "The tunnel could not be activated due to an internal error") - case TunnelActivationError.tunnelActivationFailedNoInternetConnection: - return ("Activation failure", "No internet connection") + switch (error) { // Importing a zip file case ZipArchiveError.cantOpenInputZipFile: @@ -47,6 +32,32 @@ class ErrorPresenter { } } + private static func errorMessage(forTunnelsManagerError error: TunnelsManagerError) -> (String, String) { + switch (error) { + // Tunnels list management + case TunnelsManagerError.tunnelNameEmpty: + return ("No name provided", "Can't create tunnel with an empty name") + case TunnelsManagerError.tunnelAlreadyExistsWithThatName: + return ("Name already exists", "A tunnel with that name already exists") + case TunnelsManagerError.vpnSystemErrorOnListingTunnels: + return ("Unable to list tunnels", "Internal error") + case TunnelsManagerError.vpnSystemErrorOnAddTunnel: + return ("Unable to create tunnel", "Internal error") + case TunnelsManagerError.vpnSystemErrorOnModifyTunnel: + return ("Unable to modify tunnel", "Internal error") + case TunnelsManagerError.vpnSystemErrorOnRemoveTunnel: + return ("Unable to remove tunnel", "Internal error") + + // Tunnel activation + case TunnelsManagerError.tunnelActivationAttemptFailed: + return ("Activation failure", "The tunnel could not be activated due to an internal error") + case TunnelsManagerError.tunnelActivationFailedInternalError: + return ("Activation failure", "The tunnel could not be activated due to an internal error") + case TunnelsManagerError.tunnelActivationFailedNoInternetConnection: + return ("Activation failure", "No internet connection") + } + } + static func showErrorAlert(error: Error, from sourceVC: UIViewController?, onDismissal: (() -> Void)? = nil, onPresented: (() -> Void)? = nil) { guard let sourceVC = sourceVC else { return } diff --git a/WireGuard/WireGuard/UI/iOS/MainViewController.swift b/WireGuard/WireGuard/UI/iOS/MainViewController.swift index 18853d9..0d2d681 100644 --- a/WireGuard/WireGuard/UI/iOS/MainViewController.swift +++ b/WireGuard/WireGuard/UI/iOS/MainViewController.swift @@ -36,8 +36,12 @@ class MainViewController: UISplitViewController { self.preferredDisplayMode = .allVisible // Create the tunnels manager, and when it's ready, inform tunnelsListVC - TunnelsManager.create { [weak self] tunnelsManager in - guard let tunnelsManager = tunnelsManager else { return } + TunnelsManager.create { [weak self] result in + if let error = result.error { + ErrorPresenter.showErrorAlert(error: error, from: self) + return + } + let tunnelsManager: TunnelsManager = result.value! guard let s = self else { return } s.tunnelsManager = tunnelsManager @@ -52,7 +56,7 @@ class MainViewController: UISplitViewController { } extension MainViewController: TunnelsManagerActivationDelegate { - func tunnelActivationFailed(tunnel: TunnelContainer, error: TunnelActivationError) { + func tunnelActivationFailed(tunnel: TunnelContainer, error: TunnelsManagerError) { ErrorPresenter.showErrorAlert(error: error, from: self) } } diff --git a/WireGuard/WireGuard/UI/iOS/TunnelEditTableViewController.swift b/WireGuard/WireGuard/UI/iOS/TunnelEditTableViewController.swift index 74c6e57..ca5e6a8 100644 --- a/WireGuard/WireGuard/UI/iOS/TunnelEditTableViewController.swift +++ b/WireGuard/WireGuard/UI/iOS/TunnelEditTableViewController.swift @@ -99,14 +99,13 @@ class TunnelEditTableViewController: UITableViewController { } else { // We're adding a new tunnel tunnelsManager.add(tunnelConfiguration: tunnelConfiguration, - activateOnDemandSetting: activateOnDemandSetting) { [weak self] (tunnel, error) in - if let error = error { + activateOnDemandSetting: activateOnDemandSetting) { [weak self] result in + if let error = result.error { ErrorPresenter.showErrorAlert(error: error, from: self) } else { + let tunnel: TunnelContainer = result.value! self?.dismiss(animated: true, completion: nil) - if let tunnel = tunnel { - self?.delegate?.tunnelSaved(tunnel: tunnel) - } + self?.delegate?.tunnelSaved(tunnel: tunnel) } } } diff --git a/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift b/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift index 3cc3fa1..ae39587 100644 --- a/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift +++ b/WireGuard/WireGuard/UI/iOS/TunnelsListTableViewController.swift @@ -182,8 +182,8 @@ class TunnelsListTableViewController: UIViewController { let fileBaseName = url.deletingPathExtension().lastPathComponent.trimmingCharacters(in: .whitespacesAndNewlines) if let fileContents = try? String(contentsOf: url), let tunnelConfiguration = try? WgQuickConfigFileParser.parse(fileContents, name: fileBaseName) { - tunnelsManager.add(tunnelConfiguration: tunnelConfiguration) { (_, error) in - if let error = error { + tunnelsManager.add(tunnelConfiguration: tunnelConfiguration) { [weak self] result in + if let error = result.error { ErrorPresenter.showErrorAlert(error: error, from: self) } } @@ -207,8 +207,8 @@ extension TunnelsListTableViewController: UIDocumentPickerDelegate { extension TunnelsListTableViewController: QRScanViewControllerDelegate { func addScannedQRCode(tunnelConfiguration: TunnelConfiguration, qrScanViewController: QRScanViewController, completionHandler: (() -> Void)?) { - tunnelsManager?.add(tunnelConfiguration: tunnelConfiguration) { (_, error) in - if let error = error { + tunnelsManager?.add(tunnelConfiguration: tunnelConfiguration) { result in + if let error = result.error { ErrorPresenter.showErrorAlert(error: error, from: qrScanViewController, onDismissal: completionHandler) } else { completionHandler?() diff --git a/WireGuard/WireGuard/VPN/TunnelsManager.swift b/WireGuard/WireGuard/VPN/TunnelsManager.swift index 3e6c9c2..77eb8e5 100644 --- a/WireGuard/WireGuard/VPN/TunnelsManager.swift +++ b/WireGuard/WireGuard/VPN/TunnelsManager.swift @@ -13,23 +13,46 @@ protocol TunnelsManagerListDelegate: class { } protocol TunnelsManagerActivationDelegate: class { - func tunnelActivationFailed(tunnel: TunnelContainer, error: TunnelActivationError) + func tunnelActivationFailed(tunnel: TunnelContainer, error: TunnelsManagerError) } -enum TunnelActivationError: Error { +enum TunnelsManagerError: Error { + case tunnelNameEmpty + case tunnelAlreadyExistsWithThatName + case vpnSystemErrorOnListingTunnels + case vpnSystemErrorOnAddTunnel + case vpnSystemErrorOnModifyTunnel + case vpnSystemErrorOnRemoveTunnel + case tunnelActivationAttemptFailed // startTunnel() throwed case tunnelActivationFailedInternalError // startTunnel() succeeded, but activation failed case tunnelActivationFailedNoInternetConnection // startTunnel() succeeded, but activation failed since no internet - case attemptingActivationWhenTunnelIsNotInactive - case attemptingDeactivationWhenTunnelIsInactive } -enum TunnelManagementError: Error { - case tunnelAlreadyExistsWithThatName - case tunnelInvalidName - case vpnSystemErrorOnAddTunnel - case vpnSystemErrorOnModifyTunnel - case vpnSystemErrorOnRemoveTunnel +enum TunnelsManagerResult { + case success(T) + case failure(TunnelsManagerError) + + var value: T? { + switch (self) { + case .success(let v): return v + case .failure(_): return nil + } + } + + var error: TunnelsManagerError? { + switch (self) { + case .success(_): return nil + case .failure(let e): return e + } + } + + var isSuccess: Bool { + switch (self) { + case .success(_): return true + case .failure(_): return false + } + } } class TunnelsManager { @@ -46,32 +69,33 @@ class TunnelsManager { self.tunnels = tunnelProviders.map { TunnelContainer(tunnel: $0) }.sorted { $0.name < $1.name } } - static func create(completionHandler: @escaping (TunnelsManager?) -> Void) { + static func create(completionHandler: @escaping (TunnelsManagerResult) -> Void) { #if targetEnvironment(simulator) // NETunnelProviderManager APIs don't work on the simulator - completionHandler(TunnelsManager(tunnelProviders: [])) + completionHandler(.success(TunnelsManager(tunnelProviders: []))) #else NETunnelProviderManager.loadAllFromPreferences { (managers, error) in if let error = error { os_log("Failed to load tunnel provider managers: %{public}@", log: OSLog.default, type: .debug, "\(error)") + completionHandler(.failure(TunnelsManagerError.vpnSystemErrorOnListingTunnels)) return } - completionHandler(TunnelsManager(tunnelProviders: managers ?? [])) + completionHandler(.success(TunnelsManager(tunnelProviders: managers ?? []))) } #endif } func add(tunnelConfiguration: TunnelConfiguration, activateOnDemandSetting: ActivateOnDemandSetting = ActivateOnDemandSetting.defaultSetting, - completionHandler: @escaping (TunnelContainer?, TunnelManagementError?) -> Void) { + completionHandler: @escaping (TunnelsManagerResult) -> Void) { let tunnelName = tunnelConfiguration.interface.name if tunnelName.isEmpty { - completionHandler(nil, TunnelManagementError.tunnelAlreadyExistsWithThatName) + completionHandler(.failure(TunnelsManagerError.tunnelNameEmpty)) return } if self.tunnels.contains(where: { $0.name == tunnelName }) { - completionHandler(nil, TunnelManagementError.tunnelAlreadyExistsWithThatName) + completionHandler(.failure(TunnelsManagerError.tunnelAlreadyExistsWithThatName)) return } @@ -87,7 +111,7 @@ class TunnelsManager { defer { self?.isAddingTunnel = false } guard (error == nil) else { os_log("Add: Saving configuration failed: %{public}@", log: OSLog.default, type: .error, "\(error!)") - completionHandler(nil, TunnelManagementError.vpnSystemErrorOnAddTunnel) + completionHandler(.failure(TunnelsManagerError.vpnSystemErrorOnAddTunnel)) return } if let s = self { @@ -95,7 +119,7 @@ class TunnelsManager { s.tunnels.append(tunnel) s.tunnels.sort { $0.name < $1.name } s.tunnelsListDelegate?.tunnelAdded(at: s.tunnels.firstIndex(of: tunnel)!) - completionHandler(tunnel, nil) + completionHandler(.success(tunnel)) } } } @@ -110,18 +134,18 @@ class TunnelsManager { return } let tail = tunnelConfigurations.dropFirst() - self.add(tunnelConfiguration: head) { [weak self, tail] (_, error) in + self.add(tunnelConfiguration: head) { [weak self, tail] (result) in DispatchQueue.main.async { - self?.addMultiple(tunnelConfigurations: tail, numberSuccessful: numberSuccessful + (error == nil ? 1 : 0), completionHandler: completionHandler) + self?.addMultiple(tunnelConfigurations: tail, numberSuccessful: numberSuccessful + (result.isSuccess ? 1 : 0), completionHandler: completionHandler) } } } func modify(tunnel: TunnelContainer, tunnelConfiguration: TunnelConfiguration, - activateOnDemandSetting: ActivateOnDemandSetting, completionHandler: @escaping (TunnelManagementError?) -> Void) { + activateOnDemandSetting: ActivateOnDemandSetting, completionHandler: @escaping (TunnelsManagerError?) -> Void) { let tunnelName = tunnelConfiguration.interface.name if tunnelName.isEmpty { - completionHandler(TunnelManagementError.tunnelAlreadyExistsWithThatName) + completionHandler(TunnelsManagerError.tunnelNameEmpty) return } @@ -132,7 +156,7 @@ class TunnelsManager { var oldName: String? if (isNameChanged) { if self.tunnels.contains(where: { $0.name == tunnelName }) { - completionHandler(TunnelManagementError.tunnelAlreadyExistsWithThatName) + completionHandler(TunnelsManagerError.tunnelAlreadyExistsWithThatName) return } oldName = tunnel.name @@ -149,7 +173,7 @@ class TunnelsManager { defer { self?.isModifyingTunnel = false } guard (error == nil) else { os_log("Modify: Saving configuration failed: %{public}@", log: OSLog.default, type: .error, "\(error!)") - completionHandler(TunnelManagementError.vpnSystemErrorOnModifyTunnel) + completionHandler(TunnelsManagerError.vpnSystemErrorOnModifyTunnel) return } if let s = self { @@ -173,7 +197,7 @@ class TunnelsManager { tunnel.isActivateOnDemandEnabled = tunnelProviderManager.isOnDemandEnabled guard (error == nil) else { os_log("Modify: Re-loading after saving configuration failed: %{public}@", log: OSLog.default, type: .error, "\(error!)") - completionHandler(TunnelManagementError.vpnSystemErrorOnModifyTunnel) + completionHandler(TunnelsManagerError.vpnSystemErrorOnModifyTunnel) return } completionHandler(nil) @@ -185,7 +209,7 @@ class TunnelsManager { } } - func remove(tunnel: TunnelContainer, completionHandler: @escaping (TunnelManagementError?) -> Void) { + func remove(tunnel: TunnelContainer, completionHandler: @escaping (TunnelsManagerError?) -> Void) { let tunnelProviderManager = tunnel.tunnelProvider isDeletingTunnel = true @@ -194,7 +218,7 @@ class TunnelsManager { defer { self?.isDeletingTunnel = false } guard (error == nil) else { os_log("Remove: Saving configuration failed: %{public}@", log: OSLog.default, type: .error, "\(error!)") - completionHandler(TunnelManagementError.vpnSystemErrorOnRemoveTunnel) + completionHandler(TunnelsManagerError.vpnSystemErrorOnRemoveTunnel) return } if let s = self { @@ -214,18 +238,17 @@ class TunnelsManager { return tunnels[index] } - func startActivation(of tunnel: TunnelContainer, completionHandler: @escaping (Error?) -> Void) { + func startActivation(of tunnel: TunnelContainer, completionHandler: @escaping (TunnelsManagerError?) -> Void) { guard (tunnel.status == .inactive) else { - completionHandler(TunnelActivationError.attemptingActivationWhenTunnelIsNotInactive) return } - func _startActivation(of tunnel: TunnelContainer, completionHandler: @escaping (Error?) -> Void) { + func _startActivation(of tunnel: TunnelContainer, completionHandler: @escaping (TunnelsManagerError?) -> Void) { tunnel.onActivationCommitted = { [weak self] (success) in if (!success) { let error = (InternetReachability.currentStatus() == .notReachable ? - TunnelActivationError.tunnelActivationFailedNoInternetConnection : - TunnelActivationError.tunnelActivationFailedInternalError) + TunnelsManagerError.tunnelActivationFailedNoInternetConnection : + TunnelsManagerError.tunnelActivationFailedInternalError) self?.activationDelegate?.tunnelActivationFailed(tunnel: tunnel, error: error) } } @@ -305,7 +328,7 @@ class TunnelContainer: NSObject { } } - fileprivate func startActivation(completionHandler: @escaping (Error?) -> Void) { + fileprivate func startActivation(completionHandler: @escaping (TunnelsManagerError?) -> Void) { assert(status == .inactive || status == .restarting || status == .waiting) guard let tunnelConfiguration = tunnelConfiguration() else { fatalError() } @@ -318,10 +341,10 @@ class TunnelContainer: NSObject { fileprivate func startActivation(recursionCount: UInt = 0, lastError: Error? = nil, tunnelConfiguration: TunnelConfiguration, - completionHandler: @escaping (Error?) -> Void) { + completionHandler: @escaping (TunnelsManagerError?) -> Void) { if (recursionCount >= 8) { os_log("startActivation: Failed after 8 attempts. Giving up with %{public}@", log: OSLog.default, type: .error, "\(lastError!)") - completionHandler(TunnelActivationError.tunnelActivationAttemptFailed) + completionHandler(TunnelsManagerError.tunnelActivationAttemptFailed) return } @@ -335,7 +358,7 @@ class TunnelContainer: NSObject { tunnelProvider.saveToPreferences { [weak self] (error) in if (error != nil) { os_log("Error saving tunnel after re-enabling: %{public}@", log: OSLog.default, type: .error, "\(error!)") - completionHandler(error) + completionHandler(TunnelsManagerError.tunnelActivationAttemptFailed) return } os_log("startActivation: Tunnel saved after re-enabling", log: OSLog.default, type: .info) @@ -354,28 +377,25 @@ class TunnelContainer: NSObject { os_log("startActivation: Success", log: OSLog.default, type: .debug) completionHandler(nil) } catch (let error) { - os_log("startActivation: Error starting tunnel. Examining error", log: OSLog.default, type: .debug) guard let vpnError = error as? NEVPNError else { - os_log("Failed to activate tunnel: %{public}@", log: OSLog.default, type: .debug, "\(error)") + os_log("Failed to activate tunnel: Error: %{public}@", log: OSLog.default, type: .debug, "\(error)") status = .inactive - completionHandler(error) + completionHandler(TunnelsManagerError.tunnelActivationAttemptFailed) return } guard (vpnError.code == NEVPNError.configurationInvalid || vpnError.code == NEVPNError.configurationStale) else { - os_log("Failed to activate tunnel: %{public}@", log: OSLog.default, type: .debug, "\(error)") + os_log("Failed to activate tunnel: VPN Error: %{public}@", log: OSLog.default, type: .debug, "\(error)") status = .inactive - completionHandler(error) + completionHandler(TunnelsManagerError.tunnelActivationAttemptFailed) return } assert(vpnError.code == NEVPNError.configurationInvalid || vpnError.code == NEVPNError.configurationStale) - os_log("startActivation: Error says: %{public}@", log: OSLog.default, type: .debug, - vpnError.code == NEVPNError.configurationInvalid ? "Configuration invalid" : "Configuration stale") os_log("startActivation: Will reload tunnel and then try to start it. ", log: OSLog.default, type: .info) tunnelProvider.loadFromPreferences { [weak self] (error) in if (error != nil) { - os_log("Failed to activate tunnel: %{public}@", log: OSLog.default, type: .debug, "\(error!)") + os_log("startActivation: Error reloading tunnel: %{public}@", log: OSLog.default, type: .debug, "\(error!)") self?.status = .inactive - completionHandler(error) + completionHandler(TunnelsManagerError.tunnelActivationAttemptFailed) return } os_log("startActivation: Tunnel reloaded", log: OSLog.default, type: .info) -- cgit v1.2.3-59-g8ed1b