419081 - Password prompting code doesn't check new passwords against all possible existing passwords

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

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

See bug 382437 comment 43. We already check the new login details against the password we used to pre-fill the dialog, but if there is more than one existing login, the new password might also conflict with one of those.

This might be the cause of some of the duplicates of bug 397799, since as far as I can tell the symptoms would be the same (notification bar not disappearing when you click "remember password", password not being saved).

Comment 1

[Justin Dolske [:Dolske]]

Bug 419417 also ran across this issue, as a complication from a different problem.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

This patch basically just adds a call to |this._repickSelectedLogin()| (and an implementation of that).

I also did a bit of cleanup by making promptAuth() return early if it's not going to save the login (instead of skipping over some blocks it won't enter)... That allows removing a level of |if() { }| checks, so the code is indented less.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (whitespace ignored)

Comment 4

[Justin Dolske [:Dolske]]

(updated patch coming, now that bug 382437 has landed)

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Extends the work done in patch v.1 into the newly added promptUsernameAndPassword() interface.

The changes after the prompt service call there are the same as those made to promptAuth() in v.1.

The code before the prompt service call needed a bit more modification... The problem is that if the caller provided a username/password, we didn't search existing logins, so we had nothing to compare with afterwards. This change also has the nice side effect that if the caller specifies a username, we'll now fill in the password if there's already a saved login with that username.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (whitespace ignored)

Comment 7

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

That code seems to use the first selected login's username with the passed in password, if you only pass in a username, which seems wrong offhand. Shouldn't it should either use both passed in values, or neither?

Comment 8

[Justin Dolske [:Dolske]]

(In reply to comment #7)
> That code seems to use the first selected login's username with the passed in
> password, if you only pass in a username, which seems wrong offhand. Shouldn't
> it should either use both passed in values, or neither?

s/username, which/password, which/ there?

I was thinking of the case where a caller knows the username (because it's safe to store in a pref or anywhere), but not the password... Say, a mail program with accounts for "[e-mail]" and "[e-mail]".

If the caller supplies both a username and password, we look for an existing login with the same username. We use the caller's provided value, and may end up calling modifyLogin() afterwards unless. [It not really clear to me which password is the "right" one to prefill, so I assume the caller specified that password for a reason.]

If the caller supplies just a password... Oh. The only use case I can see for this is prompting for a password-only login. We should probably look for an existing login without the username set:

    if (aUsername.value || aPassword.value)
        selectedLogin = this._repickSelectedLogin(foundLogins, aUsername.value)

Comment 9

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

I guess that makes sense.

Mark, do you have any idea what the existing callers expect? Are there any callers that do these kinds of weird things? How does it compare against the old impl ( http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/wallet/src/singsign.cpp&rev=1.263#2664 , looks like)?

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Yay, now with tests for the auth prompt.

This also changes an out param in nsIAuthPrompt to inout, should have caught this in bug 382437 when we modified a couple others.

Comment 12

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

Comment on attachment 308263 [details] [diff] [review]
Patch v.3

I did a quick comparison between the behavior with this patch vs. the previous behavior of wallet code (or what I could figure out from reading singsign.cpp, at least). The old code seemed to always prompt the user to pick from a list of logins, based solely on the passed in username (all logins for the site where shown if no username was passed in). In appears to overwrite both passed in values with the values of the selected login (though it could be that it only set those that have a value - I'm not sure whether it supported "logins" with null or missing usernames/passwords).

That means that this patch will introduce different behavior for the "password only" and "password and username" cases, when existing logins exist for the realm - we'll end up using the passed-in password where the old wallet code used the selected login's password. I don't really care either way, assuming we can sort out what any potential users expect and make sure this is workable.

Comment 13

[Justin Dolske [:Dolske]]

Checking in toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js;
  new revision: 1.31; previous revision: 1.30
Checking in toolkit/components/passwordmgr/test/Makefile.in;
  new revision: 1.12; previous revision: 1.11
Checking in toolkit/components/passwordmgr/test/test_prompt.html;
  initial revision: 1.1
Checking in netwerk/base/public/nsIAuthPrompt.idl;
  new revision: 1.7; previous revision: 1.6