Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't show key icon for empty passwords #890

Merged
merged 12 commits into from
Jul 23, 2024
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>
<Scheme
LastUpgradeVersion = "1540"
version = "1.7">
version = "1.8">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might worth checking with the team before upgrading this (and SubscriptionTests.xcscheme below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I'll revert before merging as this isn't really the place.

<BuildAction
parallelizeBuildables = "YES"
buildImplicitDependencies = "YES"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="UTF-8"?>
<Scheme
LastUpgradeVersion = "1540"
version = "1.7">
version = "1.8">
<BuildAction
parallelizeBuildables = "YES"
buildImplicitDependencies = "YES"
Expand Down
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
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 {
amddg44 marked this conversation as resolved.
Show resolved Hide resolved
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
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
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
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
Loading