468697 - Password sync support

Status: RESOLVED FIXED

(Cloud Services :: General, defect)

Description

[Dan Mills [:thunder]]

Sync logins from pwmanager.

Comment 2

[Anant Narayanan [:anant]]

Reopening as this is the real bug for 0.3

Comment 3

[Anant Narayanan [:anant]]

Attachment: LoginManagerStorage shim for Weave

This component sets itself as a LoginManagerStorage module so that Weave may intercept LoginManager activity in it's password module tracker. The component uses the actual StorageManager to store the login data it receives.

Comment 4

[Anant Narayanan [:anant]]

A note on observer notifications created by the components:

event, topic, data
-- 
addLogin, "weave:login-shim", "added"
removeLogin, "weave:login-shim", "removed"
modifyLogin, "weave:login-shim", "modified"
removeAllLogins, "weave:login-shim", "deleted"

Comment 5

[Dan Mills [:thunder]]

Comment on attachment 361389 [details] [diff] [review]
LoginManagerStorage shim for Weave


>+    addLogin : function (aLogin) {
>+        this.log("--- addLogin ---");
>+        this._tee.addLogin(aLogin);
>+        this._observer.notifyObservers(this, "weave:login-shim", "added");
>+    },

I think what you actually want is:

this._observer.notifyObservers(this, "weave:login-shim:added", aLogin);

(and similar changes for removed, modified).

removeAllLogins should have a topic like "...:remove-all" or such.  "deleted" makes it sound like it's only one.

Comment 6

[Dan Mills [:thunder]]

Justin, if you are planning to add observer notifications to login manager and would prefer we use the same topics you intend to use, we can do that.  Just let Anant know.

Comment 7

[Anant Narayanan [:anant]]

Attachment: Password sync engine for 0.3

Following resolution of bugs 477917 and 467463, here's an updated version of the Password sync engine.

The changeItemID and update method haven't been implemented because I don't understand their purpose and if they'll be applicable to the password store. I've also not created a new type_record for passwords, creating an object directly in createRecord instead.

Also, we're iterating over a list of logins to find the one with a particular GUID. We can change this to use findLogins() when the interface to query by GUID is provided.

Comment 9

[Dan Mills [:thunder]]

Comment on attachment 364313 [details] [diff] [review]
Password sync engine for 0.3


>+  __proto__: SyncEngine.prototype,
>+  name: "Passwords",

nit: we use lowercase for the name, to keep the collection names all lowercase.

>+  changeItemID: function PasswordStore__changeItemID(oldID, newID) {
>+    /* FIXME: What does this mean? */
>+    this._log.debug("changeItemID called, unimplemented!");
>+  },

This means two clients have the same login:password:realm:whatever combination, but with different IDs.  In that case, the engine will change the local ID to match the remote one, so they in effect become the same item across the two clients.

Simply search for the local item with 'oldID', and change its GUID to be 'newID'.

>+  createRecord: function PasswordStore__createRecord(id) {
>+    let record = {};
>+    let login = this._getByGUID(id);
>+    
>+    if (login) {
>+      record.GUID = id;
>+      record.cleartext = {};
>+      record.cleartext.hostname = login.hostname;
...

You should implement a LoginRecord type that derives from CryptoWrapper.  Or at the very least create a CryptoWrapper here (though a typed record would be preferred).  If you don't have the encrypt/decrypt methods this code will fail:

http://hg.mozilla.org/labs/weave/file/tip/modules/engines.js#l427

Using a type record makes it cleaner because you don't have the 'cleartext' in there (since your type record can have getters for that info).

>+  update: function PasswordStore__update(record) {
>+    /* FIXME: Use nsILoginManagerStorage modifyLogin? */
>+    this._log.debug("update called, unimplemented!");

Please implement :-)

>+  _init: function PasswordTracker_init() {
>+    this.__proto__.__proto__._init.call(this);
>+    let observerService = Cc["@mozilla.org/observer-service;1"].
>+                          getService(Ci.nsIObserverService);
>+    observerService.addObserver(this, 'passwordmgr-storage-changed', false);

nit: Observers module is preferred.

Your tracker is missing code to track which items have changed, so they can be synced up on the next sync.

Comment 10

[Anant Narayanan [:anant]]

Attachment: Password sync engine for 0.3 (v2)

Okay, updated with implemented type record and missing methods. For some reason, resource gets called with a URI of 'null', causing an exception when a new channel is created. Is this because I've not implemented the typed record correctly?

Comment 11

[Dan Mills [:thunder]]

Attachment: Password sync engine for 0.3 (v3)

Fixes null Resource problem--createRecord has to set the encryption URI.  Also fixes a typo when calling nsIPropertyBag2's setPropertyAsAUTF8String.

There are 3 problems with this patch still:

1) Sync fails when applying records:

Error while applying incoming record: Could not convert JavaScript argument arg 0 [nsILoginManager.addLogin]

2) It is too slow when there are a lot of existing passwords

3) It's missing the UI bits in the pref pane

Comment 12

[Anant Narayanan [:anant]]

Attachment: Password sync engine for 0.3 (v4)

Okay, one more pass at this. We're caching loginInfo items now. Reconciliation works fine, but oddly enough addLogin was never called even once in my tests, it always tried to change the GUIDs to something that already exists (a case which results in the code not doing the guid change)

Comment 13

[Anant Narayanan [:anant]]

Attachment: Password sync engine for 0.3 (v5)

5 times' a charm! (I promise it works this time) :)

Comment 14

[Dan Mills [:thunder]]

Attachment: Password sync - v6

here's my current version.  there are several differences from v5 (it doesn't derive from v5, I started with v3)

This version doesn't work because the password store gets created twice somehow, and the second one never gets the loginInfo cache created, which causes itemExists to fail.

Comment 15

[Anant Narayanan [:anant]]

I think we're more or less done with this; we can reopen if any other major bugs are found down the road :)

Comment 16

[Ayan Shah[:ashah]]

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

Comment 17

[Ayan Shah[:ashah]]

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