Skip to content

Commit

Permalink
Don't show key icon for empty passwords (#890)
Browse files Browse the repository at this point in the history 
* Add functions to fetch multiple WebsiteCredentials

* UserScripts use credentials instead of accounts

* Make tests compile (not pass)

* Scheme updates?

* Update test for changes

* Include partial domains when fetching credentials

* Add some AutofillUserScript unit tests

* Make sure partial matches logic tested

* Decrypt partial matched credentials

* Revert accidental scheme changes
  • Loading branch information
@graeme
graeme authored Jul 23, 2024
1 parent c77520f commit b2f5a71
Show file tree
Hide file tree
Showing 8 changed files with 244 additions and 53 deletions.
47 changes: 33 additions & 14 deletions Sources/BrowserServicesKit/Autofill/AutofillUserScript+SecureVault.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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,
Expand All @@ -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]) {
Expand Down
49 changes: 49 additions & 0 deletions Sources/BrowserServicesKit/SecureVault/AutofillDatabaseProvider.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ 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]
Expand Down Expand Up @@ -386,6 +388,53 @@ public final class DefaultAutofillDatabaseProvider: GRDBSecureStorageDatabasePro
return nil
}

public func websiteCredentialsForDomain(_ domain: String) throws -> [SecureVaultModels.WebsiteCredentials] {
try db.read {
try websiteCredentialsForDomain(domain, in: $0)
}
}

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]()

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 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)
Expand Down
48 changes: 41 additions & 7 deletions Sources/BrowserServicesKit/SecureVault/AutofillSecureVault.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ 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 websiteCredentialsWithPartialMatchesFor(eTLDplus1: String) throws -> [SecureVaultModels.WebsiteCredentials]
func websiteCredentialsFor(accountId: Int64) throws -> SecureVaultModels.WebsiteCredentials?
@discardableResult
func storeWebsiteCredentials(_ credentials: SecureVaultModels.WebsiteCredentials) throws -> Int64
Expand Down Expand Up @@ -309,6 +311,34 @@ public class DefaultAutofillSecureVault<T: AutofillDatabaseProvider>: 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
}
}

public func websiteCredentialsWithPartialMatchesFor(eTLDplus1: String) throws -> [SecureVaultModels.WebsiteCredentials] {
lock.lock()
defer {
lock.unlock()
}
do {
let credentials = try self.providers.database.websiteCredentialsForTopLevelDomain(eTLDplus1)
return try credentials.map(decryptCredentials(_:))
} catch {
throw SecureStorageError.databaseError(cause: error)
}
}

public func websiteCredentialsFor(accountId: Int64) throws -> SecureVaultModels.WebsiteCredentials? {
lock.lock()
defer {
Expand All @@ -318,14 +348,8 @@ public class DefaultAutofillSecureVault<T: AutofillDatabaseProvider>: 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)
Expand Down Expand Up @@ -632,6 +656,16 @@ public class DefaultAutofillSecureVault<T: AutofillDatabaseProvider>: 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<DatabaseResult>(_ operation: () throws -> DatabaseResult) throws -> DatabaseResult {
lock.lock()
defer {
Expand Down
69 changes: 49 additions & 20 deletions Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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, 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)
completionHandler([], [], [], self.credentialsProvider)
} else {
completionHandler(accounts, identities, cards, self.credentialsProvider)
completionHandler(credentials, identities, cards, self.credentialsProvider)
}
}
} else {
Expand Down Expand Up @@ -746,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)
Expand Down Expand Up @@ -790,6 +776,49 @@ 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 {
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? {
Expand Down
4 changes: 2 additions & 2 deletions Tests/BrowserServicesKitTests/Autofill/AutofillVaultUserScriptTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,6 @@ class AutofillVaultUserScriptTests: XCTestCase {
}

class MockSecureVaultDelegate: AutofillSecureVaultDelegate {

enum CallbackType {
case didRequestCreditCardsManagerForDomain
case didRequestIdentitiesManagerForDomain
Expand Down Expand Up @@ -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,
Expand Down
Loading

0 comments on commit b2f5a71

Please sign in to comment.