make modifyLogin() smarter than just remove+add

RESOLVED FIXED in mozilla1.9.1

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1
fixed1.9.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 352374 [details] [diff] [review]
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.
(Assignee)

Updated

10 years ago
Assignee: nobody → dolske
(Assignee)

Comment 1

10 years ago
Created attachment 352391 [details] [diff] [review]
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).
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.
(Assignee)

Comment 3

10 years ago
Yeah, we could do a:

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

Comment 4

10 years ago
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+
(Assignee)

Updated

10 years ago
Attachment #352391 - Flags: approval1.9.1?
(Assignee)

Comment 5

10 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/884a5c12182a
Status: NEW → RESOLVED
Last Resolved: 10 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+
(Assignee)

Comment 7

10 years ago
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d1fd2f91887b
Keywords: fixed1.9.1
Whiteboard: [baking]
You need to log in before you can comment on or make changes to this bug.