Backed out changeset 3d9e6839b65e (bug 1812235) for xpc failure on test_sync_deprecate_credit_card_v4.js . CLOSED TREE

This commit is contained in:
Narcis Beleuzu 2023-01-27 17:17:00 +02:00
parent 9e96e91239
commit 00d577f2fd
4 changed files with 9 additions and 373 deletions

View File

@ -322,53 +322,3 @@ add_task(async function test_migrateEncryptedCreditCardNumber() {
Assert.ok(v1record.deleted);
Assert.ok(v2record.deleted);
});
add_task(async function test_migrateDeprecatedCreditCardV4() {
let path = getTempFile(TEST_STORE_FILE_NAME).path;
let profileStorage = new FormAutofillStorage(path);
await profileStorage.initialize();
let records = [
{
guid: "test-guid1",
version: CREDIT_CARD_SCHEMA_VERSION,
"cc-name": "Alice",
_sync: {
changeCounter: 0,
lastSyncedFields: {},
},
},
{
guid: "test-guid2",
version: 4,
"cc-name": "Timothy",
_sync: {
changeCounter: 0,
lastSyncedFields: {},
},
},
{
guid: "test-guid3",
version: 4,
"cc-name": "Bob",
},
];
profileStorage._store.data.creditCards = records;
for (let idx = 0; idx < records.length; idx++) {
await profileStorage.creditCards._migrateRecord(records[idx], idx);
}
profileStorage.creditCards.pullSyncChanges();
// Record that has already synced before, do not sync again
equal(getSyncChangeCounter(profileStorage.creditCards, records[0].guid), 0);
// alaways force sync v4 record
equal(records[1].version, CREDIT_CARD_SCHEMA_VERSION);
equal(getSyncChangeCounter(profileStorage.creditCards, records[1].guid), 1);
equal(records[2].version, CREDIT_CARD_SCHEMA_VERSION);
equal(getSyncChangeCounter(profileStorage.creditCards, records[2].guid), 1);
});

View File

