394611 - Always prompt the user before changing a stored password

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

Bug 226735 changed the "Save login? Yes/NotNow/Never" modal prompt to a notification bar.

The same thing could be done for the other pwmgr prompts, such as the prompt to confirm a password change.

With this change, we should also consider removing the case where pwmgr will update the password for a stored login without confirmation. It's probably doing this to avoid being annoying, but if there are cases where it might be doing the wrong thing always prompting with a notification bar wouldn't be a burden for the user. This issue was raised at recent pwmgr security/design review.

Comment 1

[Daniel Veditz [:dveditz]]

The auto-saving of changed passwords was probably inherited behavior from the old wallet-based PwMgr. I think the belief was that if the user manually changed the password from what was replayed they must have a very good reason, and the only reason must be that they want to change it.

This bites me all the time on the 'mailman' admin interface where there's no username (there's only one admin) and each mailing list typically has a different admin password (since different groups of people administer different lists). The password manager is completely useless on the site because each time it replays the password for the last list I administered rather than the one I want. What I would _like_ to happen in that case (but maybe it's too hard) is for PwMgr to remember the password for the list I edit 80% of the time and to ignore me (or ask) when I manually change the password 20% of the time.

On the other side of the coin, though, if there's a strong indication of a password-change form (current-PW plus two identical PW) it'd be a real bummer if people lost their new password because they didn't notice the subtle notification bar instead of the current in-your-face prompt. Not that I like modal dialogs, but password changes are rare enough that it may be a warranted case.

Comment 2

[(no longer active)]

Now that bug 408797 has landed, do we want a notification bar for other types of password manager prompts as well?

Comment 3

[Justin Dolske [:Dolske]]

I don't think any more prompting changes should be made for Firefox 3 at this point (or probably even can, given the string freeze).

Comment 4

[Justin Dolske [:Dolske]]

Mutating a bit...

Bug 408797 basically fixed this bug as titled; the only prompt that isn't a notification bar is the rarely-encountered promptToChangePasswordWithUsernames() prompt. We can spin that off, or just fix it along the way.

The remaining interesting idea in this bug is to get rid of the automatic password changing. Or at least reconsider when that can happen. It's nice to try and be clever about changing the password without needlessly prompting the user (especially with an irritating modal dialog), but I've seen a few cases of this biting users when the change was not desired.

Comment 5

[mmortal03]

Yep, it should prompt the user in a different way (to change it) if a previously saved login name is typed in, but with a different password.  Something along the lines of:

 "Has the password for this login name changed, or did you just accidentally mistype it?"
Choices:
1. It has changed, so change the saved password to this new one.
2. I mistyped it, so leave the saved password alone.
3. I don't want Firefox to remember the password for this login name anymore.

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Comment 10

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

Comment on attachment 340022 [details] [diff] [review]
Patch v.2

>diff --git a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>+                if (notifyBox)
>+                    this._showChangeLoginNotification(notifyBox,
>+                                                      selectedLogin, newLogin);
>+                else
>+                    this._pwmgr.modifyLogin(selectedLogin, newLogin);

I guess the idea behind not just calling this.promptToChangePassword is that the dialog-after-dialog is annoying, whereas notification-after-dialog isn't? I'm a bit wary of making the behavior diverge here based on the method used to prompt (dialog vs. notification)...

>diff --git a/toolkit/components/passwordmgr/test/test_notifications.html b/toolkit/components/passwordmgr/test/test_notifications.html

>+      case 18:
>+        // Check for change-password bar, u+p login on u+p form.
>+        is(gotUser, "notifyu1", "Checking submitted username");
>+        is(gotPass, "pass2",    "Checking submitted password");
>+        bar = getNotificationBar(notifyBox, "password-change");
>+        ok(bar, "got notification bar");
>+        clickNotificationButton(bar, "Change");
>+
>+        // cleanup
>+        login1.password = "pass2";
>+        pwmgr.removeLogin(login1);

This removeLogin is also verifying that the change worked, right? Behavior in case 20 is also tested by the removeLogin call - is it worth checking the behaviors of case 17 and 19 (don't change and change, respectively)?

The check you added in getNotificationBar presumably tests _removeLoginNotifications. Can we get tests for the HTTP auth change password notification too?

Comment 11

[Justin Dolske [:Dolske]]

(In reply to comment #10)

> I guess the idea behind not just calling this.promptToChangePassword is that
> the dialog-after-dialog is annoying, whereas notification-after-dialog isn't?
> I'm a bit wary of making the behavior diverge here based on the method used to
> prompt (dialog vs. notification)...

That's true, although I did it because the user won't know if the login worked until after the request is sent -- but if we prompt we'll block before sending the request.

Hmm, I suppose a better approach (for both cases) would be to monitor the request that's using the new password -- if it's successful just silently change the password (since we know it worked), and if it failed don't do anything. I'm not sure how to do that offhand, though. (Presumably it's just gluing a listener onto something.)

> This removeLogin is also verifying that the change worked, right? Behavior in
> case 20 is also tested by the removeLogin call - is it worth checking the
> behaviors of case 17 and 19 (don't change and change, respectively)?

No, the removeLogin is mainly just keeping things tidy so we don't end up with a pile of logins at the end of the test. But I suppose it would be good to test the actual button-click results... Shouldn't have anything going wrong at that point, but the extra coverage is obviously good. Looks like that's not already tested anywhere. :/

> The check you added in getNotificationBar presumably tests
> _removeLoginNotifications. Can we get tests for the HTTP auth change password
> notification too?

Hmm, I thought I did. But I don't see it in the patch or my tree. I wonder what happened. *sigh*

Comment 12

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

(In reply to comment #11)
> No, the removeLogin is mainly just keeping things tidy so we don't end up with
> a pile of logins at the end of the test.

Right, I know it wasn't intentional. It does end up testing the result unintentionally, though, right? The removeLogin will fail if the change failed.

Comment 13

[Justin Dolske [:Dolske]]

(In reply to comment #12)

> The removeLogin will fail if the change failed.

Right. At least, with storage-mozStorage it does. The legacy module seems to only throw if the specified host doesn't exist.

And, back to the original point:

(In reply to comment #10)

> This removeLogin is also verifying that the change worked, right? Behavior in
> case 20 is also tested by the removeLogin call - is it worth checking the
> behaviors of case 17 and 19 (don't change and change, respectively)?

These cases should already be handled, because the tests after them depend on their side effects. For example, if test 17 somehow did change the login, test 18 would fail because the changed value would be filled in and wouldn't match the value the test expects.

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Updated with HTTP auth prompt tests.

Comment 15

[Justin Dolske [:Dolske]]

Pushed changeset 2f36cde1694c.

Comment 16

[Justin Dolske [:Dolske]]

Backed out, seems to have caused leaks on OS X and Windows unit test boxes (although not Linux, interestingly...).

Comment 17

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Filed bug 459620 on the memory leak. I think it's an uncommon situation that the test just happened to exercise (and is actually unrelated to what this bug is fixing), so I've just added a simple workaround in the test.

Pushed changeset 44d774cce259.