Closed Bug 1299784 Opened 4 years ago Closed 4 years ago

Sync pings should include a device id.

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: markh, Assigned: tcsc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Desktop pings aren't recording a device ID, which makes some problems difficult to analyze (eg, bug 1298758) - we can't tell 2 same-OS devices apart from each other when tracking repair behaviour. I think we should hash the fxa device ID and include it in each of our pings.
Blocks: 1298758
Rank: 1
Ryan, I suspect you have a better grasp of the considerations here - can you think of any operational or privacy concerns with writing a device ID the sync ping? If not, is it worth hashing the ID - the hash function isn't going to be secret so there may be no value in bothering to hash it.
Flags: needinfo?(rfkelly)
If unhashed, including the deviceid means we could take a sync ping and look up the corresponding user in FxA, finding out their email address etc.  We probably don't want that.

If hashed using all public data, we couldn't do that direct lookup, but we could start from a specific user in our FxA data, hash their device ids, and then look for any pings from that user in telemetry.  That's still not ideal.

It would be better if this were a HMAC using a secret key, in the same way that we hash the userid.  Then only folks with access to the key could perform that above lookup.  But of course you can't calculate that client-side.

Possible idea: include the metrics_uid in the hash, like SHA256(device_id + metrics_uid) or similar.   That would give you a stable unique identifier that can't be mapped back to the user's FxA identity without knowing the metrics HMAC key.
Flags: needinfo?(rfkelly)
Assignee: nobody → tchiovoloni
Status: NEW → ASSIGNED
Comment on attachment 8789035 [details]
Bug 1299784 - Include a hashed version of the device ID with the sync ping

I'm aware that there's still discussion around the right thing to do here and that this will likely change. 

Also, this patch was made under the assumption that metrics_uid is the hashed_fxa_uid we use elsewhere in the ping (it seems to be from looking at the tokenserver code, but I'm not sure).
Attachment #8789035 - Flags: feedback?(markh)
> Also, this patch was made under the assumption that metrics_uid is the hashed_fxa_uid
> we use elsewhere in the ping (it seems to be from looking at the tokenserver code, but I'm not sure).

Yep, that's the one I meant.
Comment on attachment 8789035 [details]
Bug 1299784 - Include a hashed version of the device ID with the sync ping

https://reviewboard.mozilla.org/r/77312/#review75680

This looks great to me. I think we should consider getting another bug on file so the UID and DeviceID isn't repeated in every sync (ie, appears only once in the entire ping) - I think that would also mean we need to observe a couple of other events relating to the device or the user changing and submit queued pings at that time, but that seems OK.

Can you please put a new version up with the improved comment, carry this r+ forward and request review from bsmedberg?

::: services/sync/modules/telemetry.js:231
(Diff revision 1)
>      if (error) {
>        this.failureReason = transformError(error);
>      }
>  
>      try {
>        this.uid = Weave.Service.identity.hashedUID();

let's comment here about why we are doing things this way (ie, paraphrasing Ryan's comment on this)
Attachment #8789035 - Flags: review+
Priority: P2 → P1
Comment on attachment 8789035 [details]
Bug 1299784 - Include a hashed version of the device ID with the sync ping

https://reviewboard.mozilla.org/r/77312/#review77062
Attachment #8789035 - Flags: review?(benjamin) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0123be9d2c87
Include a hashed version of the device ID with the sync ping r=bsmedberg,markh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0123be9d2c87
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.