Bug 1582780 - Dont modify a empty-username login onGeneratedPasswordEditedOrFilled unless it is the auto-saved login. r=MattN a=lizzard
authorSam Foster <sfoster@mozilla.com>
Mon, 23 Sep 2019 20:19:54 +0000
changeset 555355 d577eddf788820e8c6fbf527d6f9807ffe5acc38
parent 555354 31d0b6ad87ec2998f0ec8f647c61a03fceb86fc9
child 555356 480bfb1bbe281d42699cd234f307c4f2edcde941
push id2165
push userffxbld-merge
push dateMon, 14 Oct 2019 16:30:58 +0000
treeherdermozilla-release@0eae18af659f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN, lizzard
bugs1582780
milestone70.0
Bug 1582780 - Dont modify a empty-username login onGeneratedPasswordEditedOrFilled unless it is the auto-saved login. r=MattN a=lizzard Differential Revision: https://phabricator.services.mozilla.com/D46679
toolkit/components/passwordmgr/LoginManagerParent.jsm
toolkit/components/passwordmgr/test/browser/browser_generated_password_doorhangers.js
--- a/toolkit/components/passwordmgr/LoginManagerParent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ -758,18 +758,19 @@ this.LoginManagerParent = {
     }
 
     let framePrincipalOrigin =
       browsingContext.currentWindowGlobal.documentPrincipal.origin;
     let generatedPW = this._generatedPasswordsByPrincipalOrigin.get(
       framePrincipalOrigin
     );
 
-    let autoSaveLogin = true;
+    let shouldAutoSaveLogin = true;
     let loginToChange = null;
