Skip to content
This repository was archived by the owner on Feb 24, 2025. It is now read-only.

Commit 5b274fa

Browse files
graemegraeme
authored
Passwords: Update save pixel with backfilled value (#1209)
<!-- 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/1208273769335188/1209241071149720/f iOS PR: duckduckgo/iOS#3903 macOS PR: duckduckgo/macos-browser#3807 What kind of version bump will this require?: Major **Optional**: Tech Design URL: CC: **Description**: We're appending the backfilled parameter to the _update_ pixels, but not the _save_ pixels. So, for example, the case we're fixing for deviantart is not being captured if you're coming in fresh. Meaning, if you don't have credentials saved, we do backfill, but the pixel doesn't have backfilled=true, so we can't count it. I think at some point we started discussing only about the _update_ pixel and that was misinterpreted as don't change anything for _save_, but that was not the intention. The goal is to update the pixel firing logic both on macOS and iOS to match the above requirements **Steps to test this PR**: On both platforms... 1. Go to https://www.deviantart.com/users/login 2. Type in a username and hit Next 3. Type in a password and hit submit 4. You will see the Save password prompt with username and password 5. Check that you see `backfilled=true` in the save pixel on presentation, confirmation and dismissal 6. Save the password 7. Repeat steps 1 & 2 8. Type in a new password 9. You will see the Update password prompt with username and password 10. Check that you see `backfilled=true` in the update pixel on presentation, confirmation and dismissal <!-- 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)
1 parent 82d0d83 commit 5b274fa

File tree

2 files changed

+45
-19
lines changed

2 files changed

+45
-19
lines changed

‎Sources/BrowserServicesKit/SecureVault/SecureVaultManager.swift‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ public struct AutofillData {
3333
public let credentials: SecureVaultModels.WebsiteCredentials?
3434
public let creditCard: SecureVaultModels.CreditCard?
3535
public var automaticallySavedCredentials: Bool
36+
public var backfilled: Bool
3637
}
3738

3839
public protocol SecureVaultManagerDelegate: AnyObject, SecureVaultReporting {
@@ -223,6 +224,7 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
223224
}
224225

225226
var autofilldata = data
227+
var backfilled = false
226228

227229
if shouldAllowPartialFormSaves,
228230
let partialAccountUsername = partialFormSaveManager.partialAccount(forDomain: domain)?.username,
@@ -234,6 +236,7 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
234236
password: credentials.password,
235237
autogenerated: credentials.autogenerated
236238
)
239+
backfilled = true
237240
}
238241

