Bug 1285577 - part 2: keep track of added logins in an import and allow removing them, r=MattN a=lizzard
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 30 Nov 2016 11:50:38 +0000
changeset 353346 cba88336e84fb0e1f68798b2a80b9156e4887ff1
parent 353345 1b044befe15a91d611ec5cb24f72fcf8e21a5396
child 353347 81bd0d4207b25329b4a05143a13de8ed5c7922c7
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-esr52@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN, lizzard
bugs1285577
milestone52.0a2
Bug 1285577 - part 2: keep track of added logins in an import and allow removing them, r=MattN a=lizzard MozReview-Commit-ID: JZbOkmZ7ZZG
browser/components/migration/AutoMigrate.jsm
browser/components/migration/MigrationUtils.jsm
browser/components/migration/tests/unit/test_automigration.js
--- a/browser/components/migration/AutoMigrate.jsm
+++ b/browser/components/migration/AutoMigrate.jsm
@@ -28,16 +28,18 @@ const kSyncTopic = "fxaccounts:onlogin";
 
 const kNotificationId = "abouthome-automigration-undo";
 
 Cu.import("resource:///modules/MigrationUtils.jsm");
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "LoginHelper",
+                                  "resource://gre/modules/LoginHelper.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
 
 const AutoMigrate = {
   get resourceTypesToUse() {
     let {BOOKMARKS, HISTORY, PASSWORDS} = Ci.nsIBrowserProfileMigrator;
     return BOOKMARKS | HISTORY | PASSWORDS;
   },
@@ -433,14 +435,27 @@ const AutoMigrate = {
       yield PlacesUtils.bookmarks.remove(guid, {preventRemovalOfNonEmptyFolders: true}).catch(err => {
         if (err && err.message != "Cannot remove a non-empty folder.") {
           Cu.reportError(err);
         }
       });
     }
   }),
 
+  _removeUnchangedLogins: Task.async(function* (logins) {
+    for (let login of logins) {
+      let foundLogins = LoginHelper.searchLoginsWithObject({guid: login.guid});
+      if (foundLogins.length) {
+        let foundLogin = foundLogins[0];
+        foundLogin.QueryInterface(Ci.nsILoginMetaInfo);
+        if (foundLogin.timePasswordChanged == login.timePasswordChanged) {
+          Services.logins.removeLogin(foundLogin);
+        }
+      }
+    }
+  }),
+
   QueryInterface: XPCOMUtils.generateQI(
     [Ci.nsIObserver, Ci.nsINavBookmarkObserver, Ci.nsISupportsWeakReference]
   ),
 };
 
 AutoMigrate.init();
--- a/browser/components/migration/MigrationUtils.jsm
+++ b/browser/components/migration/MigrationUtils.jsm
@@ -969,17 +969,25 @@ this.MigrationUtils = Object.freeze({
 
   insertVisitsWrapper(places, options) {
     this._importQuantities.history += places.length;
     return PlacesUtils.asyncHistory.updatePlaces(places, options);
   },
 
   insertLoginWrapper(login) {
     this._importQuantities.logins++;
-    return LoginHelper.maybeImportLogin(login);
+    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});
+    }
   },
 
   initializeUndoData() {
     gKeepUndoData = true;
     gUndoData = new Map([["bookmarks", []], ["visits", new Map()], ["logins", []]]);
   },
 
   _postProcessUndoData: Task.async(function*(state) {
--- a/browser/components/migration/tests/unit/test_automigration.js
+++ b/browser/components/migration/tests/unit/test_automigration.js
@@ -1,11 +1,12 @@
 "use strict";
 
 Cu.import("resource:///modules/MigrationUtils.jsm");
+Cu.import("resource://gre/modules/LoginHelper.jsm");
 Cu.import("resource://gre/modules/PlacesUtils.jsm");
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://testing-common/TestUtils.jsm");
 Cu.import("resource://testing-common/PlacesTestUtils.jsm");
 let AutoMigrateBackstage = Cu.import("resource:///modules/AutoMigrate.jsm"); /* globals AutoMigrate */
 
 let gShimmedMigratorKeyPicker = null;
 let gShimmedMigrator = null;
@@ -379,8 +380,63 @@ add_task(function* testBookmarkRemovalBy
   Assert.ok(itemFromDB, "Folder we inserted in migration is still there because of new kids.");
   for (let removedGuid of itemsToRemove) {
     let dbResultForGuid = yield PlacesUtils.bookmarks.fetch(removedGuid);
     let dbgStr = dbResultForGuid && dbResultForGuid.title;
     Assert.equal(null, dbResultForGuid, "Should not be able to find items that should have been removed, but found " + dbgStr);
   }
   yield PlacesUtils.bookmarks.eraseEverything();
 });
+
+add_task(function* checkUndoLoginsState() {
+  MigrationUtils.initializeUndoData();
+  MigrationUtils.insertLoginWrapper({
+    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 = (yield MigrationUtils.stopAndRetrieveUndoData()).get("logins");
+  Assert.deepEqual([{guid, timePasswordChanged}], undoLoginData);
+  Services.logins.removeAllLogins();
+});
+
+add_task(function* testLoginsRemovalByUndo() {
+  MigrationUtils.initializeUndoData();
+  MigrationUtils.insertLoginWrapper({
+    username: "foo",
+    password: "bar",
+    hostname: "https://example.com",
+    formSubmitURL: "https://example.com/",
+    timeCreated: new Date(),
+  });
+  MigrationUtils.insertLoginWrapper({
+    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({
+    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 = (yield MigrationUtils.stopAndRetrieveUndoData()).get("logins");
+
+  yield 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,
+                   "changed example.org entry should have persisted.");
+  Services.logins.removeAllLogins();
+});
+