corysanin/tubestation
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 1433593 - Clean up usages of LoginHelper.maybeImportLogin r=MattN

Browse Source 
In bug 1426721 we added a bulk interface for importing logins, which
works in a background thread. This patch cleans up the single-login
interface and updates the remaining usages to consume the bulk
interface.

MozReview-Commit-ID: IziLXkO5dxQ
This commit is contained in:
Doug Thayer
2018-02-15 10:26:44 -08:00
parent e49ed57c0a
commit 38979fb27e
7 changed files with 112 additions and 165 deletions
Download Patch File Download Diff File Expand all files Collapse all files

42
browser/components/migration/IEProfileMigrator.js
View File

@@ -122,7 +122,7 @@ IE7FormPasswords.prototype = {
}
},
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
@@ -138,7 +138,7 @@ IE7FormPasswords.prototype = {
uris.push(uri);
}
this._migrateURIs(uris);
await this._migrateURIs(uris);
aCallback(true);
},
@@ -146,7 +146,7 @@ IE7FormPasswords.prototype = {
* 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;
@@ -166,6 +166,7 @@ IE7FormPasswords.prototype = {
* to the Firefox password manager.
*/
let logins = [];
for (let uri of uris) {
try {
// remove the query and the ref parts of the URL
@@ -200,11 +201,23 @@ IE7FormPasswords.prototype = {
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) {
@@ -221,27 +234,6 @@ IE7FormPasswords.prototype = {
_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.

13
browser/components/migration/MSMigrationUtils.jsm
View File

@@ -748,7 +748,7 @@ WindowsVaultFormPasswords.prototype = {
* 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] &&
@@ -785,6 +785,8 @@ WindowsVaultFormPasswords.prototype = {
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
@@ -830,12 +832,11 @@ WindowsVaultFormPasswords.prototype = {
// 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);
@@ -850,6 +851,10 @@ WindowsVaultFormPasswords.prototype = {
item = item.increment();
}
}
if (logins.length > 0) {
await MigrationUtils.insertLoginsWrapper(logins);
}
} catch (e) {
Cu.reportError(e);
migrationSucceeded = false;

13
browser/components/migration/MigrationUtils.jsm
View File

@@ -1034,19 +1034,6 @@ this.MigrationUtils = Object.freeze({
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);

26
browser/components/migration/tests/unit/test_automigration.js
View File

@@ -168,16 +168,16 @@ add_task(async function checkUndoPreconditions() {
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);
@@ -239,12 +239,12 @@ 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");
@@ -441,13 +441,13 @@ add_task(async function testBookmarkRemovalByUndo() {
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);
@@ -459,28 +459,28 @@ add_task(async function checkUndoLoginsState() {
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");

119
toolkit/components/passwordmgr/LoginHelper.jsm
View File

@@ -208,51 +208,6 @@ this.LoginHelper = {
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,
@@ -591,37 +546,9 @@ this.LoginHelper = {
},
/**
* 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.
*/
@@ -655,9 +582,8 @@ this.LoginHelper = {
}
// 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);
@@ -685,13 +611,46 @@ this.LoginHelper = {
}
}
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);
},

6
toolkit/components/passwordmgr/nsLoginManager.js
View File

@@ -312,17 +312,21 @@ LoginManager.prototype = {
let ciphertexts = await crypto.encryptMany(plaintexts);
let usernames = ciphertexts.slice(0, logins.length);
let passwords = ciphertexts.slice(logins.length);
let resultLogins = new Array(logins.length);
for (let i = 0; i < logins.length; i++) {
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);
resultLogins[i] = 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;
resultLogins[i].username = plaintextUsername;
resultLogins[i].password = plaintextPassword;
}
return resultLogins;
},
/**

58
toolkit/components/passwordmgr/test/unit/test_maybeImportLogin.js
View File

@@ -13,23 +13,23 @@ 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});
@@ -41,62 +41,62 @@ add_task(function test_new_logins() {
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}`);
@@ -105,23 +105,23 @@ add_task(function test_different_passwords() {
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}`);
@@ -129,36 +129,36 @@ add_task(function test_different_usernames() {
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});