239242
let vault = try? self.vault ?? AutofillSecureVaultFactory.makeVault(reporter: self.delegate)
@@ -309,7 +312,7 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
309312
// Update/Prompt in 3rd party password manager (if enabled)
310313
if let passwordManager = passwordManager, passwordManager.isEnabled {
311314
if !shouldSilentlySave {
312-
let dataToPrompt = try existingEntries(for: domain, autofillData: autofilldata)
315+
let dataToPrompt = try existingEntries(for: domain, autofillData: autofilldata, backfilled: backfilled)
313316
delegate?.secureVaultManager(self, promptUserToStoreAutofillData: dataToPrompt, withTrigger: data.trigger)
314317
autosaveAccount = nil
315318
autosaveAccountCreatedInSession = false
@@ -323,7 +326,7 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
323326

324327
// Prompt or notify on form submissions and clean any partial accounts for this instance (tab)
325328
if !shouldSilentlySave {
326-
var dataToPrompt = try existingEntries(for: domain, autofillData: autofilldata)
329+
var dataToPrompt = try existingEntries(for: domain, autofillData: autofilldata, backfilled: backfilled)
327330

328331
// On form submissions use the local value for autogenerated credentials
329332
// In this case, the JS field should be treated more like "form has some autofilled data"
@@ -658,7 +661,8 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
658661
}
659662

660663
func existingEntries(for domain: String,
661-
autofillData: AutofillUserScript.DetectedAutofillData
664+
autofillData: AutofillUserScript.DetectedAutofillData,
665+
backfilled: Bool
662666
) throws -> AutofillData {
663667
let vault = try self.vault ?? AutofillSecureVaultFactory.makeVault(reporter: self.delegate)
664668

@@ -679,7 +683,8 @@ extension SecureVaultManager: AutofillSecureVaultDelegate {
679683
return AutofillData(identity: proposedIdentity,
680684
credentials: proposedCredentials,
681685
creditCard: proposedCard,
682-
automaticallySavedCredentials: autofillData.hasAutogeneratedCredentials)
686+
automaticallySavedCredentials: autofillData.hasAutogeneratedCredentials,
687+
backfilled: backfilled)
683688
}
684689

685690
private func existingIdentity(with autofillData: AutofillUserScript.DetectedAutofillData,

‎Tests/BrowserServicesKitTests/SecureVault/SecureVaultManagerTests.swift‎

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ class SecureVaultManagerTests: XCTestCase {
7272

7373
func testWhenGettingExistingEntries_AndNoAutofillDataWasProvided_AndNoEntriesExist_ThenReturnValueIsNil() throws {
7474
let autofillData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: nil, creditCard: nil, trigger: nil)
75-
let entries = try manager.existingEntries(for: "domain.com", autofillData: autofillData)
75+
let entries = try manager.existingEntries(for: "domain.com", autofillData: autofillData, backfilled: false)
7676

7777
XCTAssertNil(entries.credentials)
7878
XCTAssertNil(entries.identity)
@@ -83,7 +83,7 @@ class SecureVaultManagerTests: XCTestCase {
8383
let card = paymentMethod(cardNumber: "5555555555555557", cardholderName: "Name", cvv: "123", month: 1, year: 2022)
8484

8585
let autofillData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: nil, creditCard: card, trigger: nil)
86-
let entries = try manager.existingEntries(for: "domain.com", autofillData: autofillData)
86+
let entries = try manager.existingEntries(for: "domain.com", autofillData: autofillData, backfilled: false)
8787

8888
XCTAssertNil(entries.credentials)
8989
XCTAssertNil(entries.identity)
@@ -96,7 +96,7 @@ class SecureVaultManagerTests: XCTestCase {
9696
try self.testVault.storeCreditCard(card)
9797

9898
let autofillData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: nil, creditCard: card, trigger: nil)
99-
let entries = try manager.existingEntries(for: "domain.com", autofillData: autofillData)
99+
let entries = try manager.existingEntries(for: "domain.com", autofillData: autofillData, backfilled: false)
100100

101101
XCTAssertNil(entries.credentials)
102102
XCTAssertNil(entries.identity)
@@ -107,7 +107,7 @@ class SecureVaultManagerTests: XCTestCase {
107107
let identity = identity(name: ("First", "Middle", "Last"), addressStreet: "Address Street")
108108

109109
let autofillData = AutofillUserScript.DetectedAutofillData(identity: identity, credentials: nil, creditCard: nil, trigger: nil)
110-
let entries = try manager.existingEntries(for: "domain.com", autofillData: autofillData)
110+
let entries = try manager.existingEntries(for: "domain.com", autofillData: autofillData, backfilled: false)
111111

112112
XCTAssertNil(entries.credentials)
113113
XCTAssertNil(entries.creditCard)
@@ -120,7 +120,7 @@ class SecureVaultManagerTests: XCTestCase {
120120
try self.testVault.storeIdentity(identity)
121121

122122
let autofillData = AutofillUserScript.DetectedAutofillData(identity: identity, credentials: nil, creditCard: nil, trigger: nil)
123-
let entries = try manager.existingEntries(for: "domain.com", autofillData: autofillData)
123+
let entries = try manager.existingEntries(for: "domain.com", autofillData: autofillData, backfilled: false)
124124

125125
XCTAssertNil(entries.credentials)
126126
XCTAssertNil(entries.identity)
@@ -548,7 +548,7 @@ class SecureVaultManagerTests: XCTestCase {
548548
// Autofill prompted data tests
549549
incomingCredentials = AutofillUserScript.IncomingCredentials(username: "[email protected]", password: "m4nu4lP4sswOrd", autogenerated: true)
550550
incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
551-
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData)
551+
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData, backfilled: false)
552552
XCTAssertEqual(entries?.credentials?.account.username, "[email protected]")
553553
XCTAssertEqual(entries?.credentials?.password, Data("m4nu4lP4sswOrd".utf8))
554554

@@ -580,7 +580,7 @@ class SecureVaultManagerTests: XCTestCase {
580580

581581
incomingCredentials = AutofillUserScript.IncomingCredentials(username: "[email protected]", password: "QNKs6k4a-axYX@aRQW", autogenerated: true)
582582
incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
583-
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData)
583+
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData, backfilled: false)
584584

585585
// Confirm autofill entries are present
586586
XCTAssertEqual(entries?.credentials?.account.username, "[email protected]")
@@ -612,7 +612,7 @@ class SecureVaultManagerTests: XCTestCase {
612612
// Autofill prompted data tests
613613
incomingCredentials = AutofillUserScript.IncomingCredentials(username: "[email protected]", password: "QNKs6k212aYX@aRQW", autogenerated: true)
614614
incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
615-
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData)
615+
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData, backfilled: false)
616616
XCTAssertEqual(entries?.credentials?.account.username, "[email protected]")
617617
XCTAssertEqual(entries?.credentials?.password, Data("QNKs6k212aYX@aRQW".utf8))
618618

@@ -639,7 +639,7 @@ class SecureVaultManagerTests: XCTestCase {
639639
// Autofill prompted data tests
640640
incomingCredentials = AutofillUserScript.IncomingCredentials(username: "[email protected]", password: "m4nu4lP4sswOrd", autogenerated: true)
641641
incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
642-
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData)
642+
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData, backfilled: false)
643643
XCTAssertEqual(entries?.credentials?.account.username, "[email protected]")
644644
XCTAssertEqual(entries?.credentials?.password, Data("m4nu4lP4sswOrd".utf8))
645645

@@ -705,7 +705,7 @@ class SecureVaultManagerTests: XCTestCase {
705705
incomingCredentials = AutofillUserScript.IncomingCredentials(username: "[email protected]", password: "m4nu4lP4sswOrd", autogenerated: true)
706706
incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
707707

708-
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData)
708+
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData, backfilled: false)
709709
XCTAssertEqual(entries?.credentials?.account.username, "[email protected]")
710710
XCTAssertEqual(entries?.credentials?.password, Data("m4nu4lP4sswOrd".utf8))
711711
}
@@ -731,7 +731,7 @@ class SecureVaultManagerTests: XCTestCase {
731731
incomingCredentials = AutofillUserScript.IncomingCredentials(username: "[email protected]", password: "m4nu4lP4sswOrd", autogenerated: true)
732732
incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
733733

734-
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData)
734+
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData, backfilled: false)
735735
XCTAssertEqual(entries?.credentials?.account.username, "[email protected]")
736736
XCTAssertEqual(entries?.credentials?.password, Data("m4nu4lP4sswOrd".utf8))
737737

