Add add-on signed status to FHR

RESOLVED FIXED in Firefox 41

Status

()

defect
P5
normal
Rank:
55
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

(Blocks 1 bug)

unspecified
mozilla41
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify ?

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [hijacking][fxsearch])

Attachments

(1 attachment)

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

Updated

5 years ago
Blocks: signed-addons
No longer blocks: 1038071
Assignee

Updated

5 years ago
Flags: firefox-backlog+
Flags: qe-verify?
Points: --- → 2
Assignee

Updated

4 years ago
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.

Comment 3

4 years ago
Yes, this would be in the environment block of unified telemetry.
Flags: needinfo?(benjamin)
Assignee

Updated

4 years ago
Whiteboard: [fxsearch][searchhijacking]
Assignee

Updated

4 years ago
Priority: -- → P5

Updated

4 years ago
Whiteboard: [fxsearch][searchhijacking] → [hijacking][fxsearch]

Updated

4 years ago
Rank: 55
Assignee

Updated

4 years ago
Depends on: 1169744
Posted 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
Status: NEW → ASSIGNED
Attachment #8613064 - Flags: review?(vdjeric)
Attachment #8613064 - Flags: review?(gfritzsche)
Comment on attachment 8613064 [details] [diff] [review]
patch

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

r+ for data collection review
Attachment #8613064 - Flags: review?(vdjeric) → review+
Assignee

Updated

4 years ago
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]
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" (?).
Attachment #8613064 - Flags: review?(alessio.placitelli) → review+
https://hg.mozilla.org/mozilla-central/rev/3c6c755707d5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.