Closed Bug 1062388 Opened 7 years ago Closed 6 years ago

Add add-on signed status to FHR


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




Tracking Status
firefox41 --- fixed


(Reporter: mossop, Assigned: mossop)



(Whiteboard: [hijacking][fxsearch])


(1 file)

Report whether add-ons are signed or not in FHR (this overlaps a little with bug 1062386 perhaps).
Blocks: signed-addons
No longer blocks: 1038071
Flags: firefox-backlog+
Flags: qe-verify?
Points: --- → 2
Depends on: 1124128, 1038068
Benjamin, would that be relevant to add in the Environment addon data?
Flags: needinfo?(benjamin)
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.
Yes, this would be in the environment block of unified telemetry.
Flags: needinfo?(benjamin)
Whiteboard: [fxsearch][searchhijacking]
Priority: -- → P5
Whiteboard: [fxsearch][searchhijacking] → [hijacking][fxsearch]
Rank: 55
Depends on: 1169744
Attached patch patchSplinter Review
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.
Assignee: nobody → dtownsend
Attachment #8613064 - Flags: review?(vdjeric)
Attachment #8613064 - Flags: review?(gfritzsche)
Comment on attachment 8613064 [details] [diff] [review]

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

r+ for data collection review
Attachment #8613064 - Flags: review?(vdjeric) → review+
Attachment #8613064 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
Mossop: btw, I noticed in about:telemetry that addons already have a "-signed" suffix in their version string. Is that enough?
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
Comment on attachment 8613064 [details] [diff] [review]

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" (?).
Attachment #8613064 - Flags: review?(alessio.placitelli) → review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.