Skip to content

Commit

Permalink
Use E-TLD+1 when matching domain for login saves (#1222)
Browse files Browse the repository at this point in the history 
<!--
Note: This checklist is a reminder of our shared engineering
expectations.
-->

Please review the release process for BrowserServicesKit
[here](https://app.asana.com/0/1200194497630846/1200837094583426).

**Required**:

Task/Issue URL: https://app.asana.com/0/72649045549333/1209272524872735
iOS PR: duckduckgo/iOS#3943
macOS PR: duckduckgo/macos-browser#3844
What kind of version bump will this require?: Major

**Description**:

Right now, we will offer to autofill into a page which is considered an
E-TLD+1 match.
But we aren’t as good at comparing E-TLD+1 when it comes to considering
whether to offer to save it or not.
We should detect we have a domain saved already and don’t offer to save
when entering credentials for an equivalent subdomain
This should also apply to other scenarios like offering to update the
saved password; we should also consider E-TLD+1 matches

**Steps to test this PR**:
https://app.asana.com/0/1203822806345703/1209279526650235

<!--
Before submitting a PR, please ensure you have tested the combinations
you expect the reviewer to test, then delete configurations you *know*
do not need explicit testing.

Using a simulator where a physical device is unavailable is acceptable.
-->

**OS Testing**:

* [ ] iOS 14
* [ ] iOS 15
* [ ] iOS 16
* [ ] macOS 10.15
* [ ] macOS 11
* [ ] macOS 12

---
###### Internal references:
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
  • Loading branch information
@graeme
graeme authored Feb 12, 2025
1 parent ea5957b commit d9305ba
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 47 deletions.
62 changes: 26 additions & 36 deletions Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,9 +104,6 @@ public class SecureVaultManager {
// Third party password manager
private let passwordManager: PasswordManager?

// This property can be removed once all platforms will search for partial account matches as the default expected behaviour.
private let includePartialAccountMatches: Bool

private let shouldAllowPartialFormSaves: Bool

public let tld: TLD?
Expand Down Expand Up @@ -134,12 +131,10 @@ public class SecureVaultManager {

public init(vault: (any AutofillSecureVault)? = nil,
passwordManager: PasswordManager? = nil,
includePartialAccountMatches: Bool = false,
shouldAllowPartialFormSaves: Bool = false,
tld: TLD? = nil) {
self.vault = vault
self.passwordManager = passwordManager
self.includePartialAccountMatches = includePartialAccountMatches
self.shouldAllowPartialFormSaves = shouldAllowPartialFormSaves
self.tld = tld
if let tld {
Expand Down Expand Up @@ -181,7 +176,7 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {

if delegate.secureVaultManagerIsEnabledStatus(self, forType: .password) {
let accountsCount = try vault.accountsCount()
getCredentials(forDomain: domain, from: vault, or: passwordManager, withPartialMatches: includePartialAccountMatches) { [weak self] credentials, error in
getCredentials(forDomain: domain, from: vault, or: passwordManager) { [weak self] credentials, error in
guard let self = self else { return }
if let error = error {
Logger.secureVault.error("Error requesting autofill init data: \(error.localizedDescription, privacy: .public)")
Expand Down Expand Up @@ -352,8 +347,7 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
do {
let vault = try self.vault ?? AutofillSecureVaultFactory.makeVault(reporter: self.delegate)
getAccounts(for: domain, from: vault,
or: passwordManager,
withPartialMatches: includePartialAccountMatches) { [weak self] accounts, error in
or: passwordManager) { [weak self] accounts, error in
guard let self = self else { return }
if let error = error {
Logger.secureVault.error("Error requesting accounts: \(error.localizedDescription, privacy: .public)")
Expand Down Expand Up @@ -381,8 +375,7 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {

getAccounts(for: domain,
from: vault,
or: passwordManager,
withPartialMatches: includePartialAccountMatches) { [weak self] accounts, error in
or: passwordManager) { [weak self] accounts, error in
guard let self = self else { return }
if let error = error {
Logger.secureVault.error("Error requesting accounts: \(error.localizedDescription, privacy: .public)")
Expand Down Expand Up @@ -590,7 +583,12 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
let pass: String = credentials.password ?? ""

let vault = try self.vault ?? AutofillSecureVaultFactory.makeVault(reporter: self.delegate)
let accounts = try vault.accountsFor(domain: domain)
let accounts: [SecureVaultModels.WebsiteAccount]
if let eTLDplus1 = eTLDplus1(for: domain) {
accounts = try vault.accountsWithPartialMatchesFor(eTLDplus1: eTLDplus1)
} else {
accounts = try vault.accountsFor(domain: domain)
}
var currentAccount: SecureVaultModels.WebsiteAccount

// Grab an existing autosave account (if any)
Expand Down Expand Up @@ -707,10 +705,14 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
return nil
}

guard let accounts = try? vault.accountsFor(domain: domain),
// Matching account (username) or account with empty username for domain
var account = accounts.first(where: { $0.username == credentials.username || $0.username.isNilOrEmpty }) else {
let accounts: [SecureVaultModels.WebsiteAccount]
if let eTLDplus1 = eTLDplus1(for: domain) {
accounts = try vault.accountsWithPartialMatchesFor(eTLDplus1: eTLDplus1)
} else {
accounts = try vault.accountsFor(domain: domain)
}

guard var account = accounts.first(where: { $0.username == credentials.username || $0.username.isNilOrEmpty }) else {
// No existing credentials found. This is a new entry
let account = SecureVaultModels.WebsiteAccount(username: credentials.username ?? "", domain: domain)
return SecureVaultModels.WebsiteCredentials(account: account, password: passwordData)
Expand Down Expand Up @@ -770,24 +772,18 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
private func getAccounts(for domain: String,
from vault: any AutofillSecureVault,
or passwordManager: PasswordManager?,
withPartialMatches: Bool = false,
completion: @escaping ([SecureVaultModels.WebsiteAccount], Error?) -> Void) {
if let passwordManager = passwordManager,
passwordManager.isEnabled {
passwordManager.accountsFor(domain: domain, completion: completion)
} else {
do {
if withPartialMatches {
guard let eTLDplus1 = eTLDplus1(for: domain) else {
completion([], nil)
return
}
let accounts = try vault.accountsWithPartialMatchesFor(eTLDplus1: eTLDplus1)
completion(accounts, nil)
} else {
let accounts = try vault.accountsFor(domain: domain)
completion(accounts, nil)
guard let eTLDplus1 = eTLDplus1(for: domain) else {
completion([], nil)
return
}
let accounts = try vault.accountsWithPartialMatchesFor(eTLDplus1: eTLDplus1)
completion(accounts, nil)
} catch {
completion([], error)
}
Expand All @@ -814,24 +810,18 @@ 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)
guard let eTLDplus1 = eTLDplus1(for: domain) else {
completion([], nil)
return
}
let accounts = try vault.websiteCredentialsWithPartialMatchesFor(eTLDplus1: eTLDplus1)
completion(accounts, nil)
} catch {
completion([], error)
}
Expand Down
21 changes: 10 additions & 11 deletions Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ class SecureVaultManagerTests: XCTestCase {
private var testVault: (any AutofillSecureVault)!
private var secureVaultManagerDelegate: MockSecureVaultManagerDelegate!
private var manager: SecureVaultManager!
static let tld = TLD()
var tld: TLD {
Self.tld
}

override func setUp() {
super.setUp()
Expand All @@ -66,7 +70,7 @@ class SecureVaultManagerTests: XCTestCase {

self.testVault = DefaultAutofillSecureVault(providers: providers)
self.secureVaultManagerDelegate = MockSecureVaultManagerDelegate()
self.manager = SecureVaultManager(vault: self.testVault, shouldAllowPartialFormSaves: true)
self.manager = SecureVaultManager(vault: self.testVault, shouldAllowPartialFormSaves: true, tld: tld)
self.manager.delegate = secureVaultManagerDelegate
}

Expand Down Expand Up @@ -211,7 +215,7 @@ class SecureVaultManagerTests: XCTestCase {
}
}

self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: true, tld: TLD())
self.manager = SecureVaultManager(vault: self.testVault, tld: tld)
self.secureVaultManagerDelegate = SecureVaultDelegate()
self.manager.delegate = self.secureVaultManagerDelegate

Expand Down Expand Up @@ -256,7 +260,7 @@ class SecureVaultManagerTests: XCTestCase {
}
}

self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: true, tld: TLD())
self.manager = SecureVaultManager(vault: self.testVault, tld: tld)
self.secureVaultManagerDelegate = SecureVaultDelegate()
self.manager.delegate = self.secureVaultManagerDelegate

Expand Down Expand Up @@ -286,13 +290,8 @@ class SecureVaultManagerTests: XCTestCase {
waitForExpectations(timeout: 0.1)
}

func testWhenRequestingAutofillInitDataWithDomainAndPort_withPartialMatches_ThenDataIsReturned() throws {
self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: true, tld: TLD())
try assertWhenRequestingAutofillInitDataWithDomainAndPort_ThenDataIsReturned()
}

func testWhenRequestingAutofillInitDataWithDomainAndPort_withoutPartialMatches_ThenDataIsReturned() throws {
self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: false, tld: TLD())
func testWhenRequestingAutofillInitDataWithDomainAndPort_ThenDataIsReturned() throws {
self.manager = SecureVaultManager(vault: self.testVault, tld: tld)
try assertWhenRequestingAutofillInitDataWithDomainAndPort_ThenDataIsReturned()
}

Expand Down Expand Up @@ -347,7 +346,7 @@ class SecureVaultManagerTests: XCTestCase {
}
}

self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: true, tld: TLD())
self.manager = SecureVaultManager(vault: self.testVault, tld: tld)
self.secureVaultManagerDelegate = SecureVaultDelegate()
self.manager.delegate = self.secureVaultManagerDelegate

Expand Down

0 comments on commit d9305ba

Please sign in to comment.