@@ -768,7 +768,7 @@ class SecureVaultManagerTests: XCTestCase {
768768
incomingCredentials = AutofillUserScript.IncomingCredentials(username: "[email protected]", password: "m4nu4lP4sswOrd", autogenerated: false)
769769
incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
770770

771-
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData)
771+
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData, backfilled: false)
772772
XCTAssertEqual(entries?.credentials?.account.username, "[email protected]")
773773
XCTAssertEqual(entries?.credentials?.password, Data("m4nu4lP4sswOrd".utf8))
774774

@@ -843,7 +843,7 @@ class SecureVaultManagerTests: XCTestCase {
843843
incomingCredentials = AutofillUserScript.IncomingCredentials(username: "[email protected]", password: "m4nu4lP4sswOrd", autogenerated: false)
844844
incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
845845

846-
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData)
846+
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData, backfilled: false)
847847
XCTAssertEqual(entries?.credentials?.account.username, "[email protected]")
848848
XCTAssertEqual(entries?.credentials?.password, Data("m4nu4lP4sswOrd".utf8))
849849

@@ -866,7 +866,7 @@ class SecureVaultManagerTests: XCTestCase {
866866
// Check stored
867867
incomingCredentials = AutofillUserScript.IncomingCredentials(username: "[email protected]", password: "m4nu4lP4sswOrd", autogenerated: false)
868868
incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
869-
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData)
869+
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData, backfilled: false)
870870
XCTAssertEqual(entries?.credentials?.account.username, "[email protected]")
871871
XCTAssertEqual(entries?.credentials?.password, Data("m4nu4lP4sswOrd".utf8))
872872

