diff --git a/Providers/RustSyncManager.swift b/Providers/RustSyncManager.swift index 5d8f9497b442..1ea76842b55d 100644 --- a/Providers/RustSyncManager.swift +++ b/Providers/RustSyncManager.swift @@ -328,83 +328,50 @@ public class RustSyncManager: NSObject, SyncManager { public let description = "Failed to get sync engine and key data." } - func shouldSyncLogins(completion: @escaping (Bool) -> Void) { - if !(self.prefs.boolForKey(PrefsKeys.HasVerifiedRustLogins) ?? false) { - // We should only sync logins when the verify logins step hasn't been successfully - // completed. Otherwise logins could exist in the database that can't be decrypted - // and would prevent logins from syncing if they're not removed. - - self.profile?.logins.verifyLogins { successfullyVerified in - self.prefs.setBool(successfullyVerified, forKey: PrefsKeys.HasVerifiedRustLogins) - completion(successfullyVerified) - } - } else { - // Successful logins verification already occurred so login syncing can proceed - completion(true) - } - } - func getEnginesAndKeys(engines: [RustSyncManagerAPI.TogglableEngine], completion: @escaping (([String], [String: String])) -> Void) { - var localEncryptionKeys: [String: String] = [:] - var rustEngines: [String] = [] - var registeredPlaces = false - - 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) - case .passwords: - self.shouldSyncLogins { shouldSync in - if shouldSync, let key = loginKey { - self.profile?.logins.registerWithSyncManager() - localEncryptionKeys[engine.rawValue] = key - rustEngines.append(engine.rawValue) - } - } - 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) - } - } - } - - 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)) - } - } + var localEncryptionKeys: [String: String] = [:] + 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 + 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() { + 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 .bookmarks, .history: + if !registeredPlaces { + profile?.places.registerWithSyncManager() + registeredPlaces = true + } + rustEngines.append(engine.rawValue) + } + } + + completion((rustEngines, localEncryptionKeys)) + } private func doSync(params: SyncParams, completion: @escaping (SyncResult) -> Void) { beginSyncing() diff --git a/Shared/Prefs.swift b/Shared/Prefs.swift index 7bfd1dfacede..71d0639d5307 100644 --- a/Shared/Prefs.swift +++ b/Shared/Prefs.swift @@ -12,7 +12,6 @@ public struct PrefsKeys { // Global sync state for rust sync manager public static let RustSyncManagerPersistedState = "rustSyncManagerPersistedStateKey" - public static let HasVerifiedRustLogins = "hasVerifiedRustLogins" public static let KeyLastSyncFinishTime = "lastSyncFinishTime" public static let KeyDefaultHomePageURL = "KeyDefaultHomePageURL" diff --git a/Storage/Rust/RustLogins.swift b/Storage/Rust/RustLogins.swift index fef131f31f8d..695f52b447ca 100644 --- a/Storage/Rust/RustLogins.swift +++ b/Storage/Rust/RustLogins.swift @@ -355,9 +355,8 @@ public extension LoginEntry { } public enum LoginEncryptionKeyError: Error { - case noKeyCreated case illegalState - case dbRecordCountVerificationError(String) + case noKeyCreated } public class RustLoginEncryptionKeys { @@ -547,72 +546,6 @@ public class RustLogins { } } - private func deleteInvalidLogins(key: String, - logins: [EncryptedLogin], - completion: @escaping (Bool) -> Void) { - // Create a list of IDs from saved logins that can't be decrypted - let loginsToDelete = logins - .filter { (try? decryptLogin(login: $0, encryptionKey: key)) == nil } - .map { $0.record.id } - - // Delete all the logins that can't be decrypted - self.deleteLogins(ids: loginsToDelete).upon { deleteResults in - var verified = true - for result in deleteResults { - if case let .failure(err) = result { - let errMsg = "Login could not be deleted during verification" - self.logger.log(errMsg, - level: .warning, - category: .storage, - description: err.localizedDescription) - verified = false - } - } - completion(verified) - } - } - - public func verifyLogins(completion: @escaping (Bool) -> Void) { - queue.async { - self.listLogins().upon { loginResult in - switch loginResult { - case let .failure(error): - self.logger.log("Logins could not be retrieved for verification", - level: .warning, - category: .storage, - description: error.localizedDescription) - completion(false) - return - case let .success(logins): - guard !logins.isEmpty else { - // If there are no logins we don't need to go through this verification - // process in the future so we return true. - completion(true) - return - } - - let rustKeys = RustLoginEncryptionKeys() - guard let key = rustKeys.keychain.string(forKey: rustKeys.loginPerFieldKeychainKey) else { - // If the key is missing during the verification process, we wipe the database and - // recreate the key. - self.resetLoginsAndKey(rustKeys: rustKeys) { resetResult in - if case let .failure(error) = resetResult { - self.logger.log("Logins and key could not be reset during verification", - level: .warning, - category: .storage, - description: error.localizedDescription) - } - return - } - completion(false) - return - } - self.deleteInvalidLogins(key: key, logins: logins, completion: completion) - } - } - } - } - private func close() -> NSError? { storage = nil isOpen = false @@ -758,20 +691,15 @@ public class RustLogins { return } - 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)) - } + 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)) } } + return deferred } @@ -806,18 +734,12 @@ public class RustLogins { return } - 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)) - } + 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)) } } @@ -893,106 +815,75 @@ public class RustLogins { keychain.removeObject(forKey: rustKeys.loginsSaltKeychainKey, withAccessibility: .afterFirstUnlock) } - 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) { + public func getStoredKey() throws -> String { 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 { - completion(.success(key!)) + return key! } else { logger.log("Logins key was corrupted, new one generated", level: .warning, category: .storage) GleanMetrics.LoginsStoreKeyRegeneration.corrupt.record() - self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion) + _ = self.wipeLocalEngine() + + return try rustKeys.createAndStoreKey() } } catch let error as NSError { - logger.log("Error validating logins encryption key", + logger.log("Error retrieving 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() - 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) + return try rustKeys.createAndStoreKey() + } catch let error as NSError { + throw error + } 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() - logger.log("Logins key lost, new one generated", - level: .warning, - category: .storage) - GleanMetrics.LoginsStoreKeyRegeneration.lost.record() - self.resetLoginsAndKey(rustKeys: rustKeys, completion: completion) + return try rustKeys.createAndStoreKey() + } catch let error as NSError { + throw error + } case (.none, .none): - // 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)) - } - } + // 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 } default: // If none of the above cases apply, we're in a state that shouldn't be possible but is disallowed nonetheless - completion(.failure(LoginEncryptionKeyError.illegalState as NSError)) + throw LoginEncryptionKeyError.illegalState } + + // This must be declared again for Swift's sake even though the above switch statement handles all cases + throw LoginEncryptionKeyError.illegalState } } diff --git a/firefox-ios/Tests/ClientTests/PasswordManagerViewModelTests.swift b/firefox-ios/Tests/ClientTests/PasswordManagerViewModelTests.swift index a27468f38ee0..921c77c456dd 100644 --- a/firefox-ios/Tests/ClientTests/PasswordManagerViewModelTests.swift +++ b/firefox-ios/Tests/ClientTests/PasswordManagerViewModelTests.swift @@ -44,19 +44,14 @@ class PasswordManagerViewModelTests: XCTestCase { "username": "username\(i)", "password": "password\(i)" ]) - 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 addResult = self.viewModel.profile.logins.addLogin(login: login) + XCTAssertTrue(addResult.value.isSuccess) + XCTAssertNotNil(addResult.value.successValue) } let logins = self.viewModel.profile.logins.listLogins().value XCTAssertTrue(logins.isSuccess) XCTAssertNotNil(logins.successValue) - - waitForExpectations(timeout: 10, handler: nil) } func testQueryLogins() { diff --git a/firefox-ios/Tests/ClientTests/RustSyncManagerTests.swift b/firefox-ios/Tests/ClientTests/RustSyncManagerTests.swift index 461715d02369..b1af94f57bfc 100644 --- a/firefox-ios/Tests/ClientTests/RustSyncManagerTests.swift +++ b/firefox-ios/Tests/ClientTests/RustSyncManagerTests.swift @@ -54,7 +54,6 @@ 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/firefox-ios/Tests/StorageTests/RustLoginsTests.swift b/firefox-ios/Tests/StorageTests/RustLoginsTests.swift index 5099bcb60bef..265e7d36687d 100644 --- a/firefox-ios/Tests/StorageTests/RustLoginsTests.swift +++ b/firefox-ios/Tests/StorageTests/RustLoginsTests.swift @@ -6,85 +6,10 @@ 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 MockListLoginsFailure: RustLogins { - override func listLogins() -> Deferred> { - let deferred = Deferred>() - queue.async { - let error = LoginsStoreError.UnexpectedLoginsApiError(reason: "Database is closed") - deferred.fill(Maybe(failure: error as MaybeErrorType)) - } - return deferred - } -} - -class MockListLoginsEmpty: RustLogins { - override func listLogins() -> Deferred> { - let deferred = Deferred>() - queue.async { - deferred.fill(Maybe(success: [EncryptedLogin]())) - } - return deferred - } -} - class RustLoginsTests: XCTestCase { var files: FileAccessor! var logins: RustLogins! - var mockLogins: MockRustLogins! - var mockListLoginsFailure: MockListLoginsFailure! - var mockListLoginsEmpty: MockListLoginsEmpty! - - let keychain = MZKeychainWrapper.sharedClientAppContainerKeychain - let canaryPhraseKey = "canaryPhrase" - let loginKeychainKey = "appservices.key.logins.perfield" + var encryptionKey: String! override func setUp() { super.setUp() @@ -97,55 +22,14 @@ class RustLoginsTests: XCTestCase { let databasePath = URL(fileURLWithPath: rootDirectory, isDirectory: true).appendingPathComponent("testLoginsPerField.db").path try? files.remove("testLoginsPerField.db") + if let key = try? createKey() { + encryptionKey = key + } else { + XCTFail("Encryption key wasn't created") + } + logins = RustLogins(sqlCipherDatabasePath: sqlCipherDatabasePath, databasePath: databasePath) _ = logins.reopenIfClosed() - - mockLogins = MockRustLogins(sqlCipherDatabasePath: sqlCipherDatabasePath, databasePath: databasePath) - - self.keychain.removeObject(forKey: self.canaryPhraseKey, withAccessibility: .afterFirstUnlock) - self.keychain.removeObject(forKey: self.loginKeychainKey, withAccessibility: .afterFirstUnlock) - } 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 setUpMockListLoginsFailure() { - files = MockFiles() - - if let rootDirectory = try? files.getAndEnsureDirectory() { - let sqlCipherDatabasePath = URL(fileURLWithPath: rootDirectory, isDirectory: true).appendingPathComponent("testlogins.db").path - try? files.remove("testlogins.db") - - let databasePath = URL(fileURLWithPath: rootDirectory, isDirectory: true).appendingPathComponent("testLoginsPerField.db").path - try? files.remove("testLoginsPerField.db") - - mockListLoginsFailure = MockListLoginsFailure(sqlCipherDatabasePath: sqlCipherDatabasePath, - databasePath: databasePath) - _ = mockListLoginsFailure.reopenIfClosed() - } else { - XCTFail("Could not retrieve root directory") - } - } - - func setUpMockListLoginsEmpty() { - files = MockFiles() - - if let rootDirectory = try? files.getAndEnsureDirectory() { - let sqlCipherDatabasePath = URL(fileURLWithPath: rootDirectory, isDirectory: true).appendingPathComponent("testlogins.db").path - try? files.remove("testlogins.db") - - let databasePath = URL(fileURLWithPath: rootDirectory, isDirectory: true).appendingPathComponent("testLoginsPerField.db").path - try? files.remove("testLoginsPerField.db") - - mockListLoginsEmpty = MockListLoginsEmpty(sqlCipherDatabasePath: sqlCipherDatabasePath, - databasePath: databasePath) - _ = mockListLoginsEmpty.reopenIfClosed() } else { XCTFail("Could not retrieve root directory") } @@ -234,81 +118,4 @@ 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) - } - - func testVerifyWithFailedList() { - // Mocking a failed call to listLogins within verifyLogins - setUpMockListLoginsFailure() - - let exp = expectation(description: "\(#function)\(#line)") - mockListLoginsFailure.verifyLogins { result in - exp.fulfill() - - // Check that verification failed - XCTAssertFalse(result) - } - waitForExpectations(timeout: 5) - } - - func testVerifyWithNoLogins() { - setUpMockListLoginsEmpty() - - let exp = expectation(description: "\(#function)\(#line)") - mockListLoginsEmpty.verifyLogins { result in - exp.fulfill() - - // Check that verification succeeds when there are no saved logins - XCTAssertTrue(result) - } - waitForExpectations(timeout: 5) - } } diff --git a/firefox-ios/Tests/UnitTest.xctestplan b/firefox-ios/Tests/UnitTest.xctestplan index 102f8d2cdb27..72d1f8c8e028 100644 --- a/firefox-ios/Tests/UnitTest.xctestplan +++ b/firefox-ios/Tests/UnitTest.xctestplan @@ -96,7 +96,6 @@ "skippedTests" : [ "ETPCoverSheetTests", "IntroViewControllerTests\/testBasicSetupReturnsExpectedItems()", - "RustSyncManagerTests\/testGetEnginesAndKeys()", "RustSyncManagerTests\/testGetEnginesAndKeysWithNoKey()", "RustSyncManagerTests\/testUpdateEnginePrefs_bookmarksEnabled()", "TabManagerTests\/testDeleteSelectedTab()", @@ -120,7 +119,6 @@ }, { "skippedTests" : [ - "RustLoginsTests\/testGetStoredKeyWithKeychainReset()", "RustLoginsTests\/testUpdateLogin()", "TestBrowserDB\/testMovesDB()" ],