Login Manager should store a GUID for logins.

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Password Manager
P1
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

({fixed1.9.1, relnote})

Trunk
mozilla1.9.2a1
fixed1.9.1, relnote
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 years ago
Weave wants to be able to access and assign GUIDs for the data items it syncs.

For the most part, this should be easy to add to the storage-mozStorage backend...

* add a |guid| column
* expose it as a new attribute on nsILoginInfo
* assign a random GUID to existing (migrated) logins
* extend findLogins() to take a GUID? add a findLoginByGUID()? neither?

Need to think through how some of the other interfaces should handle this. EG:
* should removeLogin / modifyLogin / login1.matches(login2) check GUIDs?
* should addLogin() generate the ID so the caller doesn't have to?

I suppose login.GUID == 0 could indicate "ignore me" (or "autogenerate me"). Although that kind of wildcarding has been a bit of a headache where it's used for other fields.

Not sure how this would impact alternate backends (eg OS X Keychain / Gnome Keyring) that might not have GUIDs available.

Comment 1

9 years ago
I'd like to see at least the schema changes for the GUID land ASAP, so that in the worst case Weave can bypass the login manager APIs and use mozStorage directly.  Of course, I'd rather not have to do that.

For reference, Places generates GUIDs on the fly when one is requested for a particular item.  However, this is because Places already has (non-globally-unique) IDs that can be used to reference items.  So Justin's idea of generate GUIDs for all items makes some sense if we plan to use that as the sole externally-accessible ID for a login.

Note, for Weave to use login manager APIs, it needs the ability to both get *and* set the GUID.  This is because Weave may decide an item on profile A is actually the same as an item on profile B, even though their GUIDs are different.

Comment 2

9 years ago
Justin, assigning to you as you asked me to.
I'd set blocking, but... I'm not sure which one to set. Help :)
Assignee: nobody → dolske
(Assignee)

Updated

9 years ago
Flags: blocking1.9.1?
Target Milestone: --- → mozilla1.9.1b3
(Assignee)

Comment 3

9 years ago
Created attachment 352239 [details] [diff] [review]
Patch v.1 (WIP)

This reworks the DB upgrade/downgrade code, and adds a GUID. Needs tests, and a change to modifyLogin() to retain the guid.
(Assignee)

Updated

9 years ago
Depends on: 468916
I guess it does no harm to keep ID around, even though GUIDs should be distinct, eliminating the need for ID.

Also, (from the patch) |_dbMigrateToVersion2()| should probably make use of |_dbCreateStatement()| for consistency's sake.
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Comment 5

9 years ago
I was slightly concerned about perf due to using nsIUUIDGenerator, because it has platform-specific native implementations (seems a little silly to not just always use NSS, but whatever).

I benchmarked total time to create 10,000 GUIDs in a loop (on my MBP, in VMs)...
  Windows = 45ms
     OS X = 67ms
    Linux = 42ms

That's fast enough that it's not a concern (certainly for the 1 call when adding a login, and well as the 1-time hit when we add a GUID for existing logins when upgrading).

Comment 6

9 years ago
Yay! Thanks Justin!
(Assignee)

Comment 7

9 years ago
Created attachment 355188 [details] [diff] [review]
Patch v.2
Attachment #352239 - Attachment is obsolete: true
Attachment #355188 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Attachment #355188 - Flags: review?(sdwilsh)
(Assignee)

Comment 8

9 years ago
The GUID parts of this patch are fairly straight forward -- we add a GUID column to the DB, and have addLogin() assign a GUID when creating a login.

