Bug 1388674 - Only prompt to save logins if a login field was modified by the user. r=MattN
☠☠ backed out by dbb0fff9e65c ☠ ☠
authorprathiksha <prathikshaprasadsuman@gmail.com>
Mon, 18 Nov 2019 23:55:27 +0000
changeset 502517 09e3e82fb4396ae250cc5e92c7af8447ff5a5683
parent 502516 5caf9e9337388ac29a786e9600a6af240bb00275
child 502518 4f123ce2cc07772d932aae792a39a38f3ce765fa
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1388674
milestone72.0a1
Bug 1388674 - Only prompt to save logins if a login field was modified by the user. r=MattN Differential Revision: https://phabricator.services.mozilla.com/D24556
modules/libpref/init/all.js
toolkit/components/passwordmgr/LoginHelper.jsm
toolkit/components/passwordmgr/LoginManagerChild.jsm
toolkit/components/passwordmgr/test/mochitest/mochitest.ini
toolkit/components/passwordmgr/test/mochitest/test_submit_without_field_modifications.html
--- a/modules/libpref/init/all.js
+++ b/modules/libpref/init/all.js
@@ -3882,16 +3882,17 @@ pref("signon.autofillForms",            
 pref("signon.autofillForms.autocompleteOff", true);
 pref("signon.autofillForms.http",           false);
 pref("signon.autologin.proxy",              false);
 pref("signon.formlessCapture.enabled",      true);
 pref("signon.generation.available",         false);
 pref("signon.generation.enabled",           false);
 pref("signon.privateBrowsingCapture.enabled", false);
 pref("signon.storeWhenAutocompleteOff",     true);
+pref("signon.userInputRequiredToCapture.enabled", true);
 pref("signon.debug",                        false);
 pref("signon.recipes.path",                 "chrome://passwordmgr/content/recipes.json");
 pref("signon.schemeUpgrades",               false);
 pref("signon.includeOtherSubdomainsInLookup", false);
 // This temporarily prevents the master password to reprompt for autocomplete.
 pref("signon.masterPasswordReprompt.timeout_ms", 900000); // 15 Minutes
 pref("signon.showAutoCompleteFooter", false);
 pref("signon.showAutoCompleteOrigins", false);
--- a/toolkit/components/passwordmgr/LoginHelper.jsm
+++ b/toolkit/components/passwordmgr/LoginHelper.jsm
@@ -77,16 +77,19 @@ this.LoginHelper = {
     );
     this.schemeUpgrades = Services.prefs.getBoolPref("signon.schemeUpgrades");
     this.showAutoCompleteFooter = Services.prefs.getBoolPref(
       "signon.showAutoCompleteFooter"
     );
     this.storeWhenAutocompleteOff = Services.prefs.getBoolPref(
       "signon.storeWhenAutocompleteOff"
     );
+    this.userInputRequiredToCapture = Services.prefs.getBoolPref(
+      "signon.userInputRequiredToCapture.enabled"
+    );
   },
 
   createLogger(aLogPrefix) {
     let getMaxLogLevel = () => {
       return this.debug ? "Debug" : "Warn";
     };
 
     // Create a new instance of the ConsoleAPI so we can control the maxLogLevel with a pref.
--- a/toolkit/components/passwordmgr/LoginManagerChild.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerChild.jsm
@@ -242,21 +242,30 @@ const observer = {
             aEvent.target
           );
         }
         break;
       }
 
       // Used to watch for changes to fields filled with generated passwords.
       case "input": {
-        if (docState.generatedPasswordFields.has(aEvent.target)) {
+        let field = aEvent.target;
+        if (docState.generatedPasswordFields.has(field)) {
           LoginManagerChild.forWindow(
             window
           )._maybeStopTreatingAsGeneratedPasswordField(aEvent);
         }
+        if (
+          field.hasBeenTypePassword ||
+          LoginHelper.isUsernameFieldType(field)
+        ) {
+          // flag this form as user-modified
+          let formLikeRoot = FormLikeFactory.findRootForField(field);
+          docState.fieldModificationsByRootElement.set(formLikeRoot, true);
+        }
         break;
       }
 
       case "keydown": {
         if (
           aEvent.keyCode == aEvent.DOM_VK_TAB ||
           aEvent.keyCode == aEvent.DOM_VK_RETURN
         ) {
@@ -865,16 +874,22 @@ this.LoginManagerChild = class LoginMana
    *
    * @param {LoginForm} form to fetch the logins for then try autofill.
    */
   _fetchLoginsFromParentAndFillForm(form) {
     if (!LoginHelper.enabled) {
       return;
     }
 
+    // set up input event listeners so we know if the user has interacted with these fields
+    form.rootElement.addEventListener("input", observer, {
+      capture: true,
+      mozSystemGroup: true,
+    });
+
     this._getLoginDataFromParent(form, { showMasterPassword: true })
       .then(this.loginsFound.bind(this))
       .catch(Cu.reportError);
   }
 
   /**
    * Retrieves a reference to the state object associated with the given
    * document. This is initialized to an object with default values.
@@ -890,16 +905,17 @@ this.LoginManagerChild = class LoginMana
         /**
          * Keeps track of fields we've filled with generated passwords
          */
         generatedPasswordFields: new WeakSet(),
         /**
          * Keeps track of logins that were last submitted.
          */
         lastSubmittedValuesByRootElement: new WeakMap(),
+        fieldModificationsByRootElement: new WeakMap(),
         loginFormRootElements: new WeakSet(),
       };
       this._loginFormStateByDocument.set(document, loginFormState);
     }
     return loginFormState;
   }
 
   /**
@@ -1552,19 +1568,27 @@ this.LoginManagerChild = class LoginMana
       )
     ) {
       log(
         "(form submission ignored -- already submitted with the same username and password)"
       );
       return;
     }
 
-    let autoFilledLogin = this.stateForDocument(doc).fillsByRootElement.get(
-      form.rootElement
-    );
+    let docState = this.stateForDocument(doc);
+    let fieldsModified = this._formHasModifiedFields(formLikeRoot);
+    if (!fieldsModified && LoginHelper.userInputRequiredToCapture) {
+      // we know no fields in this form had user modifications, so don't prompt
+      log(
+        "(form submission ignored -- submitting values that are not changed by the user)"
+      );
+      return;
+    }
+
+    let autoFilledLogin = docState.fillsByRootElement.get(form.rootElement);
 
     let detail = {
       origin,
       formActionOrigin,
       autoFilledLoginGuid: autoFilledLogin && autoFilledLogin.guid,
       usernameField: mockUsername,
       newPasswordField: mockPassword,
       oldPasswordField: mockOldPassword,
@@ -1811,17 +1835,16 @@ this.LoginManagerChild = class LoginMana
       userTriggered = false,
     } = {}
   ) {
     if (ChromeUtils.getClassName(form) === "HTMLFormElement") {
       throw new Error("_fillForm should only be called with LoginForm objects");
     }
 
     log("_fillForm", form.elements);
-    let usernameField;
     // Will be set to one of AUTOFILL_RESULT in the `try` block.
     let autofillResult = -1;
     const AUTOFILL_RESULT = {
       FILLED: 0,
       NO_PASSWORD_FIELD: 1,
       PASSWORD_DISABLED_READONLY: 2,
       NO_LOGINS_FIT: 3,
       NO_SAVED_LOGINS: 4,
@@ -1829,41 +1852,40 @@ this.LoginManagerChild = class LoginMana
       EXISTING_USERNAME: 6,
       MULTIPLE_LOGINS: 7,
       NO_AUTOFILL_FORMS: 8,
       AUTOCOMPLETE_OFF: 9,
       INSECURE: 10,
       PASSWORD_AUTOCOMPLETE_NEW_PASSWORD: 11,
     };
 
+    // Heuristically determine what the user/pass fields are
+    // We do this before checking to see if logins are stored,
+    // so that the user isn't prompted for a master password
+    // without need.
+    let [usernameField, passwordField] = this._getFormFields(
+      form,
+      false,
+      recipes
+    );
+
     try {
       // Nothing to do if we have no matching (excluding form action
       // checks) logins available, and there isn't a need to show
       // the insecure form warning.
       if (
         !foundLogins.length &&
         (InsecurePasswordUtils.isFormSecure(form) ||
           !LoginHelper.showInsecureFieldWarning)
       ) {
         // We don't log() here since this is a very common case.
         autofillResult = AUTOFILL_RESULT.NO_SAVED_LOGINS;
         return;
       }
 
-      // Heuristically determine what the user/pass fields are
-      // We do this before checking to see if logins are stored,
-      // so that the user isn't prompted for a master password
-      // without need.
-      let passwordField;
-      [usernameField, passwordField] = this._getFormFields(
-        form,
-        false,
-        recipes
-      );
-
       // If we have a password inputElement parameter and it's not
       // the same as the one heuristically found, use the parameter
       // one instead.
       if (inputElement) {
         if (inputElement.type == "password") {
           passwordField = inputElement;
           if (!clobberUsername) {
             usernameField = null;
@@ -2146,16 +2168,24 @@ this.LoginManagerChild = class LoginMana
       }
 
       this.sendAsyncMessage("PasswordManager:formProcessed", {
         formid: form.rootElement.id,
       });
     }
   }
 
+  _formHasModifiedFields(formLikeRoot) {
+    let state = this.stateForDocument(formLikeRoot.ownerDocument);
+    let fieldsModified = state.fieldModificationsByRootElement.get(
+      formLikeRoot
+    );
+    return fieldsModified;
+  }
+
   /**
    * Given a field, determine whether that field was last filled as a username
    * field AND whether the username is still filled in with the username AND
    * whether the associated password field has the matching password.
    *
    * @note This could possibly be unified with getFieldContext but they have
    * slightly different use cases. getFieldContext looks up recipes whereas this
    * method doesn't need to since it's only returning a boolean based upon the
--- a/toolkit/components/passwordmgr/test/mochitest/mochitest.ini
+++ b/toolkit/components/passwordmgr/test/mochitest/mochitest.ini
@@ -155,14 +155,15 @@ skip-if = e10s || toolkit == 'android' #
 [test_password_length.html]
 scheme = https
 skip-if = toolkit == 'android' # bug 1527403
 [test_prompt_promptAuth.html]
 skip-if = os == "linux" || toolkit == 'android' # Tests desktop prompts
 [test_prompt_promptAuth_proxy.html]
 skip-if = e10s || os == "linux" || toolkit == 'android' # Tests desktop prompts
 [test_recipe_login_fields.html]
+[test_submit_without_field_modifications.html]
 [test_username_focus.html]
 skip-if = toolkit == 'android' # android:autocomplete.
 [test_xhr.html]
 skip-if = toolkit == 'android' # Tests desktop prompts
 [test_xhr_2.html]
 
new file mode 100644
--- /dev/null
+++ b/toolkit/components/passwordmgr/test/mochitest/test_submit_without_field_modifications.html
@@ -0,0 +1,233 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>Don't send onFormSubmit message on navigation if the user did not interact
+    with the login fields</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/AddTask.js"></script>
+  <script type="text/javascript" src="pwmgr_common.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<p id="display"></p>
+
+<div id="content">
+  <iframe id="loginFrame">
+  </iframe>
+</div>
+
+<pre id="test"></pre>
+<script>
+const { TestUtils } = SpecialPowers.Cu.import("resource://testing-common/TestUtils.jsm");
+SimpleTest.requestFlakyTimeout("Giving a chance for the unexpected popup to show");
+let iframe = document.getElementById("loginFrame");
+
+function waitForLoad() {
+  return new Promise(resolve => {
+    function handleLoad() {
+      iframe.removeEventListener("load", handleLoad);
+      resolve();
+    }
+    iframe.addEventListener("load", handleLoad);
+  });
+}
+
+ async function setup(pageUrl) {
+  let loadPromise = waitForLoad();
+  iframe.src = pageUrl || "https://example.org/tests/toolkit/components/passwordmgr/test/mochitest/formless_basic.html";
+
+  await loadPromise;
+  let win = SpecialPowers.wrap(iframe.contentWindow);
+  let iframeDoc = win.document;
+
+  let link = document.createElement("a");
+  link.setAttribute("href", "http://mochi.test:8888");
+  iframeDoc.body.appendChild(link);
+
+  return {iframeDoc, link};
+}
+
+function setValue(doc) {
+  // assign to .value directly, we deliberately don't emulate user input
+  doc.getElementById("form-basic-username").value = "user";
+  doc.getElementById("form-basic-password").value = "pass";
+}
+
+add_task(async function test_init() {
+  await SpecialPowers.pushPrefEnv({"set": [
+    ["signon.userInputRequiredToCapture.enabled", true],
+  ]});
+});
+
+add_task(async function test_no_message_on_navigation() {
+  // If login field values were set by the website, we don't message to save the
+  // login values if the user did not interact with the fields before submiting.
+  let elements = await setup();
+  setValue(elements.iframeDoc);
+  await promiseFormsProcessed();
+
+  let submitMessageSent = false;
+  getSubmitMessage().then(value => {
+    submitMessageSent = true;
+  });
+
+  let loadPromise = waitForLoad();
+  SpecialPowers.wrap(elements.link).click();
+  await loadPromise;
+
+  // allow time to pass before concluding no onFormSubmit message was sent
+  await new Promise(res => setTimeout(res, 1000));
+  ok(!submitMessageSent, "onFormSubmit message is not sent on navigation since the login fields were not modified");
+});
+
+add_task(async function test_prefd_off_message_on_navigation() {
+  // Confirm the pref controls capture behavior with non-user-set field values.
+  await SpecialPowers.pushPrefEnv({"set": [
+    ["signon.userInputRequiredToCapture.enabled", false],
+  ]});
+
+  let elements = await setup();
+  setValue(elements.iframeDoc);
+  await promiseFormsProcessed();
+
+  let promiseSubmitMessage = getSubmitMessage();
+  SpecialPowers.wrap(elements.link).click();
+  await promiseSubmitMessage;
+  info("onFormSubmit message was sent as expected after navigation");
+
+  SpecialPowers.popPrefEnv();
+});
+
+add_task(async function test_message_with_user_interaction_on_navigation() {
+  let elements = await setup();
+  setValue(elements.iframeDoc);
+  await promiseFormsProcessed();
+
+  let username = elements.iframeDoc.getElementById("form-basic-username");
+  username.setUserInput("foo");
+  let promiseSubmitMessage = getSubmitMessage();
+  SpecialPowers.wrap(elements.link).click();
+  await promiseSubmitMessage;
+  info("onFormSubmit message was sent as expected after user interaction");
+});
+
+add_task(async function test_empty_form_with_input_handler() {
+  let elements = await setup();
+  await promiseFormsProcessed();
+
+  elements.iframeDoc.getElementById("form-basic-username").setUserInput("user");
+  elements.iframeDoc.getElementById("form-basic-password").setUserInput("pass");
+
+  let promiseSubmitMessage = getSubmitMessage();
+  SpecialPowers.wrap(elements.link).click();
+  await promiseSubmitMessage;
+  info("onFormSubmit message was sent as expected after user interaction");
+});
+
+add_task(async function test_message_on_autofill_without_user_interaction() {
+  runInParent(function addLogin() {
+    const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
+    let login = Cc["@mozilla.org/login-manager/loginInfo;1"]
+                        .createInstance(Ci.nsILoginInfo);
+    login.init("https://example.org", "https://example.org", null,
+               "user1", "pass1", "", "");
+
+    Services.logins.addLogin(login);
+  });
+
+  let elements = await setup();
+  await promiseFormsProcessed();
+
+  // Check for autofilled values.
+  let doc = elements.iframeDoc;
+  let uname = doc.getElementById("form-basic-username");
+  let pword = doc.getElementById("form-basic-password");
+  is(uname.value, "user1", "Username was autofilled correctly");
+  is(pword.value, "pass1", "Password was autofilled correctly");
+
+  let promiseSubmitMessage = getSubmitMessage();
+  let loadPromise = waitForLoad();
+  SpecialPowers.wrap(elements.link).click();
+  await loadPromise;
+
+  let messageData = await promiseSubmitMessage;
+  ok(messageData.autoFilledLoginGuid, "Message was sent with autoFilledLoginGuid");
+});
+
+add_task(async function test_message_on_autofill_with_user_interaction() {
+  runInParent(function addLogin() {
+    const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
+    let login = Cc["@mozilla.org/login-manager/loginInfo;1"]
+                        .createInstance(Ci.nsILoginInfo);
+    login.init("https://example.org", "https://example.org", null,
+               "user1", "pass1", "", "");
+
+    Services.logins.addLogin(login);
+  });
+
+  let elements = await setup();
+  await promiseFormsProcessed();
+  // Check for autofilled values.
+  let doc = elements.iframeDoc;
+  let uname = doc.getElementById("form-basic-username");
+  let pword = doc.getElementById("form-basic-password");
+  is(uname.value, "user1", "Username was autofilled correctly");
+  is(pword.value, "pass1", "Password was autofilled correctly");
+
+  elements.iframeDoc.getElementById("form-basic-username").setUserInput("newuser");
+  let promiseSubmitMessage = getSubmitMessage();
+  let loadPromise = waitForLoad();
+  SpecialPowers.wrap(elements.link).click();
+  await loadPromise;
+
+  let messageData = await promiseSubmitMessage;
+  ok(messageData.autoFilledLoginGuid, "Message was sent with autoFilledLoginGuid");
+  is(messageData.usernameField.value, "newuser", "Message was sent with correct usernameField.value");
+  info("Message was sent as expected after user interaction");
+});
+
+add_task(async function test_no_message_on_user_input_from_other_form() {
+  // ensure input into unrelated fields on the page don't change login form modified-ness
+  let elements = await setup("http://example.com/tests/toolkit/components/passwordmgr/test/mochitest/form_basic.html");
+  await promiseFormsProcessed();
+  info("initial form processed");
+  // Add a form which will not be submitted and an input associated with that form
+  let doc = SpecialPowers.wrap(elements.iframeDoc);
+  let loginForm = doc.querySelector("form");
+  let fragment = doc.createDocumentFragment();
+  let otherForm = doc.createElement("form");
+  otherForm.id ="otherForm";
+  fragment.appendChild(otherForm);
+
+  let alienField = doc.createElement("input");
+  alienField.type = "text"; // not a password field
+  alienField.setAttribute("form", "otherForm");
+  // new field is child of the login, but a member of different non-login form via its .form property
+  loginForm.appendChild(alienField);
+  doc.body.appendChild(fragment);
+
+  await TestUtils.waitForTick();
+
+  SpecialPowers.wrap(alienField).setUserInput("something");
+
+  let submitMessageSent = false;
+  getSubmitMessage().then(data => {
+    info("submit mesage data: " + JSON.stringify(data));
+    submitMessageSent = true;
+  });
+
+  info("submitting the form");
+  let loadPromise = waitForLoad();
+  SpecialPowers.wrap(elements.link).click();
+  info("waiting for response to load");
+  await loadPromise;
+
+  // allow time to pass before concluding no onFormSubmit message was sent
+  await new Promise(res => setTimeout(res, 1000));
+  ok(!submitMessageSent, "onFormSubmit message is not sent on navigation since no login fields were modified");
+});
+
+</script>
+</body>
+</html>