Bug 1598717 - Hide the "Firefox will save this password for this website" text when we wouldn't auto-save due to an existing login. r=sfoster
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Wed, 04 Dec 2019 21:34:08 +0000
changeset 505571 c1aa563394ec0c856df2cc7ef50e3c1b2d3e8085
parent 505570 499b0c27da4fb10f06b17b0b6ad433ed9563412b
child 505572 daf59b9083fb24d85cabe7c19f0a47d62b53cbb4
push id102349
push usermozilla@noorenberghe.ca
push dateWed, 04 Dec 2019 23:41:22 +0000
treeherderautoland@58bfe894a732 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfoster
bugs1598717
milestone73.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1598717 - Hide the "Firefox will save this password for this website" text when we wouldn't auto-save due to an existing login. r=sfoster Differential Revision: https://phabricator.services.mozilla.com/D55741
toolkit/components/passwordmgr/LoginAutoCompleteResult.jsm
toolkit/components/passwordmgr/LoginManagerChild.jsm
toolkit/components/passwordmgr/LoginManagerParent.jsm
toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_doAutocompleteSearch.js
toolkit/components/passwordmgr/test/unit/test_login_autocomplete_result.js
toolkit/content/widgets/autocomplete-richlistitem.js
--- a/toolkit/components/passwordmgr/LoginAutoCompleteResult.jsm
+++ b/toolkit/components/passwordmgr/LoginAutoCompleteResult.jsm
@@ -191,19 +191,24 @@ class LoginAutocompleteItem extends Auto
       });
     } else {
       Services.logins.removeLogin(this._login);
     }
   }
 }
 
 class GeneratedPasswordAutocompleteItem extends AutocompleteItem {
-  constructor(generatedPassword) {
+  constructor(generatedPassword, willAutoSaveGeneratedPassword) {
     super("generatedPassword");
-    this.comment = generatedPassword;
+    XPCOMUtils.defineLazyGetter(this, "comment", () => {
+      return JSON.stringify({
+        generatedPassword,
+        willAutoSaveGeneratedPassword,
+      });
+    });
     this.value = generatedPassword;
 
     XPCOMUtils.defineLazyGetter(this, "label", () => {
       return getLocalizedString("useASecurelyGeneratedPassword");
     });
   }
 }
 
@@ -218,17 +223,24 @@ class LoginsFooterAutocompleteItem exten
   }
 }
 
 // nsIAutoCompleteResult implementation
 function LoginAutoCompleteResult(
   aSearchString,
   matchingLogins,
   formOrigin,
-  { generatedPassword, isSecure, actor, isPasswordField, hostname }
+  {
+    generatedPassword,
+    willAutoSaveGeneratedPassword,
+    isSecure,
+    actor,
+    isPasswordField,
+    hostname,
+  }
 ) {
   let hidingFooterOnPWFieldAutoOpened = false;
   function isFooterEnabled() {
     // We need to check LoginHelper.enabled here since the insecure warning should
     // appear even if pwmgr is disabled but the footer should never appear in that case.
     if (!LoginHelper.showAutoCompleteFooter || !LoginHelper.enabled) {
       return false;
     }
@@ -286,17 +298,22 @@ function LoginAutoCompleteResult(
       })
     );
     this._rows.push(item);
   }
 
   // The footer comes last if it's enabled
   if (isFooterEnabled()) {
     if (generatedPassword) {
-      this._rows.push(new GeneratedPasswordAutocompleteItem(generatedPassword));
+      this._rows.push(
+        new GeneratedPasswordAutocompleteItem(
+          generatedPassword,
+          willAutoSaveGeneratedPassword
+        )
+      );
     }
     this._rows.push(new LoginsFooterAutocompleteItem(hostname));
   }
 
   // Determine the result code and default index.
   if (this.matchCount > 0) {
     this.searchResult = Ci.nsIAutoCompleteResult.RESULT_SUCCESS;
     this.defaultIndex = 0;
@@ -438,33 +455,34 @@ LoginAutoComplete.prototype = {
     }
     let isPasswordField = aElement.type == "password";
     let hostname = aElement.ownerDocument.documentURIObject.host;
 
     let loginManagerActor = LoginManagerChild.forWindow(aElement.ownerGlobal);
 
     let completeSearch = (
       autoCompleteLookupPromise,
-      { generatedPassword, logins }
+      { generatedPassword, logins, willAutoSaveGeneratedPassword }
     ) => {
       // If the search was canceled before we got our
       // results, don't bother reporting them.
       if (this._autoCompleteLookupPromise !== autoCompleteLookupPromise) {
         return;
       }
       let formOrigin = LoginHelper.getLoginOrigin(
         aElement.ownerDocument.documentURI
       );
       this._autoCompleteLookupPromise = null;
       let results = new LoginAutoCompleteResult(
         aSearchString,
         logins,
         formOrigin,
         {
           generatedPassword,
+          willAutoSaveGeneratedPassword,
           actor: loginManagerActor,
           isSecure,
           isPasswordField,
           hostname,
         }
       );
       aCallback.onSearchCompletion(results);
     };
--- a/toolkit/components/passwordmgr/LoginManagerChild.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerChild.jsm
@@ -627,16 +627,17 @@ this.LoginManagerChild = class LoginMana
       "PasswordManager:autoCompleteLogins",
       messageData
     );
 
     return resultPromise.then(result => {
       return {
         generatedPassword: result.generatedPassword,
         logins: LoginHelper.vanillaObjectsToLogins(result.logins),
+        willAutoSaveGeneratedPassword: result.willAutoSaveGeneratedPassword,
       };
     });
   }
 
   setupProgressListener(window) {
     if (!LoginHelper.formlessCaptureEnabled) {
       return;
     }
--- a/toolkit/components/passwordmgr/LoginManagerParent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ -446,29 +446,42 @@ class LoginManagerParent extends JSWindo
       // for on password fields.
       if (isPasswordField) {
         return true;
       }
       return match && match.toLowerCase().startsWith(searchStringLower);
     });
 
     let generatedPassword = null;
