468916 - make modifyLogin() smarter than just remove+add

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

The current implementation of modifyLogin in storage-mozStorage.js is a bit lazy, in that it's just a call to removeLogin() and addLogin() wrapped in a transaction.

I'd like to change this to actually modifying an existing row. This is specifically needed for bug 467463, so that we can modify a login without changing its GUID. This is clear way of operation, although it does mean adding a bit of extra code instead of just leveraging removeLogin/addLogin.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Adds a bit of testing for the "modify a non-existent login" case, other tests cover normal usage cases. (Specifically, the prompt tests exercise changing passwords).

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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

>+        let userCanceled, encUsername, encPassword;
>+        // Get the encrypted value of the username and password.
>+        [encUsername, userCanceled] = this._encrypt(newLogin.username);
>+        if (userCanceled)
>+            throw "User canceled master password entry, login not modified.";
>+
>+        [encPassword, userCanceled] = this._encrypt(newLogin.password);
>+        // Probably can't hit this case, but for completeness...
>+        if (userCanceled)
>+            throw "User canceled master password entry, login not modified.";

We now do this in a couple places (here and |_addLogin()|), just with different messages for the throw. Does it make sense to pull this into another method, or is it fine as is?

Otherwise r+ by me.

Comment 3

[Justin Dolske [:Dolske]]

Yeah, we could do a:

[encUser, encPass, userCanceled] = encryptStuff(aLogin);

Comment 4

[Justin Dolske [:Dolske]]

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

Not sure if I can delegate reviews of my own patches. :)

Comment 5

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/884a5c12182a

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner

Comment 7

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d1fd2f91887b