555755 - Sync password manager timestamps and other metadata

Status: NEW

(Firefox :: Sync, defect, P3)

Description

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

Bug 465636 added timestamps to the password manager. We should sync that.

Comment 1

[Ed Lee :Mardak]

What does syncing that get us? Does the api let us create login infos with a given timestamp? (GMT?)

Comment 2

[Mike Connor [:mconnor]]

Syncing will get us MRU for passwords, at least.  I would _really_ like that in some cases!

Comment 3

[Ed Lee :Mardak]

Just making sure, this is if you have multiple logins to the same site. For server-wins for old changes, we can do this without these extra timestamps.

Either way, I guess we'll be relying on the right client clocks.

Comment 4

[Mike Connor [:mconnor]]

This is metadata that's going to be useful for the core user experience, in time.  not immediately critical, and only affects trunk, so punting to 2.0 at least.

Comment 5

[Gregory Szorc [:gps]]

rnewman: Is this implemented/possible on Android?

Comment 6

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #5)
> rnewman: Is this implemented/possible on Android?

We get these:

    SyncColumns.DATE_CREATED,
    SyncColumns.DATE_MODIFIED,

    Passwords.TIME_LAST_USED,
    Passwords.TIMES_USED

The DB is the same as desktop (plus a deleted items table), but the access layer is all new and under our control. If it's in the DB, we can get it and sync it.

Comment 7

[Richard Newman [:rnewman]]

Docs written before the feature. Howzat?

https://github.com/mozilla-services/docs/commit/48b67c083c690a927a86d23579804cd5607b9083

(Not deployed to docs.s.m.c yet.)

Comment 8

[Richard Newman [:rnewman]]

Taking a dependency on Bug 720592. Not having that fix will result in this situation:

* Stable pair of devices.
* Device A modifies a password and syncs.
* Device B tries to use its existing copy of the password. (It'll fail, presumably, but it doesn't matter.)
* Device B syncs.

Because we now track timestamps, and Sync doesn't have even a remotely sophisticated record merging or conflict resolution algorithm, *the local record will win* because its timestamp is newer.

In a world where clock skew exists, we can even get data loss in seemingly safe scenarios (indeed, we do so today):

* Stable pair.
* Device B uses a password.
* Device A changes the password _later_ and syncs.
* Device B syncs. It thinks its earlier change happened later, because its clock is fast. It'll overwrite the password change with the old password.

If the clock is fast by a lot, this will happen a lot. Which is bad.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Part 0: clean up passwords.js. v1

r=me.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Part 1: Sync password manager timestamps (desktop). v1

This patch shuffles nsILoginMetaInfo values in and out of Sync password records.

No tests yet, so not requesting review.

This is necessary but not sufficient for this feature; we'll need more sophisticated reconciling before we can be confident in its safety.

Comment 11

[Richard Newman [:rnewman]]

Also: the login manager storage layer will send a change notification when the metadata for a password record changes:

267         this._sendNotification("modifyLogin", [oldStoredLogin, newLogin]);

we should skip changes that aren't 'material', which will avoid major conflicts during record application (but not avoid the need for merging to avoid losing data).

See line ~314 in passwords.js.

Comment 12

[Richard Newman [:rnewman]]

Attachment: Part 2: don't listen to change notifications for password timestamps. v1

Untested, but this looks like it should work, no?

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8505896 [details] [diff] [review]
Part 1: Sync password manager timestamps (desktop). v1

