Support Autofill Domains with Port Number Suffixes (#775)

* Updates SecureVaultModels to return sorted Accounts when the target domain has a port suffix * Updates SecureVaultManager getAccounts method to handle domains with ports, and localhost * Removes SecureVaultManagerDelegate default extension implementation, which had the ability to cause an infinite loop * Add URLComponentsExtenions method to return eTLDPlus1 with a port suffix * Add tests for all of the above
duckduckgo · Apr 15, 2024 · 90e789b · 90e789b
1 parent b0749d2
commit 90e789b
Show file tree

Hide file tree

Showing 6 changed files with 433 additions and 20 deletions.
diff --git a/Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift b/Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift
@@ -77,12 +77,6 @@ public protocol SecureVaultManagerDelegate: AnyObject, SecureVaultErrorReporting
 
 }
 
-extension SecureVaultManagerDelegate {
-    func secureVaultManagerIsEnabledStatus(_ manager: SecureVaultManager, forType type: AutofillType? = nil) -> Bool {
-        return secureVaultManagerIsEnabledStatus(manager, forType: type)
-    }
-}
-
 public protocol PasswordManager: AnyObject {
 
     var isEnabled: Bool { get }
@@ -156,7 +150,7 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
                                                                  SecureVaultModels.CredentialsProvider) -> Void) {
 
         do {
-            guard let delegate = delegate, delegate.secureVaultManagerIsEnabledStatus(self) else {
+            guard let delegate = delegate, delegate.secureVaultManagerIsEnabledStatus(self, forType: nil) else {
                 completionHandler([], [], [], credentialsProvider)
                 return
             }
@@ -752,15 +746,23 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
         } else {
             do {
                 if withPartialMatches {
-                    guard let currentUrlComponents = AutofillDomainNameUrlMatcher().normalizeSchemeForAutofill(domain),
-                          let tld = tld,
-                          let eTLDplus1 = currentUrlComponents.eTLDplus1(tld: tld)
-                    else {
+                    guard var currentUrlComponents = AutofillDomainNameUrlMatcher().normalizeSchemeForAutofill(domain) else {
                         completion([], nil)
                         return
                     }
-                    let accounts = try vault.accountsWithPartialMatchesFor(eTLDplus1: eTLDplus1)
-                    completion(accounts, nil)
+
+                    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)
+                    }
                 } else {
                     let accounts = try vault.accountsFor(domain: domain)
                     completion(accounts, nil)

diff --git a/Sources/BrowserServicesKit/SecureVault/SecureVaultModels.swift b/Sources/BrowserServicesKit/SecureVault/SecureVaultModels.swift
@@ -543,9 +543,9 @@ extension SecureVaultModels.CreditCard: SecureVaultAutofillEquatable {
 // MARK: - WebsiteAccount Array extensions
 extension Array where Element == SecureVaultModels.WebsiteAccount {
 
-    public func sortedForDomain(_ targetDomain: String, tld: TLD, removeDuplicates: Bool = false) -> [SecureVaultModels.WebsiteAccount] {
+    public func sortedForDomain(_ targetDomain: String, tld: TLD, removeDuplicates: Bool = false, urlMatcher: AutofillDomainNameUrlMatcher = AutofillDomainNameUrlMatcher()) -> [SecureVaultModels.WebsiteAccount] {
 
-        guard let targetTLD = extractTLD(domain: targetDomain, tld: tld) else {
+        guard let targetTLD = extractTLD(domain: targetDomain, tld: tld, urlMatcher: urlMatcher) else {
             return []
         }
 
@@ -569,10 +569,11 @@ extension Array where Element == SecureVaultModels.WebsiteAccount {
         return (removeDuplicates ? result.removeDuplicates() : result).filter { $0.domain?.isEmpty == false }
     }
 
-    private func extractTLD(domain: String, tld: TLD) -> String? {
-        var urlComponents = URLComponents()
-        urlComponents.host = domain
-        return urlComponents.eTLDplus1(tld: tld)
+    private func extractTLD(domain: String, tld: TLD, urlMatcher: AutofillDomainNameUrlMatcher) -> String? {
+        guard var urlComponents = urlMatcher.normalizeSchemeForAutofill(domain) else { return nil }
+        guard urlComponents.host != .localhost else { return domain }
+        return urlComponents.eTLDplus1WithPort(tld: tld)
+
     }
 
     // Last Used > Last Updated > Alphabetical Domain > Alphabetical Username > Empty Usernames

diff --git a/Sources/Common/Extensions/URLComponentsExtension.swift b/Sources/Common/Extensions/URLComponentsExtension.swift
@@ -28,4 +28,15 @@ extension URLComponents {
         return tld.extractSubdomain(from: self.host?.lowercased())
     }
 
+    mutating public func eTLDplus1WithPort(tld: TLD) -> String? {
+        guard let port = self.port else {
+            return tld.eTLDplus1(self.host?.lowercased())
+        }
+
+        self.port = nil
+        guard let etldPlus1 = tld.eTLDplus1(self.host?.lowercased()) else { return nil }
+
+        return "\(etldPlus1):\(port)"
+    }
+
 }
diff --git a/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift b/Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift
@@ -16,6 +16,7 @@
 //  limitations under the License.
 //
 
+import Common
 import XCTest
 import UserScript
 import SecureStorage
@@ -195,6 +196,175 @@ class SecureVaultManagerTests: XCTestCase {
         waitForExpectations(timeout: 0.1)
     }
 
+    func testWhenRequestingCredentialsWithDomainAndPort_ThenFillActionIsReturned() throws {
+
+        // Given
+        class SecureVaultDelegate: MockSecureVaultManagerDelegate {
+            override func secureVaultManager(_ manager: SecureVaultManager,
+                                             promptUserToAutofillCredentialsForDomain domain: String,
+                                             withAccounts accounts: [SecureVaultModels.WebsiteAccount],
+                                             withTrigger trigger: AutofillUserScript.GetTriggerType,
+                                             onAccountSelected account: @escaping (SecureVaultModels.WebsiteAccount?) -> Void,
+                                             completionHandler: @escaping (SecureVaultModels.WebsiteAccount?) -> Void) {
+                XCTAssertEqual(accounts.count, 1, "One account should have been returned")
+                completionHandler(accounts[0])
+            }
+        }
+
+        self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: true, tld: TLD())
+        self.secureVaultManagerDelegate = SecureVaultDelegate()
+        self.manager.delegate = self.secureVaultManagerDelegate
+
+        let triggerType = AutofillUserScript.GetTriggerType.userInitiated
+
+        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]
+
+        // credential for the account
+        let credentials = SecureVaultModels.WebsiteCredentials(account: account, password: "password".data(using: .utf8)!)
+        try self.testVault.storeWebsiteCredentials(credentials)
+
+        let subType = AutofillUserScript.GetAutofillDataSubType.username
+        let expect = expectation(description: #function)
+
+        // When
+        manager.autofillUserScript(mockAutofillUserScript, didRequestCredentialsForDomain: domain, subType: subType, trigger: triggerType) { credentials, _, action in
+
+            // Then
+            XCTAssertEqual(action, .fill)
+            XCTAssertEqual(credentials!.password, "password".data(using: .utf8)!)
+            XCTAssertEqual(credentials!.account.username, "dax")
+            expect.fulfill()
+        }
+        waitForExpectations(timeout: 0.1)
+    }
+
+    func testWhenRequestingCredentialsWithLocalhost_ThenFillActionIsReturned() throws {
+
+        // Given
+        class SecureVaultDelegate: MockSecureVaultManagerDelegate {
+            override func secureVaultManager(_ manager: SecureVaultManager,
+                                             promptUserToAutofillCredentialsForDomain domain: String,
+                                             withAccounts accounts: [SecureVaultModels.WebsiteAccount],
+                                             withTrigger trigger: AutofillUserScript.GetTriggerType,
+                                             onAccountSelected account: @escaping (SecureVaultModels.WebsiteAccount?) -> Void,
+                                             completionHandler: @escaping (SecureVaultModels.WebsiteAccount?) -> Void) {
+                XCTAssertEqual(accounts.count, 1, "One account should have been returned")
+                completionHandler(accounts[0])
+            }
+        }
+
+        self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: true, tld: TLD())
+        self.secureVaultManagerDelegate = SecureVaultDelegate()
+        self.manager.delegate = self.secureVaultManagerDelegate
+
+        let triggerType = AutofillUserScript.GetTriggerType.userInitiated
+
+        let domain = "\(String.localhost):1234"
+        let username = "dax"
+        let account = SecureVaultModels.WebsiteAccount(id: "1", title: nil, username: username, domain: domain, created: Date(), lastUpdated: Date())
+        self.mockDatabaseProvider._accounts = [account]
+
+        // credential for the account
+        let credentials = SecureVaultModels.WebsiteCredentials(account: account, password: "password".data(using: .utf8)!)
+        try self.testVault.storeWebsiteCredentials(credentials)
+
+        let subType = AutofillUserScript.GetAutofillDataSubType.username
+        let expect = expectation(description: #function)
+
+        // When
+        manager.autofillUserScript(mockAutofillUserScript, didRequestCredentialsForDomain: domain, subType: subType, trigger: triggerType) { credentials, _, action in
+
+            // Then
+            XCTAssertEqual(action, .fill)
+            XCTAssertEqual(credentials!.password, "password".data(using: .utf8)!)
+            XCTAssertEqual(credentials!.account.username, "dax")
+            expect.fulfill()
+        }
+        waitForExpectations(timeout: 0.1)
+    }
+
+    func testWhenRequestingAutofillInitDataWithDomainAndPort_ThenDataIsReturned() throws {
+
+        // Given
+        class SecureVaultDelegate: MockSecureVaultManagerDelegate {
+            override func secureVaultManager(_ manager: SecureVaultManager,
+                                             promptUserToAutofillCredentialsForDomain domain: String,
+                                             withAccounts accounts: [SecureVaultModels.WebsiteAccount],
+                                             withTrigger trigger: AutofillUserScript.GetTriggerType,
+                                             onAccountSelected account: @escaping (SecureVaultModels.WebsiteAccount?) -> Void,
+                                             completionHandler: @escaping (SecureVaultModels.WebsiteAccount?) -> Void) {
+                XCTAssertEqual(accounts.count, 1, "One account should have been returned")
+                completionHandler(accounts[0])
+            }
+        }
+
+        self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: true, tld: TLD())
+        self.secureVaultManagerDelegate = SecureVaultDelegate()
+        self.manager.delegate = self.secureVaultManagerDelegate
+
+        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 credentials = SecureVaultModels.WebsiteCredentials(account: account, password: "password".data(using: .utf8)!)
+        try self.testVault.storeWebsiteCredentials(credentials)
+
+        let expect = expectation(description: #function)
+
+        // When
+        manager.autofillUserScript(mockAutofillUserScript, didRequestAutoFillInitDataForDomain: domain) { accounts, _, _, _ in
+
+            // Then
+            XCTAssertTrue(accounts.count == 1)
+            XCTAssertEqual(accounts.first!, account)
+            expect.fulfill()
+        }
+        waitForExpectations(timeout: 0.1)
+    }
+
+    func testWhenRequestingAccountsWithDomainAndPort_ThenDataIsReturned() throws {
+
+        // Given
+        class SecureVaultDelegate: MockSecureVaultManagerDelegate {
+            override func secureVaultManager(_ manager: SecureVaultManager,
+                                             promptUserToAutofillCredentialsForDomain domain: String,
+                                             withAccounts accounts: [SecureVaultModels.WebsiteAccount],
+                                             withTrigger trigger: AutofillUserScript.GetTriggerType,
+                                             onAccountSelected account: @escaping (SecureVaultModels.WebsiteAccount?) -> Void,
+                                             completionHandler: @escaping (SecureVaultModels.WebsiteAccount?) -> Void) {
+                XCTAssertEqual(accounts.count, 1, "One account should have been returned")
+                completionHandler(accounts[0])
+            }
+        }
+
+        self.manager = SecureVaultManager(vault: self.testVault, includePartialAccountMatches: true, tld: TLD())
+        self.secureVaultManagerDelegate = SecureVaultDelegate()
+        self.manager.delegate = self.secureVaultManagerDelegate
+
+        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 credentials = SecureVaultModels.WebsiteCredentials(account: account, password: "password".data(using: .utf8)!)
+        try self.testVault.storeWebsiteCredentials(credentials)
+
+        let expect = expectation(description: #function)
+
+        // When
+        manager.autofillUserScript(mockAutofillUserScript, didRequestAccountsForDomain: domain) { accounts, _ in
+            // Then
+            XCTAssertTrue(accounts.count == 1)
+            XCTAssertEqual(accounts.first!, account)
+            expect.fulfill()
+        }
+        waitForExpectations(timeout: 0.1)
+    }
+
     func testWhenRequestingCredentialsWithPasswordSubtype_ThenCredentialsAreNotFiltered() throws {
         class SecureVaultDelegate: MockSecureVaultManagerDelegate {
             override func secureVaultManager(_ manager: SecureVaultManager,
@@ -723,7 +893,7 @@ private class MockSecureVaultManagerDelegate: SecureVaultManagerDelegate {
 
     private(set) var promptedAutofillData: AutofillData?
 
-    func secureVaultManagerIsEnabledStatus(_: SecureVaultManager) -> Bool {
+    func secureVaultManagerIsEnabledStatus(_ manager: SecureVaultManager, forType type: AutofillType?) -> Bool {
         return true
     }