1445309 - Document review process for adding HCT permissions

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Chris H-C :chutten]

Editing permissions to add HCT permission to a host isn't as simple as getting an r+ from a peer.

We need to decide on an appropriate series of reviews to gain approval for each host we wish to grant permission, in order to ensure we're not causing more problems than we're trying to solve.

bug 1444114 is the first consumer of this process, so it'll determine the broad strokes of what we need to document.

Comment 1

[Jan-Erik Rediger [:janerik]]

Attachment: Document review process for adding HCT permissions

Comment 2

[Chris H-C :chutten]

Comment on attachment 8962696 [details] [diff] [review]
Document review process for adding HCT permissions

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

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst
@@ +270,5 @@
>  
> +Data Review
> +===========
> +
> +Adding the ``hc_telemetry`` permission for a new domain in `browser/app/permissions <https://dxr.mozilla.org/mozilla-central/source/browser/app/permissions>`_ might require additional data review.

Let's go with "requires <link>Data Collection Review</link to data collection docs> as we are enabling a new method of data collection.".

Generally speaking implementors should already have Data Collection Review for their collection plans, but it doesn't hurt to double-check with Data Stewards.

@@ +274,5 @@
> +Adding the ``hc_telemetry`` permission for a new domain in `browser/app/permissions <https://dxr.mozilla.org/mozilla-central/source/browser/app/permissions>`_ might require additional data review.
> +
> +If the domain is already on the `list of restricted domains <https://hg.mozilla.org/integration/mozilla-inbound/file/39e131181d44/modules/libpref/init/all.js#l5120>`_
> +(configured by the ``extensions.webextensions.restrictedDomains`` preference), web extensions don't run on this domain and therefore cannot access the Telemetry API.
> +No additional approval is necessary.

Double-check this wording and that of the following paragraph with :pauljt. There may be specific cases to call out and specific language to use (like "Security Review" instead of "privacy review") and some resources (process documentation'd be nice) we can link to.

@@ -300,4 @@
>    let hostURI = Services.io.newURI("https://example.mozilla.org");
>    Services.perms.add(hostURI, "hc_telemetry", Services.perms.ALLOW_ACTION);
>  
> -

This line removal seems unrelated.

Comment 3

[Jan-Erik Rediger [:janerik]]

Attachment: Document review process for adding HCT permissions

Fixed the new line issue.
Changed the wording and added a link to data review.

:pauljt: Looping you in as :chutten suggested. Can you please check the wording and let me know if there's any good process documentation I can link to?

Comment 4

[Chris H-C :chutten]

Comment on attachment 8963135 [details] [diff] [review]
Document review process for adding HCT permissions

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

::: c/toolkit/components/telemetry/docs/collection/hybrid-content.rst
@@ +273,5 @@
> +
> +Adding the ``hc_telemetry`` permission for a new domain in `browser/app/permissions <https://dxr.mozilla.org/mozilla-central/source/browser/app/permissions>`_
> +requires `Data Collection Review <https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval>`_ as we are enabling a new method of data collection.
> +
> +If the domain is already on the `list of restricted domains <https://hg.mozilla.org/integration/mozilla-inbound/file/39e131181d44/modules/libpref/init/all.js#l5120>`_

Might want to start this off with some explanation "Giving a domain permission to use Hybrid Content Telemetry also gives any Extensions running on this domain permission to use Hybrid Content Telemetry." before explaining why the restricted domain list is helpful.

We should also be specific about saying "Hybrid Content Telemetry" since this doesn't give you unfettered access to Telemetry's JS API.

We should also call them "Extensions" instead of "Web Extensions" to align with product's nomenclature. (There are no "Web Extensions" any more. There are only "Extensions" and "Legacy Extensions")

Comment 5

[Jan-Erik Rediger [:janerik]]

Attachment: Document review process for adding HCT permissions

Comment 6

[Chris H-C :chutten]

Comment on attachment 8963576 [details] [diff] [review]
Document review process for adding HCT permissions

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

I like it. r?pauljt to make sure this is consistent with how he wants this sort of thing handled.

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Comment on attachment 8963135 [details] [diff] [review]
Document review process for adding HCT permissions

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

Thanks for the heads up. That looks like a reasonable process to me.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b45a79dd2a3
Document review process for adding HCT permissions r=pauljt

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4b45a79dd2a3