author | Doug Thayer <dothayer@mozilla.com> |
Thu, 15 Feb 2018 10:26:44 -0800 | |
changeset 759195 | 5b11c18c647ad00aa0e938b96908524ad0edc5b8 |
parent 754572 | e43f2f6ea111c2d059d95fa9a71516b869a69698 |
child 759196 | c50552ead3c092705112b991669891d1f726ef51 |
push id | 100304 |
push user | bmo:dothayer@mozilla.com |
push date | Fri, 23 Feb 2018 21:18:38 +0000 |
reviewers | MattN |
bugs | 1433593, 1426721 |
milestone | 60.0a1 |
--- a/browser/components/migration/IEProfileMigrator.js +++ b/browser/components/migration/IEProfileMigrator.js @@ -117,41 +117,41 @@ IE7FormPasswords.prototype = { let count = key.valueCount; key.close(); return count > 0; } catch (e) { return false; } }, - migrate(aCallback) { + async migrate(aCallback) { let historyEnumerator = Cc["@mozilla.org/profile/migrator/iehistoryenumerator;1"]. createInstance(Ci.nsISimpleEnumerator); let uris = []; // the uris of the websites that are going to be migrated while (historyEnumerator.hasMoreElements()) { let entry = historyEnumerator.getNext().QueryInterface(Ci.nsIPropertyBag2); let uri = entry.get("uri").QueryInterface(Ci.nsIURI); // MSIE stores some types of URLs in its history that we don't handle, like HTMLHelp // and others. Since we are not going to import the logins that are performed in these URLs // we can just skip them. if (!["http", "https", "ftp"].includes(uri.scheme)) { continue; } uris.push(uri); } - this._migrateURIs(uris); + await this._migrateURIs(uris); aCallback(true); }, /** * Migrate the logins that were saved for the uris arguments. * @param {nsIURI[]} uris - the uris that are going to be migrated. */ - _migrateURIs(uris) { + async _migrateURIs(uris) { this.ctypesKernelHelpers = new MSMigrationUtils.CtypesKernelHelpers(); this._crypto = new OSCrypto(); let nsIWindowsRegKey = Ci.nsIWindowsRegKey; let key = Cc["@mozilla.org/windows-registry-key;1"]. createInstance(nsIWindowsRegKey); key.open(nsIWindowsRegKey.ROOT_KEY_CURRENT_USER, kLoginsKey, nsIWindowsRegKey.ACCESS_READ); @@ -161,16 +161,17 @@ IE7FormPasswords.prototype = { /* The logins are stored in the registry, where the key is a hashed URL and its * value contains the encrypted details for all logins for that URL. * * First iterate through IE history, hashing each URL and looking for a match. If * found, decrypt the value, using the URL as a salt. Finally add any found logins * to the Firefox password manager. */ + let logins = []; for (let uri of uris) { try { // remove the query and the ref parts of the URL let urlObject = new URL(uri.spec); let url = urlObject.origin + urlObject.pathname; // if the current url is already processed, it should be skipped if (urlsSet.has(url)) { continue; @@ -195,21 +196,33 @@ IE7FormPasswords.prototype = { } // extract the login details from the decrypted data let ieLogins = this._extractDetails(data, uri); // if at least a credential was found in the current data, successfullyDecryptedValues should // be incremented by one if (ieLogins.length) { successfullyDecryptedValues++; } - this._addLogins(ieLogins); + for (let ieLogin of ieLogins) { + logins.push({ + username: ieLogin.username, + password: ieLogin.password, + hostname: ieLogin.url, + timeCreated: ieLogin.creation, + }); + } } catch (e) { Cu.reportError("Error while importing logins for " + uri.spec + ": " + e); } } + + if (logins.length > 0) { + await MigrationUtils.insertLoginsWrapper(logins); + } + // if the number of the imported values is less than the number of values in the key, it means // that not all the values were imported and an error should be reported if (successfullyDecryptedValues < key.valueCount) { Cu.reportError("We failed to decrypt and import some logins. " + "This is likely because we didn't find the URLs where these " + "passwords were submitted in the IE history and which are needed to be used " + "as keys in the decryption."); } @@ -217,37 +230,16 @@ IE7FormPasswords.prototype = { key.close(); this._crypto.finalize(); this.ctypesKernelHelpers.finalize(); }, _crypto: null, /** - * Add the logins to the password manager. - * @param {Object[]} logins - array of the login details. - */ - _addLogins(ieLogins) { - for (let ieLogin of ieLogins) { - try { - // create a new login - let login = { - username: ieLogin.username, - password: ieLogin.password, - hostname: ieLogin.url, - timeCreated: ieLogin.creation, - }; - MigrationUtils.insertLoginWrapper(login); - } catch (e) { - Cu.reportError(e); - } - } - }, - - /** * Extract the details of one or more logins from the raw decrypted data. * @param {string} data - the decrypted data containing raw information. * @param {nsURI} uri - the nsURI of page where the login has occur. * @returns {Object[]} array of objects where each of them contains the username, password, URL, * and creation time representing all the logins found in the data arguments. */ _extractDetails(data, uri) { // the structure of the header of the IE7 decrypted data for all the logins sharing the same URL
--- a/browser/components/migration/MSMigrationUtils.jsm +++ b/browser/components/migration/MSMigrationUtils.jsm @@ -743,17 +743,17 @@ WindowsVaultFormPasswords.prototype = { * Otherwise, check if there are passwords in the vault. * @param {function} aCallback - a callback called when the migration is done. * @param {boolean} [aOnlyCheckExists=false] - if aOnlyCheckExists is true, just check if there are some * passwords to migrate. Import the passwords from the vault and call aCallback otherwise. * @return true if there are passwords in the vault and aOnlyCheckExists is set to true, * false if there is no password in the vault and aOnlyCheckExists is set to true, undefined if * aOnlyCheckExists is set to false. */ - migrate(aCallback, aOnlyCheckExists = false) { + async migrate(aCallback, aOnlyCheckExists = false) { // check if the vault item is an IE/Edge one function _isIEOrEdgePassword(id) { return id[0] == INTERNET_EXPLORER_EDGE_GUID[0] && id[1] == INTERNET_EXPLORER_EDGE_GUID[1] && id[2] == INTERNET_EXPLORER_EDGE_GUID[2] && id[3] == INTERNET_EXPLORER_EDGE_GUID[3]; } @@ -780,16 +780,18 @@ WindowsVaultFormPasswords.prototype = { // enumerate all the available items. This api is going to return a table of all the // available items and item is going to point to the first element of this table. error = ctypesVaultHelpers._functions.VaultEnumerateItems(vault, VAULT_ENUMERATE_ALL_ITEMS, itemCount.address(), item.address()); if (error != RESULT_SUCCESS) { throw new Error("Unable to enumerate Vault items: " + error); } + + let logins = []; for (let j = 0; j < itemCount.value; j++) { try { // if it's not an ie/edge password, skip it if (!_isIEOrEdgePassword(item.contents.schemaId.id)) { continue; } let url = item.contents.pResourceElement.contents.itemValue.readString(); let realURL; @@ -825,36 +827,39 @@ WindowsVaultFormPasswords.prototype = { // to seconds since epoch and multiply to get milliseconds: creation = ctypesKernelHelpers. fileTimeToSecondsSinceEpoch(item.contents.highLastModified, item.contents.lowLastModified) * 1000; } catch (ex) { // Ignore exceptions in the dates and just create the login for right now. } // create a new login - let login = { + logins.push({ username, password, hostname: realURL.prePath, timeCreated: creation, - }; - MigrationUtils.insertLoginWrapper(login); + }); // close current item error = ctypesVaultHelpers._functions.VaultFree(credential); if (error == FREE_CLOSE_FAILED) { throw new Error("Unable to free item: " + error); } } catch (e) { migrationSucceeded = false; Cu.reportError(e); } finally { // move to next item in the table returned by VaultEnumerateItems item = item.increment(); } } + + if (logins.length > 0) { + await MigrationUtils.insertLoginsWrapper(logins); + } } catch (e) { Cu.reportError(e); migrationSucceeded = false; } finally { if (successfulVaultOpen) { // close current vault error = ctypesVaultHelpers._functions.VaultCloseVault(vault); if (error == FREE_CLOSE_FAILED) {
--- a/browser/components/migration/MigrationUtils.jsm +++ b/browser/components/migration/MigrationUtils.jsm @@ -1029,29 +1029,16 @@ this.MigrationUtils = Object.freeze({ insertVisitsWrapper(places, options) { this._importQuantities.history += places.length; if (gKeepUndoData) { this._updateHistoryUndo(places); } return PlacesUtils.asyncHistory.updatePlaces(places, options, true); }, - insertLoginWrapper(login) { - this._importQuantities.logins++; - let insertedLogin = LoginHelper.maybeImportLogin(login); - // Note that this means that if we import a login that has a newer password - // than we know about, we will update the login, and an undo of the import - // will not revert this. This seems preferable over removing the login - // outright or storing the old password in the undo file. - if (insertedLogin && gKeepUndoData) { - let {guid, timePasswordChanged} = insertedLogin; - gUndoData.get("logins").push({guid, timePasswordChanged}); - } - }, - async insertLoginsWrapper(logins) { this._importQuantities.logins += logins.length; let inserted = await LoginHelper.maybeImportLogins(logins); // Note that this means that if we import a login that has a newer password // than we know about, we will update the login, and an undo of the import // will not revert this. This seems preferable over removing the login // outright or storing the old password in the undo file. if (gKeepUndoData) {
--- a/browser/components/migration/tests/unit/test_IE7_passwords.js +++ b/browser/components/migration/tests/unit/test_IE7_passwords.js @@ -111,16 +111,27 @@ const TESTED_WEBSITES = { }, ], }, reddit: { uri: makeURI("http://www.reddit.com/"), hash: "B644028D1C109A91EC2C4B9D1F145E55A1FAE42065", data: [12, 0, 0, 0, 152, 0, 0, 0, 212, 0, 0, 0, 87, 73, 67, 75, 24, 0, 0, 0, 8, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 32, 8, 234, 114, 153, 212, 208, 1, 1, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, 0, 0, 0, 97, 93, 131, 116, 153, 212, 208, 1, 3, 0, 0, 0, 14, 0, 0, 0, 97, 93, 131, 116, 153, 212, 208, 1, 16, 0, 0, 0, 48, 0, 0, 0, 88, 150, 78, 174, 153, 212, 208, 1, 4, 0, 0, 0, 58, 0, 0, 0, 88, 150, 78, 174, 153, 212, 208, 1, 29, 0, 0, 0, 118, 0, 0, 0, 79, 102, 137, 34, 154, 212, 208, 1, 15, 0, 0, 0, 150, 0, 0, 0, 79, 102, 137, 34, 154, 212, 208, 1, 30, 0, 0, 0, 97, 0, 0, 0, 0, 0, 252, 140, 173, 138, 146, 48, 0, 0, 66, 0, 105, 0, 116, 0, 116, 0, 101, 0, 32, 0, 98, 0, 101, 0, 115, 0, 116, 0, 228, 0, 116, 0, 105, 0, 103, 0, 101, 0, 110, 0, 0, 0, 205, 145, 110, 127, 198, 91, 1, 120, 0, 0, 31, 4, 48, 4, 64, 4, 62, 4, 59, 4, 76, 4, 32, 0, 67, 4, 65, 4, 63, 4, 53, 4, 72, 4, 61, 4, 62, 4, 32, 0, 65, 4, 49, 4, 64, 4, 62, 4, 72, 4, 53, 4, 61, 4, 46, 0, 32, 0, 18, 4, 62, 4, 57, 4, 66, 4, 56, 4, 0, 0, 40, 6, 51, 6, 69, 6, 32, 0, 39, 6, 68, 6, 68, 6, 71, 6, 32, 0, 39, 6, 68, 6, 49, 6, 45, 6, 69, 6, 70, 6, 0, 0, 118, 0, 101, 0, 117, 0, 105, 0, 108, 0, 108, 0, 101, 0, 122, 0, 32, 0, 108, 0, 101, 0, 32, 0, 118, 0, 233, 0, 114, 0, 105, 0, 102, 0, 105, 0, 101, 0, 114, 0, 32, 0, 224, 0, 32, 0, 110, 0, 111, 0, 117, 0, 118, 0, 101, 0, 97, 0, 117, 0, 0, 0], logins: [ + // This login is present in the data, but should be stripped out + // by the validation rules of the importer: + // { + // "username": "a", + // "password": "", + // "hostname": "http://www.reddit.com", + // "formSubmitURL": "", + // "httpRealm": null, + // "usernameField": "", + // "passwordField": "" + // }, { username: "購読を", password: "Bitte bestätigen", hostname: "http://www.reddit.com", formSubmitURL: "", httpRealm: null, usernameField: "", passwordField: "", @@ -315,17 +326,17 @@ add_task(async function test_passwordsNo let logins = Services.logins.getAllLogins({}); Assert.equal(logins.length, 0, "There are no logins at the beginning of the test"); let uris = []; // the uris of the migrated logins for (let url of TESTED_URLS) { uris.push(makeURI(url)); // in this test, there is no IE login data in the registry, so after the migration, the number // of logins in the store should be 0 - migrator._migrateURIs(uris); + await migrator._migrateURIs(uris); logins = Services.logins.getAllLogins({}); Assert.equal(logins.length, 0, "There are no logins after doing the migration without adding values to the registry"); } }); add_task(async function test_passwordsAvailable() { if (AppConstants.isPlatformAndVersionAtLeast("win", "6.2")) { @@ -366,17 +377,17 @@ add_task(async function test_passwordsAv // new value with the encrypted data backupAndStore(Storage2Key, website.hash, crypto.encryptData(crypto.arrayToString(website.data), website.uri.spec, true)); Assert.ok(migrator.exists, "The migrator has to exist"); uris.push(website.uri); hashes.push(website.hash); - migrator._migrateURIs(uris); + await migrator._migrateURIs(uris); logins = Services.logins.getAllLogins({}); // check that the number of logins in the password manager has increased as expected which means // that all the values for the current website were imported loginCount += website.logins.length; Assert.equal(logins.length, loginCount, "The number of logins has increased after the migration"); // NB: because telemetry records any login data passed to the login manager, it // also gets told about logins that are duplicates or invalid (for one reason
--- a/browser/components/migration/tests/unit/test_automigration.js +++ b/browser/components/migration/tests/unit/test_automigration.js @@ -163,26 +163,26 @@ add_task(async function checkUndoPrecond getSourceProfiles() { info("Read sourceProfiles"); return null; }, getMigrateData(profileToMigrate) { this._getMigrateDataArgs = profileToMigrate; return Ci.nsIBrowserProfileMigrator.BOOKMARKS; }, - migrate(types, startup, profileToMigrate) { + async migrate(types, startup, profileToMigrate) { this._migrateArgs = [types, startup, profileToMigrate]; if (shouldAddData) { // Insert a login and check that that worked. - MigrationUtils.insertLoginWrapper({ + await MigrationUtils.insertLoginsWrapper([{ hostname: "www.mozilla.org", formSubmitURL: "http://www.mozilla.org", username: "user", password: "pass", - }); + }]); } TestUtils.executeSoon(function() { Services.obs.notifyObservers(null, "Migration:Ended", undefined); }); }, }; gShimmedMigratorKeyPicker = function() { @@ -234,22 +234,22 @@ add_task(async function checkUndoPrecond /** * Fake a migration and then try to undo it to verify all data gets removed. */ add_task(async function checkUndoRemoval() { MigrationUtils.initializeUndoData(); Preferences.set("browser.migrate.automigrate.browser", "automationbrowser"); // Insert a login and check that that worked. - MigrationUtils.insertLoginWrapper({ + await MigrationUtils.insertLoginsWrapper([{ hostname: "www.mozilla.org", formSubmitURL: "http://www.mozilla.org", username: "user", password: "pass", - }); + }]); let storedLogins = Services.logins.findLogins({}, "www.mozilla.org", "http://www.mozilla.org", null); Assert.equal(storedLogins.length, 1, "Should have 1 login"); // Insert a bookmark and check that we have exactly 1 bookmark for that URI. await MigrationUtils.insertBookmarkWrapper({ parentGuid: PlacesUtils.bookmarks.toolbarGuid, url: "http://www.example.org/", @@ -436,56 +436,56 @@ add_task(async function testBookmarkRemo let dbgStr = dbResultForGuid && dbResultForGuid.title; Assert.equal(null, dbResultForGuid, "Should not be able to find items that should have been removed, but found " + dbgStr); } await PlacesUtils.bookmarks.eraseEverything(); }); add_task(async function checkUndoLoginsState() { MigrationUtils.initializeUndoData(); - MigrationUtils.insertLoginWrapper({ + await MigrationUtils.insertLoginsWrapper([{ username: "foo", password: "bar", hostname: "https://example.com", formSubmitURL: "https://example.com/", timeCreated: new Date(), - }); + }]); let storedLogins = Services.logins.findLogins({}, "https://example.com", "", ""); let storedLogin = storedLogins[0]; storedLogin.QueryInterface(Ci.nsILoginMetaInfo); let {guid, timePasswordChanged} = storedLogin; let undoLoginData = (await MigrationUtils.stopAndRetrieveUndoData()).get("logins"); Assert.deepEqual([{guid, timePasswordChanged}], undoLoginData); Services.logins.removeAllLogins(); }); add_task(async function testLoginsRemovalByUndo() { MigrationUtils.initializeUndoData(); - MigrationUtils.insertLoginWrapper({ + await MigrationUtils.insertLoginsWrapper([{ username: "foo", password: "bar", hostname: "https://example.com", formSubmitURL: "https://example.com/", timeCreated: new Date(), - }); - MigrationUtils.insertLoginWrapper({ + }]); + await MigrationUtils.insertLoginsWrapper([{ username: "foo", password: "bar", hostname: "https://example.org", formSubmitURL: "https://example.org/", timeCreated: new Date(new Date().getTime() - 10000), - }); + }]); // This should update the existing login - LoginHelper.maybeImportLogin({ + await LoginHelper.maybeImportLogins([{ username: "foo", password: "bazzy", hostname: "https://example.org", formSubmitURL: "https://example.org/", timePasswordChanged: new Date(), - }); + }]); Assert.equal(1, LoginHelper.searchLoginsWithObject({hostname: "https://example.org", formSubmitURL: "https://example.org/"}).length, "Should be only 1 login for example.org (that was updated)"); let undoLoginData = (await MigrationUtils.stopAndRetrieveUndoData()).get("logins"); await AutoMigrate._removeUnchangedLogins(undoLoginData); Assert.equal(0, LoginHelper.searchLoginsWithObject({hostname: "https://example.com", formSubmitURL: "https://example.com/"}).length, "unchanged example.com entry should have been removed."); Assert.equal(1, LoginHelper.searchLoginsWithObject({hostname: "https://example.org", formSubmitURL: "https://example.org/"}).length,
--- a/toolkit/components/passwordmgr/LoginHelper.jsm +++ b/toolkit/components/passwordmgr/LoginHelper.jsm @@ -203,61 +203,16 @@ this.LoginHelper = { // newURI will throw for some values e.g. chrome://FirefoxAccounts return false; } } return false; }, - /** - * Helper to check if there are any duplicates of the existing login. If the - * duplicates need to have the password updated, this performs that update. - * - * @param {nsILoginInfo} aLogin - The login to search for. - * @return {boolean} - true if duplicates exist, otherwise false. - */ - checkForDuplicatesAndMaybeUpdate(aLogin) { - // 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({}, aLogin.hostname, - aLogin.formSubmitURL, - aLogin.httpRealm); - // 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 => aLogin.matches(l, false /* ignorePassword */))) { - return true; - } - // 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 (aLogin.username == existingLogin.username) { - foundMatchingLogin = true; - existingLogin.QueryInterface(Ci.nsILoginMetaInfo); - if (aLogin.password != existingLogin.password & - aLogin.timePasswordChanged > existingLogin.timePasswordChanged) { - // if a login with the same username and different password already exists and it's older - // than the current one, update its password and timestamp. - let propBag = Cc["@mozilla.org/hash-property-bag;1"]. - createInstance(Ci.nsIWritablePropertyBag); - propBag.setProperty("password", aLogin.password); - propBag.setProperty("timePasswordChanged", aLogin.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 true; - } - - return false; - }, - doLoginsMatch(aLogin1, aLogin2, { ignorePassword = false, ignoreSchemes = false, }) { if (aLogin1.httpRealm != aLogin2.httpRealm || aLogin1.username != aLogin2.username) return false; @@ -586,47 +541,19 @@ this.LoginHelper = { fieldType == "tel" || fieldType == "number") { return true; } 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.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; - if (this.checkForDuplicatesAndMaybeUpdate(login)) { - return null; - } - return Services.logins.addLogin(login); - }, - - /** - * Equivalent to maybeImportLogin, except asynchronous and for multiple logins. + * For each login, 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[]} loginDatas - For each login, the data that needs to be added. * @returns {nsILoginInfo[]} the newly added logins, filtered if no login was added. */ async maybeImportLogins(loginDatas) { let loginsToAdd = []; let loginMap = new Map(); for (let loginData of loginDatas) { // create a new login @@ -650,19 +577,18 @@ this.LoginHelper = { // out from the bulk APIs below us. this.checkLoginValues(login); } catch (e) { Cu.reportError(e); continue; } // First, we need to check the logins that we've already decided to add, to - // see if this is a duplicate. This should mirror the logic inside - // checkForDuplicatesAndMaybeUpdate, but only for the array of logins we're - // adding. + // see if this is a duplicate. This should mirror the logic below for + // existingLogins, but only for the array of logins we're adding. let newLogins = loginMap.get(login.hostname) || []; if (!newLogins) { loginMap.set(login.hostname, newLogins); } else { if (newLogins.some(l => login.matches(l, false /* ignorePassword */))) { continue; } let foundMatchingNewLogin = false; @@ -680,23 +606,56 @@ this.LoginHelper = { } } if (foundMatchingNewLogin) { continue; } } - if (this.checkForDuplicatesAndMaybeUpdate(login)) { + // 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); + // 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 */))) { + continue; + } + // 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) { + 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, 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) { continue; } newLogins.push(login); loginsToAdd.push(login); } + if (!loginsToAdd.length) { + return []; + } return Services.logins.addLogins(loginsToAdd); }, /** * 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/nsLoginManager.js +++ b/toolkit/components/passwordmgr/nsLoginManager.js @@ -238,36 +238,23 @@ LoginManager.prototype = { let passwordsCountHistogram = clearAndGetHistogram("PWMGR_NUM_PASSWORDS_PER_HOSTNAME"); for (let count of hostnameCount.values()) { passwordsCountHistogram.add(count); } }, - - - - /* ---------- Primary Public interfaces ---------- */ - - - - /** - * @type Promise - * This promise is resolved when initialization is complete, and is rejected - * in case the asynchronous part of initialization failed. + * Ensures that a login isn't missing any necessary fields. + * + * @param login + * The login to check. */ - initializationPromise: null, - - - /** - * Add a new login to login storage. - */ - addLogin(login) { + _checkLogin(login) { // Sanity check the login if (login.hostname == null || login.hostname.length == 0) { throw new Error("Can't add a login with a null or empty hostname."); } // For logins w/o a username, set to "", not null. if (login.username == null) { throw new Error("Can't add a login with a null username."); @@ -286,18 +273,40 @@ LoginManager.prototype = { // We have a HTTP realm. Can't have a form submit URL. if (login.formSubmitURL != null) { throw new Error("Can't add a login with both a httpRealm and formSubmitURL."); } } else { // Need one or the other! throw new Error("Can't add a login without a httpRealm or formSubmitURL."); } + }, + + + /* ---------- Primary Public interfaces ---------- */ + + + + + /** + * @type Promise + * This promise is resolved when initialization is complete, and is rejected + * in case the asynchronous part of initialization failed. + */ + initializationPromise: null, + + + /** + * Add a new login to login storage. + */ + addLogin(login) { + this._checkLogin(login); + // Look for an existing entry. var logins = this.findLogins({}, login.hostname, login.formSubmitURL, login.httpRealm); if (logins.some(l => login.matches(l, true))) { throw new Error("This login already exists."); } @@ -307,27 +316,40 @@ LoginManager.prototype = { async addLogins(logins) { let crypto = Cc["@mozilla.org/login-manager/crypto/SDR;1"]. getService(Ci.nsILoginManagerCrypto); let plaintexts = logins.map(l => l.username).concat(logins.map(l => l.password)); let ciphertexts = await crypto.encryptMany(plaintexts); let usernames = ciphertexts.slice(0, logins.length); let passwords = ciphertexts.slice(logins.length); + let resultLogins = []; for (let i = 0; i < logins.length; i++) { + try { + this._checkLogin(logins[i]); + } catch (e) { + Cu.reportError(e); + continue; + } + let plaintextUsername = logins[i].username; let plaintextPassword = logins[i].password; logins[i].username = usernames[i]; logins[i].password = passwords[i]; log.debug("Adding login"); - this._storage.addLogin(logins[i], true); + let resultLogin = this._storage.addLogin(logins[i], true); // Reset the username and password to keep the same guarantees as addLogin logins[i].username = plaintextUsername; logins[i].password = plaintextPassword; + + resultLogin.username = plaintextUsername; + resultLogin.password = plaintextPassword; + resultLogins.push(resultLogin); } + return resultLogins; }, /** * Remove the specified login from the stored logins. */ removeLogin(login) { log.debug("Removing login"); return this._storage.removeLogin(login);
--- a/toolkit/components/passwordmgr/test/unit/test_maybeImportLogin.js +++ b/toolkit/components/passwordmgr/test/unit/test_maybeImportLogin.js @@ -8,162 +8,162 @@ 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({ +add_task(async function test_new_logins() { + let [importedLogin] = await LoginHelper.maybeImportLogins([{ 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({ + [importedLogin] = await LoginHelper.maybeImportLogins([{ 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({ +add_task(async function test_duplicate_logins() { + let [importedLogin] = await LoginHelper.maybeImportLogins([{ 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({ + [importedLogin] = await LoginHelper.maybeImportLogins([{ 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({ +add_task(async function test_different_passwords() { + let [importedLogin] = await LoginHelper.maybeImportLogins([{ 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({ + [importedLogin] = await LoginHelper.maybeImportLogins([{ 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({ + [importedLogin] = await LoginHelper.maybeImportLogins([{ 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({ +add_task(async function test_different_usernames() { + let [importedLogin] = await LoginHelper.maybeImportLogins([{ 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({ + [importedLogin] = await LoginHelper.maybeImportLogins([{ 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({ +add_task(async function test_different_targets() { + let [importedLogin] = await LoginHelper.maybeImportLogins([{ 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({ + [importedLogin] = await LoginHelper.maybeImportLogins([{ 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({ + [importedLogin] = await LoginHelper.maybeImportLogins([{ 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(); });