386150 - Can't save a login containing only a password

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

Bug 380222 introduced a regression for an uncommon (?) case... If a form has only a password field, we don't offer to save it. [If a login already is stored, though, we will fill in the password for that login in the form when it loads... You just can't save new logins this way]

One fix would be to just allow saving logins with a blank username field, like the old code apparently did. An improvement would be to prompt for a username (or description) for the login... This would help if you later encounter a form on the site requiring your username too. If there really is no username for the site, but there are multiple passwords applicable somehow, then entering a description would allow for a UI that allows to to chose between passwords (as we otherwise have nothing to show, except the password itself).

Comment 1

[Phil Ringnalda (:philor)]

> uncommon (?)

Other than Tinderbox admin (it doesn't matter if it's uncommon, if you're
causing mconnor pain), I've hit them in the proximate cause of fixing it in the
first place, Mailman admin pages
(http://www.google.com/search?q=%22delivered%20by%20mailman%22 say 1.3 million
pages), and some libraries, where you get database access by using your library
card number in a password field.

Comment 3

[Justin Dolske [:Dolske]]

Bug 388215 reports that HTTP authentication need to allow password-only logins as well.

Comment 4

[Mike Connor [:mconnor]]

M7 if possible, shouldn't be that hard, I hope?

Comment 5

[Mike Connor [:mconnor]]

Missing the freeze, moving out.

Comment 6

[Robert Kaiser]

From what I hear, mailnews needs that functionality as well, adding dependency for this.

Comment 7

[Bill Gianopoulos [:WG9s]]

This also impacts logging into many on-line banking sites.  Any site using sitekey (which many banks including Bank of America now do) have this issue.  This is NOT an uncommon case as speculated in the initial report.

Comment 8

[Justin Dolske [:Dolske]]

FWIW, in bug 391514 I noticed that USAA's PIN entry page had:

<div style="display: none">
    <input type="text" name="hidden_text_input"
        maxlength="1" size="1"value=""/>
</div>

This may imply some PIN-only forms will actually look like like a username+password to Login Manager (with a blank username). Both cases should probably be handled.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch, work in progress

Completely untested, but in theory it works. :-)

I tried to have to be smart when checking for an existing login, when preparing to see if it should ask to save the login as a new entry (or change an existing entry's password)... This should help prevent saving 2 logins on sites that actually do use a username, but sometimes don't prompt for it. For example:

1st visit: "Please login. [    Bob]   [   ****]"
2nd visit: "Hi Bob! Enter your password to login.  [   ****]"

Currently we can use the username+password login from the first visit to fill in the password-only form on the second visit, and then Login Manager ignores the form submission on the second visit.

With allowing Login Manager to save password-only logins, I'd still want to prevent prompting Bob to save a login on his 2nd visit. Similarly, consider if Bob chose not to save his password on his 1st visit, but did on his 2nd... Now he clears his cookies and visits a 3rd time, and is prompted for both a username and password. We should fill in the password field, and not prompt to save the login when he submits the form. [Actually, it might be nice if we prompted to ask if the existing password-only login should have the username assigned with it, but I'm not sure the extra complexity is worth it. We could also handle password changes better, again with more complexity.]

I'm also considering adding a special case to _fillDoc(), for sites that have both a username+password login and password-only login. The canonical example is banking sites that have a normal login and PIN. If we have a form with a username and password field, we should preferentially fill in a username+password login over the password-only login. [With the current patch, you'd have to use the autocomplete dropdown to select one of the two logins.]

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1

Added the special case mentioned in he last comment.

