1062388 - Add add-on signed status to FHR

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P5)

Description

[Dave Townsend [:mossop]]

Report whether add-ons are signed or not in FHR (this overlaps a little with bug 1062386 perhaps).

Comment 1

[Georg Fritzsche [:gfritzsche]]

Benjamin, would that be relevant to add in the Environment addon data?

Comment 2

[Dave Townsend [:mossop]]

Bug 1038068 adds a "signedState" property to add-ons that we'd like to show up in reporting so we can understand how much of the add-on population has been updated to signed add-ons.

Comment 3

[Benjamin Smedberg]

Yes, this would be in the environment block of unified telemetry.

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch

This adds signedState to the active add-ons in the environment and adds a signed add-on to test it. I found some weirdness in the test, test_addonsAndPlugins technically isn't checking the environment with the newly installed add-on as it isn't waiting for the environment to update so for test_signedAddon I had to add a change listener.

One issue that might need to be addressed is that we don't check signing for services and so internally they have a signedState of undefined, I don't know if the server will complain about that when all the other values are numeric. We could normalize undefined to some other number, 0 is SIGNEDSTATE_MISSING but I think I'd rather use a unique number to mean "we didn't check". Let me know what you'd prefer.

Spinning this through try now to double check but it passes all tests locally and I don't think there is any reason to expect this to cause more failures.

We'd like to land this a.s.a.p. as we have need of this data to verify that add-on signing on AMO is working as expected.

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 8613064 [details] [diff] [review]
patch

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

r+ for data collection review

Comment 6

[Vladan Djeric (:vladan)]

Mossop: btw, I noticed in about:telemetry that addons already have a "-signed" suffix in their version string. Is that enough?

Comment 7

[Dave Townsend [:mossop]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #6)
> Mossop: btw, I noticed in about:telemetry that addons already have a
> "-signed" suffix in their version string. Is that enough?

No this is only a temporary thing that exists for add-ons that are already on AMO. New add-ons uploaded to AMO won't have -signed in their version string.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8613064 [details] [diff] [review]
patch

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +503,5 @@
>          foreignInstall: addon.foreignInstall,
>          hasBinaryComponents: addon.hasBinaryComponents,
>          installDay: truncateToDays(installDate.getTime()),
>          updateDay: truncateToDays(updateDate.getTime()),
> +        signedState: addon.signedState,

It would be clearer if we only explicitly add this property if |addon.signedState != undefined|.

::: toolkit/components/telemetry/docs/environment.rst
@@ +144,5 @@
>              foreignInstall: <bool>,
>              hasBinaryComponents: <bool>
>              installDay: <number>, // days since UNIX epoch, 0 on failure
>              updateDay: <number>, // days since UNIX epoch, 0 on failure
> +            signedState: <integer> or undefined, // whether the add-on is signed by AMO

JSON.stringify drops any properties with value undefined, so the outgoing data never has this.
I think we should just document it as being present only for type "extension" (?).

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3c6c755707d5

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3c6c755707d5