Review of attachment 8505896 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/engines/passwords.js
@@ +100,5 @@
>  }
>  PasswordStore.prototype = {
>    __proto__: Store.prototype,
>  
> +  _propertyBag: function () {

I'd be inclined to call this something like _newPropertyBag as casual inspection may lead people to believe it's returning a single instance.

@@ +112,5 @@
>      if (record.formSubmitURL && record.httpRealm) {
>        this._log.warn("Record " + record.id + " has both formSubmitURL and httpRealm. Skipping.");
>        return null;
>      }
>      

and here's some trailing whitespace you might as well kill while touching this

Comment 14

[Mark Hammond [:markh] [:mhammond]]

(In reply to Richard Newman [:rnewman] from comment #12)
> Created attachment 8505914 [details] [diff] [review]
> Part 2: don't listen to change notifications for password timestamps. v1
> 
> Untested, but this looks like it should work, no?

Looks fine to me on a quick glance.

Comment 15

[Richard Newman [:rnewman]]

Attachment: Part 2: don't listen to change notifications for password timestamps. v2

Now with more qref.

Comment 16

[Richard Newman [:rnewman]]

Comment on attachment 8505895 [details] [diff] [review]
Part 0: clean up passwords.js. v1

I landed this in Bug 1097191.

Comment 17

[Chris Karlof [:ckarlof]]

I'd like to move this forward, because it's important for the 2015 passwords effort.

I propose that we start adding these additional fields, but don't use the information in the fields to necessarily make any decisions about merging or resolving conflicts -- instead we just defer to the existing merge logic in Sync.

My understanding of the current proposal in the patch is that we don't track changes to the meta-data, meaning changes to these values don't cause those records to be pushed. I'd like to challenge that a bit. What are the concerns? Traffic? 

I realize there are some interesting problems in merging the meta-data. To help make this easier, would it make sense to track the password usage metadata on a per-client basis (maybe limited to a fixed number of clients, LRU eviction), rather than having one global value we're trying to sync? E.g., 

timesUsed: [ { clientId: 1, timesUsed: 3 }, { clientId: 2, timesUsed: 6 } ]

In this case we could show the total timesUsed in UI as the sum of all the individual values. Clients would merge in values from the server for other clients, and update and push the count for themselves. If we exceed the limit, remove the one at the end of the array, push yourself on the front.

This strategy would also make some sense for the other meta-data fields ("timeLastUsed", "timeCreated", "timePasswordChanged"). In theory in the future, we could show in the UI e.g., "Created at July 12, 2014 on your Android phone."

I know that legacy clients will ignore and delete those values, but eventually updated clients will win the slap fight.

What do you think Richard?

Comment 18

[Matthew N. [:MattN]]

We also talked about just syncing timePasswordChanged and timeCreated initially and sending the other metadata fields only for use on a new sync client.

Comment 19

[Richard Newman [:rnewman]]

Apologies for the long response!

tl;dr: if you want timely exchange of counters and timestamps without increasing the risk of material data loss, clients need to track more state and merge more intelligently.


(In reply to Chris Karlof [:ckarlof] from comment #17)

> My understanding of the current proposal in the patch is that we don't track
> changes to the meta-data, meaning changes to these values don't cause those
> records to be pushed. I'd like to challenge that a bit. What are the
> concerns? Traffic? 

Not traffic; we'd only use a small score bump, so we wouldn't significantly increase sync frequency. We'd upload more data, but I'm not concerned about this.

What I am concerned about: tracking those changes would make records change -- i.e., be marked as changed on the server -- much more frequently.

That would cause this to be hit:

> I propose that we start adding these additional fields, but don't use the
> information in the fields to necessarily make any decisions about merging or
> resolving conflicts -- instead we just defer to the existing merge logic in
> Sync.

... the existing merge logic in Sync doesn't handle conflicts well enough to use with frequent conflicts where you care about *any* fields in *either* record. There's no concept of an unimportant change, or a partial change, in the protocol, and trying to add it would be very convoluted.

Bug 720592 and Bug 1098501 have a lot of discussion about how to fix this. Greg, Nick, and I are fairly confident that biting the bullet and implementing three-way merge in the client is the best way forward.


As I noted in Comment 8, we lose data today (I've seen in myself, back when we had mandatory MoCo password changes, as have other devs -- you changed a password, then your change got reverted after syncing). This is one reason I'm in favor of just fixing password sync.


But marking a record as changed every time a timestamp changes will cause an increase in conflicts, and thus increase the rate of data loss. Our current approach to syncing passwords is only acceptable because the records don't change often, and most users actually aren't syncing between multiple devices.


Sync clients do a two-way merge when there's a changed record on the server and a local change. The client literally doesn't know what changed on either end, so it just throws away the local change -- which might have been something important.

Bug 1098501 is to not do that, and to instead track the shared parent to be able to determine what changed, and merge correctly. Simply ignoring the unimportant fields for merging or reconciling isn't really possible without some amount of fine-grained change tracking.

Without it:

Best-case -- if you use devices serially and they sync completely before you use another -- things would work out. No conflicts.

Moderate case -- if you don't use devices serially etc., *but you never change your passwords* -- we get unreliable timestamps and counters, because we would throw away conflicting counts each time. Your suggestion to use per-device attributes would help with this, at the cost of making the record format bulkier.

Worst-case -- if you change passwords without being very careful about being totally in sync first -- you risk losing password changes due to activity on other devices, node reassignments, etc.


> I realize there are some interesting problems in merging the meta-data. To
> help make this easier, would it make sense to track the password usage
> metadata on a per-client basis (maybe limited to a fixed number of clients,
> LRU eviction), rather than having one global value we're trying to sync?

In a world with proper three-way merge, it's not really a big win, because conflicts on these counter/timer fields are really easy to resolve if you know the old value. 

(Server went from timesUsed = 4 to 5, local went from 4 to 5, easy to compute that new total = 6.)


Without three-way merge, splitting these fields would allow for accurate timers/counters by avoiding conflicts on those fields. We would still need to find a way to avoid uploading records -- e.g., avoid increasing the conflict rate by discarding any notion of timeliness for the new fields, and only upload a changed record when one of the material fields changed.

The obvious downside to this: it'd be weeks or months before activity on each device made it to the others. At that point I'd consider just splitting out a totally separate collection on the server for password metadata.

Comment 20

[Richard Newman [:rnewman]]

(In reply to Matthew N. [:MattN] from comment #18)
> We also talked about just syncing timePasswordChanged and timeCreated
> initially and sending the other metadata fields only for use on a new sync
> client.

Syncing those two fields is possible without increasing conflict risk -- the former only changes alongside a material change, and the latter never changes.

I'm not sure what the second half of your sentence means.

Do you mean that clients would never trigger an upload or a conflict as a result of other timestamp/counter fields, so we'd essentially accept that they'll be delayed, inaccurate approximations that simply help start a new client closer to reality than zero?

Comment 21

[Matthew N. [:MattN]]

(In reply to Richard Newman [:rnewman] from comment #20)
> Do you mean that clients would never trigger an upload or a conflict as a
> result of other timestamp/counter fields, so we'd essentially accept that
> they'll be delayed, inaccurate approximations that simply help start a new
> client closer to reality than zero?

Yes

Comment 22

[Richard Newman [:rnewman]]

Relevant to this bug: Firefox for iOS does three-way merging for passwords, and also handles counts and some times. See Bug 1158216.

Comment 23

[Mark Hammond [:markh] [:mhammond]]

Let's try and sync usage counts and any other metadata in this bug.

Comment 26

[Mark Hammond [:markh] [:mhammond]]

We should revisit this for lockbox

Comment 27

[Thom Chiovoloni [:tcsc] (ex-moco)]

Worth noting that some of the work from this got done in bug 973166.

AFAICT from the lockbox document here [0], lockbox wants both timesUsed and timeLastUsed, both of which are handled on iOS [1], but not AFAICT on Android [2].

For desktop, I think a lot of the concerns might be alleviated if we use weak upload if we see a metadata-only change -- this will allow us to upload the change while still preferring remote changes in the case of conflicts. But OTOH we need to make sure we respect ignoreAll, _ignored, and getSyncCredentialsHosts, among possible others, so maybe this is more trouble than it's worth.

[0]: https://docs.google.com/document/d/17aekZDfkzlt20ZyZOAmJHb3D964VWsMqXLEno2vGhTk

[1]: https://github.com/mozilla-mobile/firefox-ios/blob/3eb6f840f4840040e285ee3b560d551ca1f542a7/Sync/Synchronizers/LoginsSynchronizer.swift#L48-L49

[2]: https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/domain/PasswordRecord.java#90-108

Comment 28

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 555755 - Sync timesUsed and timeLastUsed for passwords weakly.

Review commit: https://reviewboard.mozilla.org/r/243220/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243220/

Comment 29

[Thom Chiovoloni [:tcsc] (ex-moco)]

This took much longer than I had hoped and was generally more of a pain than it seems like it should have been.

MattN, I'm just asking for your review for the toolkit/components/passwordmgr part, (ofc if you have comments on the rest that's fine too). It's possible that I should have split it into separate patches, let me know if you want me to do that.

Comment 30

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8974846 [details]
Bug 555755 - Sync timesUsed and timeLastUsed for passwords weakly.

https://reviewboard.mozilla.org/r/243220/#review249858

It sucks that we need to jump through these hoops and the end result probably means that these fields really aren't particularly useful, but for the reasons Richard outlined, it's very tricky to do much better. This is at least a step in the right direction though.

::: services/sync/modules/engines/passwords.js:435
(Diff revision 1)
>  
> -  async _trackLogin(login) {
> -    if (Utils.getSyncCredentialsHosts().has(login.hostname)) {
> +  _shouldTrack(login) {
> +    if (this.ignoreAll || this._ignored.includes(login.guid)) {
> +      return false;
> +    }
> -      // Skip over Weave password/passphrase changes.
> +    // Skip over Weave password/passphrase changes.

While here, let's update the comment to reflect reality: "skip over FxA credentials changes" or similar.

::: services/sync/tests/unit/test_password_engine.js:316
(Diff revision 1)
> +    newLogin.QueryInterface(Ci.nsILoginMetaInfo);
> +
> +    Assert.ok(newLogin);
> +    Assert.equal(newLogin.timeLastUsed, now - 60 * 1000 * 1000);
> +
> +    // Really this is still wrong, but it's better than if we just took the most recent change.

I'm not sure this comment adds value and may be misleading - the same comment applies for timeLastUsed, right? It might be worth a comment above both checks saying that only take these remote fields if they are greater than the values we have.

(Interestingly, this strategy will mean that some future client will be unable to "fix" values, but this whole thing sucks a bit, so I think the general strategy makes sense for now)

::: toolkit/components/passwordmgr/LoginHelper.jsm:287
(Diff revision 1)
>        // Automatically update metainfo when password is changed.
>        if (newLogin.password != aOldStoredLogin.password) {
>          newLogin.timePasswordChanged = Date.now();
>        }
> +
> +      // Update meta-info properties on if provided

typo in this comments (probably just s/on //)

Comment 31

[Matthew N. [:MattN]]

Comment on attachment 8974846 [details]
Bug 555755 - Sync timesUsed and timeLastUsed for passwords weakly.

https://reviewboard.mozilla.org/r/243220/#review249064

::: toolkit/components/passwordmgr/LoginHelper.jsm:287
(Diff revision 1)
>        // Automatically update metainfo when password is changed.
>        if (newLogin.password != aOldStoredLogin.password) {
>          newLogin.timePasswordChanged = Date.now();
>        }
> +
> +      // Update meta-info properties on if provided

I think you're missing a word or something: "on if provided"

::: toolkit/components/passwordmgr/LoginHelper.jsm:288
(Diff revision 1)
> +      if (aNewLoginData instanceof Ci.nsILoginMetaInfo) {
> +        if (aNewLoginData.timesUsed) {
> +          newLogin.timesUsed = aNewLoginData.timesUsed;
> +        }
> +        if (aNewLoginData.timeLastUsed) {
> +          newLogin.timeLastUsed = aNewLoginData.timeLastUsed;
> +        }
> +        if (aNewLoginData.timePasswordChanged) {
> +          newLogin.timePasswordChanged = aNewLoginData.timePasswordChanged;
> +        }
> +        if (aNewLoginData.timeCreated) {
> +          newLogin.timeCreated = aNewLoginData.timeCreated;
> +        }

I haven't found time to audit the usage of this method to ensure that this won't cause problems. In some ways I could see Math.min/Math.max being wanted in this method for some of the metadata rather than an overwrite.

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 8974846 [details]
Bug 555755 - Sync timesUsed and timeLastUsed for passwords weakly.

https://reviewboard.mozilla.org/r/243220/#review250936

I can't say I'm confident this won't regress anything since this is used inside `modifyLogin` which is called from many different places but worst case it's just the metadata that regresses.

Comment 33

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8974846 [details]
Bug 555755 - Sync timesUsed and timeLastUsed for passwords weakly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243220/diff/1-2/

Comment 34

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 35

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.