Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-11-23 12:51:06 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1825905: Autofill sync engine should roundtrip data it doesn't know about r=sgalich,lina

Browse Source 
Differential Revision: https://phabricator.services.mozilla.com/D174834
This commit is contained in:
Sammy Khamis 2023-04-19 20:48:15 +00:00
parent 9e484ae071
commit 7eb350d0b0
9 changed files with 194 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
browser/extensions/formautofill/test/unit/test_addressRecords.js
View File

@ -19,6 +19,7 @@ const TEST_ADDRESS_1 = {
country: "US",
tel: "+16172535702",
email: "timbl@w3.org",
"unknown-1": "an unknown field from another client",
};
const TEST_ADDRESS_2 = {
@ -62,7 +63,7 @@ const TEST_ADDRESS_WITH_EMPTY_COMPUTED_FIELD = {
const TEST_ADDRESS_WITH_INVALID_FIELD = {
"street-address": "Another Address",
invalidField: "INVALID",
email: { email: "invalidemail" },
};
const TEST_ADDRESS_EMPTY_AFTER_NORMALIZE = {
@ -81,18 +82,21 @@ const MERGE_TESTCASES = [
"given-name": "Timothy",
"street-address": "331 E. Evelyn Avenue",
tel: "+16509030800",
"unknown-1": "an unknown field from another client",
},
addressToMerge: {
"given-name": "Timothy",
"street-address": "331 E. Evelyn Avenue",
tel: "+16509030800",
country: "US",
"unknown-1": "an unknown field from another client",
},
expectedAddress: {
"given-name": "Timothy",
"street-address": "331 E. Evelyn Avenue",
tel: "+16509030800",
country: "US",
"unknown-1": "an unknown field from another client",
},
},
{
@ -470,7 +474,7 @@ add_task(async function test_add() {
await Assert.rejects(
profileStorage.addresses.add(TEST_ADDRESS_WITH_INVALID_FIELD),
/"invalidField" is not a valid field\./
/"email" contains invalid data type: object/
);
await Assert.rejects(
@ -502,7 +506,10 @@ add_task(async function test_update() {
let addresses = await profileStorage.addresses.getAll();
let guid = addresses[1].guid;
let timeLastModified = addresses[1].timeLastModified;
// We need to cheat a little due to race conditions of Date.now() when
// we're running these tests, so we subtract one and test accordingly
// in the times Date.now() returns the same timestamp
let timeLastModified = addresses[1].timeLastModified - 1;
let onChanged = TestUtils.topicObserved(
"formautofill-storage-changed",
@ -523,7 +530,7 @@ add_task(async function test_update() {
let address = await profileStorage.addresses.get(guid, { rawData: true });
Assert.equal(address.country, undefined);
Assert.notEqual(address.timeLastModified, timeLastModified);
Assert.ok(address.timeLastModified > timeLastModified);
do_check_record_matches(address, TEST_ADDRESS_3);
Assert.equal(getSyncChangeCounter(profileStorage.addresses, guid), 1);
@ -555,6 +562,7 @@ add_task(async function test_update() {
address = profileStorage.addresses._data[0];
Assert.equal(address.name, TEST_ADDRESS_WITH_EMPTY_FIELD.name);
Assert.equal(address["street-address"], undefined);
Assert.equal(address[("unknown-1", "an unknown field from another client")]);
// Empty computed fields shouldn't cause any problem.
await profileStorage.addresses.update(
@ -579,7 +587,7 @@ add_task(async function test_update() {
await Assert.rejects(
profileStorage.addresses.update(guid, TEST_ADDRESS_WITH_INVALID_FIELD),
/"invalidField" is not a valid field\./
/"email" contains invalid data type: object/
);
await Assert.rejects(
@ -681,7 +689,10 @@ MERGE_TESTCASES.forEach(testcase => {
]);
let addresses = await profileStorage.addresses.getAll();
let guid = addresses[0].guid;
let timeLastModified = addresses[0].timeLastModified;
// We need to cheat a little due to race conditions of Date.now() when
// we're running these tests, so we subtract one and test accordingly
// in the times Date.now() returns the same timestamp
let timeLastModified = addresses[0].timeLastModified - 1;
// Merge address and verify the guid in notifyObservers subject
let onMerged = TestUtils.topicObserved(
@ -711,12 +722,13 @@ MERGE_TESTCASES.forEach(testcase => {
Assert.equal(addresses.length, 1);
do_check_record_matches(addresses[0], testcase.expectedAddress);
if (testcase.noNeedToUpdate) {
Assert.equal(addresses[0].timeLastModified, timeLastModified);
// see timeLastModified for why we check -1
Assert.equal(addresses[0].timeLastModified - 1, timeLastModified);
// No need to bump the change counter if the data is unchanged.
Assert.equal(getSyncChangeCounter(profileStorage.addresses, guid), 1);
} else {
Assert.notEqual(addresses[0].timeLastModified, timeLastModified);
Assert.ok(addresses[0].timeLastModified > timeLastModified);
// Record merging should bump the change counter.
Assert.equal(getSyncChangeCounter(profileStorage.addresses, guid), 2);

9
browser/extensions/formautofill/test/unit/test_creditCardRecords.js
View File

@ -80,7 +80,7 @@ const TEST_CREDIT_CARD_WITH_2_DIGITS_YEAR = {
const TEST_CREDIT_CARD_WITH_INVALID_FIELD = {
"cc-name": "John Doe",
"cc-number": "344060747836806",
invalidField: "INVALID",
"cc-type": { invalid: "invalid" },
};
const TEST_CREDIT_CARD_WITH_INVALID_EXPIRY_DATE = {
@ -113,18 +113,21 @@ const MERGE_TESTCASES = [
"cc-number": "4929001587121045",
"cc-exp-month": 4,
"cc-exp-year": 2017,
"unknown-1": "an unknown field from another client",
},
creditCardToMerge: {
"cc-name": "John Doe",
"cc-number": "4929001587121045",
"cc-exp-month": 4,
"cc-exp-year": 2017,
"unknown-1": "an unknown field from another client",
},
expectedCreditCard: {
"cc-name": "John Doe",
"cc-number": "4929001587121045",
"cc-exp-month": 4,
"cc-exp-year": 2017,
"unknown-1": "an unknown field from another client",
},
},
{
@ -353,7 +356,7 @@ add_task(async function test_add() {
await Assert.rejects(
profileStorage.creditCards.add(TEST_CREDIT_CARD_WITH_INVALID_FIELD),
/"invalidField" is not a valid field\./
/"cc-type" contains invalid data type: object/
);
await Assert.rejects(
@ -495,7 +498,7 @@ add_task(async function test_update() {
guid,
TEST_CREDIT_CARD_WITH_INVALID_FIELD
),
/"invalidField" is not a valid field\./
/"cc-type" contains invalid data type: object/
);
await Assert.rejects(

8
browser/extensions/formautofill/test/unit/test_migrateRecords.js
View File

@ -77,12 +77,14 @@ const ADDRESS_TESTCASES = [
guid: "test-guid",
"given-name": "Timothy",
name: "John",
"unknown-1": "an unknown field from another client",
},
expectedResult: {
guid: "test-guid",
version: ADDRESS_SCHEMA_VERSION,
"given-name": "Timothy",
name: "Timothy",
"unknown-1": "an unknown field from another client",
},
},
{
@ -93,12 +95,14 @@ const ADDRESS_TESTCASES = [
version: "ABCDE",
"given-name": "Timothy",
name: "John",
"unknown-1": "an unknown field from another client",
},
expectedResult: {
guid: "test-guid",
version: ADDRESS_SCHEMA_VERSION,
"given-name": "Timothy",
name: "Timothy",
"unknown-1": "an unknown field from another client",
},
},
{
@ -193,12 +197,14 @@ const CREDIT_CARD_TESTCASES = [
guid: "test-guid",
"cc-name": "Timothy",
"cc-given-name": "John",
"unknown-1": "an unknown field from another client",
},
expectedResult: {
guid: "test-guid",
version: CREDIT_CARD_SCHEMA_VERSION,
"cc-name": "Timothy",
"cc-given-name": "Timothy",
"unknown-1": "an unknown field from another client",
},
},
{
@ -209,12 +215,14 @@ const CREDIT_CARD_TESTCASES = [
version: "ABCDE",
"cc-name": "Timothy",
"cc-given-name": "John",
"unknown-1": "an unknown field from another client",
},
expectedResult: {
guid: "test-guid",
version: CREDIT_CARD_SCHEMA_VERSION,
"cc-name": "Timothy",
"cc-given-name": "Timothy",
"unknown-1": "an unknown field from another client",
},
},
{

35
browser/extensions/formautofill/test/unit/test_reconcile.js
View File

@ -194,6 +194,8 @@ const ADDRESS_RECONCILE_TESTCASES = [
"given-name": "Mark",
"family-name": "Hammond",
country: "NZ",
// We also had an unknown field we round-tripped
foo: "bar",
},
local: [
{
@ -210,12 +212,16 @@ const ADDRESS_RECONCILE_TESTCASES = [
"given-name": "Mark",
"family-name": "Hammond",
country: "AU",
// This is a new unknown field that should send instead!
"unknown-1": "an unknown field from another client",
},
reconciled: {
guid: "e087a06dfc57",
"given-name": "Skip",
"family-name": "Hammond",
country: "AU",
// This is a new unknown field that should send instead!
"unknown-1": "an unknown field from another client",
},
},
{
@ -263,6 +269,8 @@ const ADDRESS_RECONCILE_TESTCASES = [
version: 1,
"given-name": "Mark",
"family-name": "Hammond",
// unknown fields we previously roundtripped
foo: "bar",
},
local: [
{
@ -275,6 +283,8 @@ const ADDRESS_RECONCILE_TESTCASES = [
version: 1,
"given-name": "Skip",
"family-name": "Hammond",
// New unknown field that should be the new round trip
"unknown-1": "an unknown field from another client",
},
reconciled: {
guid: "0b3a72a1bea2",
@ -290,6 +300,7 @@ const ADDRESS_RECONCILE_TESTCASES = [
version: 1,
"given-name": "Mark",
"family-name": "Hammond",
"unknown-1": "an unknown field from another client",
},
local: [
{
@ -304,18 +315,21 @@ const ADDRESS_RECONCILE_TESTCASES = [
version: 1,
"given-name": "Kip",
"family-name": "Hammond",
"unknown-1": "an unknown field from another client",
},
forked: {
// So we've forked the local record to a new GUID (and the next sync is
// going to write this as a new record)
"given-name": "Skip",
"family-name": "Hammond",
"unknown-1": "an unknown field from another client",
},
reconciled: {
// And we've updated the local version of the record to be the remote version.
guid: "62068784d089",
"given-name": "Kip",
"family-name": "Hammond",
"unknown-1": "an unknown field from another client",
},
},
{
@ -507,6 +521,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
version: CREDIT_CARD_SCHEMA_VERSION,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"unknown-1": "an unknown field from another client",
},
local: [
{
@ -524,11 +539,13 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
version: CREDIT_CARD_SCHEMA_VERSION,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"unknown-2": "a newer unknown field",
},
reconciled: {
guid: "2bbd2d8fbc6b",
"cc-name": "John Doe",
"cc-number": "4929001587121045",
"unknown-2": "a newer unknown field",
},
},
{
@ -538,6 +555,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
version: CREDIT_CARD_SCHEMA_VERSION,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"unknown-1": "unknown field",
},
local: [
{
@ -550,11 +568,13 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
version: CREDIT_CARD_SCHEMA_VERSION,
"cc-name": "John Doe",
"cc-number": "4929001587121045",
"unknown-1": "unknown field",
},
reconciled: {
guid: "e3680e9f890d",
"cc-name": "John Doe",
"cc-number": "4929001587121045",
"unknown-1": "unknown field",
},
},
@ -679,6 +699,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 12,
"unknown-1": "unknown field",
},
local: [
{
@ -695,12 +716,14 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 1,
"unknown-2": "a newer unknown field",
},
reconciled: {
guid: "e087a06dfc57",
"cc-name": "John Doe",
"cc-number": "4929001587121045",
"cc-exp-month": 1,
"unknown-2": "a newer unknown field",
},
},
{
@ -710,6 +733,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
version: CREDIT_CARD_SCHEMA_VERSION,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"unknown-1": "unknown field",
},
local: [
{
@ -728,6 +752,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-year": 2000,
"unknown-1": "unknown field",
},
reconciled: {
guid: "340a078c596f",
@ -735,6 +760,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
"cc-number": "4111111111111111",
"cc-exp-month": 12,
"cc-exp-year": 2000,
"unknown-1": "unknown field",
},
},
{
@ -773,6 +799,7 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
version: CREDIT_CARD_SCHEMA_VERSION,
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"unknown-1": "unknown field",
},
local: [
{
@ -787,18 +814,21 @@ const CREDIT_CARD_RECONCILE_TESTCASES = [
version: CREDIT_CARD_SCHEMA_VERSION,
"cc-name": "John Doe",
"cc-number": "4929001587121045",
"unknown-1": "unknown field",
},
forked: {
// So we've forked the local record to a new GUID (and the next sync is
// going to write this as a new record)
"cc-name": "John Doe",
"cc-number": "5103059495477870",
"unknown-1": "unknown field",
},
reconciled: {
// And we've updated the local version of the record to be the remote version.
guid: "62068784d089",
"cc-name": "John Doe",
"cc-number": "4929001587121045",
"unknown-1": "unknown field",
},
},
{
@ -1006,6 +1036,8 @@ add_task(async function test_reconcile_idempotent() {
version: 1,
"given-name": "Mark",
"family-name": "Hammond",
// an unknown field from a previous sync
foo: "bar",
},
{ sourceSync: true }
);
@ -1021,6 +1053,7 @@ add_task(async function test_reconcile_idempotent() {
"given-name": "Mark",
"family-name": "Hammond",
tel: "123456",
"unknown-1": "an unknown field from another client",
};
{
@ -1037,6 +1070,7 @@ add_task(async function test_reconcile_idempotent() {
"family-name": "Hammond",
organization: "Mozilla",
tel: "123456",
"unknown-1": "an unknown field from another client",
}),
"First merge should merge local and remote changes"
);
@ -1056,6 +1090,7 @@ add_task(async function test_reconcile_idempotent() {
"family-name": "Hammond",
organization: "Mozilla",
tel: "123456",
"unknown-1": "an unknown field from another client",
}),
"Second merge should not change record"
);

1
browser/extensions/formautofill/test/unit/test_storage_syncfields.js
View File

@ -19,6 +19,7 @@ const TEST_ADDRESS_1 = {
country: "US",
tel: "+1 617 253 5702",
email: "timbl@w3.org",
"unknown-1": "an unknown field we roundtrip",
};
const TEST_ADDRESS_2 = {

97
browser/extensions/formautofill/test/unit/test_sync.js
View File

@ -39,6 +39,8 @@ const TEST_PROFILE_1 = {
country: "US",
tel: "+16172535702",
email: "timbl@w3.org",
// A field this client doesn't "understand" from another client
"unknown-1": "some unknown data from another client",
};
const TEST_PROFILE_2 = {
@ -248,6 +250,10 @@ add_task(async function test_incoming_new() {
strictEqual(getSyncChangeCounter(profileStorage.addresses, profileID), 0);
strictEqual(getSyncChangeCounter(profileStorage.addresses, deletedID), 0);
// Validate incoming records with unknown fields get stored
let localRecord = await profileStorage.addresses.get(profileID);
equal(localRecord["unknown-1"], TEST_PROFILE_1["unknown-1"]);
// The sync applied new records - ensure our tracker knew it came from
// sync and didn't bump the score.
equal(engine._tracker.score, 0);
@ -537,6 +543,8 @@ add_task(async function test_applyIncoming_outgoing_restored() {
"given-name": "Timothy",
"family-name": "Berners-Lee",
"street-address": "I moved!",
// resurrection also beings back any unknown fields we had
"unknown-1": "some unknown data from another client",
})
);
@ -921,3 +929,92 @@ add_task(async function test_wipe() {
await cleanup(server);
}
});
// Other clients might have data that we aren't able to process/understand yet
// We should keep that data and ensure when we sync we don't lose that data
add_task(async function test_full_roundtrip_unknown_data() {
let { profileStorage, server, engine } = await setup();
try {
let profileID = Utils.makeGUID();
info("Incoming records with unknown fields are properly stored");
// Insert a record onto the server
server.insertWBO(
"foo",
"addresses",
new ServerWBO(
profileID,
encryptPayload({
id: profileID,
entry: Object.assign(
{
version: 1,
},
TEST_PROFILE_1
),
}),
Date.now() / 1000
)
);
// The tracker should start with no score.
equal(engine._tracker.score, 0);
await engine.setLastSync(0);
await engine.sync();
await expectLocalProfiles(profileStorage, [
{
guid: profileID,
},
]);
strictEqual(getSyncChangeCounter(profileStorage.addresses, profileID), 0);
// The sync applied new records - ensure our tracker knew it came from
// sync and didn't bump the score.
equal(engine._tracker.score, 0);
// Validate incoming records with unknown fields are correctly stored
let localRecord = await profileStorage.addresses.get(profileID);
equal(localRecord["unknown-1"], TEST_PROFILE_1["unknown-1"]);
let onChanged = TestUtils.topicObserved(
"formautofill-storage-changed",
(subject, data) => data == "update"
);
// Validate we can update the records locally and not drop any unknown fields
info("Unknown fields are sent back up to the server");
// Modify the local copy
let localCopy = Object.assign({}, TEST_PROFILE_1);
localCopy["street-address"] = "I moved!";
await profileStorage.addresses.update(profileID, localCopy);
await onChanged;
await profileStorage._saveImmediately();
let updatedCopy = await profileStorage.addresses.get(profileID);
equal(updatedCopy["street-address"], "I moved!");
// Sync our changes to the server
await engine.setLastSync(0);
await engine.sync();
let collection = server.user("foo").collection("addresses");
Assert.ok(collection.wbo(profileID));
let serverPayload = JSON.parse(
JSON.parse(collection.payload(profileID)).ciphertext
);
// The server has the updated field as well as any unknown fields
equal(
serverPayload.entry["unknown-1"],
"some unknown data from another client"
);
equal(serverPayload.entry["street-address"], "I moved!");
} finally {
await cleanup(server);
}
});

2
services/sync/tests/tps/test_addresses.js
View File

@ -28,6 +28,7 @@ const address1 = [
changes: {
organization: "W3C",
},
"unknown-1": "an unknown field from another client",
},
];
@ -44,6 +45,7 @@ const address1_after = [
country: "US",
tel: "+16172535702",
email: "timbl@w3.org",
"unknown-1": "an unknown field from another client",
},
];

2
services/sync/tests/tps/test_creditcards.js
View File

@ -18,6 +18,7 @@ const cc1 = [
"cc-number": "4716179744040592",
"cc-exp-month": 4,
"cc-exp-year": 2050,
"unknown-1": "an unknown field from another client",
changes: {
"cc-exp-year": 2051,
},
@ -30,6 +31,7 @@ const cc1_after = [
"cc-number": "4716179744040592",
"cc-exp-month": 4,
"cc-exp-year": 2051,
"unknown-1": "an unknown field from another client",
},
];

26
toolkit/components/formautofill/FormAutofillStorageBase.sys.mjs
View File

@ -48,8 +48,10 @@
* timeCreated, // in ms
* timeLastUsed, // in ms
* timeLastModified, // in ms
* timesUsed
* timesUsed,
* _sync: { ... optional sync metadata },
* ...unknown fields // We keep fields we don't understand/expect from other clients
* // to prevent data loss for other clients, we roundtrip them for sync
* }
* ],
* creditCards: [
@ -82,8 +84,10 @@
* timeCreated, // in ms
* timeLastUsed, // in ms
* timeLastModified, // in ms
* timesUsed
* timesUsed,
* _sync: { ... optional sync metadata },
* ...unknown fields // We keep fields we don't understand/expect from other clients
* // to prevent data loss for other clients, we roundtrip them for sync
* }
* ]
* }
@ -842,6 +846,14 @@ class AutofillRecords {
}
}
// When merging records, we shouldn't persist any unknown fields on the local and instead
// rely on the remote for unknown fields, so we filter the fields we know and keep the rest
Object.keys(remoteRecord)
.filter(
key =>
!this.VALID_FIELDS.includes(key) && !INTERNAL_FIELDS.includes(key)
)
.forEach(key => (mergedRecord[key] = remoteRecord[key]));
return mergedRecord;
}
@ -1372,7 +1384,15 @@ class AutofillRecords {
for (let key in record) {
if (!this.VALID_FIELDS.includes(key)) {
throw new Error(`"${key}" is not a valid field.`);
// Though we allow unknown fields, certain fields are still protected
// from being changed
if (INTERNAL_FIELDS.includes(key)) {
throw new Error(`"${key}" is not a valid field.`);
} else {
// We shouldn't try to normalize unknown fields. We'll just roundtrip them
this.log.warn(`${key} is not a known field. Skipping normalization.`);
continue;
}
}
if (typeof record[key] !== "string" && typeof record[key] !== "number") {
throw new Error(