Closed Bug 468916 Opened 14 years ago Closed 14 years ago

make modifyLogin() smarter than just remove+add


(Toolkit :: Password Manager, defect)

Not set





(Reporter: Dolske, Assigned: Dolske)



(Keywords: fixed1.9.1)


(1 file, 1 obsolete file)

Attached patch Patch v.1 (WIP) (obsolete) — Splinter Review
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.
Assignee: nobody → dolske
Attached patch Patch v.2Splinter Review
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).
Attachment #352374 - Attachment is obsolete: true
Attachment #352391 - Flags: review?(paul)
Attachment #352391 - Flags: review?(paul) → review+
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.
Yeah, we could do a:

[encUser, encPass, userCanceled] = encryptStuff(aLogin);
Comment on attachment 352391 [details] [diff] [review]
Patch v.2

Not sure if I can delegate reviews of my own patches. :)
Attachment #352391 - Flags: review?(
Attachment #352391 - Flags: review?( → review+
Attachment #352391 - Flags: approval1.9.1?
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [baking]
Comment on attachment 352391 [details] [diff] [review]
Patch v.2

Attachment #352391 - Flags: approval1.9.1? → approval1.9.1+
You need to log in before you can comment on or make changes to this bug.