From 4b075bcc95f503cc9dbcf1abb728db44e8418aa5 Mon Sep 17 00:00:00 2001 From: Davide De Rosa Date: Tue, 23 Oct 2018 10:49:35 +0200 Subject: [PATCH] Improve alerts on configuration import error Issue reporting is currently disabled because un unparsed .ovpn may contain sensitive data. --- Passepartout-iOS/AppDelegate.swift | 24 +++++++--- Passepartout-iOS/Global/IssueReporter.swift | 10 +++-- .../Organizer/WizardHostViewController.swift | 2 +- .../Resources/en.lproj/Localizable.strings | 11 +++-- Passepartout/Sources/ApplicationError.swift | 4 +- Passepartout/Sources/SwiftGen+Strings.swift | 45 ++++++++++++++----- .../TunnelKitProvider+FileConfiguration.swift | 4 +- 7 files changed, 69 insertions(+), 31 deletions(-) diff --git a/Passepartout-iOS/AppDelegate.swift b/Passepartout-iOS/AppDelegate.swift index 31688920..f8ddbce8 100644 --- a/Passepartout-iOS/AppDelegate.swift +++ b/Passepartout-iOS/AppDelegate.swift @@ -118,17 +118,29 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UISplitViewControllerDele } nav.modalPresentationStyle = .formSheet root.present(nav, animated: true, completion: nil) + } catch ApplicationError.missingConfiguration(let option) { + let message = L10n.Wizards.Host.Alerts.Missing.message(option) + alertConfigurationImportError(url: url, in: root, withMessage: message) } catch ApplicationError.unsupportedConfiguration(let option) { - let alert = Macros.alert(L10n.Organizer.Sections.Hosts.header, L10n.Wizards.Host.Alerts.unsupported(option)) - alert.addCancelAction(L10n.Global.ok) - root.present(alert, animated: true, completion: nil) + let message = L10n.Wizards.Host.Alerts.Unsupported.message(option) + alertConfigurationImportError(url: url, in: root, withMessage: message) } catch let e { - let alert = Macros.alert(L10n.Organizer.Sections.Hosts.header, L10n.Wizards.Host.Alerts.parsing(e.localizedDescription)) - alert.addCancelAction(L10n.Global.ok) - root.present(alert, animated: true, completion: nil) + let message = L10n.Wizards.Host.Alerts.Parsing.message(e.localizedDescription) + alertConfigurationImportError(url: url, in: root, withMessage: message) } return true } + + private func alertConfigurationImportError(url: URL, in vc: UIViewController, withMessage message: String) { + let alert = Macros.alert(L10n.Organizer.Sections.Hosts.header, message) +// alert.addDefaultAction(L10n.Wizards.Host.Alerts.Buttons.report) { +// var attach = IssueReporter.Attachments(debugLog: false, configurationURL: url) +// attach.description = message +// IssueReporter.shared.present(in: vc, withAttachments: attach) +// } + alert.addCancelAction(L10n.Global.cancel) + vc.present(alert, animated: true, completion: nil) + } } extension UISplitViewController { diff --git a/Passepartout-iOS/Global/IssueReporter.swift b/Passepartout-iOS/Global/IssueReporter.swift index 4718da90..473e8a5b 100644 --- a/Passepartout-iOS/Global/IssueReporter.swift +++ b/Passepartout-iOS/Global/IssueReporter.swift @@ -33,6 +33,8 @@ class IssueReporter: NSObject { let configurationURL: URL? + var description: String? + init(debugLog: Bool, configurationURL: URL?) { self.debugLog = debugLog self.configurationURL = configurationURL @@ -66,23 +68,23 @@ class IssueReporter: NSObject { let alert = Macros.alert(L10n.IssueReporter.title, L10n.IssueReporter.message) alert.addDefaultAction(L10n.IssueReporter.Buttons.accept) { VPN.shared.requestDebugLog(fallback: AppConstants.Log.debugSnapshot) { - self.composeEmail(withDebugLog: $0, configurationURL: attachments.configurationURL) + self.composeEmail(withDebugLog: $0, configurationURL: attachments.configurationURL, description: attachments.description) } } alert.addCancelAction(L10n.Global.cancel) viewController.present(alert, animated: true, completion: nil) } else { - composeEmail(withDebugLog: nil, configurationURL: attachments.configurationURL) + composeEmail(withDebugLog: nil, configurationURL: attachments.configurationURL, description: attachments.description) } } - private func composeEmail(withDebugLog debugLog: String?, configurationURL: URL?) { + private func composeEmail(withDebugLog debugLog: String?, configurationURL: URL?, description: String?) { let metadata = DebugLog(raw: "--").decoratedString() let vc = MFMailComposeViewController() vc.setToRecipients([AppConstants.IssueReporter.recipient]) vc.setSubject(L10n.IssueReporter.Email.subject(GroupConstants.App.name)) - vc.setMessageBody(L10n.IssueReporter.Email.body(metadata), isHTML: false) + vc.setMessageBody(L10n.IssueReporter.Email.body(description ?? L10n.IssueReporter.Email.description, metadata), isHTML: false) if let raw = debugLog { let attachment = DebugLog(raw: raw).decoratedData() vc.addAttachmentData(attachment, mimeType: AppConstants.IssueReporter.MIME.debugLog, fileName: AppConstants.IssueReporter.Filenames.debugLog) diff --git a/Passepartout-iOS/Scenes/Organizer/WizardHostViewController.swift b/Passepartout-iOS/Scenes/Organizer/WizardHostViewController.swift index 2fb94b15..70be468c 100644 --- a/Passepartout-iOS/Scenes/Organizer/WizardHostViewController.swift +++ b/Passepartout-iOS/Scenes/Organizer/WizardHostViewController.swift @@ -134,7 +134,7 @@ class WizardHostViewController: UITableViewController, TableModelHost, Wizard { profile.parameters = file.configuration guard !TransientStore.shared.service.containsProfile(profile) else { - let alert = Macros.alert(title, L10n.Wizards.Host.Alerts.existing) + let alert = Macros.alert(title, L10n.Wizards.Host.Alerts.Existing.message) alert.addDefaultAction(L10n.Global.ok) { self.next(withProfile: profile) } diff --git a/Passepartout/Resources/en.lproj/Localizable.strings b/Passepartout/Resources/en.lproj/Localizable.strings index 67e26879..da3f17c1 100644 --- a/Passepartout/Resources/en.lproj/Localizable.strings +++ b/Passepartout/Resources/en.lproj/Localizable.strings @@ -51,9 +51,11 @@ "wizards.host.cells.title_input.caption" = "Title"; "wizards.host.cells.title_input.placeholder" = "My Profile"; "wizards.host.sections.existing.header" = "Existing profiles"; -"wizards.host.alerts.existing" = "A host profile with the same title already exists. Replace it?"; -"wizards.host.alerts.unsupported" = "The configuration file contains an unsupported option (%@)."; -"wizards.host.alerts.parsing" = "Unable to parse the provided configuration file (%@)."; +"wizards.host.alerts.existing.message" = "A host profile with the same title already exists. Replace it?"; +"wizards.host.alerts.missing.message" = "The configuration file lacks a required option (%@)."; +"wizards.host.alerts.unsupported.message" = "The configuration file contains an unsupported option (%@)."; +"wizards.host.alerts.parsing.message" = "Unable to parse the provided configuration file (%@)."; +"wizards.host.alerts.buttons.report" = "Report an issue"; "service.welcome.message" = "Welcome to Passepartout!\n\nUse the organizer to add a new profile."; "service.sections.general.header" = "General"; @@ -175,7 +177,8 @@ "issue_reporter.buttons.accept" = "I understand"; "issue_reporter.alerts.email_not_configured.message" = "No e-mail account is configured."; "issue_reporter.email.subject" = "%@ - Report issue"; -"issue_reporter.email.body" = "Hi,\n\ndescription of the issue:\n\n%@\n\nRegards"; +"issue_reporter.email.body" = "Hi,\n\n%@\n\n%@\n\nRegards"; +"issue_reporter.email.description" = "description of the issue:"; "about.title" = "About"; "about.sections.info.header" = "General"; diff --git a/Passepartout/Sources/ApplicationError.swift b/Passepartout/Sources/ApplicationError.swift index 0d1815bc..ad91657c 100644 --- a/Passepartout/Sources/ApplicationError.swift +++ b/Passepartout/Sources/ApplicationError.swift @@ -30,10 +30,8 @@ enum ApplicationError: Error { case missingCredentials - case missingCA + case missingConfiguration(option: String) - case emptyRemotes - case unsupportedConfiguration(option: String) case migration diff --git a/Passepartout/Sources/SwiftGen+Strings.swift b/Passepartout/Sources/SwiftGen+Strings.swift index 14840762..5edc8d28 100644 --- a/Passepartout/Sources/SwiftGen+Strings.swift +++ b/Passepartout/Sources/SwiftGen+Strings.swift @@ -320,10 +320,12 @@ internal enum L10n { } internal enum Email { - /// Hi,\n\ndescription of the issue:\n\n%@\n\nRegards - internal static func body(_ p1: String) -> String { - return L10n.tr("Localizable", "issue_reporter.email.body", p1) + /// Hi,\n\n%@\n\n%@\n\nRegards + internal static func body(_ p1: String, _ p2: String) -> String { + return L10n.tr("Localizable", "issue_reporter.email.body", p1, p2) } + /// description of the issue: + internal static let description = L10n.tr("Localizable", "issue_reporter.email.description") /// %@ - Report issue internal static func subject(_ p1: String) -> String { return L10n.tr("Localizable", "issue_reporter.email.subject", p1) @@ -731,15 +733,36 @@ internal enum L10n { internal enum Host { internal enum Alerts { - /// A host profile with the same title already exists. Replace it? - internal static let existing = L10n.tr("Localizable", "wizards.host.alerts.existing") - /// Unable to parse the provided configuration file (%@). - internal static func parsing(_ p1: String) -> String { - return L10n.tr("Localizable", "wizards.host.alerts.parsing", p1) + + internal enum Buttons { + /// Report an issue + internal static let report = L10n.tr("Localizable", "wizards.host.alerts.buttons.report") } - /// The configuration file contains an unsupported option (%@). - internal static func unsupported(_ p1: String) -> String { - return L10n.tr("Localizable", "wizards.host.alerts.unsupported", p1) + + internal enum Existing { + /// A host profile with the same title already exists. Replace it? + internal static let message = L10n.tr("Localizable", "wizards.host.alerts.existing.message") + } + + internal enum Missing { + /// The configuration file lacks a required option (%@). + internal static func message(_ p1: String) -> String { + return L10n.tr("Localizable", "wizards.host.alerts.missing.message", p1) + } + } + + internal enum Parsing { + /// Unable to parse the provided configuration file (%@). + internal static func message(_ p1: String) -> String { + return L10n.tr("Localizable", "wizards.host.alerts.parsing.message", p1) + } + } + + internal enum Unsupported { + /// The configuration file contains an unsupported option (%@). + internal static func message(_ p1: String) -> String { + return L10n.tr("Localizable", "wizards.host.alerts.unsupported.message", p1) + } } } diff --git a/Passepartout/Sources/VPN/TunnelKitProvider+FileConfiguration.swift b/Passepartout/Sources/VPN/TunnelKitProvider+FileConfiguration.swift index fd526e56..7c70d081 100644 --- a/Passepartout/Sources/VPN/TunnelKitProvider+FileConfiguration.swift +++ b/Passepartout/Sources/VPN/TunnelKitProvider+FileConfiguration.swift @@ -255,13 +255,13 @@ extension TunnelKitProvider.Configuration { } guard let ca = optCA else { - throw ApplicationError.missingCA + throw ApplicationError.missingConfiguration(option: "ca") } // XXX: only reads first remote // hostnames = remotes.map { $0.0 } guard !remotes.isEmpty else { - throw ApplicationError.emptyRemotes + throw ApplicationError.missingConfiguration(option: "remote") } let hostname = remotes[0].0