Since this is the first patch to bump the schema version of signons.sqlite, the schema migration code had to be written, and existing code cleaned up. This code makes the DB more robust to upgrades and downgrades. Specifically, the case where a user tries a newer build, switches back to an older build (and perhaps saves a new login), and then eventually tries the newer build again. [The signons.txt migration issue described in bug 468180 isn't made any better or worse, though.]
Comment on attachment 355188 [details] [diff] [review]
Patch v.2

>+    _dbMigrateToVersion2 : function () {
>+        // Check to see if GUID column already exists.
>+        let exists = true;
>+        let query = "SELECT guid FROM moz_logins";
>+        let stmt;
>+        try { 
>+            stmt = this._dbConnection.createStatement(query);
>+            // (no need to execute statement, if it compiled we're good)
>+            stmt.finalize();
nit: don't need the let declaration outside of the try here, or query for that matter.

>+        // Get a list of IDs for existing logins
>+        query = "SELECT id FROM moz_logins WHERE guid isnull";
>+        stmt = this._dbConnection.createStatement(query);
>+        let wrappedStmt = Cc["@mozilla.org/storage/statement-wrapper;1"].
>+                          createInstance(Ci.mozIStorageStatementWrapper);
>+        wrappedStmt.initialize(stmt);
>+
>+        let ids = [];
>+        while (stmt.step())
>+            ids.push(stmt.getInt32(0));
Why aren't you using the function in the password manager code that is already used to create statements (it has the added benefit of wrapping it too)?  The only down side is that it caches statements, which isn't useful here.  Maybe we should add an optional boolean argument to _dbCreateStatement that indicates if it should be cached or not (default is yes)?

>+        // Generate a GUID for each login and update the DB.
>+        query = "UPDATE moz_logins SET guid = :guid WHERE id = :id";
>+        stmt = this._dbConnection.createStatement(query);
>+        wrappedStmt = Cc["@mozilla.org/storage/statement-wrapper;1"].
>+                      createInstance(Ci.mozIStorageStatementWrapper);
>+        wrappedStmt.initialize(stmt);
>+
>+        for each (let id in ids) {
>+            wrappedStmt.params.id   = id;
>+            wrappedStmt.params.guid = this._uuidService.generateUUID().toString();
>+            wrappedStmt.execute();
>+            wrappedStmt.reset();
>+        }
ditto

r=sdwilsh with the above addressed.
Attachment #355188 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 10

9 years ago
(In reply to comment #9)
           ids.push(stmt.getInt32(0));
> Why aren't you using the function in the password manager code that is already
> used to create statements (it has the added benefit of wrapping it too)?  The
> only down side is that it caches statements, which isn't useful here.

That's why. Adding a "don't cache this" flag might be a good idea.
Whiteboard: [needs review gavin]
P1 so we can get Weave testing with Beta 3
Priority: -- → P1
(Assignee)

Comment 12

9 years ago
Created attachment 356128 [details] [diff] [review]
Patch v.3

Updated with nits.

I decided to not bother with making _dbCreateStatement optionally not cache the statement, it's not that big of a deal and just adds complexity. I did, however, cleanup that function a bit so it does less dereferencing.
Attachment #355188 - Attachment is obsolete: true
Attachment #356128 - Flags: review?(gavin.sharp)
Attachment #355188 - Flags: review?(gavin.sharp)
Attachment #356128 - Flags: review?(gavin.sharp) → review+
Comment on attachment 356128 [details] [diff] [review]
Patch v.3

Worth adding a test that the GUID stays consistent across calls to modifyLogin, perhaps?
Whiteboard: [needs review gavin]
(Assignee)

Comment 14

9 years ago
(In reply to comment #13)
> Worth adding a test that the GUID stays consistent across calls to modifyLogin,
> perhaps?

Done.

Pushed http://hg.mozilla.org/mozilla-central/rev/a79a178b9ba8
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
(Assignee)

Comment 15

9 years ago
Pushed to branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3e0b2c529bf3
Keywords: fixed1.9.1
(Assignee)

Comment 16

9 years ago
Gavin noted today that we might want to relnote this for FF3.1B3...

Prior FF3.1 builds had busted pwmgr downgrade logic, so after running a build with this fix, earlier builds won't be able to use stored passwords. [They try to look for a function to downgrade v2 to v1, and that function doesn't exist.]

This particular issue isn't an issue for Firefox 2/3 users, because 3.1 is the first version to use mozStorage for logins. Nor should it be a problem in the future, because the new code is more graceful about handling unknown future DB schemas.
Keywords: relnote
(Assignee)

Updated

9 years ago
Depends on: 477917

Updated

8 years ago
Blocks: 482829

Comment 17

8 years ago
I had firefox 3.? and upgraded to the latest 3.2a1pre.
After the upgrade, I found that GreaseMonkey was not working and I wanted to go back to the previous version, but then all my passwords had gone because the format of signons.sqlite had changed.

In previous versions, a changed password manager always made a copy of the signons and used a new name (signons.txt -> signons2.txt)
We have upgrade/downgrade code for signons.sqlite, it just wasn't in some nightly builds from after Firefox 3 (and Firefox 3.1 beta 2). Running with an invalid file in those builds will cause bustage, but there's nothing we can do about that. Going forward, this won't be a problem.
You need to log in before you can comment on or make changes to this bug.