Expose telemetry client_id to about:addons and AMO remove SHIELD dependency from TAAR
Categories
(Firefox Graveyard :: Security: Review Requests, enhancement, P1)
Tracking
(firefox65+ verified, firefox66+ verified)
People
(Reporter: mlopatka, Assigned: mixedpuppy)
References
Details
(Keywords: sec-other, Whiteboard: [adv-main65-])
Attachments
(4 files, 1 obsolete file)
Comment 3•6 years ago
|
||
Comment 5•6 years ago
|
||
Comment 6•6 years ago
|
||
Comment 7•6 years ago
|
||
Comment 8•6 years ago
|
||
Comment 10•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 12•6 years ago
|
||
Assignee | ||
Comment 13•6 years ago
|
||
Reporter | ||
Comment 14•6 years ago
|
||
Assignee | ||
Comment 15•6 years ago
|
||
Comment 16•6 years ago
|
||
Assignee | ||
Comment 17•6 years ago
|
||
Reporter | ||
Comment 18•6 years ago
|
||
Comment 19•6 years ago
|
||
Comment 20•6 years ago
|
||
Assignee | ||
Comment 21•6 years ago
|
||
Assignee | ||
Comment 22•6 years ago
|
||
Comment 23•6 years ago
|
||
Assignee | ||
Comment 24•6 years ago
|
||
Comment 25•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 26•6 years ago
|
||
Comment 27•6 years ago
|
||
Comment 28•6 years ago
|
||
Assignee | ||
Comment 29•6 years ago
|
||
Comment 31•6 years ago
|
||
Reporter | ||
Comment 32•6 years ago
|
||
Reporter | ||
Comment 33•6 years ago
|
||
Comment 34•6 years ago
|
||
Comment 35•6 years ago
|
||
Updated•6 years ago
|
Reporter | ||
Comment 36•6 years ago
|
||
Assignee | ||
Comment 37•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Reporter | ||
Comment 38•6 years ago
|
||
Assignee | ||
Comment 40•6 years ago
|
||
Reporter | ||
Comment 41•6 years ago
|
||
Comment 42•6 years ago
|
||
Comment 43•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Comment 44•6 years ago
|
||
Turns out that the cookie is not passed through (seems like it's not considered first party), so our approach for this will revert to the header (see #c19).
NI the world to see if there are any concerns about this. I assume the data review portions of this are intact, and this is just about transmission method.
Updated•6 years ago
|
Comment 45•6 years ago
|
||
(Setting P1 because this was 65, and we'd like to preserve that if possible)
Updated•6 years ago
|
Comment 46•6 years ago
|
||
Turns out that the cookie is not passed through (seems like it's not considered first party), so our approach for this will revert to the header (see #c19).
Sounds good to me.
Assignee | ||
Comment 47•6 years ago
|
||
Assignee | ||
Comment 48•6 years ago
|
||
Note this will require a small change for beta uplift, specifically the webNavigation.loadURI changes to loadUriWithOptions for beta.
Comment 49•6 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #47)
Created attachment 9036718 [details]
Bug 1489531 Expose telemetry client_id header to about:addonsm-c patch to add header to request on about:addons
I tried this patch locally and made a few changes to the AMO/Disco Pane patch too. Running the Disco Pane locally inside about:addons
now works! The client ID is passed from FF to the AMO/Disco app, which then forwards this ID to the AMO API (HTTP call).
Assignee | ||
Comment 51•6 years ago
|
||
Assignee | ||
Comment 52•6 years ago
|
||
Assignee | ||
Comment 53•6 years ago
|
||
Assignee | ||
Comment 54•6 years ago
|
||
Comment on attachment 9037262 [details]
Bug 1489531 Expose telemetry client_id header to about:addons BETA
[Beta/Release Uplift Approval Request]
Feature/Bug causing the regression: None
User impact if declined: Recommendations will not be available
Is this code covered by automated tests?: No
Has the fix been verified in Nightly?: No
Needs manual test from QE?: Yes
If yes, steps to reproduce: load about:addons
load network monitor
view the "Get Add-ons" panel
in network monitor, look at request headers for the main request.
you should see a Moz-Client-ID header
Two prefs are required to be true for the header to be set:
datareporting.healthreport.uploadEnabled
browser.discovery.enabled
If either are false the header should not appear. So testing should be itterated along with changing these prefs
- both true
- uploadEnabled false, discovery.enabled true
- uploadEnabled true, discovery.enabled false
These steps would be the same for testing on m-c
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): The change is limited to about:addons. The patch adds a header when loading the discover panel on about:addons, no other browser should be affected with this.
String changes made/needed: none
Assignee | ||
Comment 55•6 years ago
|
||
@RyanVM, fyi patches approved, try runs are in comments above.
Comment 56•6 years ago
|
||
Updated•6 years ago
|
Comment 57•6 years ago
|
||
R+ from QA.
Tested on the following builds (using the steps from comment 54):
- beta try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1fff08a56737cf418d1599b66050d02d8bff99c&selectedJob=222513958
- m-c try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=baa5d6f32bba3eeb6290465f30ea022d1e78128f
Verified all the combinations of Boolean datareporting.healthreport.uploadEnabled
and browser.discovery.enabled
configs and confirmed the "Moz-Client-Id" header appeared only when both were true.
Comment 58•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Comment 60•6 years ago
|
||
Comment on attachment 9037262 [details]
Bug 1489531 Expose telemetry client_id header to about:addons BETA
[Triage Comment]
Fixes a header issue causing addon recommendations to not be available. Approved for 65.0b12.
Comment 61•6 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Comment 62•6 years ago
|
||
Marking this bug as verified based on comment #57, I've also verified using the steps provided on comment #54 and can confirm that the header only shows when both pref's listed in the above mentioned comment are set to true. Verified using Nightly 66.0a1 and 65.0b12.
Updated•5 years ago
|
Description
•