[Seems I've got the patch for bug 391514 included in this diff too, oops.]

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.2

Updated patch to trunk, eliminated one use of Array.some() to avoid confusing use of a closure.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 280012 [details] [diff] [review]
Patch for review, v.2

>Index: toolkit/components/passwordmgr/src/nsLoginManager.js

>+        // Look for an existing login that matches the form login.

>+            // If one login has a username but the other doesn't, ignore
>+            // the login when comparing and only match if they have the
>+            // same password. Otherwise, compare the logins and match even
>+            // if the passwords differ.

s/the login when comparing/the username when comparing/, right?

I guess you didn't want to add a "equalsIgnoreUsername" because of it's relatively infrequent use, and to avoid cluttering the interface? It would simplify this code a little bit, but doesn't matter much to me.

>             // Change password if needed
>             if (existingLogin.password != formLogin.password) {
>-                this.log("...Updating password for existing login.");
>-                this.modifyLogin(existingLogin, formLogin);
>+                this.log("...passwords differ, prompting to change.");
>+                prompter = getPrompter(win);
>+                prompter.promptToChangePassword(existingLogin, formLogin);
>             }

This seems like something that should be dealt with in another bug, unless it's somehow needed. I'm not sure it's the right thing to do (though I could probably change my mind given the new notification-bar prompting and bug 394611).

>             if (autofillForm) {

>                 if (usernameField && usernameField.value) {

>+                } 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.

Maybe this is an edge case not worth dealing with, but my bank uses a normal username/password login for the first page, and then brings me to an intermediate page that will ask me one of 5 preselected questions that have a password-only answer. If I were to use the password manager to save all of these answers I'd have 6 saved logins, 5 of which are "password only". Should we check that there's only one login out of all matching logins with a username, rather than checking for one out of 2?

Do you have tests for this too?

r=me with those addressed.

Comment 14

[Justin Dolske [:Dolske]]

(In reply to comment #13)
> I guess you didn't want to add a "equalsIgnoreUsername" because of it's
> relatively infrequent use, and to avoid cluttering the interface? It would
> simplify this code a little bit, but doesn't matter much to me.

Yeah, I don't think it's useful.
 
> If I were to use the password manager to save all of
> these answers I'd have 6 saved logins, 5 of which are "password only".

Actually, that's shouldn't happen -- you should only end up with 1 password-only login (just as it doesn't support two logins with the same non-blank username but different passwords).

> Do you have tests for this too?

There are some existing tests that probably need to be tweaked/enabled to test password-only logins, I'll attach a patch to do that.

Comment 15

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.3

Addressed review comments, and per IRC conversation kept the "change password?" prompt when the login doesn't have a username. Otherwise unchanged.

Also adds testcases (I moved the "checkForm" function into a shared file, so the tests diffs have a bit of extra noise).

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 281416 [details] [diff] [review]
Patch for review, v.3

>Index: toolkit/components/passwordmgr/src/nsLoginManager.js

>+                // Some bank sites ask extra auth questions using a password
>+                // field. If the user has a PIN stored, we don't want to
>+                // overwrite it without user confirmation.

I find this comment a bit confusing. How about something like:

Don't prompt the user about modifying the password for normal logins. If the form login doesn't have a username, assume it is a password-only "extra auth question" or PIN, and confirm that the user really does want to overwrite it, because some banks use multiple different such questions and overwriting them automatically is undesirable.

If that comment doesn't make sense because I don't understand what the code is doing please let me know! Also now that I reread it it's pretty similar to your comment. I'm tired.

This also means we need to file a bug on not showing http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties&rev=1.10&mark=50#50 without an empty string for password-only logins, right?

Comment 17

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.4

Clarified comment, and added a special case in the "change password?" prompting for when there's no username available.

Comment 18

[Justin Dolske [:Dolske]]

[Also ran the minor string change in the patch by Faaborg for ui-review]

Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js,v  <--  nsLoginManager.js
new revision: 1.20; previous revision: 1.19
done
Checking in toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js,v  <--  nsLoginManagerPrompter.js
new revision: 1.11; previous revision: 1.10
done
Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js,v  <--  storage-Legacy.js
new revision: 1.15; previous revision: 1.14
done
Checking in toolkit/components/passwordmgr/test/Makefile.in;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/Makefile.in,v  <--  Makefile.in
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/components/passwordmgr/test/pwmgr_common.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/pwmgr_common.js,v  <--  pwmgr_common.js
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/components/passwordmgr/test/test_0init.html;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/test_0init.html,v  <--  test_0init.html
new revision: 1.7; previous revision: 1.6
done
Checking in toolkit/components/passwordmgr/test/test_basic_form_1pw.html;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/test_basic_form_1pw.html,v  <--  test_basic_form_1pw.html
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/components/passwordmgr/test/test_basic_form_2pw_1.html;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/test_basic_form_2pw_1.html,v  <--  test_basic_form_2pw_1.html
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html,v  <--  test_basic_form_autocomplete.html
new revision: 1.4; previous revision: 1.3
done
Checking in toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties,v  <--  passwordmgr.properties
new revision: 1.11; previous revision: 1.10
done

Checking in toolkit/components/passwordmgr/test/test_basic_form_pwonly.html;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/test_basic_form_pwonly.html,v  <--  test_basic_form_pwonly.html
initial revision: 1.1
done

Comment 19

[u8313]

This seems to have regressed in 3.0 Beta (both 1 & 2). Bank of America's sitekey works for me in 2.0, but not in 3.0

Comment 20

[Justin Dolske [:Dolske]]

As far as I can get into BoA's site, I see a |autocomplete="off"| attribute on the username form, so I would assume they're doing the same with whatever you're seeing. File a new bug if you can verify this attribute isn't on the form.

Comment 21

[u8313]

It does appear the autocomplete="off" is on the form in the password page, but the 2.0.0.11 password manager fills in the password on the password page. So is this a bug in 2.0? a bug in 3.0 (in which case I'll open a new issue as you asked)? or simply a difference in behavior between 2.0 and 3.0? 

Comment 22

[Justin Dolske [:Dolske]]

autocomplete="off" stops pwmgr from saving the login. If you've previously saved it somehow (eg, if they only recently added the attribute), it will still be filled in. Not a bug, working as designed.

Comment 23

[u8313]

But it doesn't fill it in when using 3.0; it asks for my master password and then doesn't fill in anything. So I guess I'll open an issue against 3.0 that it is not working as designed.

Comment 24

[Isaac]

I'm getting [Exception... "'Can't add a login with a null or empty password.' when calling method: [nsILoginManager::addLogin]" nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)" location: "<unknown>" data: no] when trying to add save a http prompt login with *no password*.  I found bug 396163, which was marked as a dupe of this one, but the issue does not seem resolved.  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008011604 Minefield/3.0b3pre

Comment 25

[Justin Dolske [:Dolske]]

(In reply to comment #24)
> I'm getting [Exception... "'Can't add a login with a null or empty password.'

This bug was about logins with a password but no username. Your error message is about a login with a username but no password.

See bug 399703 for that issue.

Comment 26

[Paul Craciunoiu [:paulc]]

Verified functionality on:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9) Gecko/2008053008 Firefox/3.0
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008060309 Firefox/3.0

Hopefully there are no exceptions :)