+    let willAutoSaveGeneratedPassword = false;
     if (
       forcePasswordGeneration ||
       (isPasswordField &&
         autocompleteInfo.fieldName == "new-password" &&
         Services.logins.getLoginSavingEnabled(formOrigin))
     ) {
       generatedPassword = this.getGeneratedPassword();
+      let potentialConflictingLogins = LoginHelper.searchLoginsWithObject({
+        origin: formOrigin,
+        formActionOrigin: actionOrigin,
+        httpRealm: null,
+      });
+      willAutoSaveGeneratedPassword = !potentialConflictingLogins.find(
+        login => login.username == ""
+      );
     }
 
     // Convert the array of nsILoginInfo to vanilla JS objects since nsILoginInfo
     // doesn't support structured cloning.
     let jsLogins = LoginHelper.loginsToVanillaObjects(matchingLogins);
-    return { generatedPassword, logins: jsLogins };
+    return {
+      generatedPassword,
+      logins: jsLogins,
+      willAutoSaveGeneratedPassword,
+    };
   }
 
   /**
    * Expose `BrowsingContext` so we can stub it in tests.
    */
   static get _browsingContextGlobal() {
     return BrowsingContext;
   }
--- a/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_doAutocompleteSearch.js
+++ b/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_doAutocompleteSearch.js
@@ -4,93 +4,130 @@
 
 "use strict";
 
 const { sinon } = ChromeUtils.import("resource://testing-common/Sinon.jsm");
 const { LoginManagerParent } = ChromeUtils.import(
   "resource://gre/modules/LoginManagerParent.jsm"
 );
 
