From 6044098278c48d02d0871c3597c0fc89e0771b47 Mon Sep 17 00:00:00 2001 From: Danny Moesch Date: Sun, 23 Aug 2020 01:15:23 +0200 Subject: [PATCH] Refactor GitCredential to simplify it and to add tests --- pass.xcodeproj/project.pbxproj | 14 +- ...epositorySettingsTableViewController.swift | 33 ++-- .../Controllers/PasswordsViewController.swift | 30 ++-- pass/Helpers/GitCredentialPassword.swift | 53 ------- pass/Helpers/PasswordAlertPresenter.swift | 45 ++++++ .../Extensions/UIAlertActionExtension.swift | 10 +- passKit/Models/GitCredential.swift | 142 +++++++++++------- passKit/Models/PasswordStore.swift | 50 ++---- passKitTests/Models/GitCredentialTest.swift | 107 +++++++++++++ passKitTests/Models/PasswordStoreTest.swift | 20 +-- .../SyncRepositoryIntentHandler.swift | 16 +- 11 files changed, 295 insertions(+), 225 deletions(-) delete mode 100644 pass/Helpers/GitCredentialPassword.swift create mode 100644 pass/Helpers/PasswordAlertPresenter.swift create mode 100644 passKitTests/Models/GitCredentialTest.swift diff --git a/pass.xcodeproj/project.pbxproj b/pass.xcodeproj/project.pbxproj index 1936ec7..3919306 100644 --- a/pass.xcodeproj/project.pbxproj +++ b/pass.xcodeproj/project.pbxproj @@ -25,6 +25,8 @@ 30650E7323F847FC005CCD5E /* KeyImporter.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30650E7223F847FC005CCD5E /* KeyImporter.swift */; }; 306623332406F1A8000E2AD6 /* PasswordGeneratorTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 306623322406F1A7000E2AD6 /* PasswordGeneratorTest.swift */; }; 3066AD6823EE0D6500F65535 /* PGPKeyImporter.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3066AD6723EE0D6500F65535 /* PGPKeyImporter.swift */; }; + 30695E2024FA6C6500C9D46E /* PasswordAlertPresenter.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30695E1F24FA6C6500C9D46E /* PasswordAlertPresenter.swift */; }; + 30695E2524FAEF2600C9D46E /* GitCredentialTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30695E2424FAEF2600C9D46E /* GitCredentialTest.swift */; }; 30697C2A21F63C5A0064FCAC /* NotificationNames.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30697C2321F63C580064FCAC /* NotificationNames.swift */; }; 30697C2B21F63C5A0064FCAC /* Globals.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30697C2421F63C590064FCAC /* Globals.swift */; }; 30697C2C21F63C5A0064FCAC /* FileManagerExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30697C2521F63C590064FCAC /* FileManagerExtension.swift */; }; @@ -101,7 +103,6 @@ 8BA607EB4C9C8258741AC18C /* Pods_passExtension.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 14E955B67C88672AA3A40BA0 /* Pods_passExtension.framework */; }; 9A652414244BB33300DA0A41 /* UIAlertActionExtension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9A652413244BB33300DA0A41 /* UIAlertActionExtension.swift */; }; 9A8A8387402FCCCECB1232A4 /* Pods_passKitTests.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 3B2B2F844061EFA534FE9506 /* Pods_passKitTests.framework */; }; - 9AA710CA23939C68009E3213 /* GitCredentialPassword.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9AA710C923939C68009E3213 /* GitCredentialPassword.swift */; }; 9ADC954124418A5F0005402E /* PasswordStoreTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9ADC954024418A5F0005402E /* PasswordStoreTest.swift */; }; A20691F41F2A3D0E0096483D /* SecurePasteboard.swift in Sources */ = {isa = PBXBuildFile; fileRef = A20691F31F2A3D0E0096483D /* SecurePasteboard.swift */; }; A217ACE41E9BBBBD00A1A6CF /* GitConfigSettingsTableViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = A217ACE31E9BBBBD00A1A6CF /* GitConfigSettingsTableViewController.swift */; }; @@ -281,6 +282,8 @@ 30650E7223F847FC005CCD5E /* KeyImporter.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = KeyImporter.swift; sourceTree = ""; }; 306623322406F1A7000E2AD6 /* PasswordGeneratorTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PasswordGeneratorTest.swift; sourceTree = ""; }; 3066AD6723EE0D6500F65535 /* PGPKeyImporter.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PGPKeyImporter.swift; sourceTree = ""; }; + 30695E1F24FA6C6500C9D46E /* PasswordAlertPresenter.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PasswordAlertPresenter.swift; sourceTree = ""; }; + 30695E2424FAEF2600C9D46E /* GitCredentialTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GitCredentialTest.swift; sourceTree = ""; }; 30697C2321F63C580064FCAC /* NotificationNames.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = NotificationNames.swift; sourceTree = ""; }; 30697C2421F63C590064FCAC /* Globals.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Globals.swift; sourceTree = ""; }; 30697C2521F63C590064FCAC /* FileManagerExtension.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = FileManagerExtension.swift; sourceTree = ""; }; @@ -361,7 +364,6 @@ 9A1EF0B524C50EE00074FEAC /* passBetaExtension.entitlements */ = {isa = PBXFileReference; lastKnownFileType = text.plist.entitlements; path = passBetaExtension.entitlements; sourceTree = ""; }; 9A1EF0B624C50FEA0074FEAC /* passBetaShortcuts.entitlements */ = {isa = PBXFileReference; lastKnownFileType = text.plist.entitlements; path = passBetaShortcuts.entitlements; sourceTree = ""; }; 9A652413244BB33300DA0A41 /* UIAlertActionExtension.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UIAlertActionExtension.swift; sourceTree = ""; }; - 9AA710C923939C68009E3213 /* GitCredentialPassword.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GitCredentialPassword.swift; sourceTree = ""; }; 9ADC954024418A5F0005402E /* PasswordStoreTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PasswordStoreTest.swift; sourceTree = ""; }; A20691F31F2A3D0E0096483D /* SecurePasteboard.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SecurePasteboard.swift; sourceTree = ""; }; A217ACE31E9BBBBD00A1A6CF /* GitConfigSettingsTableViewController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; lineEnding = 0; path = GitConfigSettingsTableViewController.swift; sourceTree = ""; xcLanguageSpecificationIdentifier = xcode.lang.swift; }; @@ -668,9 +670,10 @@ 30C015A7214ED378005BB6DF /* Models */ = { isa = PBXGroup; children = ( + 30695E2424FAEF2600C9D46E /* GitCredentialTest.swift */, + 9ADC954024418A5F0005402E /* PasswordStoreTest.swift */, A2699ACE24027D9500F36323 /* PasswordTableEntryTest.swift */, 30B0485F209A5141001013CA /* PasswordTest.swift */, - 9ADC954024418A5F0005402E /* PasswordStoreTest.swift */, ); path = Models; sourceTree = ""; @@ -861,9 +864,9 @@ isa = PBXGroup; children = ( A2BC54C71EEE5669001FAFBD /* Objective-CBridgingHeader.h */, + 30695E1F24FA6C6500C9D46E /* PasswordAlertPresenter.swift */, A20691F31F2A3D0E0096483D /* SecurePasteboard.swift */, A2A61C1F1EEFABAD00CFE063 /* UtilsExtension.swift */, - 9AA710C923939C68009E3213 /* GitCredentialPassword.swift */, ); path = Helpers; sourceTree = ""; @@ -1571,6 +1574,7 @@ A2699ACF24027D9500F36323 /* PasswordTableEntryTest.swift in Sources */, 30FD2F78214D9E0E005E0A92 /* ParserTest.swift in Sources */, A2AA934622DE3A8000D79A00 /* PGPAgentTest.swift in Sources */, + 30695E2524FAEF2600C9D46E /* GitCredentialTest.swift in Sources */, 30BAC8C622E3BAAF00438475 /* TestBase.swift in Sources */, 30B04860209A5141001013CA /* PasswordTest.swift in Sources */, 30697C5F21F674800064FCAC /* String+UtilitiesTest.swift in Sources */, @@ -1644,8 +1648,8 @@ DC037CB21E4CAB1700609409 /* AboutRepositoryTableViewController.swift in Sources */, 30650E7323F847FC005CCD5E /* KeyImporter.swift in Sources */, A217ACE41E9BBBBD00A1A6CF /* GitConfigSettingsTableViewController.swift in Sources */, - 9AA710CA23939C68009E3213 /* GitCredentialPassword.swift in Sources */, DC037CB01E4CA51F00609409 /* GeneralSettingsTableViewController.swift in Sources */, + 30695E2024FA6C6500C9D46E /* PasswordAlertPresenter.swift in Sources */, DC037CB81E4DD1A500609409 /* AddPasswordTableViewController.swift in Sources */, DCC441521E8F6C06008A90C4 /* RawPasswordViewController.swift in Sources */, DC917BD71E2E8231000FDF54 /* AppDelegate.swift in Sources */, diff --git a/pass/Controllers/GitRepositorySettingsTableViewController.swift b/pass/Controllers/GitRepositorySettingsTableViewController.swift index e8288c5..289866b 100644 --- a/pass/Controllers/GitRepositorySettingsTableViewController.swift +++ b/pass/Controllers/GitRepositorySettingsTableViewController.swift @@ -10,7 +10,7 @@ import passKit import SVProgressHUD import UIKit -class GitRepositorySettingsTableViewController: UITableViewController { +class GitRepositorySettingsTableViewController: UITableViewController, PasswordAlertPresenter { // MARK: - View Outlet @IBOutlet var gitURLTextField: UITextField! @@ -25,6 +25,14 @@ class GitRepositorySettingsTableViewController: UITableViewController { private var sshLabel: UILabel? private let passwordStore = PasswordStore.shared + private let keychain = AppKeychain.shared + private var gitCredential: GitCredential { + GitCredential.from( + authenticationMethod: Defaults.gitAuthenticationMethod, + userName: Defaults.gitUsername, + keyStore: keychain + ) + } private var gitAuthenticationMethod: GitAuthenticationMethod { get { Defaults.gitAuthenticationMethod } set { @@ -48,16 +56,6 @@ class GitRepositorySettingsTableViewController: UITableViewController { set { Defaults.gitUsername = newValue } } - private var gitCredential: GitCredential { - switch Defaults.gitAuthenticationMethod { - case .password: - return GitCredential(credential: .http(userName: Defaults.gitUsername)) - case .key: - let privateKey: String = AppKeychain.shared.get(for: SshKey.PRIVATE.getKeychainKey()) ?? "" - return GitCredential(credential: .ssh(userName: Defaults.gitUsername, privateKey: privateKey)) - } - } - // MARK: - View Controller Lifecycle override func viewDidLoad() { @@ -72,7 +70,7 @@ class GitRepositorySettingsTableViewController: UITableViewController { override func viewWillAppear(_ animated: Bool) { super.viewWillAppear(animated) // Grey out ssh option if ssh_key is not present. - sshLabel?.isEnabled = AppKeychain.shared.contains(key: SshKey.PRIVATE.getKeychainKey()) + sshLabel?.isEnabled = keychain.contains(key: SshKey.PRIVATE.getKeychainKey()) updateAuthenticationMethodCheckView(for: gitAuthenticationMethod) } @@ -97,7 +95,7 @@ class GitRepositorySettingsTableViewController: UITableViewController { if cell == authPasswordCell { gitAuthenticationMethod = .password } else if cell == authSSHKeyCell { - if !AppKeychain.shared.contains(key: SshKey.PRIVATE.getKeychainKey()) { + if !keychain.contains(key: SshKey.PRIVATE.getKeychainKey()) { Utils.alert(title: "CannotSelectSshKey".localize(), message: "PleaseSetupSshKeyFirst.".localize(), controller: self) gitAuthenticationMethod = .password } else { @@ -177,11 +175,12 @@ class GitRepositorySettingsTableViewController: UITableViewController { SVProgressHUD.showProgress(progress, status: "CheckingOutBranch".localize(self.gitBranchName)) } + let options = self.gitCredential.getCredentialOptions(passwordProvider: self.present) + try self.passwordStore.cloneRepository( remoteRepoURL: self.gitUrl, - credential: self.gitCredential, branchName: self.gitBranchName, - requestCredentialPassword: self.requestCredentialPassword, + options: options, transferProgressBlock: transferProgressBlock, checkoutProgressBlock: checkoutProgressBlock ) @@ -301,10 +300,6 @@ class GitRepositorySettingsTableViewController: UITableViewController { present(optionMenu, animated: true) } - private func requestCredentialPassword(credential: GitCredential.Credential, lastPassword: String?) -> String? { - requestGitCredentialPassword(credential: credential, lastPassword: lastPassword, controller: self) - } - private func updateAuthenticationMethodCheckView(for method: GitAuthenticationMethod) { let passwordCheckView = authPasswordCell.viewWithTag(1001) let sshKeyCheckView = authSSHKeyCell.viewWithTag(1001) diff --git a/pass/Controllers/PasswordsViewController.swift b/pass/Controllers/PasswordsViewController.swift index b14877c..2e2eb3d 100644 --- a/pass/Controllers/PasswordsViewController.swift +++ b/pass/Controllers/PasswordsViewController.swift @@ -10,7 +10,7 @@ import passKit import SVProgressHUD import UIKit -class PasswordsViewController: UIViewController, UITableViewDataSource, UITableViewDelegate, UITabBarControllerDelegate, UISearchBarDelegate { +class PasswordsViewController: UIViewController, UITableViewDataSource, UITableViewDelegate, UITabBarControllerDelegate, UISearchBarDelegate, PasswordAlertPresenter { // Arbitrary threshold to decide whether to show folders or not for only a few entries. private static let hideSectionHeaderThreshold = 6 @@ -19,6 +19,13 @@ class PasswordsViewController: UIViewController, UITableViewDataSource, UITableV private var parentPasswordEntity: PasswordEntity? private let passwordStore = PasswordStore.shared private let keychain = AppKeychain.shared + private var gitCredential: GitCredential { + GitCredential.from( + authenticationMethod: Defaults.gitAuthenticationMethod, + userName: Defaults.gitUsername, + keyStore: keychain + ) + } private var tapTabBarTime: TimeInterval = 0 private var tapNavigationBarGestureRecognizer: UITapGestureRecognizer! @@ -30,16 +37,6 @@ class PasswordsViewController: UIViewController, UITableViewDataSource, UITableV case unsynced } - private var gitCredential: GitCredential { - switch Defaults.gitAuthenticationMethod { - case .password: - return GitCredential(credential: .http(userName: Defaults.gitUsername)) - case .key: - let privateKey: String = AppKeychain.shared.get(for: SshKey.PRIVATE.getKeychainKey()) ?? "" - return GitCredential(credential: .ssh(userName: Defaults.gitUsername, privateKey: privateKey)) - } - } - private lazy var searchController: UISearchController = { let uiSearchController = UISearchController(searchResultsController: nil) uiSearchController.searchResultsUpdater = self @@ -193,13 +190,15 @@ class PasswordsViewController: UIViewController, UITableViewDataSource, UITableV DispatchQueue.global(qos: .userInitiated).async { [unowned self] in do { - try self.passwordStore.pullRepository(credential: self.gitCredential, requestCredentialPassword: self.requestCredentialPassword) { git_transfer_progress, _ in + let pullOptions = self.gitCredential.getCredentialOptions(passwordProvider: self.present) + try self.passwordStore.pullRepository(options: pullOptions) { git_transfer_progress, _ in DispatchQueue.main.async { SVProgressHUD.showProgress(Float(git_transfer_progress.pointee.received_objects) / Float(git_transfer_progress.pointee.total_objects), status: "PullingFromRemoteRepository".localize()) } } if self.passwordStore.numberOfLocalCommits > 0 { - try self.passwordStore.pushRepository(credential: self.gitCredential, requestCredentialPassword: self.requestCredentialPassword) { current, total, _, _ in + let pushOptions = self.gitCredential.getCredentialOptions(passwordProvider: self.present) + try self.passwordStore.pushRepository(options: pushOptions) { current, total, _, _ in DispatchQueue.main.async { SVProgressHUD.showProgress(Float(current) / Float(total), status: "PushingToRemoteRepository".localize()) } @@ -212,6 +211,7 @@ class PasswordsViewController: UIViewController, UITableViewDataSource, UITableV self.syncControl.endRefreshing() } } catch { + self.gitCredential.delete() DispatchQueue.main.async { SVProgressHUD.dismiss() self.syncControl.endRefreshing() @@ -699,10 +699,6 @@ class PasswordsViewController: UIViewController, UITableViewDataSource, UITableV updateSearchResults(for: searchController) return true } - - private func requestCredentialPassword(credential: GitCredential.Credential, lastPassword: String?) -> String? { - requestGitCredentialPassword(credential: credential, lastPassword: lastPassword, controller: self) - } } extension PasswordsViewController: UISearchResultsUpdating { diff --git a/pass/Helpers/GitCredentialPassword.swift b/pass/Helpers/GitCredentialPassword.swift deleted file mode 100644 index 0ab7533..0000000 --- a/pass/Helpers/GitCredentialPassword.swift +++ /dev/null @@ -1,53 +0,0 @@ -// -// GitCredentialPassword.swift -// pass -// -// Created by Sun, Mingshen on 11/30/19. -// Copyright © 2019 Bob Sun. All rights reserved. -// - -import Foundation -import passKit -import SVProgressHUD - -public func requestGitCredentialPassword( - credential: GitCredential.Credential, - lastPassword: String?, - controller: UIViewController -) -> String? { - let sem = DispatchSemaphore(value: 0) - var password: String? - let message: String = { - switch credential { - case .http: - return "FillInGitAccountPassword.".localize() - case .ssh: - return "FillInSshKeyPassphrase.".localize() - } - }() - - DispatchQueue.main.async { - SVProgressHUD.dismiss() - let alert = UIAlertController(title: "Password".localize(), message: message, preferredStyle: .alert) - alert.addTextField { - $0.text = lastPassword ?? "" - $0.isSecureTextEntry = true - } - alert.addAction( - UIAlertAction.ok { _ in - password = alert.textFields?.first?.text - sem.signal() - } - ) - alert.addAction( - UIAlertAction.cancel { _ in - password = nil - sem.signal() - } - ) - controller.present(alert, animated: true) - } - - _ = sem.wait(timeout: .distantFuture) - return password -} diff --git a/pass/Helpers/PasswordAlertPresenter.swift b/pass/Helpers/PasswordAlertPresenter.swift new file mode 100644 index 0000000..10e6683 --- /dev/null +++ b/pass/Helpers/PasswordAlertPresenter.swift @@ -0,0 +1,45 @@ +// +// PasswordAlertPresenter.swift +// pass +// +// Created by Danny Moesch on 23.08.20. +// Copyright © 2020 Bob Sun. All rights reserved. +// + +import SVProgressHUD + +protocol PasswordAlertPresenter { + func present(message: String, lastPassword: String?) -> String? +} + +extension PasswordAlertPresenter where Self: UIViewController { + func present(message: String, lastPassword: String?) -> String? { + let sem = DispatchSemaphore(value: 0) + var password: String? + + DispatchQueue.main.async { + SVProgressHUD.dismiss() + let alert = UIAlertController(title: "Password".localize(), message: message, preferredStyle: .alert) + alert.addTextField { + $0.text = lastPassword ?? "" + $0.isSecureTextEntry = true + } + alert.addAction( + .ok { _ in + password = alert.textFields?.first?.text + sem.signal() + } + ) + alert.addAction( + .cancel { _ in + password = nil + sem.signal() + } + ) + self.present(alert, animated: true) + } + + _ = sem.wait(timeout: .distantFuture) + return password + } +} diff --git a/passKit/Extensions/UIAlertActionExtension.swift b/passKit/Extensions/UIAlertActionExtension.swift index 3928f4a..358140f 100644 --- a/passKit/Extensions/UIAlertActionExtension.swift +++ b/passKit/Extensions/UIAlertActionExtension.swift @@ -16,16 +16,12 @@ extension UIAlertAction { } } - public static func cancel(handler: ((UIAlertAction) -> Void)? = nil) -> UIAlertAction { - cancel(with: "Cancel", handler: handler) + public static func cancel(title: String = "Cancel".localize(), handler: ((UIAlertAction) -> Void)? = nil) -> UIAlertAction { + UIAlertAction(title: title, style: .cancel, handler: handler) } public static func dismiss(handler: ((UIAlertAction) -> Void)? = nil) -> UIAlertAction { - cancel(with: "Dismiss", handler: handler) - } - - public static func cancel(with _: String, handler: ((UIAlertAction) -> Void)? = nil) -> UIAlertAction { - UIAlertAction(title: "Cancel".localize(), style: .cancel, handler: handler) + cancel(title: "Dismiss".localize(), handler: handler) } public static func ok(handler: ((UIAlertAction) -> Void)? = nil) -> UIAlertAction { diff --git a/passKit/Models/GitCredential.swift b/passKit/Models/GitCredential.swift index c062afe..12220bb 100644 --- a/passKit/Models/GitCredential.swift +++ b/passKit/Models/GitCredential.swift @@ -6,75 +6,103 @@ // Copyright © 2017 Bob Sun. All rights reserved. // -import Foundation import ObjectiveGit +import SVProgressHUD public struct GitCredential { - private var credential: Credential - private let passwordStore = PasswordStore.shared + public typealias PasswordProvider = (String, String?) -> String? - public enum Credential { + private let credentialType: CredentialType + private let keyStore: KeyStore + + private enum CredentialType { case http(userName: String) case ssh(userName: String, privateKey: String) - } - public init(credential: Credential) { - self.credential = credential - } - - public func credentialProvider(requestCredentialPassword: @escaping (Credential, String?) -> String?) throws -> GTCredentialProvider { - var attempts = 0 - return GTCredentialProvider { _, _, _ -> (GTCredential?) in - var credential: GTCredential? - - switch self.credential { - case let .http(userName): - if attempts > 3 { - // After too many failures (say six), the error message "failed to authenticate ssh session" might be confusing. - return nil - } - var lastPassword = self.passwordStore.gitPassword - if lastPassword == nil || attempts != 0 { - if let requestedPassword = requestCredentialPassword(self.credential, lastPassword) { - if Defaults.isRememberGitCredentialPassphraseOn { - self.passwordStore.gitPassword = requestedPassword - } - lastPassword = requestedPassword - } else { - return nil - } - } - attempts += 1 - credential = try? GTCredential(userName: userName, password: lastPassword!) - case let .ssh(userName, privateKey): - if attempts > 0 { - // The passphrase seems correct, but the previous authentification failed. - return nil - } - var lastPassword = self.passwordStore.gitSSHPrivateKeyPassphrase - if lastPassword == nil || attempts != 0 { - if let requestedPassword = requestCredentialPassword(self.credential, lastPassword) { - if Defaults.isRememberGitCredentialPassphraseOn { - self.passwordStore.gitSSHPrivateKeyPassphrase = requestedPassword - } - lastPassword = requestedPassword - } else { - return nil - } - } - attempts += 1 - credential = try? GTCredential(userName: userName, publicKeyString: nil, privateKeyString: privateKey, passphrase: lastPassword!) + var requestPassphraseMessage: String { + switch self { + case .http: + return "FillInGitAccountPassword.".localize() + case .ssh: + return "FillInSshKeyPassphrase.".localize() } - return credential + } + + var keyStoreKey: String { + switch self { + case .http: + return Globals.gitPassword + case .ssh: + return Globals.gitSSHPrivateKeyPassphrase + } + } + + var allowedAttempts: Int { + switch self { + case .http: + return 4 + case .ssh: + return 1 + } + } + + func createGTCredential(password: String) throws -> GTCredential { + switch self { + case let .http(userName): + return try GTCredential(userName: userName, password: password) + case let .ssh(userName, privateKey): + return try GTCredential(userName: userName, publicKeyString: nil, privateKeyString: privateKey, passphrase: password) + } + } + } + + public static func from(authenticationMethod: GitAuthenticationMethod, userName: String, keyStore: KeyStore) -> Self { + switch authenticationMethod { + case .password: + return Self(credentialType: .http(userName: userName), keyStore: keyStore) + case .key: + let privateKey: String = keyStore.get(for: SshKey.PRIVATE.getKeychainKey()) ?? "" + return Self(credentialType: .ssh(userName: userName, privateKey: privateKey), keyStore: keyStore) + } + } + + public func getCredentialOptions(passwordProvider: @escaping PasswordProvider = { _, _ in nil }) -> [String: Any] { + let credentialProvider = createCredentialProvider(passwordProvider) + return [ + GTRepositoryCloneOptionsCredentialProvider: credentialProvider, + GTRepositoryRemoteOptionsCredentialProvider: credentialProvider, + ] + } + + private func createCredentialProvider(_ passwordProvider: @escaping PasswordProvider) -> GTCredentialProvider { + var attempts = 1 + return GTCredentialProvider { _, _, _ -> GTCredential? in + if attempts > self.credentialType.allowedAttempts { + return nil + } + guard let password = self.getPassword(attempts: attempts, passwordProvider: passwordProvider) else { + return nil + } + attempts += 1 + return try? self.credentialType.createGTCredential(password: password) } } public func delete() { - switch credential { - case .http: - passwordStore.gitPassword = nil - case .ssh: - passwordStore.gitSSHPrivateKeyPassphrase = nil + keyStore.removeContent(for: credentialType.keyStoreKey) + } + + private func getPassword(attempts: Int, passwordProvider: @escaping PasswordProvider) -> String? { + let lastPassword: String? = keyStore.get(for: credentialType.keyStoreKey) + if lastPassword == nil || attempts != 1 { + guard let requestedPassword = passwordProvider(credentialType.requestPassphraseMessage, lastPassword) else { + return nil + } + if Defaults.isRememberGitCredentialPassphraseOn { + keyStore.add(string: requestedPassword, for: credentialType.keyStoreKey) + } + return requestedPassword } + return lastPassword } } diff --git a/passKit/Models/PasswordStore.swift b/passKit/Models/PasswordStore.swift index 964eba5..1a06a67 100644 --- a/passKit/Models/PasswordStore.swift +++ b/passKit/Models/PasswordStore.swift @@ -173,35 +173,10 @@ public class PasswordStore { public func cloneRepository( remoteRepoURL: URL, - credential: GitCredential, branchName: String, - requestCredentialPassword: @escaping (GitCredential.Credential, String?) -> String?, - transferProgressBlock: @escaping (UnsafePointer, UnsafeMutablePointer) -> Void, - checkoutProgressBlock: @escaping (String, UInt, UInt) -> Void - ) throws { - do { - let credentialProvider = try credential.credentialProvider(requestCredentialPassword: requestCredentialPassword) - let options = [GTRepositoryCloneOptionsCredentialProvider: credentialProvider] - try cloneRepository( - remoteRepoURL: remoteRepoURL, - branchName: branchName, - transferProgressBlock: transferProgressBlock, - checkoutProgressBlock: checkoutProgressBlock, - options: options - ) - } catch { - credential.delete() - throw (error) - } - } - - public func cloneRepository( - remoteRepoURL: URL, - branchName: String, - transferProgressBlock: @escaping (UnsafePointer, UnsafeMutablePointer) -> Void, - checkoutProgressBlock: @escaping (String, UInt, UInt) -> Void, options: [AnyHashable: Any]? = nil, - completion: @escaping () -> Void = {} + transferProgressBlock: @escaping (UnsafePointer, UnsafeMutablePointer) -> Void = { _, _ in }, + checkoutProgressBlock: @escaping (String, UInt, UInt) -> Void = { _, _, _ in } ) throws { try? fm.removeItem(at: storeURL) try? fm.removeItem(at: tempStoreURL) @@ -231,7 +206,6 @@ public class PasswordStore { DispatchQueue.main.async { self.updatePasswordEntityCoreData() NotificationCenter.default.post(name: .passwordStoreUpdated, object: nil) - completion() } } @@ -252,15 +226,12 @@ public class PasswordStore { } public func pullRepository( - credential: GitCredential, - requestCredentialPassword: @escaping (GitCredential.Credential, String?) -> String?, - progressBlock: @escaping (UnsafePointer, UnsafeMutablePointer) -> Void + options: [String: Any], + progressBlock: @escaping (UnsafePointer, UnsafeMutablePointer) -> Void = { _, _ in } ) throws { guard let storeRepository = storeRepository else { throw AppError.RepositoryNotSet } - let credentialProvider = try credential.credentialProvider(requestCredentialPassword: requestCredentialPassword) - let options = [GTRepositoryRemoteOptionsCredentialProvider: credentialProvider] let remote = try GTRemote(name: "origin", in: storeRepository) try storeRepository.pull(storeRepository.currentBranch(), from: remote, withOptions: options, progress: progressBlock) Defaults.lastSyncedTime = Date() @@ -472,12 +443,13 @@ public class PasswordStore { return branches.first } - public func pushRepository(credential: GitCredential, requestCredentialPassword: @escaping (GitCredential.Credential, String?) -> String?, transferProgressBlock: @escaping (UInt32, UInt32, Int, UnsafeMutablePointer) -> Void) throws { + public func pushRepository( + options: [String: Any], + transferProgressBlock: @escaping (UInt32, UInt32, Int, UnsafeMutablePointer) -> Void = { _, _, _, _ in } + ) throws { guard let storeRepository = storeRepository else { throw AppError.RepositoryNotSet } - let credentialProvider = try credential.credentialProvider(requestCredentialPassword: requestCredentialPassword) - let options = [GTRepositoryRemoteOptionsCredentialProvider: credentialProvider] if let branch = try getLocalBranch(withName: Defaults.gitBranchName) { let remote = try GTRemote(name: "origin", in: storeRepository) try storeRepository.push(branch, to: remote, withOptions: options, progress: transferProgressBlock) @@ -741,9 +713,5 @@ public func findGPGID(from url: URL) -> String { } path = path.appendingPathComponent(".gpg-id") - do { - return try String(contentsOf: path).trimmed - } catch { - return "" - } + return (try? String(contentsOf: path))?.trimmed ?? "" } diff --git a/passKitTests/Models/GitCredentialTest.swift b/passKitTests/Models/GitCredentialTest.swift new file mode 100644 index 0000000..1d6fedb --- /dev/null +++ b/passKitTests/Models/GitCredentialTest.swift @@ -0,0 +1,107 @@ +// +// GitCredentialTest.swift +// passKitTests +// +// Created by Danny Moesch on 29.08.20. +// Copyright © 2020 Bob Sun. All rights reserved. +// + +import XCTest + +import ObjectiveGit +import SwiftyUserDefaults +@testable import passKit + +class GitCredentialTest: XCTestCase { + private static let defaultsID = "SharedDefaultsForGitCredentialTest" + + private let keyStore = DictBasedKeychain() + + override func setUp() { + super.setUp() + + keyStore.add(string: "password", for: Globals.gitPassword) + keyStore.add(string: "passphrase", for: Globals.gitSSHPrivateKeyPassphrase) + + UserDefaults().removePersistentDomain(forName: Self.defaultsID) + passKit.Defaults = DefaultsAdapter(defaults: UserDefaults(suiteName: Self.defaultsID)!, keyStore: DefaultsKeys()) + } + + override func tearDown() { + UserDefaults().removePersistentDomain(forName: Self.defaultsID) + + super.tearDown() + } + + func testDelete() { + let password = GitCredential.from(authenticationMethod: .password, userName: "user", keyStore: keyStore) + password.delete() + XCTAssertFalse(keyStore.contains(key: Globals.gitPassword)) + XCTAssertTrue(keyStore.contains(key: Globals.gitSSHPrivateKeyPassphrase)) + + let key = GitCredential.from(authenticationMethod: .key, userName: "user", keyStore: keyStore) + key.delete() + XCTAssertFalse(keyStore.contains(key: Globals.gitPassword)) + XCTAssertFalse(keyStore.contains(key: Globals.gitSSHPrivateKeyPassphrase)) + } + + func testOptions() { + let password = GitCredential.from(authenticationMethod: .password, userName: "user", keyStore: keyStore) + + let options = password.getCredentialOptions() + XCTAssertEqual(options.count, 2) + + let cloneCredentialProvider = options[GTRepositoryCloneOptionsCredentialProvider] as! GTCredentialProvider + let remoteCredentialProvider = options[GTRepositoryRemoteOptionsCredentialProvider] as! GTCredentialProvider + XCTAssertNotNil(cloneCredentialProvider) + XCTAssertEqual(cloneCredentialProvider, remoteCredentialProvider) + } + + func testPasswordCredentialProvider() { + let password = GitCredential.from(authenticationMethod: .password, userName: "user", keyStore: keyStore) + let expectation = self.expectation(description: "Password is requested.") + expectation.assertForOverFulfill = true + expectation.expectedFulfillmentCount = 3 + let options = password.getCredentialOptions { _, _ in + expectation.fulfill() + return "otherPassword" + } + let credentialProvider = options[GTRepositoryCloneOptionsCredentialProvider] as! GTCredentialProvider + + (1 ..< 5).forEach { _ in + XCTAssertNotNil(credentialProvider.credential(for: .userPassPlaintext, url: nil, userName: nil)) + } + XCTAssertNil(credentialProvider.credential(for: .userPassPlaintext, url: nil, userName: nil)) + wait(for: [expectation], timeout: 0) + } + + func testSSHKeyCredentialProvider() { + let credentialProvider = getCredentialProvider(authenticationMethod: .key) + + XCTAssertNotNil(credentialProvider.credential(for: .sshCustom, url: nil, userName: nil)) + XCTAssertNil(credentialProvider.credential(for: .sshCustom, url: nil, userName: nil)) + } + + func testCannotGetPassword() { + let credentialProvider = getCredentialProvider(authenticationMethod: .password) + + XCTAssertNotNil(credentialProvider.credential(for: .userPassPlaintext, url: nil, userName: nil)) + XCTAssertNil(credentialProvider.credential(for: .userPassPlaintext, url: nil, userName: nil)) + } + + func testSaveToKeyStore() { + let credentialProvider = getCredentialProvider(authenticationMethod: .key, password: "otherPassword") + + passKit.Defaults.isRememberGitCredentialPassphraseOn = true + keyStore.removeAllContent() + credentialProvider.credential(for: .sshCustom, url: nil, userName: nil) + + XCTAssertEqual(keyStore.get(for: Globals.gitSSHPrivateKeyPassphrase), "otherPassword") + } + + private func getCredentialProvider(authenticationMethod: GitAuthenticationMethod, password: String? = nil) -> GTCredentialProvider { + let credential = GitCredential.from(authenticationMethod: authenticationMethod, userName: "user", keyStore: keyStore) + let options = credential.getCredentialOptions { _, _ in password } + return options[GTRepositoryCloneOptionsCredentialProvider] as! GTCredentialProvider + } +} diff --git a/passKitTests/Models/PasswordStoreTest.swift b/passKitTests/Models/PasswordStoreTest.swift index 325861a..182ad76 100644 --- a/passKitTests/Models/PasswordStoreTest.swift +++ b/passKitTests/Models/PasswordStoreTest.swift @@ -12,27 +12,13 @@ import XCTest @testable import passKit class PasswordStoreTest: XCTestCase { - let cloneOptions: [String: GTCredentialProvider] = { - let credentialProvider = GTCredentialProvider { _, _, _ -> (GTCredential?) in - try? GTCredential(userName: "", password: "") - } - return [GTRepositoryCloneOptionsCredentialProvider: credentialProvider] - }() - - let remoteRepoURL = URL(string: "https://github.com/mssun/passforios-password-store.git")! + private let remoteRepoURL = URL(string: "https://github.com/mssun/passforios-password-store.git")! func testCloneAndDecryptMultiKeys() throws { let url = URL(fileURLWithPath: "\(Globals.repositoryPath)-test") let passwordStore = PasswordStore(url: url) - let expectation = self.expectation(description: "clone") - try passwordStore.cloneRepository( - remoteRepoURL: remoteRepoURL, - branchName: "master", - transferProgressBlock: { _, _ in }, - checkoutProgressBlock: { _, _, _ in }, - options: cloneOptions, - completion: { expectation.fulfill() } - ) + try passwordStore.cloneRepository(remoteRepoURL: remoteRepoURL, branchName: "master") + expectation(for: NSPredicate { _, _ in FileManager.default.fileExists(atPath: url.path) }, evaluatedWith: nil) waitForExpectations(timeout: 3, handler: nil) [ diff --git a/passShortcuts/SyncRepositoryIntentHandler.swift b/passShortcuts/SyncRepositoryIntentHandler.swift index 74dd026..2448156 100644 --- a/passShortcuts/SyncRepositoryIntentHandler.swift +++ b/passShortcuts/SyncRepositoryIntentHandler.swift @@ -14,13 +14,11 @@ public class SyncRepositoryIntentHandler: NSObject, SyncRepositoryIntentHandling private let keychain = AppKeychain.shared private var gitCredential: GitCredential { - switch Defaults.gitAuthenticationMethod { - case .password: - return GitCredential(credential: .http(userName: Defaults.gitUsername)) - case .key: - let privateKey: String = keychain.get(for: SshKey.PRIVATE.getKeychainKey()) ?? "" - return GitCredential(credential: .ssh(userName: Defaults.gitUsername, privateKey: privateKey)) - } + GitCredential.from( + authenticationMethod: Defaults.gitAuthenticationMethod, + userName: Defaults.gitUsername, + keyStore: keychain + ) } public func handle(intent _: SyncRepositoryIntent, completion: @escaping (SyncRepositoryIntentResponse) -> Void) { @@ -33,14 +31,14 @@ public class SyncRepositoryIntentHandler: NSObject, SyncRepositoryIntentHandling return } do { - try passwordStore.pullRepository(credential: gitCredential, requestCredentialPassword: { _, _ in nil }, progressBlock: { _, _ in }) + try passwordStore.pullRepository(options: gitCredential.getCredentialOptions()) } catch { completion(SyncRepositoryIntentResponse(code: .pullFailed, userActivity: nil)) return } if passwordStore.numberOfLocalCommits > 0 { do { - try passwordStore.pushRepository(credential: gitCredential, requestCredentialPassword: { _, _ in nil }, transferProgressBlock: { _, _, _, _ in }) + try passwordStore.pushRepository(options: gitCredential.getCredentialOptions()) } catch { completion(SyncRepositoryIntentResponse(code: .pushFailed, userActivity: nil)) return