From e787b78e5761e8bc39e4a93945b69fbadf9ecc8b Mon Sep 17 00:00:00 2001 From: Graeme Arthur <2030310+graeme@users.noreply.github.com> Date: Thu, 11 Jul 2024 17:00:20 +0200 Subject: [PATCH 01/10] Add functions to fetch multiple WebsiteCredentials --- .../AutofillDatabaseProvider.swift | 29 ++++++++++++++++ .../SecureVault/AutofillSecureVault.swift | 34 +++++++++++++++---- 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift b/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift index c5890bc62..322dd96e4 100644 --- a/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift +++ b/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift @@ -29,6 +29,7 @@ public protocol AutofillDatabaseProvider: SecureStorageDatabaseProvider { @discardableResult func storeWebsiteCredentials(_ credentials: SecureVaultModels.WebsiteCredentials) throws -> Int64 + func websiteCredentialsForDomain(_ domain: String) throws -> [SecureVaultModels.WebsiteCredentials] func websiteCredentialsForAccountId(_ accountId: Int64) throws -> SecureVaultModels.WebsiteCredentials? func websiteAccountsForDomain(_ domain: String) throws -> [SecureVaultModels.WebsiteAccount] func websiteAccountsForTopLevelDomain(_ eTLDplus1: String) throws -> [SecureVaultModels.WebsiteAccount] @@ -386,6 +387,34 @@ public final class DefaultAutofillDatabaseProvider: GRDBSecureStorageDatabasePro return nil } + public func websiteCredentialsForDomain(_ domain: String) throws -> [SecureVaultModels.WebsiteCredentials] { + try db.read { + try websiteCredentialsForDomain(domain, in: $0) + } + } + + func websiteCredentialsForDomain(_ domain: String, in database: Database) throws -> [SecureVaultModels.WebsiteCredentials] { + let accounts = try SecureVaultModels.WebsiteAccount + .filter(SecureVaultModels.WebsiteAccount.Columns.domain.like(domain)) + .fetchAll(database) + + let accountIDs = accounts.compactMap(\.id) + + var result = [SecureVaultModels.WebsiteCredentials]() + + for accountID in accountIDs { + guard let accountIDInt = Int64(accountID) else { + continue + } + guard let credentials = try websiteCredentialsForAccountId(accountIDInt, in: database) else { + continue + } + result.append(credentials) + } + + return result + } + public func websiteCredentialsForAccountId(_ accountId: Int64) throws -> SecureVaultModels.WebsiteCredentials? { try db.read { try websiteCredentialsForAccountId(accountId, in: $0) diff --git a/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift b/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift index 348aabf83..116e8ad0c 100644 --- a/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift +++ b/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift @@ -61,6 +61,7 @@ public protocol AutofillSecureVault: SecureVault { func hasAccountFor(username: String?, domain: String?) throws -> Bool func updateLastUsedFor(accountId: Int64) throws + func websiteCredentialsFor(domain: String) throws -> [SecureVaultModels.WebsiteCredentials] func websiteCredentialsFor(accountId: Int64) throws -> SecureVaultModels.WebsiteCredentials? @discardableResult func storeWebsiteCredentials(_ credentials: SecureVaultModels.WebsiteCredentials) throws -> Int64 @@ -309,6 +310,31 @@ public class DefaultAutofillSecureVault: AutofillSe // MARK: - Credentials + public func websiteCredentialsFor(domain: String) throws -> [SecureVaultModels.WebsiteCredentials] { + lock.lock() + defer { + lock.unlock() + } + + do { + let credentials = try self.providers.database.websiteCredentialsForDomain(domain) + return try credentials.map(decryptCredentials(_:)) + } catch { + let error = error as? SecureStorageError ?? SecureStorageError.databaseError(cause: error) + throw error + } + } + + private func decryptCredentials(_ credentials: SecureVaultModels.WebsiteCredentials) throws -> SecureVaultModels.WebsiteCredentials { + if let password = credentials.password { + let decryptedPassword = try self.l2Decrypt(data: password) + return .init(account: credentials.account, + password: decryptedPassword) + } else { + return credentials + } + } + public func websiteCredentialsFor(accountId: Int64) throws -> SecureVaultModels.WebsiteCredentials? { lock.lock() defer { @@ -318,14 +344,8 @@ public class DefaultAutofillSecureVault: AutofillSe do { var decryptedCredentials: SecureVaultModels.WebsiteCredentials? if let credentials = try self.providers.database.websiteCredentialsForAccountId(accountId) { - if let password = credentials.password { - decryptedCredentials = .init(account: credentials.account, - password: try self.l2Decrypt(data: password)) - } else { - decryptedCredentials = credentials - } + decryptedCredentials = try decryptCredentials(credentials) } - return decryptedCredentials } catch { let error = error as? SecureStorageError ?? SecureStorageError.databaseError(cause: error) From d7e2be12c74a0dea407970b6f29890300195ba83 Mon Sep 17 00:00:00 2001 From: Graeme Arthur <2030310+graeme@users.noreply.github.com> Date: Thu, 11 Jul 2024 17:01:06 +0200 Subject: [PATCH 02/10] UserScripts use credentials instead of accounts --- .../AutofillUserScript+SecureVault.swift | 47 +++++++++++++------ .../SecureVault/SecureVaultManager.swift | 26 +++++++--- 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/Sources/BrowserServicesKit/Autofill/AutofillUserScript+SecureVault.swift b/Sources/BrowserServicesKit/Autofill/AutofillUserScript+SecureVault.swift index ca5170f5c..8fa8ab389 100644 --- a/Sources/BrowserServicesKit/Autofill/AutofillUserScript+SecureVault.swift +++ b/Sources/BrowserServicesKit/Autofill/AutofillUserScript+SecureVault.swift @@ -31,7 +31,7 @@ public protocol AutofillSecureVaultDelegate: AnyObject { var tld: TLD? { get } func autofillUserScript(_: AutofillUserScript, didRequestAutoFillInitDataForDomain domain: String, completionHandler: @escaping ( - [SecureVaultModels.WebsiteAccount], + [SecureVaultModels.WebsiteCredentials], [SecureVaultModels.Identity], [SecureVaultModels.CreditCard], SecureVaultModels.CredentialsProvider @@ -434,8 +434,8 @@ extension AutofillUserScript { func getAvailableInputTypes(_ message: UserScriptMessage, _ replyHandler: @escaping MessageReplyHandler) { let domain = hostForMessage(message) let email = emailDelegate?.autofillUserScriptDidRequestSignedInStatus(self) ?? false - vaultDelegate?.autofillUserScript(self, didRequestAutoFillInitDataForDomain: domain) { accounts, identities, cards, credentialsProvider in - let response = RequestAvailableInputTypesResponse(accounts: accounts, + vaultDelegate?.autofillUserScript(self, didRequestAutoFillInitDataForDomain: domain) { credentials, identities, cards, credentialsProvider in + let response = RequestAvailableInputTypesResponse(credentials: credentials, identities: identities, cards: cards, email: email, @@ -515,28 +515,31 @@ extension AutofillUserScript { func pmGetAutoFillInitData(_ message: UserScriptMessage, _ replyHandler: @escaping MessageReplyHandler) { let domain = hostForMessage(message) - vaultDelegate?.autofillUserScript(self, didRequestAutoFillInitDataForDomain: domain) { accounts, identities, cards, credentialsProvider in - let credentials: [CredentialObject] + vaultDelegate?.autofillUserScript(self, didRequestAutoFillInitDataForDomain: domain) { credentials, identities, cards, credentialsProvider in + let credentialObjects: [CredentialObject] if credentialsProvider.locked { - credentials = [CredentialObject(id: "provider_locked", username: "", credentialsProvider: credentialsProvider.name.rawValue)] + credentialObjects = [CredentialObject(id: "provider_locked", username: "", credentialsProvider: credentialsProvider.name.rawValue)] } else { guard let autofillWebsiteAccountMatcher = self.vaultDelegate?.autofillWebsiteAccountMatcher else { - credentials = accounts.compactMap { - guard let id = $0.id else { return nil } - return CredentialObject(id: id, username: $0.username ?? "", credentialsProvider: credentialsProvider.name.rawValue) + credentialObjects = credentials.compactMap { + if let id = $0.account.id { + return CredentialObject(id: id, username: $0.account.username ?? "", credentialsProvider: credentialsProvider.name.rawValue) + } else { + return nil + } } return } - let accountMatches = autofillWebsiteAccountMatcher.findMatches(accounts: accounts, for: domain) - credentials = self.buildCredentialObjects(accountMatches, credentialsProvider: credentialsProvider) + let accountMatches = autofillWebsiteAccountMatcher.findMatches(accounts: credentials.map(\.account), for: domain) + credentialObjects = self.buildCredentialObjects(accountMatches, credentialsProvider: credentialsProvider) } let identities: [IdentityObject] = identities.compactMap(IdentityObject.from(identity:)) let cards: [CreditCardObject] = cards.compactMap(CreditCardObject.autofillInitializationValueFrom(card:)) let success = RequestAutoFillInitDataResponse.AutofillInitSuccess(serializedInputContext: self.serializedInputContext, - credentials: credentials, + credentials: credentialObjects, creditCards: cards, identities: identities) @@ -810,8 +813,8 @@ extension AutofillUserScript.RequestAvailableInputTypesResponse { cards: [SecureVaultModels.CreditCard], email: Bool, credentialsProvider: SecureVaultModels.CredentialsProvider) { - let username = credentialsProvider.locked || credentials.filter({ $0.account.username?.isEmpty == false }).count > 0 - let password = credentialsProvider.locked || credentials.count > 0 + let username = credentialsProvider.locked || credentials.hasAtLeastOneUsername + let password = credentialsProvider.locked || credentials.hasAtLeastOnePassword let credentials = AutofillUserScript.AvailableInputTypesSuccess.AvailableInputTypesCredentials(username: username, password: password) let success = AutofillUserScript.AvailableInputTypesSuccess( credentials: credentials, @@ -825,6 +828,22 @@ extension AutofillUserScript.RequestAvailableInputTypesResponse { } +private extension Array where Element == SecureVaultModels.WebsiteCredentials { + var hasAtLeastOneUsername: Bool { + let elementsWithUsername = filter { + $0.account.username?.isEmpty == false + } + return !elementsWithUsername.isEmpty + } + + var hasAtLeastOnePassword: Bool { + let elementsWithPassword = filter { + $0.password?.isEmpty == false + } + return !elementsWithPassword.isEmpty + } +} + extension AutofillUserScript.AvailableInputTypesSuccess.AvailableInputTypesIdentities { init(identities: [SecureVaultModels.Identity]) { diff --git a/Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift b/Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift index 3a51925f3..f3a461390 100644 --- a/Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift +++ b/Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift @@ -144,7 +144,7 @@ extension SecureVaultManager: AutofillSecureVaultDelegate { public func autofillUserScript(_: AutofillUserScript, didRequestAutoFillInitDataForDomain domain: String, - completionHandler: @escaping ([SecureVaultModels.WebsiteAccount], + completionHandler: @escaping ([SecureVaultModels.WebsiteCredentials], [SecureVaultModels.Identity], [SecureVaultModels.CreditCard], SecureVaultModels.CredentialsProvider) -> Void) { @@ -167,16 +167,13 @@ extension SecureVaultManager: AutofillSecureVaultDelegate { } if delegate.secureVaultManagerIsEnabledStatus(self, forType: .password) { - getAccounts(for: domain, - from: vault, - or: passwordManager, - withPartialMatches: includePartialAccountMatches) { [weak self] accounts, error in + getCredentials(forDomain: domain, from: vault, or: passwordManager) { [weak self] credentials, error in guard let self = self else { return } if let error = error { os_log(.error, "Error requesting autofill init data: %{public}@", error.localizedDescription) completionHandler([], [], [], self.credentialsProvider) } else { - completionHandler(accounts, identities, cards, self.credentialsProvider) + completionHandler(credentials, identities, cards, self.credentialsProvider) } } } else { @@ -790,6 +787,23 @@ extension SecureVaultManager: AutofillSecureVaultDelegate { } } + private func getCredentials(forDomain domain: String, + from vault: any AutofillSecureVault, + or passwordManager: PasswordManager?, + completion: @escaping ([SecureVaultModels.WebsiteCredentials], Error?) -> Void) { + if let passwordManager = passwordManager, + passwordManager.isEnabled { + passwordManager.websiteCredentialsFor(domain: domain, completion: completion) + } else { + do { + let credentials = try vault.websiteCredentialsFor(domain: domain) + completion(credentials, nil) + } catch { + completion([], error) + } + } + } + private func existingCredentialsInPasswordManager(with autofillData: AutofillUserScript.DetectedAutofillData, domain: String, vault: any AutofillSecureVault) -> SecureVaultModels.WebsiteCredentials? { From f80eb8c973749b0445dbd44ae24245b023b403d2 Mon Sep 17 00:00:00 2001 From: Graeme Arthur <2030310+graeme@users.noreply.github.com> Date: Thu, 11 Jul 2024 17:24:00 +0200 Subject: [PATCH 03/10] Make tests compile (not pass) --- .../Autofill/AutofillVaultUserScriptTests.swift | 4 ++-- .../SecureVault/MockAutofillDatabaseProvider.swift | 5 +++++ .../SecureVault/SecureVaultManagerTests.swift | 11 ++++++----- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/Tests/BrowserServicesKitTests/Autofill/AutofillVaultUserScriptTests.swift b/Tests/BrowserServicesKitTests/Autofill/AutofillVaultUserScriptTests.swift index fb76212b6..09c5c0d6f 100644 --- a/Tests/BrowserServicesKitTests/Autofill/AutofillVaultUserScriptTests.swift +++ b/Tests/BrowserServicesKitTests/Autofill/AutofillVaultUserScriptTests.swift @@ -562,7 +562,6 @@ class AutofillVaultUserScriptTests: XCTestCase { } class MockSecureVaultDelegate: AutofillSecureVaultDelegate { - enum CallbackType { case didRequestCreditCardsManagerForDomain case didRequestIdentitiesManagerForDomain @@ -623,7 +622,8 @@ class MockSecureVaultDelegate: AutofillSecureVaultDelegate { func autofillUserScript(_: BrowserServicesKit.AutofillUserScript, didRequestAutoFillInitDataForDomain domain: String, - completionHandler: @escaping ([BrowserServicesKit.SecureVaultModels.WebsiteAccount], [BrowserServicesKit.SecureVaultModels.Identity], [BrowserServicesKit.SecureVaultModels.CreditCard], BrowserServicesKit.SecureVaultModels.CredentialsProvider) -> Void) { + completionHandler: @escaping ([BrowserServicesKit.SecureVaultModels.WebsiteCredentials], [BrowserServicesKit.SecureVaultModels.Identity], [BrowserServicesKit.SecureVaultModels.CreditCard], BrowserServicesKit.SecureVaultModels.CredentialsProvider) -> Void) { + } func autofillUserScript(_: AutofillUserScript, diff --git a/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift b/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift index e45bb6a4f..efdfa958c 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift @@ -35,6 +35,7 @@ internal class MockAutofillDatabaseProvider: AutofillDatabaseProvider { var _creditCards = [Int64: SecureVaultModels.CreditCard]() var _forDomain = [String]() var _credentialsDict = [Int64: SecureVaultModels.WebsiteCredentials]() + var _credentialsForDomainDict = [String: [SecureVaultModels.WebsiteCredentials]]() var _note: SecureVaultModels.Note? var db: DatabaseWriter @@ -66,6 +67,10 @@ internal class MockAutofillDatabaseProvider: AutofillDatabaseProvider { return _credentialsDict[accountId] } + func websiteCredentialsForDomain(_ domain: String) throws -> [BrowserServicesKit.SecureVaultModels.WebsiteCredentials] { + return _credentialsForDomainDict[domain] ?? [] + } + func websiteAccountsForDomain(_ domain: String) throws -> [SecureVaultModels.WebsiteAccount] { self._forDomain.append(domain) return _accounts diff --git a/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift b/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift index 6273a75f6..5c44bf8c1 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift @@ -310,17 +310,18 @@ class SecureVaultManagerTests: XCTestCase { let account = SecureVaultModels.WebsiteAccount(id: "1", title: nil, username: username, domain: domain, created: Date(), lastUpdated: Date()) self.mockDatabaseProvider._accounts = [account] - let credentials = SecureVaultModels.WebsiteCredentials(account: account, password: "password".data(using: .utf8)!) - try self.testVault.storeWebsiteCredentials(credentials) + let storedCredentials = SecureVaultModels.WebsiteCredentials(account: account, password: "password".data(using: .utf8)!) + try self.testVault.storeWebsiteCredentials(storedCredentials) let expect = expectation(description: #function) // When - manager.autofillUserScript(mockAutofillUserScript, didRequestAutoFillInitDataForDomain: domain) { accounts, _, _, _ in + manager.autofillUserScript(mockAutofillUserScript, didRequestAutoFillInitDataForDomain: domain) { credentials, _, _, _ in // Then - XCTAssertTrue(accounts.count == 1) - XCTAssertEqual(accounts.first!, account) + XCTAssertTrue(credentials.count == 1) + XCTAssertEqual(credentials.first?.account.id, storedCredentials.account.id) + XCTAssertEqual(credentials.first?.password, storedCredentials.password) expect.fulfill() } waitForExpectations(timeout: 0.1) From 33f02f735d68fb8d37cb72b0d98fee43e96f1b18 Mon Sep 17 00:00:00 2001 From: Graeme Arthur <2030310+graeme@users.noreply.github.com> Date: Thu, 11 Jul 2024 17:24:20 +0200 Subject: [PATCH 04/10] Scheme updates? --- .../xcshareddata/xcschemes/BrowserServicesKit-Package.xcscheme | 2 +- .../xcode/xcshareddata/xcschemes/SubscriptionTests.xcscheme | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.swiftpm/xcode/xcshareddata/xcschemes/BrowserServicesKit-Package.xcscheme b/.swiftpm/xcode/xcshareddata/xcschemes/BrowserServicesKit-Package.xcscheme index 5d23935cc..784a12c37 100644 --- a/.swiftpm/xcode/xcshareddata/xcschemes/BrowserServicesKit-Package.xcscheme +++ b/.swiftpm/xcode/xcshareddata/xcschemes/BrowserServicesKit-Package.xcscheme @@ -1,7 +1,7 @@ + version = "1.8"> + version = "1.8"> Date: Mon, 15 Jul 2024 15:24:26 +0200 Subject: [PATCH 05/10] Update test for changes --- .../SecureVault/SecureVaultManagerTests.swift | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift b/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift index 5c44bf8c1..0279c26f0 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift @@ -308,10 +308,8 @@ class SecureVaultManagerTests: XCTestCase { let domain = "domain.com:1234" let username = "dax" let account = SecureVaultModels.WebsiteAccount(id: "1", title: nil, username: username, domain: domain, created: Date(), lastUpdated: Date()) - self.mockDatabaseProvider._accounts = [account] - let storedCredentials = SecureVaultModels.WebsiteCredentials(account: account, password: "password".data(using: .utf8)!) - try self.testVault.storeWebsiteCredentials(storedCredentials) + self.mockDatabaseProvider._credentialsForDomainDict[domain] = [storedCredentials] let expect = expectation(description: #function) @@ -319,7 +317,7 @@ class SecureVaultManagerTests: XCTestCase { manager.autofillUserScript(mockAutofillUserScript, didRequestAutoFillInitDataForDomain: domain) { credentials, _, _, _ in // Then - XCTAssertTrue(credentials.count == 1) + XCTAssertEqual(credentials.count, 1) XCTAssertEqual(credentials.first?.account.id, storedCredentials.account.id) XCTAssertEqual(credentials.first?.password, storedCredentials.password) expect.fulfill() From 164d487b026d108eb211d3a1a815f5684d842f6b Mon Sep 17 00:00:00 2001 From: Graeme Arthur <2030310+graeme@users.noreply.github.com> Date: Tue, 16 Jul 2024 15:23:16 +0200 Subject: [PATCH 06/10] Include partial domains when fetching credentials --- .../AutofillDatabaseProvider.swift | 22 ++++++++- .../SecureVault/AutofillSecureVault.swift | 27 +++++++--- .../SecureVault/SecureVaultManager.swift | 49 ++++++++++++------- .../MockAutofillDatabaseProvider.swift | 4 ++ 4 files changed, 77 insertions(+), 25 deletions(-) diff --git a/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift b/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift index 322dd96e4..b145523ea 100644 --- a/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift +++ b/Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift @@ -30,6 +30,7 @@ public protocol AutofillDatabaseProvider: SecureStorageDatabaseProvider { @discardableResult func storeWebsiteCredentials(_ credentials: SecureVaultModels.WebsiteCredentials) throws -> Int64 func websiteCredentialsForDomain(_ domain: String) throws -> [SecureVaultModels.WebsiteCredentials] + func websiteCredentialsForTopLevelDomain(_ domain: String) throws -> [SecureVaultModels.WebsiteCredentials] func websiteCredentialsForAccountId(_ accountId: Int64) throws -> SecureVaultModels.WebsiteCredentials? func websiteAccountsForDomain(_ domain: String) throws -> [SecureVaultModels.WebsiteAccount] func websiteAccountsForTopLevelDomain(_ eTLDplus1: String) throws -> [SecureVaultModels.WebsiteAccount] @@ -393,11 +394,15 @@ public final class DefaultAutofillDatabaseProvider: GRDBSecureStorageDatabasePro } } - func websiteCredentialsForDomain(_ domain: String, in database: Database) throws -> [SecureVaultModels.WebsiteCredentials] { + private func websiteCredentialsForDomain(_ domain: String, in database: Database) throws -> [SecureVaultModels.WebsiteCredentials] { let accounts = try SecureVaultModels.WebsiteAccount .filter(SecureVaultModels.WebsiteAccount.Columns.domain.like(domain)) .fetchAll(database) + return try websiteCredentialsForAccounts(accounts, in: database) + } + + private func websiteCredentialsForAccounts(_ accounts: [SecureVaultModels.WebsiteAccount], in database: Database) throws -> [SecureVaultModels.WebsiteCredentials] { let accountIDs = accounts.compactMap(\.id) var result = [SecureVaultModels.WebsiteCredentials]() @@ -415,6 +420,21 @@ public final class DefaultAutofillDatabaseProvider: GRDBSecureStorageDatabasePro return result } + public func websiteCredentialsForTopLevelDomain(_ eTLDplus1: String) throws -> [SecureVaultModels.WebsiteCredentials] { + try db.read { + return try websiteCredentialsForTopLevelDomain(eTLDplus1, in: $0) + } + } + + private func websiteCredentialsForTopLevelDomain(_ eTLDplus1: String, in database: Database) throws -> [SecureVaultModels.WebsiteCredentials] { + let query = SecureVaultModels.WebsiteAccount + .filter(Column(SecureVaultModels.WebsiteAccount.Columns.domain.name) == eTLDplus1 || + Column(SecureVaultModels.WebsiteAccount.Columns.domain.name).like("%." + eTLDplus1)) + let accounts = try query.fetchAll(database) + + return try websiteCredentialsForAccounts(accounts, in: database) + } + public func websiteCredentialsForAccountId(_ accountId: Int64) throws -> SecureVaultModels.WebsiteCredentials? { try db.read { try websiteCredentialsForAccountId(accountId, in: $0) diff --git a/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift b/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift index 116e8ad0c..1c8024967 100644 --- a/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift +++ b/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift @@ -62,6 +62,7 @@ public protocol AutofillSecureVault: SecureVault { func updateLastUsedFor(accountId: Int64) throws func websiteCredentialsFor(domain: String) throws -> [SecureVaultModels.WebsiteCredentials] + func websiteCredentialsWithPartialMatchesFor(eTLDplus1: String) throws -> [SecureVaultModels.WebsiteCredentials] func websiteCredentialsFor(accountId: Int64) throws -> SecureVaultModels.WebsiteCredentials? @discardableResult func storeWebsiteCredentials(_ credentials: SecureVaultModels.WebsiteCredentials) throws -> Int64 @@ -325,13 +326,15 @@ public class DefaultAutofillSecureVault: AutofillSe } } - private func decryptCredentials(_ credentials: SecureVaultModels.WebsiteCredentials) throws -> SecureVaultModels.WebsiteCredentials { - if let password = credentials.password { - let decryptedPassword = try self.l2Decrypt(data: password) - return .init(account: credentials.account, - password: decryptedPassword) - } else { - return credentials + public func websiteCredentialsWithPartialMatchesFor(eTLDplus1: String) throws -> [SecureVaultModels.WebsiteCredentials] { + lock.lock() + defer { + lock.unlock() + } + do { + return try self.providers.database.websiteCredentialsForTopLevelDomain(eTLDplus1) + } catch { + throw SecureStorageError.databaseError(cause: error) } } @@ -652,6 +655,16 @@ public class DefaultAutofillSecureVault: AutofillSe // MARK: - Private + private func decryptCredentials(_ credentials: SecureVaultModels.WebsiteCredentials) throws -> SecureVaultModels.WebsiteCredentials { + if let password = credentials.password { + let decryptedPassword = try self.l2Decrypt(data: password) + return .init(account: credentials.account, + password: decryptedPassword) + } else { + return credentials + } + } + private func executeThrowingDatabaseOperation(_ operation: () throws -> DatabaseResult) throws -> DatabaseResult { lock.lock() defer { diff --git a/Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift b/Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift index f3a461390..95953f2ea 100644 --- a/Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift +++ b/Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift @@ -167,7 +167,7 @@ extension SecureVaultManager: AutofillSecureVaultDelegate { } if delegate.secureVaultManagerIsEnabledStatus(self, forType: .password) { - getCredentials(forDomain: domain, from: vault, or: passwordManager) { [weak self] credentials, error in + getCredentials(forDomain: domain, from: vault, or: passwordManager, withPartialMatches: includePartialAccountMatches) { [weak self] credentials, error in guard let self = self else { return } if let error = error { os_log(.error, "Error requesting autofill init data: %{public}@", error.localizedDescription) @@ -743,23 +743,12 @@ extension SecureVaultManager: AutofillSecureVaultDelegate { } else { do { if withPartialMatches { - guard var currentUrlComponents = AutofillDomainNameUrlMatcher().normalizeSchemeForAutofill(domain) else { + guard let eTLDplus1 = eTLDplus1(for: domain) else { completion([], nil) return } - - if currentUrlComponents.host == .localhost { - let accounts = try vault.accountsWithPartialMatchesFor(eTLDplus1: domain) - completion(accounts, nil) - } else { - guard let tld = tld, let eTLDplus1 = currentUrlComponents.eTLDplus1WithPort(tld: tld) else { - completion([], nil) - return - } - - let accounts = try vault.accountsWithPartialMatchesFor(eTLDplus1: eTLDplus1) - completion(accounts, nil) - } + let accounts = try vault.accountsWithPartialMatchesFor(eTLDplus1: eTLDplus1) + completion(accounts, nil) } else { let accounts = try vault.accountsFor(domain: domain) completion(accounts, nil) @@ -790,20 +779,46 @@ extension SecureVaultManager: AutofillSecureVaultDelegate { private func getCredentials(forDomain domain: String, from vault: any AutofillSecureVault, or passwordManager: PasswordManager?, + withPartialMatches: Bool, completion: @escaping ([SecureVaultModels.WebsiteCredentials], Error?) -> Void) { if let passwordManager = passwordManager, passwordManager.isEnabled { passwordManager.websiteCredentialsFor(domain: domain, completion: completion) } else { do { - let credentials = try vault.websiteCredentialsFor(domain: domain) - completion(credentials, nil) + if withPartialMatches { + guard let eTLDplus1 = eTLDplus1(for: domain) else { + completion([], nil) + return + } + let accounts = try vault.websiteCredentialsWithPartialMatchesFor(eTLDplus1: eTLDplus1) + completion(accounts, nil) + } else { + let credentials = try vault.websiteCredentialsFor(domain: domain) + completion(credentials, nil) + } } catch { completion([], error) } } } + private func eTLDplus1(for domain: String) -> String? { + guard var currentUrlComponents = AutofillDomainNameUrlMatcher().normalizeSchemeForAutofill(domain) else { + return nil + } + + if currentUrlComponents.host == .localhost { + return domain + } else { + guard let tld = tld, let eTLDplus1 = currentUrlComponents.eTLDplus1WithPort(tld: tld) else { + return nil + } + + return eTLDplus1 + } + } + private func existingCredentialsInPasswordManager(with autofillData: AutofillUserScript.DetectedAutofillData, domain: String, vault: any AutofillSecureVault) -> SecureVaultModels.WebsiteCredentials? { diff --git a/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift b/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift index efdfa958c..d116f3f94 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/MockAutofillDatabaseProvider.swift @@ -71,6 +71,10 @@ internal class MockAutofillDatabaseProvider: AutofillDatabaseProvider { return _credentialsForDomainDict[domain] ?? [] } + func websiteCredentialsForTopLevelDomain(_ eTLDplus1: String) throws -> [BrowserServicesKit.SecureVaultModels.WebsiteCredentials] { + return _credentialsForDomainDict[eTLDplus1] ?? [] + } + func websiteAccountsForDomain(_ domain: String) throws -> [SecureVaultModels.WebsiteAccount] { self._forDomain.append(domain) return _accounts From 9c42c2b0ba1fe86e81378cfcdbd6505ab68cb685 Mon Sep 17 00:00:00 2001 From: Graeme Arthur <2030310+graeme@users.noreply.github.com> Date: Wed, 17 Jul 2024 11:29:52 +0200 Subject: [PATCH 07/10] Add some AutofillUserScript unit tests --- .../SecureVault/AutofillUserScriptTests.swift | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/Tests/BrowserServicesKitTests/SecureVault/AutofillUserScriptTests.swift b/Tests/BrowserServicesKitTests/SecureVault/AutofillUserScriptTests.swift index 1e326e878..a29c6a6ae 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/AutofillUserScriptTests.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/AutofillUserScriptTests.swift @@ -61,4 +61,49 @@ class AutofillUserScriptTests: XCTestCase { XCTAssertEqual(responseFromAccounts.success.credentials.username, true) } + func testWhenPasswordsAreNil_ThenAvailableInputTypesPasswordIsFalse() { + let credentialsList = createListOfCredentials(withPassword: nil) + let credentialsProvider = SecureVaultModels.CredentialsProvider(name: SecureVaultModels.CredentialsProvider.Name.duckduckgo, locked: false) + let responseFromCredentials = AutofillUserScript.RequestAvailableInputTypesResponse(credentials: credentialsList, + identities: [], + cards: [], + email: false, + credentialsProvider: credentialsProvider) + XCTAssertEqual(responseFromCredentials.success.credentials.password, false) + } + + func testWhenAllPasswordsAreEmpty_ThenAvailableInputTypesPasswordIsFalse() { + let credentialsList = createListOfCredentials(withPassword: "".data(using: .utf8)!) + let credentialsProvider = SecureVaultModels.CredentialsProvider(name: SecureVaultModels.CredentialsProvider.Name.duckduckgo, locked: false) + let responseFromCredentials = AutofillUserScript.RequestAvailableInputTypesResponse(credentials: credentialsList, + identities: [], + cards: [], + email: false, + credentialsProvider: credentialsProvider) + XCTAssertEqual(responseFromCredentials.success.credentials.password, false) + } + + func testWhenAtLeastOnePasswordIsNonNilOrEmpty_ThenAvailableInputTypesPasswordIsTrue() { + var credentialsList = createListOfCredentials(withPassword: nil) + let account = credentialsList.first?.account + let credentialsWithPassword = SecureVaultModels.WebsiteCredentials(account: account!, password: "password".data(using: .utf8)!) + credentialsList.append(credentialsWithPassword) + let credentialsProvider = SecureVaultModels.CredentialsProvider(name: SecureVaultModels.CredentialsProvider.Name.duckduckgo, locked: false) + let responseFromCredentials = AutofillUserScript.RequestAvailableInputTypesResponse(credentials: credentialsList, + identities: [], + cards: [], + email: false, + credentialsProvider: credentialsProvider) + XCTAssertEqual(responseFromCredentials.success.credentials.password, true) + } + + private func createListOfCredentials(withPassword password: Data?) -> [SecureVaultModels.WebsiteCredentials] { + var credentialsList = [SecureVaultModels.WebsiteCredentials]() + for i in 0...10 { + let account = SecureVaultModels.WebsiteAccount(id: "id\(i)", username: "username\(i)", domain: "domain.com", created: Date(), lastUpdated: Date()) + let credentials = SecureVaultModels.WebsiteCredentials(account: account, password: password) + credentialsList.append(credentials) + } + return credentialsList + } } From 963cc0f158c0a4fd9bd34264a858494a1b3c1020 Mon Sep 17 00:00:00 2001 From: Graeme Arthur <2030310+graeme@users.noreply.github.com> Date: Mon, 22 Jul 2024 12:13:42 +0200 Subject: [PATCH 08/10] Make sure partial matches logic tested --- .../SecureVault/SecureVaultManagerTests.swift | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift b/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift index 0279c26f0..9558d670d 100644 --- a/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift +++ b/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift @@ -286,9 +286,17 @@ class SecureVaultManagerTests: XCTestCase { waitForExpectations(timeout: 0.1) } - func testWhenRequestingAutofillInitDataWithDomainAndPort_ThenDataIsReturned() throws { + func testWhenRequestingAutofillInitDataWithDomainAndPort_withPartialMatches_ThenDataIsReturned() throws { + self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: true, tld: TLD()) + try assertWhenRequestingAutofillInitDataWithDomainAndPort_ThenDataIsReturned() + } - // Given + func testWhenRequestingAutofillInitDataWithDomainAndPort_withoutPartialMatches_ThenDataIsReturned() throws { + self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: false, tld: TLD()) + try assertWhenRequestingAutofillInitDataWithDomainAndPort_ThenDataIsReturned() + } + + func assertWhenRequestingAutofillInitDataWithDomainAndPort_ThenDataIsReturned(file: StaticString = #file, line: UInt = #line) throws { class SecureVaultDelegate: MockSecureVaultManagerDelegate { override func secureVaultManager(_ manager: SecureVaultManager, promptUserToAutofillCredentialsForDomain domain: String, @@ -301,7 +309,6 @@ class SecureVaultManagerTests: XCTestCase { } } - self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: true, tld: TLD()) self.secureVaultManagerDelegate = SecureVaultDelegate() self.manager.delegate = self.secureVaultManagerDelegate @@ -317,9 +324,9 @@ class SecureVaultManagerTests: XCTestCase { manager.autofillUserScript(mockAutofillUserScript, didRequestAutoFillInitDataForDomain: domain) { credentials, _, _, _ in // Then - XCTAssertEqual(credentials.count, 1) - XCTAssertEqual(credentials.first?.account.id, storedCredentials.account.id) - XCTAssertEqual(credentials.first?.password, storedCredentials.password) + XCTAssertEqual(credentials.count, 1, file: file, line: line) + XCTAssertEqual(credentials.first?.account.id, storedCredentials.account.id, file: file, line: line) + XCTAssertEqual(credentials.first?.password, storedCredentials.password, file: file, line: line) expect.fulfill() } waitForExpectations(timeout: 0.1) From a088eb3ca863f9ed8a6f67c5cbe9664a628a0506 Mon Sep 17 00:00:00 2001 From: Graeme Arthur <2030310+graeme@users.noreply.github.com> Date: Mon, 22 Jul 2024 12:54:03 +0200 Subject: [PATCH 09/10] Decrypt partial matched credentials --- .../BrowserServicesKit/SecureVault/AutofillSecureVault.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift b/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift index 1c8024967..79ffab561 100644 --- a/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift +++ b/Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift @@ -332,7 +332,8 @@ public class DefaultAutofillSecureVault: AutofillSe lock.unlock() } do { - return try self.providers.database.websiteCredentialsForTopLevelDomain(eTLDplus1) + let credentials = try self.providers.database.websiteCredentialsForTopLevelDomain(eTLDplus1) + return try credentials.map(decryptCredentials(_:)) } catch { throw SecureStorageError.databaseError(cause: error) } From 74e99cc96a401c731ee95c9d3630c6a07757c1af Mon Sep 17 00:00:00 2001 From: Graeme Arthur <2030310+graeme@users.noreply.github.com> Date: Tue, 23 Jul 2024 17:06:57 +0200 Subject: [PATCH 10/10] Revert accidental scheme changes --- .../xcshareddata/xcschemes/BrowserServicesKit-Package.xcscheme | 2 +- .../xcode/xcshareddata/xcschemes/SubscriptionTests.xcscheme | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.swiftpm/xcode/xcshareddata/xcschemes/BrowserServicesKit-Package.xcscheme b/.swiftpm/xcode/xcshareddata/xcschemes/BrowserServicesKit-Package.xcscheme index a4a34b64d..c6bb2cd4a 100644 --- a/.swiftpm/xcode/xcshareddata/xcschemes/BrowserServicesKit-Package.xcscheme +++ b/.swiftpm/xcode/xcshareddata/xcschemes/BrowserServicesKit-Package.xcscheme @@ -1,7 +1,7 @@ + version = "1.7"> + version = "1.7">