From 3eaf00f9fd777a15bc1cefc0c51329a5e87a7183 Mon Sep 17 00:00:00 2001 From: Danny Moesch Date: Sat, 20 Jul 2019 23:02:38 +0200 Subject: [PATCH 1/4] Move 'keyHandler' in KeyFileManager from constructor to function --- passKit/Helpers/KeyFileManager.swift | 6 ++---- passKitTests/Helpers/KeyFileManagerTest.swift | 8 ++++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/passKit/Helpers/KeyFileManager.swift b/passKit/Helpers/KeyFileManager.swift index 30704723..2dae9eb4 100644 --- a/passKit/Helpers/KeyFileManager.swift +++ b/passKit/Helpers/KeyFileManager.swift @@ -15,19 +15,17 @@ public class KeyFileManager { private let keyType: CryptographicKey private let keyPath: String - private let keyHandler: KeyHandler private convenience init(keyType: CryptographicKey) { self.init(keyType: keyType, keyPath: keyType.getFileSharingPath()) } - public init(keyType: CryptographicKey, keyPath: String, keyHandler: @escaping KeyHandler = AppKeychain.add) { + public init(keyType: CryptographicKey, keyPath: String) { self.keyType = keyType self.keyPath = keyPath - self.keyHandler = keyHandler } - public func importKeyAndDeleteFile() throws { + public func importKeyAndDeleteFile(keyHandler: KeyHandler = AppKeychain.shared.add) throws { guard let keyFileContent = FileManager.default.contents(atPath: keyPath) else { throw AppError.ReadingFile(URL(fileURLWithPath: keyPath).lastPathComponent) } diff --git a/passKitTests/Helpers/KeyFileManagerTest.swift b/passKitTests/Helpers/KeyFileManagerTest.swift index a372d0af..9bed027f 100644 --- a/passKitTests/Helpers/KeyFileManagerTest.swift +++ b/passKitTests/Helpers/KeyFileManagerTest.swift @@ -12,7 +12,7 @@ import XCTest class KeyFileManagerTest: XCTestCase { private static let filePath = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent("test.txt").path - private static let keyFileManager = KeyFileManager(keyType: PgpKey.PUBLIC, keyPath: filePath) { _, _ in } + private static let keyFileManager = KeyFileManager(keyType: PgpKey.PUBLIC, keyPath: filePath) override func tearDown() { try? FileManager.default.removeItem(atPath: KeyFileManagerTest.filePath) @@ -22,17 +22,17 @@ class KeyFileManagerTest: XCTestCase { func testImportKeyAndDeleteFile() throws { let fileContent = "content".data(using: .ascii) var storage: [String: Data] = [:] - let keyFileManager = KeyFileManager(keyType: PgpKey.PRIVATE, keyPath: KeyFileManagerTest.filePath) { storage[$1] = $0 } + let keyFileManager = KeyFileManager(keyType: PgpKey.PRIVATE, keyPath: KeyFileManagerTest.filePath) FileManager.default.createFile(atPath: KeyFileManagerTest.filePath, contents: fileContent, attributes: nil) - try keyFileManager.importKeyAndDeleteFile() + try keyFileManager.importKeyAndDeleteFile { storage[$1] = $0 } XCTAssertFalse(FileManager.default.fileExists(atPath: KeyFileManagerTest.filePath)) XCTAssertTrue(storage[PgpKey.PRIVATE.getKeychainKey()] == fileContent) } func testErrorReadingFile() throws { - XCTAssertThrowsError(try KeyFileManagerTest.keyFileManager.importKeyAndDeleteFile()) { + XCTAssertThrowsError(try KeyFileManagerTest.keyFileManager.importKeyAndDeleteFile { _, _ in }) { XCTAssertEqual($0 as! AppError, AppError.ReadingFile("test.txt")) } } From 5527c9856806443e0762e4d8160e50c59d3c4055 Mon Sep 17 00:00:00 2001 From: Danny Moesch Date: Sat, 20 Jul 2019 23:17:26 +0200 Subject: [PATCH 2/4] Group test support files --- pass.xcodeproj/project.pbxproj | 24 ++++++++++++------- passKitTests/{ => Testbase}/TestBase.swift | 0 passKitTests/{ => Testbase}/TestPGPKeys.swift | 0 3 files changed, 16 insertions(+), 8 deletions(-) rename passKitTests/{ => Testbase}/TestBase.swift (100%) rename passKitTests/{ => Testbase}/TestPGPKeys.swift (100%) diff --git a/pass.xcodeproj/project.pbxproj b/pass.xcodeproj/project.pbxproj index 004a6b05..c2da6f28 100644 --- a/pass.xcodeproj/project.pbxproj +++ b/pass.xcodeproj/project.pbxproj @@ -42,7 +42,6 @@ 30697C5321F63E0B0064FCAC /* PasscodeExtensionDisplay.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30697C5121F63E0B0064FCAC /* PasscodeExtensionDisplay.swift */; }; 30697C5421F63E0B0064FCAC /* CredentialProviderViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30697C5221F63E0B0064FCAC /* CredentialProviderViewController.swift */; }; 30697C5F21F674800064FCAC /* String+UtilitiesTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30697C5E21F674800064FCAC /* String+UtilitiesTest.swift */; }; - 307BF39921BC2298003A082D /* TestBase.swift in Sources */ = {isa = PBXBuildFile; fileRef = 307BF39821BC2297003A082D /* TestBase.swift */; }; 308C273A2279F9CB0016D0E2 /* SearchBarScope.swift in Sources */ = {isa = PBXBuildFile; fileRef = 302202EE222F14E400555236 /* SearchBarScope.swift */; }; 30A1D29C21AF451E00E2D1F7 /* PasswordGeneratorFlavourTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30A1D29B21AF451E00E2D1F7 /* PasswordGeneratorFlavourTest.swift */; }; 30A1D2A221B2BC6F00E2D1F7 /* TokenBuilder.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30A1D2A121B2BC6F00E2D1F7 /* TokenBuilder.swift */; }; @@ -51,6 +50,8 @@ 30A1D2AA21B32A0100E2D1F7 /* OtpTypeTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30A1D2A921B32A0100E2D1F7 /* OtpTypeTest.swift */; }; 30A1D2AC21B32C2A00E2D1F7 /* TokenBuilderTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30A1D2AB21B32C2A00E2D1F7 /* TokenBuilderTest.swift */; }; 30B04860209A5141001013CA /* PasswordTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30B0485F209A5141001013CA /* PasswordTest.swift */; }; + 30BAC8C622E3BAAF00438475 /* TestBase.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30BAC8C422E3BAAF00438475 /* TestBase.swift */; }; + 30BAC8C722E3BAAF00438475 /* TestPGPKeys.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30BAC8C522E3BAAF00438475 /* TestPGPKeys.swift */; }; 30BF5EC821EA8FB5000E4154 /* Localizable.strings in Resources */ = {isa = PBXBuildFile; fileRef = 30BF5ECA21EA8FB5000E4154 /* Localizable.strings */; }; 30BF5ED721ED2434000E4154 /* Localizable.stringsdict in Resources */ = {isa = PBXBuildFile; fileRef = 30BF5ED521ED2434000E4154 /* Localizable.stringsdict */; }; 30C25DBD21F3599E00BB27BB /* InfoPlist.strings in Resources */ = {isa = PBXBuildFile; fileRef = 30C25DBF21F3599E00BB27BB /* InfoPlist.strings */; }; @@ -99,7 +100,6 @@ A2A7813F1E97DBD9001311F5 /* QRScannerController.swift in Sources */ = {isa = PBXBuildFile; fileRef = A2A7813E1E97DBD9001311F5 /* QRScannerController.swift */; }; A2AA934422DE30DD00D79A00 /* PGPAgent.swift in Sources */ = {isa = PBXBuildFile; fileRef = A2AA934322DE30DD00D79A00 /* PGPAgent.swift */; }; A2AA934622DE3A8000D79A00 /* PGPAgentTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = A2AA934522DE3A8000D79A00 /* PGPAgentTest.swift */; }; - A2AA934822DE3F0200D79A00 /* TestPGPKeys.swift in Sources */ = {isa = PBXBuildFile; fileRef = A2AA934722DE3F0200D79A00 /* TestPGPKeys.swift */; }; DC037CA61E4B883900609409 /* OpenSourceComponentsTableViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = DC037CA51E4B883900609409 /* OpenSourceComponentsTableViewController.swift */; }; DC037CA81E4B898100609409 /* BasicStaticTableViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = DC037CA71E4B898100609409 /* BasicStaticTableViewController.swift */; }; DC037CAA1E4B8EAE00609409 /* SpecialThanksTableViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = DC037CA91E4B8EAE00609409 /* SpecialThanksTableViewController.swift */; }; @@ -252,7 +252,6 @@ 30697C5121F63E0B0064FCAC /* PasscodeExtensionDisplay.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PasscodeExtensionDisplay.swift; sourceTree = ""; }; 30697C5221F63E0B0064FCAC /* CredentialProviderViewController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CredentialProviderViewController.swift; sourceTree = ""; }; 30697C5E21F674800064FCAC /* String+UtilitiesTest.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "String+UtilitiesTest.swift"; sourceTree = ""; }; - 307BF39821BC2297003A082D /* TestBase.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = TestBase.swift; sourceTree = ""; }; 30A1D29B21AF451E00E2D1F7 /* PasswordGeneratorFlavourTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PasswordGeneratorFlavourTest.swift; sourceTree = ""; }; 30A1D2A121B2BC6F00E2D1F7 /* TokenBuilder.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = TokenBuilder.swift; sourceTree = ""; }; 30A1D2A521B2D46100E2D1F7 /* OtpType.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = OtpType.swift; sourceTree = ""; }; @@ -260,6 +259,8 @@ 30A1D2A921B32A0100E2D1F7 /* OtpTypeTest.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = OtpTypeTest.swift; sourceTree = ""; }; 30A1D2AB21B32C2A00E2D1F7 /* TokenBuilderTest.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = TokenBuilderTest.swift; sourceTree = ""; }; 30B0485F209A5141001013CA /* PasswordTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PasswordTest.swift; sourceTree = ""; }; + 30BAC8C422E3BAAF00438475 /* TestBase.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = TestBase.swift; sourceTree = ""; }; + 30BAC8C522E3BAAF00438475 /* TestPGPKeys.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = TestPGPKeys.swift; sourceTree = ""; }; 30BF5EC921EA8FB5000E4154 /* en */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = en; path = en.lproj/Localizable.strings; sourceTree = ""; }; 30BF5ED621ED2434000E4154 /* en */ = {isa = PBXFileReference; lastKnownFileType = text.plist.stringsdict; name = en; path = en.lproj/Localizable.stringsdict; sourceTree = ""; }; 30C25DA921F34D2800BB27BB /* en */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = en; path = en.lproj/Main.strings; sourceTree = ""; }; @@ -303,7 +304,6 @@ A2A7813E1E97DBD9001311F5 /* QRScannerController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = QRScannerController.swift; sourceTree = ""; }; A2AA934322DE30DD00D79A00 /* PGPAgent.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PGPAgent.swift; sourceTree = ""; }; A2AA934522DE3A8000D79A00 /* PGPAgentTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PGPAgentTest.swift; sourceTree = ""; }; - A2AA934722DE3F0200D79A00 /* TestPGPKeys.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestPGPKeys.swift; sourceTree = ""; }; A2BC54C71EEE5669001FAFBD /* Objective-CBridgingHeader.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = "Objective-CBridgingHeader.h"; sourceTree = ""; }; DC037CA51E4B883900609409 /* OpenSourceComponentsTableViewController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = OpenSourceComponentsTableViewController.swift; sourceTree = ""; }; DC037CA71E4B898100609409 /* BasicStaticTableViewController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = BasicStaticTableViewController.swift; sourceTree = ""; }; @@ -467,6 +467,15 @@ path = Extensions; sourceTree = ""; }; + 30BAC8C322E3BA4300438475 /* Testbase */ = { + isa = PBXGroup; + children = ( + 30BAC8C422E3BAAF00438475 /* TestBase.swift */, + 30BAC8C522E3BAAF00438475 /* TestPGPKeys.swift */, + ); + path = Testbase; + sourceTree = ""; + }; 30C015A3214ECF2B005BB6DF /* Parser */ = { isa = PBXGroup; children = ( @@ -566,14 +575,13 @@ A26075861EEC6F34005DB03E /* passKitTests */ = { isa = PBXGroup; children = ( + 30BAC8C322E3BA4300438475 /* Testbase */, 30697C5521F63F870064FCAC /* Extensions */, 301F6464216164670071A4CE /* Helpers */, 30C015A7214ED378005BB6DF /* Models */, 30C015A6214ED32A005BB6DF /* Parser */, A26075871EEC6F34005DB03E /* passKitTests.swift */, - 307BF39821BC2297003A082D /* TestBase.swift */, A26075891EEC6F34005DB03E /* Info.plist */, - A2AA934722DE3F0200D79A00 /* TestPGPKeys.swift */, ); path = passKitTests; sourceTree = ""; @@ -1286,11 +1294,11 @@ 301F646D216166AA0071A4CE /* AdditionFieldTest.swift in Sources */, 30FD2F78214D9E0E005E0A92 /* ParserTest.swift in Sources */, A2AA934622DE3A8000D79A00 /* PGPAgentTest.swift in Sources */, + 30BAC8C622E3BAAF00438475 /* TestBase.swift in Sources */, 30B04860209A5141001013CA /* PasswordTest.swift in Sources */, - 307BF39921BC2298003A082D /* TestBase.swift in Sources */, 30697C5F21F674800064FCAC /* String+UtilitiesTest.swift in Sources */, 3032328A22C9FBA2009EBD9C /* KeyFileManagerTest.swift in Sources */, - A2AA934822DE3F0200D79A00 /* TestPGPKeys.swift in Sources */, + 30BAC8C722E3BAAF00438475 /* TestPGPKeys.swift in Sources */, 30A1D2AA21B32A0100E2D1F7 /* OtpTypeTest.swift in Sources */, 301F6468216165290071A4CE /* ConstantsTest.swift in Sources */, 30A1D29C21AF451E00E2D1F7 /* PasswordGeneratorFlavourTest.swift in Sources */, diff --git a/passKitTests/TestBase.swift b/passKitTests/Testbase/TestBase.swift similarity index 100% rename from passKitTests/TestBase.swift rename to passKitTests/Testbase/TestBase.swift diff --git a/passKitTests/TestPGPKeys.swift b/passKitTests/Testbase/TestPGPKeys.swift similarity index 100% rename from passKitTests/TestPGPKeys.swift rename to passKitTests/Testbase/TestPGPKeys.swift From b42401343da0679bf4c9bbb06d489a5bece41def Mon Sep 17 00:00:00 2001 From: Danny Moesch Date: Sat, 20 Jul 2019 23:31:13 +0200 Subject: [PATCH 3/4] Let AppKeychain not be static only --- .../GitServerSettingTableViewController.swift | 6 +++--- .../Controllers/PasswordsViewController.swift | 2 +- passKit/Helpers/AppKeychain.swift | 18 +++++++++-------- passKit/Models/PGPAgent.swift | 20 +++++++++---------- passKit/Models/PasscodeLock.swift | 6 +++--- passKit/Models/PasswordStore.swift | 14 ++++++------- 6 files changed, 34 insertions(+), 32 deletions(-) diff --git a/pass/Controllers/GitServerSettingTableViewController.swift b/pass/Controllers/GitServerSettingTableViewController.swift index fb069ac6..d2645f2b 100644 --- a/pass/Controllers/GitServerSettingTableViewController.swift +++ b/pass/Controllers/GitServerSettingTableViewController.swift @@ -43,7 +43,7 @@ class GitServerSettingTableViewController: UITableViewController { super.viewWillAppear(animated) // Grey out ssh option if ssh_key is not present if let sshLabel = sshLabel { - sshLabel.isEnabled = AppKeychain.contains(key: SshKey.PRIVATE.getKeychainKey()) + sshLabel.isEnabled = AppKeychain.shared.contains(key: SshKey.PRIVATE.getKeychainKey()) } } override func viewDidLoad() { @@ -86,7 +86,7 @@ class GitServerSettingTableViewController: UITableViewController { SVProgressHUD.setDefaultStyle(.light) SVProgressHUD.show(withStatus: "PrepareRepository".localize()) var gitCredential: GitCredential - let privateKey: String? = AppKeychain.get(for: SshKey.PRIVATE.getKeychainKey()) + let privateKey: String? = AppKeychain.shared.get(for: SshKey.PRIVATE.getKeychainKey()) if auth == "Password" || privateKey == nil { gitCredential = GitCredential(credential: GitCredential.Credential.http(userName: username)) } else { @@ -160,7 +160,7 @@ class GitServerSettingTableViewController: UITableViewController { authenticationMethod = "Password" } else if cell == authSSHKeyCell { - if !AppKeychain.contains(key: SshKey.PRIVATE.getKeychainKey()) { + if !AppKeychain.shared.contains(key: SshKey.PRIVATE.getKeychainKey()) { Utils.alert(title: "CannotSelectSshKey".localize(), message: "PleaseSetupSshKeyFirst.".localize(), controller: self, completion: nil) authenticationMethod = "Password" } else { diff --git a/pass/Controllers/PasswordsViewController.swift b/pass/Controllers/PasswordsViewController.swift index 0fd4c893..47a1672d 100644 --- a/pass/Controllers/PasswordsViewController.swift +++ b/pass/Controllers/PasswordsViewController.swift @@ -139,7 +139,7 @@ class PasswordsViewController: UIViewController, UITableViewDataSource, UITableV SVProgressHUD.setDefaultStyle(.light) SVProgressHUD.show(withStatus: "SyncingPasswordStore".localize()) var gitCredential: GitCredential - let privateKey: String? = AppKeychain.get(for: SshKey.PRIVATE.getKeychainKey()) + let privateKey: String? = AppKeychain.shared.get(for: SshKey.PRIVATE.getKeychainKey()) if SharedDefaults[.gitAuthenticationMethod] == "Password" || privateKey == nil { gitCredential = GitCredential(credential: GitCredential.Credential.http(userName: SharedDefaults[.gitUsername]!)) } else { diff --git a/passKit/Helpers/AppKeychain.swift b/passKit/Helpers/AppKeychain.swift index adb0c627..88cc0dd2 100644 --- a/passKit/Helpers/AppKeychain.swift +++ b/passKit/Helpers/AppKeychain.swift @@ -9,36 +9,38 @@ import KeychainAccess public class AppKeychain { + + public static let shared = AppKeychain() - private static let keychain = Keychain(service: Globals.bundleIdentifier, accessGroup: Globals.groupIdentifier) + private let keychain = Keychain(service: Globals.bundleIdentifier, accessGroup: Globals.groupIdentifier) .accessibility(.whenUnlockedThisDeviceOnly) .synchronizable(false) - public static func add(data: Data?, for key: String) { + public func add(data: Data?, for key: String) { keychain[data: key] = data } - public static func add(string: String?, for key: String) { + public func add(string: String?, for key: String) { keychain[key] = string } - public static func contains(key: String) -> Bool { + public func contains(key: String) -> Bool { return (try? keychain.contains(key)) ?? false } - public static func get(for key: String) -> Data? { + public func get(for key: String) -> Data? { return try? keychain.getData(key) } - public static func get(for key: String) -> String? { + public func get(for key: String) -> String? { return try? keychain.getString(key) } - public static func removeContent(for key: String) { + public func removeContent(for key: String) { try? keychain.remove(key) } - public static func removeAllContent() { + public func removeAllContent() { try? keychain.removeAll() } } diff --git a/passKit/Models/PGPAgent.swift b/passKit/Models/PGPAgent.swift index e3c521a4..bc8c739c 100644 --- a/passKit/Models/PGPAgent.swift +++ b/passKit/Models/PGPAgent.swift @@ -17,10 +17,10 @@ public class PGPAgent { // PGP passphrase public var passphrase: String? { set { - AppKeychain.add(string: newValue, for: "pgpKeyPassphrase") + AppKeychain.shared.add(string: newValue, for: "pgpKeyPassphrase") } get { - return AppKeychain.get(for: "pgpKeyPassphrase") + return AppKeychain.shared.get(for: "pgpKeyPassphrase") } } @@ -68,12 +68,12 @@ public class PGPAgent { } // Read the key data from keychain. - guard let pgpKeyData: Data = AppKeychain.get(for: keyType.getKeychainKey()) else { + guard let pgpKeyData: Data = AppKeychain.shared.get(for: keyType.getKeychainKey()) else { throw AppError.KeyImport } // Remove the key data from keychain temporary, in case the following step crashes repeatedly. - AppKeychain.removeContent(for: keyType.getKeychainKey()) + AppKeychain.shared.removeContent(for: keyType.getKeychainKey()) // Try GopenpgpwrapperReadKey first. if let key = GopenpgpwrapperReadKey(pgpKeyData) { @@ -83,7 +83,7 @@ public class PGPAgent { case .PRIVATE: self.privateKey = key } - AppKeychain.add(data: pgpKeyData, for: keyType.getKeychainKey()) + AppKeychain.shared.add(data: pgpKeyData, for: keyType.getKeychainKey()) return } @@ -98,7 +98,7 @@ public class PGPAgent { case .PRIVATE: self.privateKeyV2 = key } - AppKeychain.add(data: pgpKeyData, for: keyType.getKeychainKey()) + AppKeychain.shared.add(data: pgpKeyData, for: keyType.getKeychainKey()) return } @@ -107,13 +107,13 @@ public class PGPAgent { public func initPGPKey(from url: URL, keyType: PgpKey) throws { let pgpKeyData = try Data(contentsOf: url) - AppKeychain.add(data: pgpKeyData, for: keyType.getKeychainKey()) + AppKeychain.shared.add(data: pgpKeyData, for: keyType.getKeychainKey()) try initPGPKey(keyType) } public func initPGPKey(with armorKey: String, keyType: PgpKey) throws { let pgpKeyData = armorKey.data(using: .ascii)! - AppKeychain.add(data: pgpKeyData, for: keyType.getKeychainKey()) + AppKeychain.shared.add(data: pgpKeyData, for: keyType.getKeychainKey()) try initPGPKey(keyType) } @@ -167,8 +167,8 @@ public class PGPAgent { } public func removePGPKeys() { - AppKeychain.removeContent(for: PgpKey.PUBLIC.getKeychainKey()) - AppKeychain.removeContent(for: PgpKey.PRIVATE.getKeychainKey()) + AppKeychain.shared.removeContent(for: PgpKey.PUBLIC.getKeychainKey()) + AppKeychain.shared.removeContent(for: PgpKey.PRIVATE.getKeychainKey()) passphrase = nil publicKey = nil privateKey = nil diff --git a/passKit/Models/PasscodeLock.swift b/passKit/Models/PasscodeLock.swift index 541def57..fa4d7744 100644 --- a/passKit/Models/PasscodeLock.swift +++ b/passKit/Models/PasscodeLock.swift @@ -12,7 +12,7 @@ public class PasscodeLock { private static let identifier = Globals.bundleIdentifier + "passcode" /// Cached passcode to avoid frequent access to Keychain - private var passcode: String? = AppKeychain.get(for: PasscodeLock.identifier) + private var passcode: String? = AppKeychain.shared.get(for: PasscodeLock.identifier) /// Constructor used to migrate passcode from SharedDefaults to Keychain private init() { @@ -27,7 +27,7 @@ public class PasscodeLock { } public func save(passcode: String) { - AppKeychain.add(string: passcode, for: PasscodeLock.identifier) + AppKeychain.shared.add(string: passcode, for: PasscodeLock.identifier) self.passcode = passcode } @@ -36,7 +36,7 @@ public class PasscodeLock { } public func delete() { - AppKeychain.removeContent(for: PasscodeLock.identifier) + AppKeychain.shared.removeContent(for: PasscodeLock.identifier) passcode = nil } } diff --git a/passKit/Models/PasswordStore.swift b/passKit/Models/PasswordStore.swift index fbbef0c0..845ab4b3 100644 --- a/passKit/Models/PasswordStore.swift +++ b/passKit/Models/PasswordStore.swift @@ -39,19 +39,19 @@ public class PasswordStore { public var gitPassword: String? { set { - AppKeychain.add(string: newValue, for: "gitPassword") + AppKeychain.shared.add(string: newValue, for: "gitPassword") } get { - return AppKeychain.get(for: "gitPassword") + return AppKeychain.shared.get(for: "gitPassword") } } public var gitSSHPrivateKeyPassphrase: String? { set { - AppKeychain.add(string: newValue, for: "gitSSHPrivateKeyPassphrase") + AppKeychain.shared.add(string: newValue, for: "gitSSHPrivateKeyPassphrase") } get { - return AppKeychain.get(for: "gitSSHPrivateKeyPassphrase") + return AppKeychain.shared.get(for: "gitSSHPrivateKeyPassphrase") } } @@ -130,7 +130,7 @@ public class PasswordStore { } public func initGitSSHKey(with armorKey: String) throws { - AppKeychain.add(string: armorKey, for: SshKey.PRIVATE.getKeychainKey()) + AppKeychain.shared.add(string: armorKey, for: SshKey.PRIVATE.getKeychainKey()) } public func repositoryExisted() -> Bool { @@ -642,7 +642,7 @@ public class PasswordStore { self.pgpAgent?.removePGPKeys() - AppKeychain.removeAllContent() + AppKeychain.shared.removeAllContent() deleteCoreData(entityName: "PasswordEntity") @@ -726,7 +726,7 @@ public class PasswordStore { Defaults.remove(.gitSSHKeySource) Defaults.remove(.gitSSHPrivateKeyArmor) Defaults.remove(.gitSSHPrivateKeyURL) - AppKeychain.removeContent(for: SshKey.PRIVATE.getKeychainKey()) + AppKeychain.shared.removeContent(for: SshKey.PRIVATE.getKeychainKey()) gitSSHPrivateKeyPassphrase = nil } From 5c7d4e55a4c11bee6c6ec220b9a374c20c85fbd3 Mon Sep 17 00:00:00 2001 From: Danny Moesch Date: Sat, 20 Jul 2019 23:36:44 +0200 Subject: [PATCH 4/4] Introduce KeyStore protocol in order to provide specialized key store implementations for tests With the DictBasedKeychain the main AppKeychain is not influenced by tests. The previous implementation led to an empty Keychain requiring a new setup of the simulator. --- pass.xcodeproj/project.pbxproj | 8 ++++ passKit/Helpers/AppKeychain.swift | 2 +- passKit/Helpers/KeyStore.swift | 19 +++++++++ passKit/Models/PGPAgent.swift | 30 +++++++------ passKitTests/Models/PGPAgentTest.swift | 16 +++---- passKitTests/Testbase/DictBasedKeychain.swift | 42 +++++++++++++++++++ 6 files changed, 93 insertions(+), 24 deletions(-) create mode 100644 passKit/Helpers/KeyStore.swift create mode 100644 passKitTests/Testbase/DictBasedKeychain.swift diff --git a/pass.xcodeproj/project.pbxproj b/pass.xcodeproj/project.pbxproj index c2da6f28..87d3997d 100644 --- a/pass.xcodeproj/project.pbxproj +++ b/pass.xcodeproj/project.pbxproj @@ -52,6 +52,8 @@ 30B04860209A5141001013CA /* PasswordTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30B0485F209A5141001013CA /* PasswordTest.swift */; }; 30BAC8C622E3BAAF00438475 /* TestBase.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30BAC8C422E3BAAF00438475 /* TestBase.swift */; }; 30BAC8C722E3BAAF00438475 /* TestPGPKeys.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30BAC8C522E3BAAF00438475 /* TestPGPKeys.swift */; }; + 30BAC8CB22E3BB6C00438475 /* DictBasedKeychain.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30BAC8CA22E3BB6C00438475 /* DictBasedKeychain.swift */; }; + 30BAC8CD22E3BB9700438475 /* KeyStore.swift in Sources */ = {isa = PBXBuildFile; fileRef = 30BAC8CC22E3BB9700438475 /* KeyStore.swift */; }; 30BF5EC821EA8FB5000E4154 /* Localizable.strings in Resources */ = {isa = PBXBuildFile; fileRef = 30BF5ECA21EA8FB5000E4154 /* Localizable.strings */; }; 30BF5ED721ED2434000E4154 /* Localizable.stringsdict in Resources */ = {isa = PBXBuildFile; fileRef = 30BF5ED521ED2434000E4154 /* Localizable.stringsdict */; }; 30C25DBD21F3599E00BB27BB /* InfoPlist.strings in Resources */ = {isa = PBXBuildFile; fileRef = 30C25DBF21F3599E00BB27BB /* InfoPlist.strings */; }; @@ -261,6 +263,8 @@ 30B0485F209A5141001013CA /* PasswordTest.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PasswordTest.swift; sourceTree = ""; }; 30BAC8C422E3BAAF00438475 /* TestBase.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = TestBase.swift; sourceTree = ""; }; 30BAC8C522E3BAAF00438475 /* TestPGPKeys.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = TestPGPKeys.swift; sourceTree = ""; }; + 30BAC8CA22E3BB6C00438475 /* DictBasedKeychain.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DictBasedKeychain.swift; sourceTree = ""; }; + 30BAC8CC22E3BB9700438475 /* KeyStore.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = KeyStore.swift; sourceTree = ""; }; 30BF5EC921EA8FB5000E4154 /* en */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = en; path = en.lproj/Localizable.strings; sourceTree = ""; }; 30BF5ED621ED2434000E4154 /* en */ = {isa = PBXFileReference; lastKnownFileType = text.plist.stringsdict; name = en; path = en.lproj/Localizable.stringsdict; sourceTree = ""; }; 30C25DA921F34D2800BB27BB /* en */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = en; path = en.lproj/Main.strings; sourceTree = ""; }; @@ -470,6 +474,7 @@ 30BAC8C322E3BA4300438475 /* Testbase */ = { isa = PBXGroup; children = ( + 30BAC8CA22E3BB6C00438475 /* DictBasedKeychain.swift */, 30BAC8C422E3BAAF00438475 /* TestBase.swift */, 30BAC8C522E3BAAF00438475 /* TestPGPKeys.swift */, ); @@ -632,6 +637,7 @@ 30697C2521F63C590064FCAC /* FileManagerExtension.swift */, 30697C2421F63C590064FCAC /* Globals.swift */, 3032327322C7F710009EBD9C /* KeyFileManager.swift */, + 30BAC8CC22E3BB9700438475 /* KeyStore.swift */, 30697C2321F63C580064FCAC /* NotificationNames.swift */, 30697C2621F63C590064FCAC /* PasswordGeneratorFlavour.swift */, 302202EE222F14E400555236 /* SearchBarScope.swift */, @@ -1283,6 +1289,7 @@ 30697C2E21F63C5A0064FCAC /* Utils.swift in Sources */, 30697C4521F63CAB0064FCAC /* Password.swift in Sources */, 30697C4421F63CAB0064FCAC /* PasswordEntity.swift in Sources */, + 30BAC8CD22E3BB9700438475 /* KeyStore.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; @@ -1292,6 +1299,7 @@ files = ( 30A1D2AC21B32C2A00E2D1F7 /* TokenBuilderTest.swift in Sources */, 301F646D216166AA0071A4CE /* AdditionFieldTest.swift in Sources */, + 30BAC8CB22E3BB6C00438475 /* DictBasedKeychain.swift in Sources */, 30FD2F78214D9E0E005E0A92 /* ParserTest.swift in Sources */, A2AA934622DE3A8000D79A00 /* PGPAgentTest.swift in Sources */, 30BAC8C622E3BAAF00438475 /* TestBase.swift in Sources */, diff --git a/passKit/Helpers/AppKeychain.swift b/passKit/Helpers/AppKeychain.swift index 88cc0dd2..8f704924 100644 --- a/passKit/Helpers/AppKeychain.swift +++ b/passKit/Helpers/AppKeychain.swift @@ -8,7 +8,7 @@ import KeychainAccess -public class AppKeychain { +public class AppKeychain: KeyStore { public static let shared = AppKeychain() diff --git a/passKit/Helpers/KeyStore.swift b/passKit/Helpers/KeyStore.swift new file mode 100644 index 00000000..d57a51fa --- /dev/null +++ b/passKit/Helpers/KeyStore.swift @@ -0,0 +1,19 @@ +// +// KeyStore.swift +// passKit +// +// Created by Danny Moesch on 20.07.19. +// Copyright © 2019 Bob Sun. All rights reserved. +// + +import Foundation + +public protocol KeyStore { + func add(data: Data?, for key: String) + func add(string: String?, for key: String) + func contains(key: String) -> Bool + func get(for key: String) -> Data? + func get(for key: String) -> String? + func removeContent(for key: String) + func removeAllContent() +} diff --git a/passKit/Models/PGPAgent.swift b/passKit/Models/PGPAgent.swift index bc8c739c..871cebe2 100644 --- a/passKit/Models/PGPAgent.swift +++ b/passKit/Models/PGPAgent.swift @@ -12,15 +12,21 @@ import KeychainAccess import Gopenpgpwrapper public class PGPAgent { + + private let keyStore: KeyStore + + public init(keyStore: KeyStore = AppKeychain.shared) { + self.keyStore = keyStore + } public var pgpKeyID: String? // PGP passphrase public var passphrase: String? { set { - AppKeychain.shared.add(string: newValue, for: "pgpKeyPassphrase") + keyStore.add(string: newValue, for: "pgpKeyPassphrase") } get { - return AppKeychain.shared.get(for: "pgpKeyPassphrase") + return keyStore.get(for: "pgpKeyPassphrase") } } @@ -68,12 +74,12 @@ public class PGPAgent { } // Read the key data from keychain. - guard let pgpKeyData: Data = AppKeychain.shared.get(for: keyType.getKeychainKey()) else { + guard let pgpKeyData: Data = keyStore.get(for: keyType.getKeychainKey()) else { throw AppError.KeyImport } // Remove the key data from keychain temporary, in case the following step crashes repeatedly. - AppKeychain.shared.removeContent(for: keyType.getKeychainKey()) + keyStore.removeContent(for: keyType.getKeychainKey()) // Try GopenpgpwrapperReadKey first. if let key = GopenpgpwrapperReadKey(pgpKeyData) { @@ -83,7 +89,7 @@ public class PGPAgent { case .PRIVATE: self.privateKey = key } - AppKeychain.shared.add(data: pgpKeyData, for: keyType.getKeychainKey()) + keyStore.add(data: pgpKeyData, for: keyType.getKeychainKey()) return } @@ -98,7 +104,7 @@ public class PGPAgent { case .PRIVATE: self.privateKeyV2 = key } - AppKeychain.shared.add(data: pgpKeyData, for: keyType.getKeychainKey()) + keyStore.add(data: pgpKeyData, for: keyType.getKeychainKey()) return } @@ -107,19 +113,19 @@ public class PGPAgent { public func initPGPKey(from url: URL, keyType: PgpKey) throws { let pgpKeyData = try Data(contentsOf: url) - AppKeychain.shared.add(data: pgpKeyData, for: keyType.getKeychainKey()) + keyStore.add(data: pgpKeyData, for: keyType.getKeychainKey()) try initPGPKey(keyType) } public func initPGPKey(with armorKey: String, keyType: PgpKey) throws { let pgpKeyData = armorKey.data(using: .ascii)! - AppKeychain.shared.add(data: pgpKeyData, for: keyType.getKeychainKey()) + keyStore.add(data: pgpKeyData, for: keyType.getKeychainKey()) try initPGPKey(keyType) } public func initPGPKeyFromFileSharing() throws { - try KeyFileManager.PublicPgp.importKeyAndDeleteFile() - try KeyFileManager.PrivatePgp.importKeyAndDeleteFile() + try KeyFileManager.PublicPgp.importKeyAndDeleteFile(keyHandler: keyStore.add) + try KeyFileManager.PrivatePgp.importKeyAndDeleteFile(keyHandler: keyStore.add) try initPGPKeys() } @@ -167,8 +173,8 @@ public class PGPAgent { } public func removePGPKeys() { - AppKeychain.shared.removeContent(for: PgpKey.PUBLIC.getKeychainKey()) - AppKeychain.shared.removeContent(for: PgpKey.PRIVATE.getKeychainKey()) + keyStore.removeContent(for: PgpKey.PUBLIC.getKeychainKey()) + keyStore.removeContent(for: PgpKey.PRIVATE.getKeychainKey()) passphrase = nil publicKey = nil privateKey = nil diff --git a/passKitTests/Models/PGPAgentTest.swift b/passKitTests/Models/PGPAgentTest.swift index e3f7db03..2c1f18d1 100644 --- a/passKitTests/Models/PGPAgentTest.swift +++ b/passKitTests/Models/PGPAgentTest.swift @@ -11,14 +11,8 @@ import XCTest @testable import passKit class PGPAgentTest: XCTestCase { - - override func setUp() { - PGPAgent().removePGPKeys() - } - - override func tearDown() { - PGPAgent().removePGPKeys() - } + + private let keychain = DictBasedKeychain() func basicEncryptDecrypt(pgpAgent: PGPAgent) -> Bool { // Encrypt and decrypt. @@ -33,7 +27,7 @@ class PGPAgentTest: XCTestCase { } func testInitPGPKey() { - let pgpAgent = PGPAgent() + let pgpAgent = PGPAgent(keyStore: keychain) // [RSA2048] Setup keys. try? pgpAgent.initPGPKey(with: PGP_RSA2048_PUBLIC_KEY, keyType: .PUBLIC) @@ -41,7 +35,7 @@ class PGPAgentTest: XCTestCase { XCTAssertTrue(pgpAgent.isImported) XCTAssertEqual(pgpAgent.pgpKeyID, "A1024DAE") XCTAssertTrue(self.basicEncryptDecrypt(pgpAgent: pgpAgent)) - let pgpAgent2 = PGPAgent() + let pgpAgent2 = PGPAgent(keyStore: keychain) try? pgpAgent2.initPGPKeys() // load from the keychain XCTAssertTrue(self.basicEncryptDecrypt(pgpAgent: pgpAgent2)) pgpAgent.removePGPKeys() @@ -88,7 +82,7 @@ class PGPAgentTest: XCTestCase { } func testInitPGPKeyBadPrivateKeys() { - let pgpAgent = PGPAgent() + let pgpAgent = PGPAgent(keyStore: keychain) let plainData = "Hello World!".data(using: .utf8)! // [RSA2048] Setup the public key. diff --git a/passKitTests/Testbase/DictBasedKeychain.swift b/passKitTests/Testbase/DictBasedKeychain.swift new file mode 100644 index 00000000..cf3e7d4e --- /dev/null +++ b/passKitTests/Testbase/DictBasedKeychain.swift @@ -0,0 +1,42 @@ +// +// DictBasedKeychain.swift +// passKitTests +// +// Created by Danny Moesch on 20.07.19. +// Copyright © 2019 Bob Sun. All rights reserved. +// + +import Foundation +import passKit + +class DictBasedKeychain: KeyStore { + private var store: [String: Any] = [:] + + public func add(data: Data?, for key: String) { + store[key] = data + } + + public func add(string: String?, for key: String) { + store[key] = string + } + + public func contains(key: String) -> Bool { + return store[key] != nil + } + + public func get(for key: String) -> Data? { + return store[key] as? Data + } + + public func get(for key: String) -> String? { + return store[key] as? String + } + + public func removeContent(for key: String) { + store.removeValue(forKey: key) + } + + public func removeAllContent() { + store.removeAll() + } +}