From a4310630a8c080a96849a0c0cafcc0c9685814e3 Mon Sep 17 00:00:00 2001 From: lougeniaC64 Date: Fri, 17 Nov 2023 08:52:40 -0500 Subject: [PATCH] Refactor FXIOS-7619 [v121] update key retrieval logic (#16973) (cherry picked from commit 88e695935bb2b8a89558fd2c1a2c0c1c41f51672) --- Providers/RustSyncManager.swift | 73 +++++---- Storage/Rust/RustLogins.swift | 143 ++++++++++++------ .../PasswordManagerViewModelTests.swift | 11 +- Tests/ClientTests/RustSyncManagerTests.swift | 1 + Tests/StorageTests/RustLoginsTests.swift | 111 ++++++++++++++ Tests/UnitTest.xctestplan | 2 + 6 files changed, 259 insertions(+), 82 deletions(-) diff --git a/Providers/RustSyncManager.swift b/Providers/RustSyncManager.swift index 3f524d800164..3f2c21001181 100644 --- a/Providers/RustSyncManager.swift +++ b/Providers/RustSyncManager.swift @@ -332,43 +332,58 @@ public class RustSyncManager: NSObject, SyncManager { var rustEngines: [String] = [] var registeredPlaces = false - for engine in engines.filter({ syncManagerAPI.rustTogglableEngines.contains($0) }) { - switch engine { - case .tabs: - profile?.tabs.registerWithSyncManager() - rustEngines.append(engine.rawValue) - case .passwords: - profile?.logins.registerWithSyncManager() - if let key = try? profile?.logins.getStoredKey() { - localEncryptionKeys[engine.rawValue] = key + let registerEngines: (String?) -> Void = { loginKey in + for engine in engines.filter({ self.syncManagerAPI.rustTogglableEngines.contains($0) }) { + switch engine { + case .tabs: + self.profile?.tabs.registerWithSyncManager() rustEngines.append(engine.rawValue) - } else { - logger.log("Login encryption key could not be retrieved for syncing", - level: .warning, - category: .sync) - } - case .creditcards: - if self.creditCardAutofillEnabled { - profile?.autofill.registerWithSyncManager() - if let key = try? profile?.autofill.getStoredKey() { + case .passwords: + self.profile?.logins.registerWithSyncManager() + if let key = loginKey { localEncryptionKeys[engine.rawValue] = key rustEngines.append(engine.rawValue) - } else { - logger.log("Credit card encryption key could not be retrieved for syncing", - level: .warning, - category: .sync) } + case .creditcards: + if self.creditCardAutofillEnabled { + self.profile?.autofill.registerWithSyncManager() + if let key = try? self.profile?.autofill.getStoredKey() { + localEncryptionKeys[engine.rawValue] = key + rustEngines.append(engine.rawValue) + } else { + self.logger.log("Credit card encryption key could not be retrieved for syncing", + level: .warning, + category: .sync) + } + } + case .bookmarks, .history: + if !registeredPlaces { + self.profile?.places.registerWithSyncManager() + registeredPlaces = true + } + rustEngines.append(engine.rawValue) } - case .bookmarks, .history: - if !registeredPlaces { - profile?.places.registerWithSyncManager() - registeredPlaces = true - } - rustEngines.append(engine.rawValue) } } - completion((rustEngines, localEncryptionKeys)) + let shouldSyncLogins = engines.contains(RustSyncManagerAPI.TogglableEngine.passwords) + if shouldSyncLogins { + profile?.logins.getStoredKey { result in + switch result { + case .success(let key): + registerEngines(key) + case .failure(let err): + self.logger.log("Login encryption key could not be retrieved for syncing: \(err)", + level: .warning, + category: .sync) + registerEngines(nil) + } + completion((rustEngines, localEncryptionKeys)) + } + } else { + registerEngines(nil) + completion((rustEngines, localEncryptionKeys)) + } } private func doSync(params: SyncParams, completion: @escaping (SyncResult) -> Void) { diff --git a/Storage/Rust/RustLogins.swift b/Storage/Rust/RustLogins.swift index 695f52b447ca..de1294f2a917 100644 --- a/Storage/Rust/RustLogins.swift +++ b/Storage/Rust/RustLogins.swift @@ -355,8 +355,9 @@ public extension LoginEntry { } public enum LoginEncryptionKeyError: Error { - case illegalState case noKeyCreated + case illegalState + case dbRecordCountVerificationError(String) } public class RustLoginEncryptionKeys { @@ -691,15 +692,20 @@ public class RustLogins { return } - do { - let key = try self.getStoredKey() - let id = try self.storage?.add(login: login, encryptionKey: key).record.id - deferred.fill(Maybe(success: id!)) - } catch let err as NSError { - deferred.fill(Maybe(failure: err)) + self.getStoredKey { result in + switch result { + case .success(let key): + do { + let id = try self.storage?.add(login: login, encryptionKey: key).record.id + deferred.fill(Maybe(success: id!)) + } catch let err as NSError { + deferred.fill(Maybe(failure: err)) + } + case .failure(let err): + deferred.fill(Maybe(failure: err)) + } } } - return deferred } @@ -734,12 +740,18 @@ public class RustLogins { return } - do { - let key = try self.getStoredKey() - _ = try self.storage?.update(id: id, login: login, encryptionKey: key) - deferred.fill(Maybe(success: ())) - } catch let err as NSError { - deferred.fill(Maybe(failure: err)) + self.getStoredKey { result in + switch result { + case .success(let key): + do { + _ = try self.storage?.update(id: id, login: login, encryptionKey: key) + deferred.fill(Maybe(success: ())) + } catch let err as NSError { + deferred.fill(Maybe(failure: err)) + } + case .failure(let err): + deferred.fill(Maybe(failure: err)) + } } } @@ -815,75 +827,106 @@ public class RustLogins { keychain.removeObject(forKey: rustKeys.loginsSaltKeychainKey, withAccessibility: .afterFirstUnlock) } - public func getStoredKey() throws -> String { + private func resetLoginsAndKey(rustKeys: RustLoginEncryptionKeys, + completion: @escaping (Result) -> Void) { + self.wipeLocalEngine().upon { result in + guard result.isSuccess else { + completion(.failure(result.failureValue! as NSError)) + return + } + + do { + let key = try rustKeys.createAndStoreKey() + completion(.success(key)) + } catch let error as NSError { + self.logger.log("Error creating logins encryption key", + level: .warning, + category: .storage, + description: error.localizedDescription) + completion(.failure(error)) + } + } + } + + public func getStoredKey(completion: @escaping (Result) -> Void) { let rustKeys = RustLoginEncryptionKeys() let key = rustKeys.keychain.string(forKey: rustKeys.loginPerFieldKeychainKey) let encryptedCanaryPhrase = rustKeys.keychain.string(forKey: rustKeys.canaryPhraseKey) switch(key, encryptedCanaryPhrase) { case (.some(key), .some(encryptedCanaryPhrase)): - // We expected the key to be present, and it is. do { let canaryIsValid = try checkCanary( canary: encryptedCanaryPhrase!, text: rustKeys.canaryPhrase, encryptionKey: key!) + if canaryIsValid { - return key! + completion(.success(key!)) } else { logger.log("Logins key was corrupted, new one generated", level: .warning, category: .storage) GleanMetrics.LoginsStoreKeyRegeneration.corrupt.record() - _ = self.wipeLocalEngine() - - return try rustKeys.createAndStoreKey() + self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion) } } catch let error as NSError { - logger.log("Error retrieving logins encryption key", + logger.log("Error validating logins encryption key", level: .warning, category: .storage, description: error.localizedDescription) + completion(.failure(error)) } case (.some(key), .none): // The key is present, but we didn't expect it to be there. - do { - logger.log("Logins key lost due to storage malfunction, new one generated", - level: .warning, - category: .storage) - GleanMetrics.LoginsStoreKeyRegeneration.other.record() - _ = self.wipeLocalEngine() - return try rustKeys.createAndStoreKey() - } catch let error as NSError { - throw error - } + logger.log("Logins key lost due to storage malfunction, new one generated", + level: .warning, + category: .storage) + GleanMetrics.LoginsStoreKeyRegeneration.other.record() + self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion) case (.none, .some(encryptedCanaryPhrase)): // We expected the key to be present, but it's gone missing on us. - do { - logger.log("Logins key lost, new one generated", - level: .warning, - category: .storage) - GleanMetrics.LoginsStoreKeyRegeneration.lost.record() - _ = self.wipeLocalEngine() - return try rustKeys.createAndStoreKey() - } catch let error as NSError { - throw error - } + logger.log("Logins key lost, new one generated", + level: .warning, + category: .storage) + GleanMetrics.LoginsStoreKeyRegeneration.lost.record() + self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion) case (.none, .none): - // We didn't expect the key to be present, and it's not (which is the case for first-time calls). - do { - return try rustKeys.createAndStoreKey() - } catch let error as NSError { - throw error + // We didn't expect the key to be present, which either means this is a first-time + // call or the key data has been cleared from the keychain. + + self.hasSyncedLogins().upon { result in + guard result.failureValue == nil else { + completion(.failure(result.failureValue! as NSError)) + return + } + + guard let hasLogins = result.successValue else { + let msg = "Failed to verify logins count before attempting to reset key" + completion(.failure(LoginEncryptionKeyError.dbRecordCountVerificationError(msg) as NSError)) + return + } + + if hasLogins { + // Since the key data isn't present and we have login records in + // the database, we both clear the databbase and the reset the key. + self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion) + } else { + // There are no records in the database so we don't need to wipe any + // existing login records. We just need to create a new key. + do { + let key = try rustKeys.createAndStoreKey() + completion(.success(key)) + } catch let error as NSError { + completion(.failure(error)) + } + } } default: // If none of the above cases apply, we're in a state that shouldn't be possible but is disallowed nonetheless - throw LoginEncryptionKeyError.illegalState + completion(.failure(LoginEncryptionKeyError.illegalState as NSError)) } - - // This must be declared again for Swift's sake even though the above switch statement handles all cases - throw LoginEncryptionKeyError.illegalState } } diff --git a/Tests/ClientTests/PasswordManagerViewModelTests.swift b/Tests/ClientTests/PasswordManagerViewModelTests.swift index 921c77c456dd..a27468f38ee0 100644 --- a/Tests/ClientTests/PasswordManagerViewModelTests.swift +++ b/Tests/ClientTests/PasswordManagerViewModelTests.swift @@ -44,14 +44,19 @@ class PasswordManagerViewModelTests: XCTestCase { "username": "username\(i)", "password": "password\(i)" ]) - let addResult = self.viewModel.profile.logins.addLogin(login: login) - XCTAssertTrue(addResult.value.isSuccess) - XCTAssertNotNil(addResult.value.successValue) + let addExp = expectation(description: "\(#function)\(#line)") + self.viewModel.profile.logins.addLogin(login: login).upon { addResult in + XCTAssertTrue(addResult.isSuccess) + XCTAssertNotNil(addResult.successValue) + addExp.fulfill() + } } let logins = self.viewModel.profile.logins.listLogins().value XCTAssertTrue(logins.isSuccess) XCTAssertNotNil(logins.successValue) + + waitForExpectations(timeout: 10, handler: nil) } func testQueryLogins() { diff --git a/Tests/ClientTests/RustSyncManagerTests.swift b/Tests/ClientTests/RustSyncManagerTests.swift index b1af94f57bfc..461715d02369 100644 --- a/Tests/ClientTests/RustSyncManagerTests.swift +++ b/Tests/ClientTests/RustSyncManagerTests.swift @@ -54,6 +54,7 @@ class RustSyncManagerTests: XCTestCase { func testGetEnginesAndKeys() { let engines: [RustSyncManagerAPI.TogglableEngine] = [.bookmarks, .creditcards, .history, .passwords, .tabs] + rustSyncManager.getEnginesAndKeys(engines: engines) { (engines, keys) in XCTAssertEqual(engines.count, 5) diff --git a/Tests/StorageTests/RustLoginsTests.swift b/Tests/StorageTests/RustLoginsTests.swift index 265e7d36687d..21f1c2bfbdad 100644 --- a/Tests/StorageTests/RustLoginsTests.swift +++ b/Tests/StorageTests/RustLoginsTests.swift @@ -6,11 +6,64 @@ import XCTest import Shared @testable import Storage +class MockRustLogins: RustLogins { + var logins = [Int: LoginEntry]() + var id = 0 + + override func reopenIfClosed() -> NSError? { + return nil + } + + override func addLogin(login: LoginEntry) -> Deferred> { + let deferred = Deferred>() + queue.async { + self.id += 1 + self.logins[self.id] = login + deferred.fill(Maybe(success: String(self.id))) + } + return deferred + } + + public func mockListLogins() -> Deferred> { + let deferred = Deferred>() + queue.async { + let list = self.logins.map { (key, value) in + return value + } + deferred.fill(Maybe(success: list)) + } + return deferred + } + + override func wipeLocalEngine() -> Success { + let deferred = Success() + queue.async { + self.logins.removeAll() + self.id = 0 + deferred.fill(Maybe(success: ())) + } + return deferred + } + + override func hasSyncedLogins() -> Deferred> { + let deferred = Deferred>() + queue.async { + deferred.fill(Maybe(success: !self.logins.isEmpty)) + } + return deferred + } +} + class RustLoginsTests: XCTestCase { var files: FileAccessor! var logins: RustLogins! + var mockLogins: MockRustLogins! var encryptionKey: String! + let keychain = MZKeychainWrapper.sharedClientAppContainerKeychain + let canaryPhraseKey = "canaryPhrase" + let loginKeychainKey = "appservices.key.logins.perfield" + override func setUp() { super.setUp() files = MockFiles() @@ -30,11 +83,19 @@ class RustLoginsTests: XCTestCase { logins = RustLogins(sqlCipherDatabasePath: sqlCipherDatabasePath, databasePath: databasePath) _ = logins.reopenIfClosed() + + mockLogins = MockRustLogins(sqlCipherDatabasePath: sqlCipherDatabasePath, databasePath: databasePath) } else { XCTFail("Could not retrieve root directory") } } + override func tearDown() { + super.tearDown() + self.keychain.removeObject(forKey: self.canaryPhraseKey, withAccessibility: .afterFirstUnlock) + self.keychain.removeObject(forKey: self.loginKeychainKey, withAccessibility: .afterFirstUnlock) + } + func addLogin() -> Deferred> { let login = LoginEntry(fromJSONDict: [ "hostname": "https://example.com", @@ -118,4 +179,54 @@ class RustLoginsTests: XCTestCase { XCTAssertTrue(getResult2.isSuccess) XCTAssertNil(getResult2.successValue!) } + + func testGetStoredKeyWithKeychainReset() { + // Here we are checking that, if the database has login records and the logins + // key data has been removed from the keychain, calling logins.getStoredKey will + // remove the logins from the database, recreate logins key data, and return the + // new key. + + let login = LoginEntry(fromJSONDict: [ + "hostname": "https://example.com", + "formSubmitUrl": "https://example.com", + "username": "username", + "password": "password" + ]) + mockLogins.addLogin(login: login).upon { addResult in + XCTAssertTrue(addResult.isSuccess) + XCTAssertNotNil(addResult.successValue) + } + + mockLogins.mockListLogins().upon { listResult in + XCTAssertTrue(listResult.isSuccess) + XCTAssertNotNil(listResult.successValue) + XCTAssertEqual(listResult.successValue!.count, 1) + } + + // Simulate losing the key data while a logins record exists in the database + self.keychain.removeObject(forKey: self.canaryPhraseKey, withAccessibility: .afterFirstUnlock) + self.keychain.removeObject(forKey: self.loginKeychainKey, withAccessibility: .afterFirstUnlock) + XCTAssertNil(self.keychain.string(forKey: self.canaryPhraseKey)) + XCTAssertNil(self.keychain.string(forKey: self.loginKeychainKey)) + + let expectation = expectation(description: "\(#function)\(#line)") + mockLogins.getStoredKey { result in + // Check that we successfully retrieved a key + XCTAssertNotNil(try? result.get()) + + // check that the logins were wiped from the database + self.mockLogins.mockListLogins().upon { listResult2 in + XCTAssertTrue(listResult2.isSuccess) + XCTAssertNotNil(listResult2.successValue) + XCTAssertEqual(listResult2.successValue!.count, 0) + } + + // Check that new key data was created + XCTAssertNotNil(self.keychain.string(forKey: self.canaryPhraseKey)) + XCTAssertNotNil(self.keychain.string(forKey: self.loginKeychainKey)) + + expectation.fulfill() + } + waitForExpectations(timeout: 5) + } } diff --git a/Tests/UnitTest.xctestplan b/Tests/UnitTest.xctestplan index 72d1f8c8e028..102f8d2cdb27 100644 --- a/Tests/UnitTest.xctestplan +++ b/Tests/UnitTest.xctestplan @@ -96,6 +96,7 @@ "skippedTests" : [ "ETPCoverSheetTests", "IntroViewControllerTests\/testBasicSetupReturnsExpectedItems()", + "RustSyncManagerTests\/testGetEnginesAndKeys()", "RustSyncManagerTests\/testGetEnginesAndKeysWithNoKey()", "RustSyncManagerTests\/testUpdateEnginePrefs_bookmarksEnabled()", "TabManagerTests\/testDeleteSelectedTab()", @@ -119,6 +120,7 @@ }, { "skippedTests" : [ + "RustLoginsTests\/testGetStoredKeyWithKeychainReset()", "RustLoginsTests\/testUpdateLogin()", "TestBrowserDB\/testMovesDB()" ],