--- a/services/sync/modules/engines/passwords.js
+++ b/services/sync/modules/engines/passwords.js
@@ -7,45 +7,65 @@ var EXPORTED_SYMBOLS = ["PasswordEngine"
ChromeUtils.import("resource://gre/modules/Services.jsm");
ChromeUtils.import("resource://services-sync/record.js");
ChromeUtils.import("resource://services-sync/constants.js");
ChromeUtils.import("resource://services-sync/collection_validator.js");
ChromeUtils.import("resource://services-sync/engines.js");
ChromeUtils.import("resource://services-sync/util.js");
ChromeUtils.import("resource://services-common/async.js");
-const SYNCABLE_LOGIN_FIELDS = [
+// These trigger a sync if changed
+const STRONG_LOGIN_FIELDS = [
// `nsILoginInfo` fields.
"hostname",
"formSubmitURL",
"httpRealm",
"username",
"password",
"usernameField",
"passwordField",
// `nsILoginMetaInfo` fields.
"timeCreated",
"timePasswordChanged",
];
-// Compares two logins to determine if their syncable fields changed. The login
-// manager fires `modifyLogin` for changes to all fields, including ones we
-// don't sync. In particular, `timeLastUsed` changes shouldn't mark the login
-// for upload; otherwise, we might overwrite changed passwords before they're
-// downloaded (bug 973166).
-function isSyncableChange(oldLogin, newLogin) {
+// We flag the login field for weak upload if changed (so that incoming records
+// with "strong" changes are prioritized).
+const WEAK_LOGIN_FIELDS = [
+ "timesUsed",
+ "timeLastUsed",
+];
+
+const LOGIN_SYNC_STRATEGY = Object.freeze({
+ IGNORE: 0,
+ WEAK_UPLOAD: 1,
+ STRONG_UPLOAD: 2,
+});
+
+const SYNCABLE_LOGIN_FIELDS = new Map([
+ ...STRONG_LOGIN_FIELDS.map(field => [field, LOGIN_SYNC_STRATEGY.STRONG_UPLOAD]),
+ ...WEAK_LOGIN_FIELDS.map(field => [field, LOGIN_SYNC_STRATEGY.WEAK_UPLOAD]),
+]);
+
+
+// Compares two logins to determine if their syncable fields changed, and if
+// they have, whether or not we should flag the login for weak (which defers
+// changes to incoming records) or strong (which is prioritized over incoming
+// changes) upload.
+function getSyncStrategy(oldLogin, newLogin) {
oldLogin.QueryInterface(Ci.nsILoginMetaInfo).QueryInterface(Ci.nsILoginInfo);
newLogin.QueryInterface(Ci.nsILoginMetaInfo).QueryInterface(Ci.nsILoginInfo);
- for (let property of SYNCABLE_LOGIN_FIELDS) {
+ let result = LOGIN_SYNC_STRATEGY.IGNORE;
+ for (let [property, strategy] of SYNCABLE_LOGIN_FIELDS) {
if (oldLogin[property] != newLogin[property]) {
- return true;
+ result = Math.max(result, strategy);
}
}
- return false;
+ return result;
}
function LoginRec(collection, id) {
CryptoWrapper.call(this, collection, id);
}
LoginRec.prototype = {
__proto__: CryptoWrapper.prototype,
_logName: "Sync.Record.Login",
@@ -55,20 +75,21 @@ LoginRec.prototype = {
if (o.password) {
o.password = "X".repeat(o.password.length);
}
return JSON.stringify(o);
}
};
Utils.deferGetSet(LoginRec, "cleartext", [
- "hostname", "formSubmitURL",
- "httpRealm", "username", "password", "usernameField", "passwordField",
- "timeCreated", "timePasswordChanged",
- ]);
+ "hostname", "formSubmitURL",
+ "httpRealm", "username", "password", "usernameField", "passwordField",
+ "timeCreated", "timePasswordChanged",
+ "timeLastUsed", "timesUsed",
+]);
function PasswordEngine(service) {
SyncEngine.call(this, "Passwords", service);
}
PasswordEngine.prototype = {
__proto__: SyncEngine.prototype,
_storeObj: PasswordStore,
@@ -186,17 +207,22 @@ PasswordStore.prototype = {
info.QueryInterface(Ci.nsILoginMetaInfo);
info.guid = record.id;
if (record.timeCreated) {
info.timeCreated = record.timeCreated;
}
if (record.timePasswordChanged) {
info.timePasswordChanged = record.timePasswordChanged;
}
-
+ if (record.timesUsed) {
+ info.timesUsed = record.timesUsed;
+ }
+ if (record.timeLastUsed) {
+ info.timeLastUsed = record.timeLastUsed;
+ }
return info;
},
async _getLoginFromGUID(id) {
let prop = this._newPropertyBag();
prop.setPropertyAsAUTF8String("guid", id);
let logins = Services.logins.searchLogins({}, prop);
@@ -211,17 +237,17 @@ PasswordStore.prototype = {
return null;
},
async getAllIDs() {
let items = {};
let logins = Services.logins.getAllLogins({});
for (let i = 0; i < logins.length; i++) {
- // Skip over Weave password/passphrase entries.
+ // Skip over FxA credential changes
let metaInfo = logins[i].QueryInterface(Ci.nsILoginMetaInfo);
if (Utils.getSyncCredentialsHosts().has(metaInfo.hostname)) {
continue;
}
items[metaInfo.guid] = metaInfo;
}
@@ -267,16 +293,18 @@ PasswordStore.prototype = {
record.password = login.password;
record.usernameField = login.usernameField;
record.passwordField = login.passwordField;
// Optional fields.
login.QueryInterface(Ci.nsILoginMetaInfo);
record.timeCreated = login.timeCreated;
record.timePasswordChanged = login.timePasswordChanged;
+ record.timeLastUsed = login.timeLastUsed;
+ record.timesUsed = login.timesUsed;
return record;
},
async create(record) {
let login = this._nsLoginInfoFromRecord(record);
if (!login) {
return;
@@ -312,16 +340,24 @@ PasswordStore.prototype = {
}
this._log.trace("Updating " + record.hostname);
let newinfo = this._nsLoginInfoFromRecord(record);
if (!newinfo) {
return;
}
+ loginItem.QueryInterface(Ci.nsILoginMetaInfo);
+ if (loginItem.timeLastUsed && (loginItem.timeLastUsed > newinfo.timeLastUsed)) {
+ newinfo.timeLastUsed = loginItem.timeLastUsed;
+ }
+ if (loginItem.timesUsed && (loginItem.timesUsed > newinfo.timesUsed)) {
+ newinfo.timesUsed = loginItem.timesUsed;
+ }
+
try {
Services.logins.modifyLogin(loginItem, newinfo);
} catch (ex) {
this._log.debug(`Modifying record ${record.id} resulted in exception; not modifying`, ex);
}
},
async wipe() {
@@ -350,23 +386,33 @@ PasswordTracker.prototype = {
// A single add, remove or change or removing all items
// will trigger a sync for MULTI_DEVICE.
switch (data) {
case "modifyLogin": {
subject.QueryInterface(Ci.nsIArrayExtensions);
let oldLogin = subject.GetElementAt(0);
let newLogin = subject.GetElementAt(1);
- if (!isSyncableChange(oldLogin, newLogin)) {
- this._log.trace(`${data}: Ignoring change for ${newLogin.guid}`);
- break;
- }
- const tracked = await this._trackLogin(newLogin);
- if (tracked) {
- this._log.trace(`${data}: Tracking change for ${newLogin.guid}`);
+ let strategy = getSyncStrategy(oldLogin, newLogin);
+ switch (strategy) {
+ case LOGIN_SYNC_STRATEGY.IGNORE:
+ this._log.trace(`${data}: Ignoring change for ${newLogin.guid}`);
+ break;
+ case LOGIN_SYNC_STRATEGY.WEAK_UPLOAD:
+ if (this._shouldTrack(newLogin)) {
+ this._log.trace(`${data}: Tracking change for ${newLogin.guid} (weak)`);
+ this.engine.addForWeakUpload(newLogin.guid);
+ this.score += SCORE_INCREMENT_XLARGE;
+ }
+ break;
+ case LOGIN_SYNC_STRATEGY.STRONG_UPLOAD:
+ if (await this._trackLogin(newLogin)) {
+ this._log.trace(`${data}: Tracking change for ${newLogin.guid} (strong)`);
+ }
+ break;
}
break;
}
case "addLogin":
case "removeLogin":
subject.QueryInterface(Ci.nsILoginMetaInfo).QueryInterface(Ci.nsILoginInfo);
const tracked = await this._trackLogin(subject);
@@ -377,19 +423,26 @@ PasswordTracker.prototype = {
case "removeAllLogins":
this._log.trace(data);
this.score += SCORE_INCREMENT_XLARGE;
break;
}
},
+ _shouldTrack(login) {
+ if (this.ignoreAll || this._ignored.includes(login.guid)) {
+ return false;
+ }
+ // Skip over Weave password/passphrase changes.
+ return !Utils.getSyncCredentialsHosts().has(login.hostname);
+ },
+
async _trackLogin(login) {
- if (Utils.getSyncCredentialsHosts().has(login.hostname)) {
- // Skip over Weave password/passphrase changes.
+ if (!this._shouldTrack(login)) {
return false;
}
const added = await this.addChangedID(login.guid);
if (!added) {
return false;
}
this.score += SCORE_INCREMENT_XLARGE;
return true;
--- a/services/sync/tests/unit/test_password_engine.js
+++ b/services/sync/tests/unit/test_password_engine.js
@@ -15,48 +15,70 @@ async function cleanup(engine, server) {
Service.recordManager.clearCache();
await promiseStopServer(server);
}
add_task(async function setup() {
await Service.engineManager.unregister("addons"); // To silence errors.
});
-add_task(async function test_ignored_fields() {
+add_task(async function test_weak_fields() {
_("Only changes to syncable fields should be tracked");
let engine = Service.engineManager.get("passwords");
let server = await serverForFoo(engine);
await SyncTestingInfrastructure(server);
enableValidationPrefs();
let login = Services.logins.addLogin(new LoginInfo("https://example.com", "",
null, "username", "password", "", ""));
login.QueryInterface(Ci.nsILoginMetaInfo); // For `guid`.
+ let login2 = Services.logins.addLogin(new LoginInfo("https://example.com/2", "",
+ null, "username", "password", "", ""));
+ login2.QueryInterface(Ci.nsILoginMetaInfo);
+
engine._tracker.start();
try {
let nonSyncableProps = new PropertyBag();
nonSyncableProps.setProperty("timeLastUsed", Date.now());
nonSyncableProps.setProperty("timesUsed", 3);
Services.logins.modifyLogin(login, nonSyncableProps);
+ await engine._tracker.asyncObserver.promiseObserversComplete();
let noChanges = await engine.pullNewChanges();
- deepEqual(noChanges, {}, "Should not track non-syncable fields");
+ deepEqual(noChanges, {}, "Should not track weakly synced fields in tracker");
+ deepEqual([...engine._needWeakUpload.keys()], [login.guid],
+ "Should track weakly synced fields in _needWeakUpload");
let syncableProps = new PropertyBag();
syncableProps.setProperty("username", "newuser");
Services.logins.modifyLogin(login, syncableProps);
+ await engine._tracker.asyncObserver.promiseObserversComplete();
let changes = await engine.pullNewChanges();
deepEqual(Object.keys(changes), [login.guid],
- "Should track syncable fields");
+ "Should still track syncable fields if it's in weak upload set");
+ // Note: we remove it from the weak set during strong upload if it's in both,
+ // so it really doesn't matter if it's in both.
+
+ syncableProps = new PropertyBag();
+ // Add both a "weak" field and a strong field
+ syncableProps.setProperty("username", "user2");
+ syncableProps.setProperty("timesUsed", 2);
+
+ Services.logins.modifyLogin(login2, syncableProps);
+ await engine._tracker.asyncObserver.promiseObserversComplete();
+
+ // Should still end up in the tracker
+ changes = await engine.pullNewChanges();
+ deepEqual(Object.keys(changes).sort(), [login.guid, login2.guid].sort());
} finally {
await cleanup(engine, server);
}
});
add_task(async function test_ignored_sync_credentials() {
_("Sync credentials in login manager should be ignored");
@@ -242,8 +264,73 @@ add_task(async function test_sync_passwo
let validation = engineInfo.validation;
ok(validation, "Engine should have validation info");
} finally {
await cleanup(engine, server);
}
});
+
+add_task(async function test_apply_maxed_fields() {
+ _("Only changes to syncable fields should be tracked");
+
+ let engine = Service.engineManager.get("passwords");
+
+ let server = await serverForFoo(engine);
+ await SyncTestingInfrastructure(server);
+
+ enableValidationPrefs();
+
+ let login = Services.logins.addLogin(new LoginInfo("https://www.example.com", "",
+ null, "user", "pass", "", ""));
+ login.QueryInterface(Ci.nsILoginMetaInfo);
+
+ let now = Date.now();
+
+ let metaProps = new PropertyBag();
+ metaProps.setProperty("timeLastUsed", now - 60 * 1000 * 1000);
+ metaProps.setProperty("timesUsed", 3);
+ Services.logins.modifyLogin(login, metaProps);
+
+ try {
+
+ await engine._tracker.stop();
+ await engine._store.update({
+ id: login.guid,
+ hostname: login.hostname,
+ formSubmitURL: login.formSubmitURL,
+ httpRealm: login.httpRealm,
+ username: login.username,
+ password: login.password,
+ usernameField: login.usernameField,
+ passwordField: login.passwordField,
+ timeLastUsed: now - 120 * 1000 * 1000,
+ timesUsed: 4,
+ });
+ await engine._tracker.start();
+ let newLogin = await engine._store._getLoginFromGUID(login.guid);
+ newLogin.QueryInterface(Ci.nsILoginMetaInfo);
+
+ Assert.ok(newLogin);
+ Assert.equal(newLogin.timeLastUsed, now - 60 * 1000 * 1000);
+
+ // Really this is still wrong, but it's better than if we just took the most
+ // recent change.
+ //
+ // To clarify, it's still wrong basically because we don't have the shared
+ // parent. Although, even if we did, we still could know that 4 is wrong
+ // here:
+ //
+ // We know the timeLastUsed of the incoming record is 120s ago and it had 4
+ // uses, and our timeLastUsed is 3 as of 60s ago. And so it's guaranteed
+ // that the "true" timesUsed is at least 5 (since as of our last sync it
+ // would have to have been 2).
+ //
+ // That said, if we wanted to fix this, we really should just store the
+ // common parent -- or at least the timesUsed/timeLastUsed for the common
+ // parent -- which would require less tricky logic, and be more robust.
+ Assert.equal(newLogin.timesUsed, 4);
+
+ } finally {
+ await cleanup(engine, server);
+ }
+});
--- a/services/sync/tests/unit/test_password_store.js
+++ b/services/sync/tests/unit/test_password_store.js
@@ -26,17 +26,17 @@ async function checkRecord(name, record,
let stored_record = logins[0].QueryInterface(Ci.nsILoginMetaInfo);
if (timeCreated !== undefined) {
Assert.equal(stored_record.timeCreated, expectedTimeCreated);
}
if (timePasswordChanged !== undefined) {
if (recordIsUpdated) {
- Assert.ok(stored_record.timePasswordChanged >= expectedTimePasswordChanged);
+ Assert.greaterOrEqual(stored_record.timePasswordChanged, expectedTimePasswordChanged);
} else {
Assert.equal(stored_record.timePasswordChanged, expectedTimePasswordChanged);
}
return stored_record.timePasswordChanged;
}
} else {
Assert.ok(!(await store.getAllIDs())[record.id]);
}
@@ -126,23 +126,23 @@ async function test_apply_same_record_wi
it is passed as an argument to changePassword. */
var timePasswordChanged = 100;
timePasswordChanged = await changePassword("A", "http://a.tn", "password", 1, 100,
100, 100, timePasswordChanged, true);
timePasswordChanged = await changePassword("A", "http://a.tn", "password", 1, 100,
100, 800, timePasswordChanged, true,
true);
timePasswordChanged = await changePassword("A", "http://a.tn", "password", 1, 500,
- 100, 800, timePasswordChanged, true,
+ 500, 800, timePasswordChanged, true,
true);
- timePasswordChanged = await changePassword("A", "http://a.tn", "password2", 1, 500,
- 100, 1536213005222, timePasswordChanged,
+ timePasswordChanged = await changePassword("A", "http://a.tn", "password2", 1, 50,
+ 50, 1536213005222, timePasswordChanged,
true, true);
- timePasswordChanged = await changePassword("A", "http://a.tn", "password2", 1, 500,
- 100, 800, timePasswordChanged, true, true);
+ timePasswordChanged = await changePassword("A", "http://a.tn", "password2", 1, 100,
+ 100, 1536213005222, timePasswordChanged, true, true);
/* eslint-enable no-unsed-vars */
}
async function test_LoginRec_toString(store, recordData) {
let rec = await store.createRecord(recordData.id);
ok(rec);
ok(!rec.toString().includes(rec.password));
}
--- a/toolkit/components/passwordmgr/LoginHelper.jsm
+++ b/toolkit/components/passwordmgr/LoginHelper.jsm
@@ -294,16 +294,32 @@ var LoginHelper = {
aNewLoginData.username, aNewLoginData.password,
aNewLoginData.usernameField, aNewLoginData.passwordField);
newLogin.QueryInterface(Ci.nsILoginMetaInfo);
// Automatically update metainfo when password is changed.
if (newLogin.password != aOldStoredLogin.password) {
newLogin.timePasswordChanged = Date.now();
}
+
+ // Update meta-info properties if provided
+ if (aNewLoginData instanceof Ci.nsILoginMetaInfo) {
+ if (aNewLoginData.timesUsed) {
+ newLogin.timesUsed = aNewLoginData.timesUsed;
+ }
+ if (aNewLoginData.timeLastUsed) {
+ newLogin.timeLastUsed = aNewLoginData.timeLastUsed;
+ }
+ if (aNewLoginData.timePasswordChanged) {
+ newLogin.timePasswordChanged = aNewLoginData.timePasswordChanged;
+ }
+ if (aNewLoginData.timeCreated) {
+ newLogin.timeCreated = aNewLoginData.timeCreated;
+ }
+ }
} else if (aNewLoginData instanceof Ci.nsIPropertyBag) {
// Clone the existing login, along with all its properties.
newLogin = aOldStoredLogin.clone();
newLogin.QueryInterface(Ci.nsILoginMetaInfo);
// Automatically update metainfo when password is changed.
// (Done before the main property updates, lest the caller be
// explicitly updating both .password and .timePasswordChanged)