Closed Bug 468916 Opened 13 years ago Closed 13 years ago

make modifyLogin() smarter than just remove+add

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

(Keywords: fixed1.9.1)

Attachments

(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)
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?(gavin.sharp)
Attachment #352391 - Flags: review?(gavin.sharp) → review+
Attachment #352391 - Flags: approval1.9.1?
Pushed http://hg.mozilla.org/mozilla-central/rev/884a5c12182a
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [baking]
Comment on attachment 352391 [details] [diff] [review]
Patch v.2

a191=beltzner
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.