387617 - addLogin() should fail if user cancels master password prompt

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

During the review of bug 381164, Gavin asked about what happens in the following situation:

1) User starts Firefox, master password is enabled (but not yet entered)
2) User enters a new login into a form and submits
3) Password Manager asks if user wants to save the login, user clicks yes
4) Password Mananger attempts to encrypt the new login (to immediately write it to disk), which triggers the prompt for entering the master password
5) User clicks Cancel instead of entering the master password

The current code simply keeps the new login on memory. If the user later enters their master password and does something to cause flushing the login data to disk, the login will then be encrypted and written.

This is kind of a weird case, because the user has given us conflicting instructions -- we were told to save the password, but then canceled the needed master password prompt.

I now think it's better to have addLogin() fail in this case, and not keep the login in memory. Consider the following scenario:

1) User does the above stuff, clicking cancel at the master password prompt because they can't remember the master password.
2) User later remembers their master password, and decides to repeat the login procedure to make sure pwmgr saved the login
3) Pwmgr never prompts to save the login, because it already has the login stored in memory (just not written to disk)
4) User is frustrated next time they run Firefox, because it seems to have forgotten their password.

Having addLogin() fail also makes usage of the Master Password simpler to explain: it basically protects reading and *writing* stored logins.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1

This seems to do the trick, although it's a little more code than I would have liked.

Comment 2

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

Comment on attachment 272376 [details] [diff] [review]
Patch for review, v.1

I guess we can't really test this, given that it requires cancelling password prompts and such, right? Perhaps bug 388550 could help us here...

We should probably also be using Components.exception for these instead of just throwing strings, I think that provides more useful exceptions for external callers (that call via xpconnect). Can happen in a followup (I think there are existing cases that need to be fixed too).

Are there any concerns about callers of these functions not handling the exceptions properly?

r=me with those addressed.

Comment 3

[Justin Dolske [:Dolske]]

I think the addLogin() case could be tested in Litmus, using the steps described above. The removeLogin() case would be hard to test, though, without an extension. I don't think Firefox can ever actually try to delete a login without having requested it first, which would require entering a master password. [If the master password expires at just the right instant, I suppose that might happen.]

I'll file a followup for Components.exceptions, and double check that callers can deal properly with a throw.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.2

Comment 5

[Justin Dolske [:Dolske]]

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

[Bah, pressed enter at the wrong time. Stupid BZ...]

Seems I never got around to checking-in the reviewed patch... When updating it to current trunk, I decided I didn't really like the way it was handling failures. This is a little more brute-force, but I think what it's going is clearer.

Comment 6

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

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

r=me, I like this one better :) (though same review comments apply)

Comment 7

[Justin Dolske [:Dolske]]

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.14; previous revision: 1.13
done

Filed bug 394686 fox the Components.exception issue.