Closed Bug 419081 Opened 14 years ago Closed 14 years ago

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

Categories

(Toolkit :: Password Manager, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: Gavin, Assigned: Dolske)

References

Details

Attachments

(1 file, 4 obsolete files)

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).
Flags: blocking-firefox3?
Summary: Password prompting logic doesn't check passwords against all possible existing passwords → Password prompting code doesn't check new passwords against all possible existing passwords
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Bug 419417 also ran across this issue, as a complication from a different problem.
Attached patch Patch v.1 (obsolete) — Splinter Review
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.
Assignee: nobody → dolske
Status: NEW → ASSIGNED
Attachment #306474 - Flags: review?(gavin.sharp)
Attached patch Patch v.1 (whitespace ignored) (obsolete) — Splinter Review
Attachment #306474 - Flags: review?(gavin.sharp) → review+
Target Milestone: --- → Firefox 3
(updated patch coming, now that bug 382437 has landed)
Attached patch Patch v.2 (obsolete) — Splinter Review
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.
Attachment #306474 - Attachment is obsolete: true
Attachment #306475 - Attachment is obsolete: true
Attachment #307119 - Flags: review?(gavin.sharp)
Attached patch Patch v.2 (whitespace ignored) (obsolete) — Splinter Review
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?
(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 "gavin@mozilla.com" and "gavinbot@mozilla.com".

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)
Target Milestone: Firefox 3 → Firefox 3 beta5
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)?
Duplicate of this bug: 421554
Attached patch Patch v.3Splinter Review
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.
Attachment #307119 - Attachment is obsolete: true
Attachment #307120 - Attachment is obsolete: true
Attachment #308263 - Flags: review?(gavin.sharp)
Attachment #307119 - Flags: review?(gavin.sharp)
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.
Attachment #308263 - Flags: review?(gavin.sharp) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Duplicate of this bug: 425006
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.