433762 - Sync stored passwords

Status: RESOLVED FIXED

(Cloud Services :: General, enhancement, P1)

Description

[Jono Xia]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5
Build Identifier: 

Implement an engine (and store and syncCore) for stored passwords so that they can be included in the data that's synced.

Reproducible: Always

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (work in progress)

This is mostly done, and mostly untested. :-) The main todos here are understanding a few bits I was unsure about, and figuring out how to represent a login as a GUID (serializing it into a string like "host:realm:password:etc" like the cookie sync does will probably work, although it's a little ugly.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: shim for login manager

This shim isn't currently needed, but it'll probably be useful starting point for when Weave supports incremental/as-needed syncing. This sits between the login manager and its storage module, and can intercept all changes for sync purposes. 

Attaching it here just so I don't lose it later. :-)

Comment 3

[Dan Mills [:thunder]]

Very hot.  I'm reading it now, comments:

+    // XXX use findLogins() to see if it exists?
...
+    // XXX Look for existing login with command.GUID?
+    //     ...looks like GUID can be anything (cookie is host:path:name)
+    // can specified old login not exist locally? fallback to addLogin?
+    // can specified new login already exist locally? fallback to removeLogin?

By the time commands make it to the store, they should already be known to apply to it.  That is, due to the way that commands are generated, you shouldn't get an edit command for an item that doesn't exist locally.  Fallback is generally not appropriate.

However, erroring in that case would be appropriate.  You can either throw an exception (which will stop execution of the sync of your data type), or log an error and continue, depending on the severity of the problem.

+  resetGUIDs: function PasswordStore_resetGUIDs() {
+    // XXX not sure what's up here?
+  }

You may ignore this if you are not randomly generating GUIDs.  The idea is to "start over" in cases where you've done a full wipe of the server, but if your GUID generation is deterministic and derived solely from the content you are syncing, there is nothing to reset.

+  _commandLike: function PSC_commandLike(a, b) {
+    // XXX what's this?
+    return false;
+  }

This probably does not apply to your data type.  When GUIDs are generated randomly, we use this function to compare two 'create' commands.  If they appear to be the same item we turn one of the 'create' commands into a 'change guid' command, so they become the same object going forward.

For example:

I add a bookmark called 'foo' with URI 'http://foo/' to my bookmarks bar on computer A.
I add another bookmark with the same title and URI to my bookmarks bar on computer B.

Then I sync.  Weave will catch this condition and instead of duplicating the bookmark, it'll make their GUIDs the same.  They will become the same item.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Yay, this works, although I haven't banged on it extensively.

A few nits:

1) Weave's preferences are not set up right. The prefs controlled by the UI are not the prefs used by the code (the code uses the pref name with "engine" stuck in the middle). File a new bug, or someone just want to make the obvious change?

2) Still had problems when creating a new profile and syncing to it, getting a "Keyring does not contain a key for this user" error. This seemed to go away when I used a nightly build instead of own debug build. Not sure why that is, there's nothing interesting in my build.

3) I sometimes hit asserts in my debug build: Assertion failure: OBJ_IS_CLONED_BLOCK(obj), at /Users/dolske/ff/trunk2/mozilla/js/src/jsobj.c:2016, perhaps this is the cause of some of the wonkiness I've been seeing. JS bug tickled by something Weave is doing?

Comment 5

[Dan Mills [:thunder]]

Comment on attachment 321899 [details] [diff] [review]
Patch v.2

1) Weave's preferences are not set up right.

No need to file a bug, just go ahead and fix it (or I will, it's my fault).

2) Still had problems when creating a new profile and syncing to it, getting a
"Keyring does not contain a key for this user" error. This seemed to go away
when I used a nightly build instead of own debug build. Not sure why that is,
there's nothing interesting in my build.

File a new bug please.  We'll want to make sure this is working.

3) I sometimes hit asserts in my debug build: Assertion failure:
OBJ_IS_CLONED_BLOCK(obj), at
/Users/dolske/ff/trunk2/mozilla/js/src/jsobj.c:2016, perhaps this is the cause
of some of the wonkiness I've been seeing. JS bug tickled by something Weave is
doing?

Hmm, I'm not sure what could be the problem.  Ask #js and/or file a bug? (it's not a weave bug, though).

Comment 6

[Dan Mills [:thunder]]

Comment on attachment 321899 [details] [diff] [review]
Patch v.2

As for the patch, it looks pretty good.  I'm still a little concerned about putting the password as part of the object ids.  But I think it's ready to go in, we'll continue to improve it.

Comment 7

[Justin Dolske [:Dolske]]

(In reply to comment #5)

> File a new bug please.  We'll want to make sure this is working.

Filed bug 435133.

Comment 8

[Anant Narayanan [:anant]]

Re-opening to track re-write of Password sync for 0.3

Comment 10

[Dan Mills [:thunder]]

You should really use bug 468697 instead, this one was for 0.2, and it really was fixed at the time.

Comment 11

[Anant Narayanan [:anant]]

Done :)

Comment 12

[Ayan Shah[:ashah]]

Investigating this bug for being potential crossweave automation test case
candidate.