diff --git a/Providers/RustSyncManager.swift b/Providers/RustSyncManager.swift index 07a2aa470cb0..5d8f9497b442 100644 --- a/Providers/RustSyncManager.swift +++ b/Providers/RustSyncManager.swift @@ -328,6 +328,22 @@ 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] = [:] @@ -341,10 +357,12 @@ public class RustSyncManager: NSObject, SyncManager { self.profile?.tabs.registerWithSyncManager() rustEngines.append(engine.rawValue) case .passwords: - self.profile?.logins.registerWithSyncManager() - if let key = loginKey { - localEncryptionKeys[engine.rawValue] = key - rustEngines.append(engine.rawValue) + 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 { diff --git a/Shared/Prefs.swift b/Shared/Prefs.swift index 71d0639d5307..7bfd1dfacede 100644 --- a/Shared/Prefs.swift +++ b/Shared/Prefs.swift @@ -12,6 +12,7 @@ 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 de1294f2a917..fef131f31f8d 100644 --- a/Storage/Rust/RustLogins.swift +++ b/Storage/Rust/RustLogins.swift @@ -547,6 +547,72 @@ 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 diff --git a/firefox-ios/Tests/StorageTests/RustLoginsTests.swift b/firefox-ios/Tests/StorageTests/RustLoginsTests.swift index 21f1c2bfbdad..5099bcb60bef 100644 --- a/firefox-ios/Tests/StorageTests/RustLoginsTests.swift +++ b/firefox-ios/Tests/StorageTests/RustLoginsTests.swift @@ -54,11 +54,33 @@ class MockRustLogins: RustLogins { } } +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 encryptionKey: String! + var mockListLoginsFailure: MockListLoginsFailure! + var mockListLoginsEmpty: MockListLoginsEmpty! let keychain = MZKeychainWrapper.sharedClientAppContainerKeychain let canaryPhraseKey = "canaryPhrase" @@ -75,16 +97,13 @@ 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") } @@ -96,6 +115,42 @@ class RustLoginsTests: XCTestCase { 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") + } + } + func addLogin() -> Deferred> { let login = LoginEntry(fromJSONDict: [ "hostname": "https://example.com", @@ -229,4 +284,31 @@ class RustLoginsTests: XCTestCase { } 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) + } }