Sync password manager timestamps and other metadata

NEW
Assigned to

Status

()

P2
normal
9 years ago
2 months ago

People

(Reporter: zpao, Assigned: tcsc, Mentored)

Tracking

(Depends on: 2 bugs, Blocks: 4 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sync:passwords][sync-engine-addition])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 4 obsolete attachments)

Bug 465636 added timestamps to the password manager. We should sync that.
What does syncing that get us? Does the api let us create login infos with a given timestamp? (GMT?)
Syncing will get us MRU for passwords, at least.  I would _really_ like that in some cases!
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.
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.
Target Milestone: --- → 2.0

Updated

7 years ago
Target Milestone: 2.0 → ---

Updated

7 years ago
Priority: -- → P5
Target Milestone: --- → Future

Comment 5

7 years ago
rnewman: Is this implemented/possible on Android?
(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.
Blocks: 745408
Component: Firefox Sync: Backend → General
Priority: P5 → --
QA Contact: sync-backend → general
Whiteboard: [sync:passwords][sync-engine-addition]
Target Milestone: Future → ---
Blocks: 1079403
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Docs written before the feature. Howzat?

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

(Not deployed to docs.s.m.c yet.)
Depends on: 720592
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.
Created attachment 8505895 [details] [diff] [review]
Part 0: clean up passwords.js. v1

r=me.
Created attachment 8505896 [details] [diff] [review]
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.
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.
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?
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
(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.
Created attachment 8505923 [details] [diff] [review]
Part 2: don't listen to change notifications for password timestamps. v2

Now with more qref.
Attachment #8505914 - Attachment is obsolete: true
Depends on: 1097191
Comment on attachment 8505895 [details] [diff] [review]
Part 0: clean up passwords.js. v1

I landed this in Bug 1097191.
Attachment #8505895 - Attachment is obsolete: true
Depends on: 1098501
Priority: -- → P1
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?
Flags: needinfo?(rnewman)
We also talked about just syncing timePasswordChanged and timeCreated initially and sending the other metadata fields only for use on a new sync client.
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.
Flags: needinfo?(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?
(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
Priority: P1 → --
Relevant to this bug: Firefox for iOS does three-way merging for passwords, and also handles counts and some times. See Bug 1158216.
Component: General → Firefox Sync: Cross-client
Assignee: rnewman → nobody
Mentor: rnewman
Status: ASSIGNED → NEW
See Also: → bug 1332559

Updated

2 years ago
Priority: -- → P3

Updated

2 years ago
Blocks: 1337275
Let's try and sync usage counts and any other metadata in this bug.
Summary: Sync password manager timestamps → Sync password manager timestamps and other metadata

Updated

2 years ago
Duplicate of this bug: 1332559

Updated

2 years ago
Duplicate of this bug: 1167944
We should revisit this for lockbox
Priority: P3 → P2
(Assignee)

Comment 27

4 months ago
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
(Assignee)

Updated

3 months ago
Assignee: nobody → tchiovoloni
Comment hidden (mozreview-request)
(Assignee)

Comment 29

3 months ago
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.
Component: Firefox Sync: Cross-client → Sync
Product: Cloud Services → Firefox

Comment 30

3 months ago
mozreview-review
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 //)
Attachment #8974846 - Flags: review?(markh) → review+

Updated

3 months ago
Attachment #8505896 - Attachment is obsolete: true

Updated

3 months ago
Attachment #8505923 - Attachment is obsolete: true
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 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.
Attachment #8974846 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
You need to log in before you can comment on or make changes to this bug.