@@ -888,7 +888,7 @@ class SecureVaultManagerTests: XCTestCase {
888888
// Check stored
889889
incomingCredentials = AutofillUserScript.IncomingCredentials(username: nil, password: "m4nu4lP4sswOrd", autogenerated: false)
890890
incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
891-
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData)
891+
let entries = try? manager.existingEntries(for: "fill.dev", autofillData: incomingData, backfilled: false)
892892
XCTAssertNotEqual(entries?.credentials?.account.username, "[email protected]")
893893
XCTAssertEqual(entries?.credentials?.password, Data("m4nu4lP4sswOrd".utf8))
894894

@@ -942,6 +942,27 @@ class SecureVaultManagerTests: XCTestCase {
942942
XCTAssertEqual(secureVaultManagerDelegate.promptedAutofillData?.credentials?.account.id, String(theID))
943943
}
944944

945+
func testWhenFormSubmittedWithCompleteData_afterPartialSave_backfilledIsTrue() {
946+
// Check initial stored
947+
let partialCredentials = AutofillUserScript.IncomingCredentials(username: "[email protected]", password: nil, autogenerated: false)
948+
let partialData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: partialCredentials, creditCard: nil, trigger: .partialSave)
949+
manager.autofillUserScript(mockAutofillUserScript, didRequestStoreDataForDomain: "fill.dev", data: partialData)
950+
951+
let incomingCredentials = AutofillUserScript.IncomingCredentials(username: nil, password: "m4nu4lP4sswOrd", autogenerated: false)
952+
let incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
953+
manager.autofillUserScript(mockAutofillUserScript, didRequestStoreDataForDomain: "fill.dev", data: incomingData)
954+
955+
XCTAssertTrue(secureVaultManagerDelegate.promptedAutofillData?.backfilled ?? false)
956+
}
957+
958+
func testWhenFormSubmittedWithCompleteData_withoutPartialSave_backfilledIsFalse() {
959+
let incomingCredentials = AutofillUserScript.IncomingCredentials(username: "[email protected]", password: "m4nu4lP4sswOrd", autogenerated: false)
960+
let incomingData = AutofillUserScript.DetectedAutofillData(identity: nil, credentials: incomingCredentials, creditCard: nil, trigger: .formSubmission)
961+
manager.autofillUserScript(mockAutofillUserScript, didRequestStoreDataForDomain: "fill.dev", data: incomingData)
962+
963+
XCTAssertFalse(secureVaultManagerDelegate.promptedAutofillData?.backfilled ?? true)
964+
}
965+
945966
// MARK: - Test Utilities
946967

947968
private func identity(id: Int64? = nil, name: (String, String, String), addressStreet: String?) -> SecureVaultModels.Identity {

0 commit comments

Comments
 (0)