Bug 1285577 - part 0a: make login manager return the login we're creating with addLogin, r=MattN
This patch also addresses logic issues in LoginHelper.maybeImportLogin, renames
a parameter of maybeImportLogin to be consistent and adds a test.
MozReview-Commit-ID: 12llkOyF7ne
--- a/browser/components/migration/ChromeProfileMigrator.js
+++ b/browser/components/migration/ChromeProfileMigrator.js
@@ -443,28 +443,28 @@ function GetWindowsPasswordsResource(aPr
for (let row of rows) {
let loginInfo = {
username: row.getResultByName("username_value"),
password: crypto.
decryptData(crypto.arrayToString(row.getResultByName("password_value")),
null),
hostname: NetUtil.newURI(row.getResultByName("origin_url")).prePath,
- submitURL: null,
+ formSubmitURL: null,
httpRealm: null,
usernameElement: row.getResultByName("username_element"),
passwordElement: row.getResultByName("password_element"),
timeCreated: chromeTimeToDate(row.getResultByName("date_created") + 0).getTime(),
timesUsed: row.getResultByName("times_used") + 0,
};
try {
switch (row.getResultByName("scheme")) {
case AUTH_TYPE.SCHEME_HTML:
- loginInfo.submitURL = NetUtil.newURI(row.getResultByName("action_url")).prePath;
+ loginInfo.formSubmitURL = NetUtil.newURI(row.getResultByName("action_url")).prePath;
break;
case AUTH_TYPE.SCHEME_BASIC:
case AUTH_TYPE.SCHEME_DIGEST:
// signon_realm format is URIrealm, so we need remove URI
loginInfo.httpRealm = row.getResultByName("signon_realm")
.substring(loginInfo.hostname.length + 1);
break;
default:
--- a/toolkit/components/passwordmgr/LoginHelper.jsm
+++ b/toolkit/components/passwordmgr/LoginHelper.jsm
@@ -546,70 +546,70 @@ this.LoginHelper = {
}
return false;
},
/**
* Add the login to the password manager if a similar one doesn't already exist. Merge it
* otherwise with the similar existing ones.
* @param {Object} loginData - the data about the login that needs to be added.
+ * @returns {nsILoginInfo} the newly added login, or null if no login was added.
+ * Note that we will also return null if an existing login
+ * was modified.
*/
maybeImportLogin(loginData) {
// create a new login
let login = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
login.init(loginData.hostname,
- loginData.submitURL || (typeof(loginData.httpRealm) == "string" ? null : ""),
+ loginData.formSubmitURL || (typeof(loginData.httpRealm) == "string" ? null : ""),
typeof(loginData.httpRealm) == "string" ? loginData.httpRealm : null,
loginData.username,
loginData.password,
loginData.usernameElement || "",
loginData.passwordElement || "");
login.QueryInterface(Ci.nsILoginMetaInfo);
login.timeCreated = loginData.timeCreated;
login.timeLastUsed = loginData.timeLastUsed || loginData.timeCreated;
login.timePasswordChanged = loginData.timePasswordChanged || loginData.timeCreated;
login.timesUsed = loginData.timesUsed || 1;
// While here we're passing formSubmitURL and httpRealm, they could be empty/null and get
// ignored in that case, leading to multiple logins for the same username.
let existingLogins = Services.logins.findLogins({}, login.hostname,
login.formSubmitURL,
login.httpRealm);
- // Add the login only if it doesn't already exist
- // if the login is not already available, it's going to be added or merged with other
- // logins
- if (existingLogins.some(l => login.matches(l, true))) {
- return;
+ // Check for an existing login that matches *including* the password.
+ // If such a login exists, we do not need to add a new login.
+ if (existingLogins.some(l => login.matches(l, false /* ignorePassword */))) {
+ return null;
}
- // the login is just an update for an old one or the login is older than an existing one
+ // Now check for a login with the same username, where it may be that we have an
+ // updated password.
let foundMatchingLogin = false;
for (let existingLogin of existingLogins) {
if (login.username == existingLogin.username) {
- // Bug 1187190: Password changes should be propagated depending on timestamps.
- // this an old login or a just an update, so make sure not to add it
foundMatchingLogin = true;
+ existingLogin.QueryInterface(Ci.nsILoginMetaInfo);
if (login.password != existingLogin.password &
login.timePasswordChanged > existingLogin.timePasswordChanged) {
// if a login with the same username and different password already exists and it's older
- // than the current one, that login needs to be updated using the current one details
-
- // the existing login password and timestamps should be updated
+ // than the current one, update its password and timestamp.
let propBag = Cc["@mozilla.org/hash-property-bag;1"].
createInstance(Ci.nsIWritablePropertyBag);
propBag.setProperty("password", login.password);
propBag.setProperty("timePasswordChanged", login.timePasswordChanged);
Services.logins.modifyLogin(existingLogin, propBag);
}
}
}
// if the new login is an update or is older than an exiting login, don't add it.
if (foundMatchingLogin) {
- return;
+ return null;
}
- Services.logins.addLogin(login);
+ return Services.logins.addLogin(login);
},
/**
* Convert an array of nsILoginInfo to vanilla JS objects suitable for
* sending over IPC.
*
* NB: All members of nsILoginInfo and nsILoginMetaInfo are strings.
*/
--- a/toolkit/components/passwordmgr/nsILoginManager.idl
+++ b/toolkit/components/passwordmgr/nsILoginManager.idl
@@ -26,22 +26,23 @@ interface nsILoginManager : nsISupports
readonly attribute jsval initializationPromise;
/**
* Store a new login in the login manager.
*
* @param aLogin
* The login to be added.
+ * @return a clone of the login info with the guid set (even if it was not provided)
*
* Default values for the login's nsILoginMetaInfo properties will be
* created. However, if the caller specifies non-default values, they will
* be used instead.
*/
- void addLogin(in nsILoginInfo aLogin);
+ nsILoginInfo addLogin(in nsILoginInfo aLogin);
/**
* Remove a login from the login manager.
*
* @param aLogin
* The login to be removed.
*
--- a/toolkit/components/passwordmgr/nsILoginManagerStorage.idl
+++ b/toolkit/components/passwordmgr/nsILoginManagerStorage.idl
@@ -45,22 +45,23 @@ interface nsILoginManagerStorage : nsISu
jsval terminate();
/**
* Store a new login in the storage module.
*
* @param aLogin
* The login to be added.
+ * @return a clone of the login info with the guid set (even if it was not provided).
*
* Default values for the login's nsILoginMetaInfo properties will be
* created. However, if the caller specifies non-default values, they will
* be used instead.
*/
- void addLogin(in nsILoginInfo aLogin);
+ nsILoginInfo addLogin(in nsILoginInfo aLogin);
/**
* Remove a login from the storage module.
*
* @param aLogin
* The login to be removed.
*
--- a/toolkit/components/passwordmgr/storage-json.js
+++ b/toolkit/components/passwordmgr/storage-json.js
@@ -146,16 +146,17 @@ this.LoginManagerStorage_json.prototype
timeLastUsed: loginClone.timeLastUsed,
timePasswordChanged: loginClone.timePasswordChanged,
timesUsed: loginClone.timesUsed
});
this._store.saveSoon();
// Send a notification that a login was added.
LoginHelper.notifyStorageChanged("addLogin", loginClone);
+ return loginClone;
},
removeLogin(login) {
this._store.ensureDataReady();
let [idToDelete, storedLogin] = this._getIdForLogin(login);
if (!idToDelete)
throw new Error("No matching logins");
--- a/toolkit/components/passwordmgr/storage-mozStorage.js
+++ b/toolkit/components/passwordmgr/storage-mozStorage.js
@@ -271,16 +271,17 @@ LoginManagerStorage_mozStorage.prototype
} finally {
if (stmt) {
stmt.reset();
}
}
// Send a notification that a login was added.
LoginHelper.notifyStorageChanged("addLogin", loginClone);
+ return loginClone;
},
removeLogin : function(login) {
let [idToDelete, storedLogin] = this._getIdForLogin(login);
if (!idToDelete)
throw new Error("No matching logins");
--- a/toolkit/components/passwordmgr/test/pwmgr_common.js
+++ b/toolkit/components/passwordmgr/test/pwmgr_common.js
@@ -418,17 +418,21 @@ if (this.addMessageListener) {
let recreatedArgs = msg.args.map((arg, index) => {
if (msg.loginInfoIndices.includes(index)) {
return LoginHelper.vanillaObjectToLogin(arg);
}
return arg;
});
- return Services.logins[msg.methodName](...recreatedArgs);
+ let rv = Services.logins[msg.methodName](...recreatedArgs);
+ if (rv instanceof Ci.nsILoginInfo) {
+ rv = LoginHelper.loginToVanillaObject(rv);
+ }
+ return rv;
});
var globalMM = Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager);
globalMM.addMessageListener("RemoteLogins:onFormSubmit", function onFormSubmit(message) {
sendAsyncMessage("formSubmissionProcessed", message.data, message.objects);
});
} else {
// Code to only run in the mochitest pages (not in the chrome script).
new file mode 100644
--- /dev/null
+++ b/toolkit/components/passwordmgr/test/unit/test_maybeImportLogin.js
@@ -0,0 +1,169 @@
+"use strict";
+
+Cu.import("resource://gre/modules/Services.jsm");
+Cu.import("resource://gre/modules/LoginHelper.jsm");
+
+const HOST1 = "https://www.example.com/";
+const HOST2 = "https://www.mozilla.org/";
+
+const USER1 = "myuser";
+const USER2 = "anotheruser";
+
+const PASS1 = "mypass";
+const PASS2 = "anotherpass";
+const PASS3 = "yetanotherpass";
+
+add_task(function test_new_logins() {
+ let importedLogin = LoginHelper.maybeImportLogin({
+ username: USER1,
+ password: PASS1,
+ hostname: HOST1,
+ formSubmitURL: HOST1,
+ });
+ Assert.ok(importedLogin, "Return value should indicate imported login.");
+ let matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 1, `There should be 1 login for ${HOST1}`);
+
+ importedLogin = LoginHelper.maybeImportLogin({
+ username: USER1,
+ password: PASS1,
+ hostname: HOST2,
+ formSubmitURL: HOST2,
+ });
+
+ Assert.ok(importedLogin, "Return value should indicate another imported login.");
+ matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 1, `There should still be 1 login for ${HOST1}`);
+
+ matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST2});
+ Assert.equal(matchingLogins.length, 1, `There should also be 1 login for ${HOST2}`);
+ Assert.equal(Services.logins.getAllLogins().length, 2, "There should be 2 logins in total");
+ Services.logins.removeAllLogins();
+});
+
+add_task(function test_duplicate_logins() {
+ let importedLogin = LoginHelper.maybeImportLogin({
+ username: USER1,
+ password: PASS1,
+ hostname: HOST1,
+ formSubmitURL: HOST1,
+ });
+ Assert.ok(importedLogin, "Return value should indicate imported login.");
+ let matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 1, `There should be 1 login for ${HOST1}`);
+
+ importedLogin = LoginHelper.maybeImportLogin({
+ username: USER1,
+ password: PASS1,
+ hostname: HOST1,
+ formSubmitURL: HOST1,
+ });
+ Assert.ok(!importedLogin, "Return value should indicate no new login was imported.");
+ matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 1, `There should still be 1 login for ${HOST1}`);
+ Services.logins.removeAllLogins();
+});
+
+add_task(function test_different_passwords() {
+ let importedLogin = LoginHelper.maybeImportLogin({
+ username: USER1,
+ password: PASS1,
+ hostname: HOST1,
+ formSubmitURL: HOST1,
+ timeCreated: new Date(Date.now() - 1000),
+ });
+ Assert.ok(importedLogin, "Return value should indicate imported login.");
+ let matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 1, `There should be 1 login for ${HOST1}`);
+
+ // This item will be newer, so its password should take precedence.
+ importedLogin = LoginHelper.maybeImportLogin({
+ username: USER1,
+ password: PASS2,
+ hostname: HOST1,
+ formSubmitURL: HOST1,
+ timeCreated: new Date(),
+ });
+ Assert.ok(!importedLogin, "Return value should not indicate imported login (as we updated an existing one).");
+ matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 1, `There should still be 1 login for ${HOST1}`);
+ Assert.equal(matchingLogins[0].password, PASS2, "We should have updated the password for this login.");
+
+ // Now try to update with an older password:
+ importedLogin = LoginHelper.maybeImportLogin({
+ username: USER1,
+ password: PASS3,
+ hostname: HOST1,
+ formSubmitURL: HOST1,
+ timeCreated: new Date(Date.now() - 1000000),
+ });
+ Assert.ok(!importedLogin, "Return value should not indicate imported login (as we didn't update anything).");
+ matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 1, `There should still be 1 login for ${HOST1}`);
+ Assert.equal(matchingLogins[0].password, PASS2, "We should NOT have updated the password for this login.");
+
+ Services.logins.removeAllLogins();
+});
+
+add_task(function test_different_usernames() {
+ let importedLogin = LoginHelper.maybeImportLogin({
+ username: USER1,
+ password: PASS1,
+ hostname: HOST1,
+ formSubmitURL: HOST1,
+ });
+ Assert.ok(importedLogin, "Return value should indicate imported login.");
+ let matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 1, `There should be 1 login for ${HOST1}`);
+
+ importedLogin = LoginHelper.maybeImportLogin({
+ username: USER2,
+ password: PASS1,
+ hostname: HOST1,
+ formSubmitURL: HOST1,
+ });
+ Assert.ok(importedLogin, "Return value should indicate another imported login.");
+ matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 2, `There should now be 2 logins for ${HOST1}`);
+
+ Services.logins.removeAllLogins();
+});
+
+add_task(function test_different_targets() {
+ let importedLogin = LoginHelper.maybeImportLogin({
+ username: USER1,
+ password: PASS1,
+ hostname: HOST1,
+ formSubmitURL: HOST1,
+ });
+ Assert.ok(importedLogin, "Return value should indicate imported login.");
+ let matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 1, `There should be 1 login for ${HOST1}`);
+
+ // Not passing either a formSubmitURL or a httpRealm should be treated as
+ // the same as the previous login
+ importedLogin = LoginHelper.maybeImportLogin({
+ username: USER1,
+ password: PASS1,
+ hostname: HOST1,
+ });
+ Assert.ok(!importedLogin, "Return value should NOT indicate imported login " +
+ "(because a missing formSubmitURL and httpRealm should be duped to the existing login).");
+ matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 1, `There should still be 1 login for ${HOST1}`);
+ Assert.equal(matchingLogins[0].formSubmitURL, HOST1, "The form submission URL should have been kept.");
+
+ importedLogin = LoginHelper.maybeImportLogin({
+ username: USER1,
+ password: PASS1,
+ hostname: HOST1,
+ httpRealm: HOST1,
+ });
+ Assert.ok(importedLogin, "Return value should indicate another imported login " +
+ "as an httpRealm login shouldn't be duped.");
+ matchingLogins = LoginHelper.searchLoginsWithObject({hostname: HOST1});
+ Assert.equal(matchingLogins.length, 2, `There should now be 2 logins for ${HOST1}`);
+
+ Services.logins.removeAllLogins();
+});
+
--- a/toolkit/components/passwordmgr/test/unit/xpcshell.ini
+++ b/toolkit/components/passwordmgr/test/unit/xpcshell.ini
@@ -30,16 +30,17 @@ run-if = buildapp == "browser"
[test_legacy_validation.js]
[test_logins_change.js]
[test_logins_decrypt_failure.js]
skip-if = os == "android" # Bug 1171687: Needs fixing on Android
[test_user_autocomplete_result.js]
skip-if = os == "android"
[test_logins_metainfo.js]
[test_logins_search.js]
+[test_maybeImportLogin.js]
[test_notifications.js]
[test_OSCrypto_win.js]
skip-if = os != "win"
[test_recipes_add.js]
[test_recipes_content.js]
[test_search_schemeUpgrades.js]
[test_storage.js]
[test_telemetry.js]