Bug 1285577 - part 0a: make login manager return the login we're creating with addLogin, r=MattN draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 30 Nov 2016 11:03:21 +0000
changeset 451734 371688e1112a91b6068bbd4780d33231dbaef7fd
parent 451501 5206da513654c0e0b36293c9ce149ef9ed907a41
child 451735 03e47bcca21b951f5402c7335a91e601cb0cabfd
push id39276
push usergijskruitbosch@gmail.com
push dateTue, 20 Dec 2016 22:50:39 +0000
reviewersMattN
bugs1285577
milestone53.0a1
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
browser/components/migration/ChromeProfileMigrator.js
toolkit/components/passwordmgr/LoginHelper.jsm
toolkit/components/passwordmgr/nsILoginManager.idl
toolkit/components/passwordmgr/nsILoginManagerStorage.idl
toolkit/components/passwordmgr/storage-json.js
toolkit/components/passwordmgr/storage-mozStorage.js
toolkit/components/passwordmgr/test/pwmgr_common.js
toolkit/components/passwordmgr/test/unit/test_maybeImportLogin.js
toolkit/components/passwordmgr/test/unit/xpcshell.ini
--- 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]