@ -1,246 +0,0 @@
/**
* Tests sync functionality.
*/
/* import-globals-from ../../../../../services/sync/tests/unit/head_appinfo.js */
/* import-globals-from ../../../../../services/common/tests/unit/head_helpers.js */
/* import-globals-from ../../../../../services/sync/tests/unit/head_helpers.js */
/* import-globals-from ../../../../../services/sync/tests/unit/head_http_server.js */
"use strict";
const { Service } = ChromeUtils.import("resource://services-sync/service.js");
const { CreditCardsEngine } = ChromeUtils.import(
"resource://autofill/FormAutofillSync.jsm"
);
Services.prefs.setCharPref("extensions.formautofill.loglevel", "Trace");
initTestLogging("Trace");
const TEST_STORE_FILE_NAME = "test-profile.json";
const TEST_CREDIT_CARD_1 = {
guid: "86d961c7717a",
"cc-name": "John Doe",
"cc-number": "4111111111111111",
"cc-exp-month": 4,
"cc-exp-year": new Date().getFullYear(),
};
const TEST_CREDIT_CARD_2 = {
guid: "cf57a7ac3539",
"cc-name": "Timothy Berners-Lee",
"cc-number": "4929001587121045",
"cc-exp-month": 12,
"cc-exp-year": new Date().getFullYear() + 10,
};
function expectProfiles(profiles, expected) {
expected.sort((a, b) => a.guid.localeCompare(b.guid));
profiles.sort((a, b) => a.guid.localeCompare(b.guid));
try {
deepEqual(
profiles.map(p => p.guid),
expected.map(p => p.guid)
);
for (let i = 0; i < expected.length; i++) {
let thisExpected = expected[i];
let thisGot = profiles[i];
// always check "deleted".
equal(thisExpected.deleted, thisGot.deleted);
ok(objectMatches(thisGot, thisExpected));
}
} catch (ex) {
info("Comparing expected profiles:");
info(JSON.stringify(expected, undefined, 2));
info("against actual profiles:");
info(JSON.stringify(profiles, undefined, 2));
throw ex;
}
}
async function expectServerProfiles(collection, expected) {
const profiles = collection
.payloads()
.map(payload => Object.assign({ guid: payload.id }, payload.entry));
expectProfiles(profiles, expected);
}
async function expectLocalProfiles(profileStorage, expected) {
const profiles = await profileStorage.creditCards.getAll({
rawData: true,
includeDeleted: true,
});
expectProfiles(profiles, expected);
}
async function setup() {
let profileStorage = await initProfileStorage(TEST_STORE_FILE_NAME);
// should always start with no profiles.
Assert.equal(
(await profileStorage.creditCards.getAll({ includeDeleted: true })).length,
0
);
Services.prefs.setCharPref(
"services.sync.log.logger.engine.CreditCards",
"Trace"
);
let engine = new CreditCardsEngine(Service);
await engine.initialize();
// Avoid accidental automatic sync due to our own changes
Service.scheduler.syncThreshold = 10000000;
let syncID = await engine.resetLocalSyncID();
let server = serverForUsers(
{ foo: "password" },
{
meta: {
global: {
engines: { creditcards: { version: engine.version, syncID } },
},
},
creditcards: {},
}
);
Service.engineManager._engines.creditcards = engine;
engine.enabled = true;
engine._store._storage = profileStorage.creditCards;
generateNewKeys(Service.collectionKeys);
await SyncTestingInfrastructure(server);
let collection = server.user("foo").collection("creditcards");
return { profileStorage, server, collection, engine };
}
async function cleanup(server) {
let promiseStartOver = promiseOneObserver("weave:service:start-over:finish");
await Service.startOver();
await promiseStartOver;
await promiseStopServer(server);
}
function getTestRecords(profileStorage, version) {
return [
Object.assign({ version }, TEST_CREDIT_CARD_1),
Object.assign({ version }, TEST_CREDIT_CARD_2),
];
}
function setupServerRecords(server, records) {
for (const record of records) {
server.insertWBO(
"foo",
"creditcards",
new ServerWBO(
record.guid,
encryptPayload({
id: record.guid,
entry: Object.assign({}, record),
}),
Date.now() / 1000
)
);
}
}
/**
* We want to setup old records and run init() to migrate records.
* However, We don't have an easy way to setup an older version record with
* init() function now.
* So as a workaround, we simulate the behavior by directly setting data and then
* run migration.
*/
async function setupLocalProfilesAndRunMigration(profileStorage, records) {
for (const record of records) {
profileStorage._store.data.creditCards.push(Object.assign({}, record));
}
await Promise.all(
profileStorage.creditCards._data.map(async (record, index) =>
profileStorage.creditCards._migrateRecord(record, index)
)
);
}
// local v3, server v4
add_task(async function test_local_v3_server_v4() {
let { collection, profileStorage, server, engine } = await setup();
const V3_RECORDS = getTestRecords(profileStorage, 3);
const V4_RECORDS = getTestRecords(profileStorage, 4);
await setupLocalProfilesAndRunMigration(profileStorage, V3_RECORDS);
setupServerRecords(server, V4_RECORDS);
await engine.setLastSync(0);
await engine.sync();
await expectServerProfiles(collection, V3_RECORDS);
await expectLocalProfiles(profileStorage, V3_RECORDS);
await cleanup(server);
});
// local v4, server empty
add_task(async function test_local_v4_server_empty() {
let { collection, profileStorage, server, engine } = await setup();
const V3_RECORDS = getTestRecords(profileStorage, 3);
const V4_RECORDS = getTestRecords(profileStorage, 4);
await setupLocalProfilesAndRunMigration(profileStorage, V4_RECORDS);
await engine.setLastSync(0);
await engine.sync();
await expectServerProfiles(collection, V3_RECORDS);
await expectLocalProfiles(profileStorage, V3_RECORDS);
await cleanup(server);
});
// local v4, server v3
add_task(async function test_local_v4_server_v3() {
let { collection, profileStorage, server, engine } = await setup();
const V3_RECORDS = getTestRecords(profileStorage, 3);
const V4_RECORDS = getTestRecords(profileStorage, 4);
await setupLocalProfilesAndRunMigration(profileStorage, V4_RECORDS);
setupServerRecords(server, V3_RECORDS);
// local should be v3 before syncing.
await expectLocalProfiles(profileStorage, V3_RECORDS);
await engine.setLastSync(0);
await engine.sync();
await expectServerProfiles(collection, V3_RECORDS);
await expectLocalProfiles(profileStorage, V3_RECORDS);
await cleanup(server);
});
// local v4, server v4
add_task(async function test_local_v4_server_v4() {
let { collection, profileStorage, server, engine } = await setup();
const V3_RECORDS = getTestRecords(profileStorage, 3);
const V4_RECORDS = getTestRecords(profileStorage, 4);
await setupLocalProfilesAndRunMigration(profileStorage, V4_RECORDS);
setupServerRecords(server, V4_RECORDS);
// local should be v3 before syncing and then we ignore
// incoming v4 from server
await expectLocalProfiles(profileStorage, V3_RECORDS);
await engine.setLastSync(0);
await engine.sync();
await expectServerProfiles(collection, V3_RECORDS);
await expectLocalProfiles(profileStorage, V3_RECORDS);
await cleanup(server);
});

