467463 - Login Manager should store a GUID for logins.

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Justin Dolske [:Dolske]]

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

[Dan Mills [:thunder]]

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

[Dan Mills [:thunder]]

Justin, assigning to you as you asked me to.
I'd set blocking, but... I'm not sure which one to set. Help :)

Comment 3

[Justin Dolske [:Dolske]]

Attachment: 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.

Comment 4

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

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.

Comment 5

[Justin Dolske [:Dolske]]

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

[Dan Mills [:thunder]]

Yay! Thanks Justin!

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Comment 8

[Justin Dolske [:Dolske]]

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 9

[Shawn Wilsher :sdwilsh]

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.

Comment 10

[Justin Dolske [:Dolske]]

(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.

Comment 11

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

P1 so we can get Weave testing with Beta 3

Comment 12

[Justin Dolske [:Dolske]]

Attachment: 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.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 356128 [details] [diff] [review]
Patch v.3

Worth adding a test that the GUID stays consistent across calls to modifyLogin, perhaps?

Comment 14

[Justin Dolske [:Dolske]]

(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

Comment 15

[Justin Dolske [:Dolske]]

Pushed to branch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3e0b2c529bf3

Comment 16

[Justin Dolske [:Dolske]]

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.

Comment 17

[Martin Sch]

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)

Comment 18

[:Gavin Sharp [email: [e-mail]]]

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.