-add_task(async function test_doAutocompleteSearch_generated_noLogins() {
-  Services.prefs.setBoolPref("signon.generation.available", true); // TODO: test both with false
-  Services.prefs.setBoolPref("signon.generation.enabled", true);
-
-  let LMP = new LoginManagerParent();
-  LMP.useBrowsingContext(123);
-
-  ok(LMP.doAutocompleteSearch, "doAutocompleteSearch exists");
+// new-password to the happy path
+const NEW_PASSWORD_TEMPLATE_ARG = {
+  autocompleteInfo: {
+    section: "",
+    addressType: "",
+    contactType: "",
+    fieldName: "new-password",
+    canAutomaticallyPersist: false,
+  },
+  formOrigin: "https://example.com",
+  actionOrigin: "https://mozilla.org",
+  searchString: "",
+  previousResult: null,
+  requestId: "foo",
+  isSecure: true,
+  isPasswordField: true,
+};
 
-  // Default to the happy path
-  let arg1 = {
-    autocompleteInfo: {
-      section: "",
-      addressType: "",
-      contactType: "",
-      fieldName: "new-password",
-      canAutomaticallyPersist: false,
-    },
-    formOrigin: "https://example.com",
-    actionOrigin: "https://mozilla.org",
-    searchString: "",
-    previousResult: null,
-    requestId: "foo",
-    isSecure: true,
-    isPasswordField: true,
-  };
+add_task(async function setup() {
+  Services.prefs.setBoolPref("signon.generation.available", true);
+  Services.prefs.setBoolPref("signon.generation.enabled", true);
 
   sinon
     .stub(LoginManagerParent._browsingContextGlobal, "get")
     .withArgs(123)
     .callsFake(() => {
       return {
         currentWindowGlobal: {
           documentPrincipal: Services.scriptSecurityManager.createContentPrincipalFromOrigin(
             "https://www.example.com^userContextId=1"
           ),
         },
       };
     });
+});
 
-  let result1 = await LMP.doAutocompleteSearch(arg1);
+add_task(async function test_generated_noLogins() {
+  let LMP = new LoginManagerParent();
+  LMP.useBrowsingContext(123);
+
+  ok(LMP.doAutocompleteSearch, "doAutocompleteSearch exists");
+
+  let result1 = await LMP.doAutocompleteSearch(NEW_PASSWORD_TEMPLATE_ARG);
   equal(result1.logins.length, 0, "no logins");
   ok(result1.generatedPassword, "has a generated password");
   equal(result1.generatedPassword.length, 15, "generated password length");
+  ok(
+    result1.willAutoSaveGeneratedPassword,
+    "will auto-save when storage is empty"
+  );
 
   info("repeat the search and ensure the same password was used");
-  let result2 = await LMP.doAutocompleteSearch(arg1);
+  let result2 = await LMP.doAutocompleteSearch(NEW_PASSWORD_TEMPLATE_ARG);
   equal(result2.logins.length, 0, "no logins");
   equal(
     result2.generatedPassword,
     result1.generatedPassword,
     "same generated password"
   );
+  ok(
+    result1.willAutoSaveGeneratedPassword,
+    "will auto-save when storage is still empty"
+  );
 
   info("Check cases where a password shouldn't be generated");
 
   let result3 = await LMP.doAutocompleteSearch({
-    ...arg1,
+    ...NEW_PASSWORD_TEMPLATE_ARG,
     ...{ isPasswordField: false },
   });
   equal(
     result3.generatedPassword,
     null,
     "no generated password when not a pw. field"
   );
 
-  let arg1_2 = { ...arg1 };
+  // Deep copy since we need to modify a property of autocompleteInfo.
+  let arg1_2 = JSON.parse(JSON.stringify(NEW_PASSWORD_TEMPLATE_ARG));
   arg1_2.autocompleteInfo.fieldName = "";
   let result4 = await LMP.doAutocompleteSearch(arg1_2);
   equal(
     result4.generatedPassword,
     null,
     "no generated password when not autocomplete=new-password"
   );
 
+  LMP.useBrowsingContext(999);
   let result5 = await LMP.doAutocompleteSearch({
-    ...arg1,
-    ...{ browsingContextId: 999 },
+    ...NEW_PASSWORD_TEMPLATE_ARG,
   });
   equal(
     result5.generatedPassword,
     null,
     "no generated password with a missing browsingContextId"
   );
 });
+
+add_task(async function test_generated_emptyUsernameSavedLogin() {
+  info("Test with a login that will prevent auto-saving");
+  await LoginTestUtils.addLogin({
+    username: "",
+    password: "my-saved-password",
+    origin: NEW_PASSWORD_TEMPLATE_ARG.formOrigin,
+    formActionOrigin: NEW_PASSWORD_TEMPLATE_ARG.actionOrigin,
+  });
+
+  let LMP = new LoginManagerParent();
+  LMP.useBrowsingContext(123);
+
+  ok(LMP.doAutocompleteSearch, "doAutocompleteSearch exists");
+
+  let result1 = await LMP.doAutocompleteSearch(NEW_PASSWORD_TEMPLATE_ARG);
+  equal(result1.logins.length, 1, "1 login");
+  ok(result1.generatedPassword, "has a generated password");
+  equal(result1.generatedPassword.length, 15, "generated password length");
+  ok(
+    !result1.willAutoSaveGeneratedPassword,
+    "won't auto-save when an empty-username match is found"
+  );
+
+  LoginTestUtils.clearData();
+});
--- a/toolkit/components/passwordmgr/test/unit/test_login_autocomplete_result.js
+++ b/toolkit/components/passwordmgr/test/unit/test_login_autocomplete_result.js
@@ -1007,17 +1007,47 @@ add_task(async function test_all_pattern
       isSecure: true,
       isPasswordField: true,
       matchingLogins: [],
       items: [
         {
           value: "9ljgfd4shyktb45",
           label: "Use a Securely Generated Password",
           style: "generatedPassword",
-          comment: "9ljgfd4shyktb45",
+          comment: {
+            generatedPassword: "9ljgfd4shyktb45",
+            willAutoSaveGeneratedPassword: false,
+          },
+        },
+        {
+          value: "",
+          label: "View Saved Logins",
+          style: "loginsFooter",
+          comment: "mochi.test",
+        },
+      ],
+    },
+    {
+      description:
+        "willAutoSaveGeneratedPassword should propagate to the comment",
+      generatedPassword: "9ljgfd4shyktb45",
+      willAutoSaveGeneratedPassword: true,
+      insecureFieldWarningEnabled: true,
+      isSecure: true,
+      isPasswordField: true,
+      matchingLogins: [],
+      items: [
+        {
+          value: "9ljgfd4shyktb45",
+          label: "Use a Securely Generated Password",
+          style: "generatedPassword",
+          comment: {
+            generatedPassword: "9ljgfd4shyktb45",
+            willAutoSaveGeneratedPassword: true,
+          },
         },
         {
           value: "",
           label: "View Saved Logins",
           style: "loginsFooter",
           comment: "mochi.test",
         },
       ],
@@ -1031,17 +1061,20 @@ add_task(async function test_all_pattern
       isPasswordField: true,
       matchingLogins: [],
       searchString: "9ljgfd4shyktb45",
       items: [
         {
           value: "9ljgfd4shyktb45",
           label: "Use a Securely Generated Password",
           style: "generatedPassword",
-          comment: "9ljgfd4shyktb45",
+          comment: {
+            generatedPassword: "9ljgfd4shyktb45",
+            willAutoSaveGeneratedPassword: false,
+          },
         },
         {
           value: "",
           label: "View Saved Logins",
           style: "loginsFooter",
           comment: "mochi.test",
         },
       ],
@@ -1221,16 +1254,17 @@ add_task(async function test_all_pattern
     );
     let actual = new LoginAutoCompleteResult(
       pattern.searchString || "",
       pattern.matchingLogins,
       pattern.formOrigin || "https://mochi.test:8888",
       {
         hostname: "mochi.test",
         generatedPassword: pattern.generatedPassword,
+        willAutoSaveGeneratedPassword: !!pattern.willAutoSaveGeneratedPassword,
         isSecure: pattern.isSecure,
         isPasswordField: pattern.isPasswordField,
       }
     );
     equal(
       actual.matchCount,
       pattern.items.length,
       `${testIndex}: Check matching row count`
@@ -1249,21 +1283,23 @@ add_task(async function test_all_pattern
       equal(
         actual.getStyleAt(index),
         item.style,
         `${testIndex}: Style ${index}`
       );
       let actualComment = actual.getCommentAt(index);
       if (typeof item.comment == "object") {
         let parsedComment = JSON.parse(actualComment);
-        equal(
-          parsedComment.comment,
-          item.comment.comment,
-          `${testIndex}: Comment.comment ${index}`
-        );
+        for (let [key, val] of Object.entries(item.comment)) {
+          equal(
+            parsedComment[key],
+            val,
+            `${testIndex}: Comment.${key} ${index}`
+          );
+        }
       } else {
         equal(actualComment, item.comment, `${testIndex}: Comment ${index}`);
       }
     });
 
     if (pattern.items.length) {
       Assert.throws(
         () => actual.getValueAt(pattern.items.length),
--- a/toolkit/content/widgets/autocomplete-richlistitem.js
+++ b/toolkit/content/widgets/autocomplete-richlistitem.js
@@ -720,17 +720,16 @@
   }
 
   class MozAutocompleteGeneratedPasswordRichlistitem extends MozAutocompleteTwoLineRichlistitem {
     constructor() {
       super();
 
       this.line3 = document.createElement("div");
       this.line3.className = "label-row generated-password-autosave";
-      this.line3.textContent = this._autoSaveString;
     }
 
     get _autoSaveString() {
       if (!this.__autoSaveString) {
         let brandShorterName = Services.strings
           .createBundle("chrome://branding/locale/brand.properties")
           .GetStringFromName("brandShorterName");
         this.__autoSaveString = Services.strings
@@ -738,16 +737,24 @@
           .formatStringFromName("generatedPasswordWillBeSaved", [
             brandShorterName,
           ]);
       }
       return this.__autoSaveString;
     }
 
     _adjustAcItem() {
+      let { generatedPassword, willAutoSaveGeneratedPassword } = JSON.parse(
+        this.getAttribute("ac-label")
+      );
+      this.querySelector(".line2-label").textContent = generatedPassword;
+
+      this.line3.textContent = willAutoSaveGeneratedPassword
+        ? this._autoSaveString
+        : "";
       this.querySelector(".labels-wrapper").append(this.line3);
 
       super._adjustAcItem();
     }
   }
 
   customElements.define(
     "autocomplete-richlistitem",