View File

@ -69,6 +69,3 @@ skip-if =
[test_sync.js]
head = head.js ../../../../../services/sync/tests/unit/head_appinfo.js ../../../../../services/common/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_http_server.js
skip-if = tsan # Times out, bug 1612707
[test_sync_deprecate_credit_card_v4.js]
head = head.js ../../../../../services/sync/tests/unit/head_appinfo.js ../../../../../services/common/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_helpers.js ../../../../../services/sync/tests/unit/head_http_server.js
skip-if = tsan # Times out, bug 1612707

View File

@ -67,10 +67,10 @@
* cc-exp-month,
* cc-exp-year, // 2-digit year will be converted to 4 digits
* // upon saving
* cc-type, // Optional card network id (instrument type)
*
* // computed fields (These fields are computed based on the above fields
* // and are not allowed to be modified directly.)
* cc-type, // Optional card network id (instrument type)
* cc-given-name,
* cc-additional-name,
* cc-family-name,
@ -160,20 +160,12 @@ const CryptoHash = Components.Constructor(
);
const STORAGE_SCHEMA_VERSION = 1;
// NOTE: It's likely this number can never change.
// Please talk to the sync team before changing this!
// (And if it did ever change, it must never be "4" due to the reconcile hacks
// below which repairs credit-cards with version=4)
const ADDRESS_SCHEMA_VERSION = 1;
// Version 2: Bug 1486954 - Encrypt `cc-number`
// Version 3: Bug 1639795 - Update keystore name
// Version 4: (deprecated!!! See Bug 1812235): Bug 1667257 - Do not store `cc-type` field
// Next version should be 5
// NOTE: It's likely this number can never change.
// Please talk to the sync team before changing this!
const CREDIT_CARD_SCHEMA_VERSION = 3;
// Version 4: Bug 1667257 - Do not store `cc-type` field
const CREDIT_CARD_SCHEMA_VERSION = 4;
const VALID_ADDRESS_FIELDS = [
"given-name",
@ -216,10 +208,10 @@ const VALID_CREDIT_CARD_FIELDS = [
"cc-number",
"cc-exp-month",
"cc-exp-year",
"cc-type",
];
const VALID_CREDIT_CARD_COMPUTED_FIELDS = [
"cc-type",
"cc-given-name",
"cc-additional-name",
"cc-family-name",
@ -979,30 +971,6 @@ class AutofillRecords {
let forkedGUID = null;
// NOTE: This implies a credit-card - so it's critical ADDRESS_SCHEMA_VERSION
// never equals 4 while this code exists!
let requiresForceUpdate =
localRecord.version != remoteRecord.version && remoteRecord.version == 4;
if (requiresForceUpdate) {
// Another desktop device that is still using version=4 has created or
// modified a remote record. Here we downgrade it to version=3 so we can
// treat it normally, then cause it to be re-uploaded so other desktop
// or mobile devices can still see it.
// That device still using version=4 *will* again see it, and again
// upgrade it, but thankfully that 3->4 migration doesn't force a reupload
// of all records, or we'd be going back and forward on every sync.
// Once that version=4 device gets updated to roll back to version=3, it
// will then yet again re-upload it, this time with version=3, but the
// content will be the same here, so everything should work out in the end.
//
// If we just ignored this incoming record, it would remain on the server
// with version=4. If the device that wrote that went away (ie, never
// synced again) nothing would ever repair it back to 3, which would
// be bad because mobile would remain broken until the user edited the
// card somewhere.
remoteRecord = await this._computeMigratedRecord(remoteRecord);
}
if (sync.changeCounter === 0) {
// Local not modified. Replace local with remote.
await this._replaceRecordAt(localIndex, remoteRecord, {
@ -1035,18 +1003,6 @@ class AutofillRecords {
}
}
if (requiresForceUpdate) {
// The incoming record was version=4 and we want to re-upload it as version=3.
// We need to reach directly into self._data[] so we can poke at the
// sync metadata directly.
let indexToUpdate = this._findIndexByGUID(remoteRecord.guid);
let toUpdate = this._data[indexToUpdate];
this._getSyncMetaData(toUpdate, true).changeCounter += 1;
this.log.info(
`Flagging record ${toUpdate.guid} for re-upload after record version downgrade`
);
}
this._store.saveSoon();
Services.obs.notifyObservers(
{
@ -1358,7 +1314,7 @@ class AutofillRecords {
record.version = 0;
}
if (this._isMigrationNeeded(record.version)) {
if (record.version < this.version) {
hasChanges = true;
record = await this._computeMigratedRecord(record);
@ -1449,10 +1405,6 @@ class AutofillRecords {
);
}
_isMigrationNeeded(recordVersion) {
return recordVersion < this.version;
}
/**
* Strip the computed fields based on the record version.
*
@ -1807,13 +1759,6 @@ class CreditCardsBase extends AutofillRecords {
throw Components.Exception("", Cr.NS_ERROR_NOT_IMPLEMENTED);
}
_isMigrationNeeded(recordVersion) {
return (
// version 4 is deprecated and is rolled back to version 3
recordVersion == 4 || recordVersion < this.version
);
}
async _computeMigratedRecord(creditCard) {
if (creditCard.version <= 2) {
if (creditCard["cc-number-encrypted"]) {
@ -1844,15 +1789,9 @@ class CreditCardsBase extends AutofillRecords {
}
}
// Do not remove the migration code until we're sure no users have version 4
// credit card records (created in Fx110 or Fx111)
if (creditCard.version == 4) {
// Version 4 is deprecated, so downgrade or upgrade to the current version
// Since the only change made in version 4 is deleting `cc-type` field, so
// nothing else need to be done here expect flagging sync needed
let existingSync = this._getSyncMetaData(creditCard);
if (existingSync) {
existingSync.changeCounter++;
if (creditCard.version <= 3) {
if (creditCard["cc-type"]) {
delete creditCard["cc-type"];
}
}
@ -1946,11 +1885,7 @@ class CreditCardsBase extends AutofillRecords {
);
}
if (record.version == 4) {
// Version 4 is deprecated, we need to force downloading it from sync
// and let migration do the work to downgrade it back to the current version.
return true;
} else if (record.version < this.version) {
if (record.version < this.version) {
switch (record.version) {
case 1:
case 2: