Bug 386150 - Can't save a login containing only a password. r=gavin, a=mconnor

toolkit/components/passwordmgr/src/nsLoginManager.js

@@ -344,8 +344,9 @@ LoginManager.prototype = {
         if (login.hostname == null || login.hostname.length == 0)
             throw "Can't add a login with a null or empty hostname.";
 
-        if (login.username == null || login.username.length == 0)
-            throw "Can't add a login with a null or empty username.";
+        // For logins w/o a username, set to "", not null.
+        if (login.username == null)
+            throw "Can't add a login with a null username.";
 
         if (login.password == null || login.password.length == 0)
             throw "Can't add a login with a null or empty password.";
@@ -585,8 +586,14 @@ LoginManager.prototype = {
         }
 
         // If too few or too many fields, bail out.
-        if (pwFields.length == 0 || pwFields.length > 3)
+        if (pwFields.length == 0) {
+            this.log("(form ignored -- no password fields.)");
             return null;
+        } else if (pwFields.length > 3) {
+            this.log("(form ignored -- too many password fields. [got " +
+                        pwFields.length + "])");
+            return null;
+        }
 
         return pwFields;
     },
@@ -609,13 +616,13 @@ LoginManager.prototype = {
      * that is being changed.
      */
     _getFormFields : function (form, isSubmission) {
+        var usernameField = null;
+
         // Locate the password field(s) in the form. Up to 3 supported.
         // If there's no password field, there's nothing for us to do.
         var pwFields = this._getPasswordFields(form, isSubmission);
-        if (!pwFields) {
-            this.log("(form ignored -- either 0 or >3 pw fields.)");
+        if (!pwFields)
             return [null, null, null];
-        }
 
 
         // Locate the username field in the form by searching backwards
@@ -624,7 +631,7 @@ LoginManager.prototype = {
         // already logged in to the site. 
         for (var i = pwFields[0].index - 1; i >= 0; i--) {
             if (form.elements[i].type == "text") {
-                var usernameField = form.elements[i];
+                usernameField = form.elements[i];
                 break;
             }
         }
@@ -706,24 +713,6 @@ LoginManager.prototype = {
         };
 
         // local helper function
-        function findExistingLogin(pwmgr, hostname,
-                                   formSubmitURL, usernameField) {
-
-            var searchLogin = new pwmgr._nsLoginInfo();
-            searchLogin.init(hostname, formSubmitURL, null,
-                             usernameField.value, "",
-                             usernameField.name,  "");
-
-            var logins = pwmgr.findLogins({}, hostname, formSubmitURL, null);
-            var existingLogin;
-            var found = logins.some(function(l) {
-                                    existingLogin = l;
-                                    return searchLogin.equalsIgnorePassword(l);
-                                });
-
-            return (found ? existingLogin : null);
-        }
-
         function getPrompter(aWindow) {
             var prompterSvc = Cc["@mozilla.org/login-manager/prompter;1"].
                             createInstance(Ci.nsILoginManagerPrompter);
@@ -731,8 +720,6 @@ LoginManager.prototype = {
             return prompterSvc;
         }
 
-
-
         var doc = form.ownerDocument;
         var win = doc.defaultView;
 
@@ -771,9 +758,9 @@ LoginManager.prototype = {
 
         var formLogin = new this._nsLoginInfo();
         formLogin.init(hostname, formSubmitURL, null,
-                    (usernameField ? usernameField.value : null),
+                    (usernameField ? usernameField.value : ""),
                     newPasswordField.value,
-                    (usernameField ? usernameField.name  : null),
+                    (usernameField ? usernameField.name  : ""),
                     newPasswordField.name);
 
         // If we didn't find a username field, but seem to be changing a
@@ -814,22 +801,60 @@ LoginManager.prototype = {
             return;
         }
 
-        if (!usernameField && !oldPasswordField) {
-            this.log("XXX not handled yet");
-            return;
+
+        // Look for an existing login that matches the form login.
+        var existingLogin = null;
+        var logins = this.findLogins({}, hostname, formSubmitURL, null);
+
+        for (var i = 0; i < logins.length; i++) {
+            var same, login = logins[i];
+
+            // If one login has a username but the other doesn't, ignore
+            // the username when comparing and only match if they have the
+            // same password. Otherwise, compare the logins and match even
+            // if the passwords differ.
+            if (!login.username && formLogin.username) {
+                var restoreMe = formLogin.username;
+                formLogin.username = ""; 
+                same = formLogin.equals(login);
+                formLogin.username = restoreMe;
+            } else if (!formLogin.username && login.username) {
+                formLogin.username = login.username;
+                same = formLogin.equals(login);
+                formLogin.username = ""; // we know it's always blank.
+            } else {
+                same = formLogin.equalsIgnorePassword(login);
+            }
+
+            if (same) {
+                existingLogin = login;
+                break;
+            }
         }
 
-        // We have a username. Look for an existing login that matches the
-        // form data (other than the password, which might be different)
-        existingLogin = findExistingLogin(this, hostname, formSubmitURL,
-                                          usernameField);
         if (existingLogin) {
             this.log("Found an existing login matching this form submission");
 
-            // Change password if needed
+            /*
+             * Change password if needed.
+             *
+             * If the login has a username, change the password w/o prompting
+             * (because we can be fairly sure there's only one password
+             * associated with the username). But for logins without a
+             * username, ask the user... Some sites use a password-only "login"
+             * in different contexts (enter your PIN, answer a security
+             * question, etc), and without a username we can't be sure if
+             * modifying an existing login is the right thing to do.
+             */
             if (existingLogin.password != formLogin.password) {
-                this.log("...Updating password for existing login.");
-                this.modifyLogin(existingLogin, formLogin);
+                if (formLogin.username) {
+                    this.log("...Updating password for existing login.");
+                    this.modifyLogin(existingLogin, formLogin);
+                } else {
+                    this.log("...passwords differ, prompting to change.");
+                    prompter = getPrompter(win);
+                    prompter.promptToChangePassword(existingLogin, formLogin);
+                }
             }
 
             return;
@@ -966,9 +991,10 @@ LoginManager.prototype = {
 
             if (autofillForm) {
 
-                // If username was specified in the form, only fill in the
-                // password if we find a matching login.
                 if (usernameField && usernameField.value) {
+                    // If username was specified in the form, only fill in the
+                    // password if we find a matching login.
+
                     var username = usernameField.value;
 
                     var matchingLogin;
@@ -979,6 +1005,19 @@ LoginManager.prototype = {
                     if (found)
                         passwordField.value = matchingLogin.password;
 
+                } else if (usernameField && logins.length == 2) {
+                    // Special case, for sites which have a normal user+pass
+                    // login *and* a password-only login (eg, a PIN)...
+                    // When we have a username field and 1 of 2 available
+                    // logins is password-only, go ahead and prefill the
+                    // one with a username.
+                    if (!logins[0].username && logins[1].username) {
+                        usernameField.value = logins[1].username;
+                        passwordField.value = logins[1].password;
+                    } else if (!logins[1].username && logins[0].username) {
+                        usernameField.value = logins[0].username;
+                        passwordField.value = logins[0].password;
+                    }
                 } else if (logins.length == 1) {
                     if (usernameField)
                         usernameField.value = logins[0].username;

toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

@@ -462,9 +462,15 @@ LoginManagerPrompter.prototype = {
     promptToChangePassword : function (aOldLogin, aNewLogin) {
         const buttonFlags = Ci.nsIPrompt.STD_YES_NO_BUTTONS;
 
-        var dialogText  = this._getLocalizedString(
+        var dialogText;
+        if (aOldLogin.username)
+            dialogText  = this._getLocalizedString(
                                     "passwordChangeText",
                                     [aOldLogin.username]);
+        else
+            dialogText  = this._getLocalizedString(
+                                    "passwordChangeTextNoUser");
+
         var dialogTitle = this._getLocalizedString(
                                     "passwordChangeTitle");
 

toolkit/components/passwordmgr/src/storage-Legacy.js

@@ -799,8 +799,8 @@ LoginManagerStorage_legacy.prototype = {
                 break;
 
             // If decryption failed (corrupt entry?) skip it.
-            // XXX remove it from the original list entirely?
-            if (!username || !password)
+            // Note that we allow password-only logins, so username con be "".
+            if (username == null || !password)
                 continue;
 
             // We could set the decrypted values on a copy of the object, to

toolkit/components/passwordmgr/test/Makefile.in

@@ -57,6 +57,7 @@ MOCHI_TESTS = \
 		test_basic_form_2pw_2.html \
 		test_basic_form_3pw_1.html \
 		test_basic_form_autocomplete.html \
+		test_basic_form_pwonly.html \
 		test_bug_227640.html \
 		test_bug_242956.html \
 		test_bug_360493_1.html \

toolkit/components/passwordmgr/test/pwmgr_common.js

@@ -29,3 +29,78 @@ function $_(formNum, name) {
 
   return element;
 }
+
+
+/*
+ * checkForm
+ *
+ * Check a form for expected values. If an argument is null, a field's
+ * expected value will be the default value.
+ *
+ * <form id="form#">
+ * checkForm(#, "foo");
+ */
+function checkForm(formNum, val1, val2, val3) {
+    var e, form = document.getElementById("form" + formNum);
+    ok(form, "Locating form " + formNum);
+
+    var numToCheck = arguments.length - 1;
+    
+    if (!numToCheck--)
+        return;
+    e = form.elements[0];
+    if (val1 == null)
+        is(e.value, e.defaultValue, "Test default value of field " + e.name +
+            " in form " + formNum);
+    else
+        is(e.value, val1, "Test value of field " + e.name +
+            " in form " + formNum);
+
+
+    if (!numToCheck--)
+        return;
+    e = form.elements[1];
+    if (val2 == null)
+        is(e.value, e.defaultValue, "Test default value of field " + e.name +
+            " in form " + formNum);
+    else
+        is(e.value, val2, "Test value of field " + e.name +
+            " in form " + formNum);
+
+
+    if (!numToCheck--)
+        return;
+    e = form.elements[2];
+    if (val3 == null)
+        is(e.value, e.defaultValue, "Test default value of field " + e.name +
+            " in form " + formNum);
+    else
+        is(e.value, val3, "Test value of field " + e.name +
+            " in form " + formNum);
+
+}
+
+
+/*
+ * checkUnmodifiedForm
+ *
+ * Check a form for unmodified values from when page was loaded.
+ *
+ * <form id="form#">
+ * checkUnmodifiedForm(#);
+ */
+function checkUnmodifiedForm(formNum) {
+    var form = document.getElementById("form" + formNum);
+    ok(form, "Locating form " + formNum);
+
+    for (var i = 0; i < form.elements.length; i++) {
+        var ele = form.elements[i];
+
+        // No point in checking form submit/reset buttons.
+        if (ele.type == "submit" || ele.type == "reset")
+            continue;
+
+        is(ele.value, ele.defaultValue, "Test to default value of field " +
+            ele.name + " in form " + formNum);
+    }
+}

toolkit/components/passwordmgr/test/test_0init.html

@@ -64,7 +64,11 @@ ok(login != null, "Adding login");
 login.init("http://localhost:8888", "http://localhost:8888", null,
     "testuser", "testpass", "uname", "pword");
 
-pwmgr.addLogin(login);
+try {
+    pwmgr.addLogin(login);
+} catch (e) {
+    ok(false, "addLogin threw: " + e);
+}
 
 // Simple check to see if everything was added fine.
 var logins = pwmgr.getAllLogins({});

toolkit/components/passwordmgr/test/test_basic_form_1pw.html

@@ -2800,34 +2800,6 @@ function startTest() {
     SimpleTest.finish();
 }
 
-function checkForm(formNum, val1, val2, val3) {
-    var form = document.getElementById("form" + formNum);
-    ok(form, "Locating form " + formNum);
-
-    is(form.elements[0].value, val1, "Test value of field " +
-            form.elements[0].name + " in form " + formNum);
-
-    if (val2 != null)
-        is(form.elements[1].value, val2, "Test value of field " +
-            form.elements[1].name + " in form " + formNum);
-
-    if (val3 != null)
-        is(form.elements[2].value, val3, "Test value of field " +
-            form.elements[2].name + " in form " + formNum);
-}
-
-function checkUnmodifiedForm(formNum) {
-    var form = document.getElementById("form" + formNum);
-    ok(form, "Locating form " + formNum);
-
-    // Ignore last two elements, they're the two buttons.
-    for (var i = 0; i < form.elements.length - 2; i++) {
-        var ele = form.elements[i];
-
-        is(ele.value, ele.defaultValue, "Test to default value of field " +
-            ele.name + " in form " + formNum);
-    }
-}
 
 window.onload = startTest;
 

toolkit/components/passwordmgr/test/test_basic_form_2pw_1.html

@@ -15656,42 +15656,6 @@ function startTest() {
     SimpleTest.finish();
 }
 
-function checkForm(formNum, val1, val2, val3) {
-    var e, form = document.getElementById("form" + formNum);
-    ok(form, "Locating form " + formNum);
-
-    
-    e = form.elements[0];
-    if (!val1)
-        is(e.value, e.defaultValue, "Test default value of field " + e.name +
-            " in form " + formNum);
-    else
-        is(e.value, val1, "Test value of field " + e.name +
-            " in form " + formNum);
-
-
-    e = form.elements[1];
-    if (!val2)
-        is(e.value, e.defaultValue, "Test default value of field " + e.name +
-            " in form " + formNum);
-    else
-        is(e.value, val2, "Test value of field " + e.name +
-            " in form " + formNum);
-
-
-    e = form.elements[2];
-    if (!val3)
-        is(e.value, e.defaultValue, "Test default value of field " + e.name +
-            " in form " + formNum);
-    else
-        is(e.value, val3, "Test value of field " + e.name +
-            " in form " + formNum);
-
-}
-
-function checkUnmodifiedForm(formNum) {
-    checkForm(formNum, null, null, null);
-}
 
 window.onload = startTest;
 

toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html

@@ -88,7 +88,7 @@ function restoreForm() {
 
 
 // Check for expected username/password in form.
-function checkForm(expectedUsername, expectedPassword) {
+function checkACForm(expectedUsername, expectedPassword) {
   is(uname.value, expectedUsername, "Checking form username");
   is(pword.value, expectedPassword, "Checking form password");
 }
@@ -130,7 +130,7 @@ function runTest(testNum) {
   switch(testNum) {
     case 1:
         // Make sure initial form is empty.
-        checkForm("", "");
+        checkACForm("", "");
         // Trigger autocomplete popup
         restoreForm();
         doKey("down");
@@ -139,9 +139,9 @@ function runTest(testNum) {
     case 2:
         // Check first entry
         doKey("down");
-        checkForm("", ""); // value shouldn't update
+        checkACForm("", ""); // value shouldn't update
         doKey("return"); // not "enter"!
-        checkForm("tempuser1", "temppass1");
+        checkACForm("tempuser1", "temppass1");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -153,7 +153,7 @@ function runTest(testNum) {
         doKey("down");
         doKey("down");
         doKey("return"); // not "enter"!
-        checkForm("testuser2", "testpass2");
+        checkACForm("testuser2", "testpass2");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -166,7 +166,7 @@ function runTest(testNum) {
         doKey("down");
         doKey("down");
         doKey("return");
-        checkForm("testuser3", "testpass3");
+        checkACForm("testuser3", "testpass3");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -180,7 +180,7 @@ function runTest(testNum) {
         doKey("down");
         doKey("down");
         doKey("return");
-        checkForm("zzzuser4", "zzzpass4");
+        checkACForm("zzzuser4", "zzzpass4");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -196,7 +196,7 @@ function runTest(testNum) {
         doKey("down"); // deselects
         doKey("down");
         doKey("return");
-        checkForm("tempuser1", "temppass1");
+        checkACForm("tempuser1", "temppass1");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -207,7 +207,7 @@ function runTest(testNum) {
         // Check the last entry via arrow-up
         doKey("up");
         doKey("return");
-        checkForm("zzzuser4", "zzzpass4");
+        checkACForm("zzzuser4", "zzzpass4");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -220,7 +220,7 @@ function runTest(testNum) {
         doKey("up");   // selects nothing!
         doKey("up");   // select last entry
         doKey("return");
-        checkForm("zzzuser4", "zzzpass4");
+        checkACForm("zzzuser4", "zzzpass4");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -238,7 +238,7 @@ function runTest(testNum) {
         doKey("up"); // deselects
         doKey("up"); // last entry
         doKey("return");
-        checkForm("zzzuser4", "zzzpass4");
+        checkACForm("zzzuser4", "zzzpass4");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -249,7 +249,7 @@ function runTest(testNum) {
         // Set first entry w/o triggering autocomplete
         doKey("down");
         doKey("right");
-        checkForm("tempuser1", ""); // empty password
+        checkACForm("tempuser1", ""); // empty password
 
         // Trigger autocomplete popup
         restoreForm();
@@ -260,7 +260,7 @@ function runTest(testNum) {
         // Set first entry w/o triggering autocomplete
         doKey("down");
         doKey("left");
-        checkForm("tempuser1", ""); // empty password
+        checkACForm("tempuser1", ""); // empty password
 
         // Trigger autocomplete popup
         restoreForm();
@@ -273,7 +273,7 @@ function runTest(testNum) {
         doKey("down");
         doKey("page_up");
         doKey("return");
-        checkForm("tempuser1", "temppass1");
+        checkACForm("tempuser1", "temppass1");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -285,7 +285,7 @@ function runTest(testNum) {
         doKey("down");
         doKey("page_down");
         doKey("return");
-        checkForm("zzzuser4", "zzzpass4");
+        checkACForm("zzzuser4", "zzzpass4");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -299,7 +299,7 @@ function runTest(testNum) {
 //        doKey("down");
 //        doKey("down");
 //        doKey("escape");
-//        checkForm("", "");
+//        checkACForm("", "");
 
         // Trigger autocomplete popup
 //        restoreForm();
@@ -323,11 +323,11 @@ function runTest(testNum) {
         // On Win/Linux, shift-backspace does not work, delete and shift-delete do.
         doKey("delete", shiftModifier);
 
-        checkForm("zzzuser4", ""); // why does a value get set here?
+        checkACForm("zzzuser4", ""); // why does a value get set here?
         numLogins = pwmgr.countLogins("http://localhost:8888", "http://autocomplete:8888", null);
         is(numLogins, 3, "Correct number of logins after deleting one");
         doKey("return");
-        checkForm("testuser2", "testpass2");
+        checkACForm("testuser2", "testpass2");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -338,7 +338,7 @@ function runTest(testNum) {
         // Check the new first entry (of 3)
         doKey("down");
         doKey("return");
-        checkForm("testuser2", "testpass2");
+        checkACForm("testuser2", "testpass2");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -350,11 +350,11 @@ function runTest(testNum) {
         doKey("down");
         doKey("down");
         doKey("delete", shiftModifier);
-        checkForm("testuser2", ""); // XXX why does a value get set here?
+        checkACForm("testuser2", ""); // XXX why does a value get set here?
         numLogins = pwmgr.countLogins("http://localhost:8888", "http://autocomplete:8888", null);
         is(numLogins, 2, "Correct number of logins after deleting one");
         doKey("return");
-        checkForm("zzzuser4", "zzzpass4");
+        checkACForm("zzzuser4", "zzzpass4");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -365,7 +365,7 @@ function runTest(testNum) {
         // Check the new second entry (of 2)
         doKey("down");
         doKey("return");
-        checkForm("testuser2", "testpass2");
+        checkACForm("testuser2", "testpass2");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -377,11 +377,11 @@ function runTest(testNum) {
         doKey("down");
         doKey("down");
         doKey("delete", shiftModifier);
-        checkForm("testuser2", ""); // XXX why does a value get set here?
+        checkACForm("testuser2", ""); // XXX why does a value get set here?
         numLogins = pwmgr.countLogins("http://localhost:8888", "http://autocomplete:8888", null);
         is(numLogins, 1, "Correct number of logins after deleting one");
         doKey("return");
-        checkForm("testuser2", "testpass2");
+        checkACForm("testuser2", "testpass2");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -392,7 +392,7 @@ function runTest(testNum) {
         // Check the new second entry (of 2)
         doKey("down");
         doKey("return");
-        checkForm("testuser2", "testpass2");
+        checkACForm("testuser2", "testpass2");
 
         // Trigger autocomplete popup
         restoreForm();
@@ -404,7 +404,7 @@ function runTest(testNum) {
         doKey("down");
         doKey("delete", shiftModifier);
         //doKey("return");
-        checkForm("", "");
+        checkACForm("", "");
         numLogins = pwmgr.countLogins("http://localhost:8888", "http://autocomplete:8888", null);
         is(numLogins, 0, "Correct number of logins after deleting one");
 

toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties

@@ -48,6 +48,7 @@ notifyBarRememberButtonText = Remember
 notifyBarRememberButtonAccessKey = R
 passwordChangeTitle = Confirm Password Change
 passwordChangeText = Would you like to have Password Manager change the stored password for %S?
+passwordChangeTextNoUser = Would you like to have Password Manager change the stored password for this login?
 userSelectText = Please confirm which user you are changing the password for
 hidePasswords=Hide Passwords
 showPasswords=Show Passwords