+    let autoSavedLogin = null;
 
     if (password != generatedPW.value) {
       // The user edited the field after generation to a non-empty value.
       log("The field containing the generated password has changed");
 
       // Record telemetry for the first edit
       if (!generatedPW.edited) {
         Services.telemetry.recordEvent(
@@ -784,17 +785,21 @@ this.LoginManagerParent = {
       // The edit was to a login that was auto-saved.
       // Note that it could have been saved in a totally different tab in the session.
       if (generatedPW.storageGUID) {
         let existingLogins = LoginHelper.searchLoginsWithObject({
           guid: generatedPW.storageGUID,
         });
 
         if (existingLogins.length) {
+          log(
+            "_onGeneratedPasswordFilledOrEdited: login to change is the auto-saved login"
+          );
           loginToChange = existingLogins[0];
+          autoSavedLogin = loginToChange;
         }
         // The generated password login may have been deleted in the meantime.
         // Proceed to maybe save a new login below.
       }
 
       generatedPW.value = password;
     }
 
@@ -840,17 +845,17 @@ this.LoginManagerParent = {
         httpRealm: null,
         ignoreActionAndRealm: false,
       });
 
       let matchedLogin = logins.find(login =>
         formLoginWithoutUsername.matches(login, true)
       );
       if (matchedLogin) {
-        autoSaveLogin = false;
+        shouldAutoSaveLogin = false;
         if (matchedLogin.password == formLoginWithoutUsername.password) {
           // This login is already saved so show no new UI.
           log(
             "_onGeneratedPasswordFilledOrEdited: Matching login already saved"
           );
           return;
         }
         log(
@@ -861,20 +866,20 @@ this.LoginManagerParent = {
       if (
         (matchedLogin = logins.find(login => formLogin.matches(login, true)))
       ) {
         // We're updating a previously-saved login
         loginToChange = matchedLogin;
       }
     }
 
-    if (autoSaveLogin) {
-      if (loginToChange) {
+    if (shouldAutoSaveLogin) {
+      if (loginToChange && loginToChange == autoSavedLogin) {
         log(
-          "_onGeneratedPasswordFilledOrEdited: auto-updating login with generated password"
+          "_onGeneratedPasswordFilledOrEdited: updating auto-saved login with changed password"
         );
 
         Services.logins.modifyLogin(
           loginToChange,
           LoginHelper.newPropertyBag({
             password,
           })
         );
@@ -912,26 +917,26 @@ this.LoginManagerParent = {
       log(
         "_onGeneratedPasswordFilledOrEdited: promptToChangePassword with autoSavedStorageGUID: " +
           autoSavedStorageGUID
       );
       prompter.promptToChangePassword(
         loginToChange,
         formLogin,
         true, // dismissed prompt
-        autoSaveLogin, // notifySaved
+        shouldAutoSaveLogin, // notifySaved
         autoSavedStorageGUID // autoSavedLoginGuid
       );
       return;
     }
     log("_onGeneratedPasswordFilledOrEdited: no matching login to save/update");
     prompter.promptToSavePassword(
       formLogin,
       true, // dismissed prompt
-      autoSaveLogin // notifySaved
+      shouldAutoSaveLogin // notifySaved
     );
   },
 
   /**
    * Maps all the <browser> elements for tabs in the parent process to the
    * current state used to display tab-specific UI.
    *
    * This mapping is not updated in case a web page is moved to a different
--- a/toolkit/components/passwordmgr/test/browser/browser_generated_password_doorhangers.js
+++ b/toolkit/components/passwordmgr/test/browser/browser_generated_password_doorhangers.js
@@ -418,16 +418,109 @@ add_task(async function autocomplete_gen
           username: "",
         },
       ]);
       await cleanupDoorhanger(notif); // cleanup the doorhanger for next test
     }
   );
 });
 
+add_task(async function autocomplete_generated_password_saved_username() {
+  // confirm behavior when filling a generated password via autocomplete
+  // into a form with username matching an existing saved login
+  await setup_withOneLogin("user1", "xyzpassword");
+  await openFormInNewTab(
+    TEST_ORIGIN + FORM_PAGE_PATH,
+    {
+      password: {
+        selector: passwordInputSelector,
+        expectedValue: "xyzpassword",
+        setValue: "",
+      },
+      username: {
+        selector: usernameInputSelector,
+        expectedValue: "user1",
+      },
+    },
+    async function taskFn(browser) {
+      let storageChangedPromise = TestUtils.topicObserved(
+        "passwordmgr-storage-changed",
+        (_, data) => data == "addLogin"
+      );
+      let confirmationHint = document.getElementById("confirmation-hint");
+      let hintPromiseShown = BrowserTestUtils.waitForEvent(
+        confirmationHint,
+        "popupshown"
+      );
+      await fillGeneratedPasswordFromACPopup(browser, passwordInputSelector);
+      info("waiting for addLogin");
+      await storageChangedPromise;
+      await verifyGeneratedPasswordWasFilled(browser, passwordInputSelector);
+      // Make sure confirmation hint was shown
+      await hintPromiseShown;
+      await verifyConfirmationHint(confirmationHint);
+
+      // Check properties of the newly auto-saved login
+      let [user1LoginSnapshot, autoSavedLogin] = verifyLogins([
+        {
+          username: "user1",
+          password: "xyzpassword", // user1 is unchanged
+        },
+        {
+          timesUsed: 1,
+          username: "",
+          passwordLength: LoginTestUtils.generation.LENGTH,
+        },
+      ]);
+
+      let notif = await openAndVerifyDoorhanger(browser, "password-change", {
+        dismissed: true,
+        anchorExtraAttr: "attention",
+        usernameValue: "user1",
+        passwordLength: LoginTestUtils.generation.LENGTH,
+      });
+
+      let promiseHidden = BrowserTestUtils.waitForEvent(
+        PopupNotifications.panel,
+        "popuphidden"
+      );
+      clickDoorhangerButton(notif, DONT_CHANGE_BUTTON);
+      await promiseHidden;
+
+      // confirm the extraAttr attribute is removed after opening & dismissing the doorhanger
+      ok(
+        !notif.anchorElement.hasAttribute("extraAttr"),
+        "Check if the extraAttr attribute was removed"
+      );
+      await cleanupDoorhanger(notif);
+
+      storageChangedPromise = TestUtils.topicObserved(
+        "passwordmgr-storage-changed",
+        (_, data) => data == "modifyLogin"
+      );
+      info("waiting for submitForm");
+      await submitForm(browser);
+      promiseHidden = BrowserTestUtils.waitForEvent(
+        PopupNotifications.panel,
+        "popuphidden"
+      );
+      clickDoorhangerButton(notif, CHANGE_BUTTON);
+      await promiseHidden;
+      await storageChangedPromise;
+      verifyLogins([
+        {
+          timesUsed: user1LoginSnapshot.timesUsed + 1,
+          username: "user1",
+          password: autoSavedLogin.password,
+        },
+      ]);
+    }
+  );
+});
+
 add_task(async function ac_gen_pw_saved_empty_un_stored_non_empty_un_in_form() {
   // confirm behavior when when the form's username field has a non-empty value
   // and there is an existing saved login with a "" username
   await setup_withOneLogin("", "xyzpassword");
   await openFormInNewTab(
     TEST_ORIGIN + FORM_PAGE_PATH,
     {
       password: {
@@ -576,16 +669,19 @@ add_task(async function autocomplete_gen
     },
     async function taskFn(browser) {
       let [savedLogin] = Services.logins.getAllLogins();
       let storageChangedPromise = TestUtils.topicObserved(
         "passwordmgr-storage-changed",
         (_, data) => data == "modifyLogin"
       );
       await fillGeneratedPasswordFromACPopup(browser, passwordInputSelector);
+      info(
+        "Filled generated password, waiting for dismissed password-change doorhanger"
+      );
       await waitForDoorhanger(browser, "password-change");
       info("Waiting to openAndVerifyDoorhanger");
       let notif = await openAndVerifyDoorhanger(browser, "password-change", {
         dismissed: true,
         anchorExtraAttr: "",
         usernameValue: "",
         passwordLength: LoginTestUtils.generation.LENGTH,
       });