1149702 - Display a note about add-ons that aren't properly signed in the add-ons manager

Status: VERIFIED FIXED

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

Comment 1

[Dave Townsend [:mossop]]

[Tracking Requested - why for this release]:

First two stages of add-ons signing work are targeted at Firefox 39.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking this, along with its friends, for 39+

Comment 3

[Dave Townsend [:mossop]]

With the patch in bug 1038068 the addon.signedState property tells us about whether an add-on is signed correctly or not.

There seems to be some UX for this in bug 1148403.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Since it sounds now like this work is more likely to ride the trains with 40, I don't think it needs tracking, unless we end up needing to uplift after all.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

Attachment 8598783 [details] suggest making this a "warning" notification (yellow stripes). I made it an "error" one since we don't allow the user to enable add-ons that have been disabled for being unsigned.

I'm not sure where between the other state checks this state check should happen. I put it between blocked and softblocked. Also not sure if I should add a check for add-on signing being required on top of checking addon.appDisabled.

Comment 6

[Dão Gottwald [:dao]]

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

>--- a/toolkit/mozapps/extensions/content/extensions.js
>+++ b/toolkit/mozapps/extensions/content/extensions.js
>@@ -3057,27 +3057,37 @@ var gDetailView = {
> 
>       this.node.setAttribute("pending", pending);
>       document.getElementById("detail-pending").textContent = gStrings.ext.formatStringFromName(
>         "details.notification." + pending,
>         [this._addon.name, gStrings.brandShortName], 2
>       );
>     } else {
>       this.node.removeAttribute("pending");
>-
>       if (this._addon.blocklistState == Ci.nsIBlocklistService.STATE_BLOCKED) {

I didn't mean to remove this...

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

Comment 8

[Dave Townsend [:mossop]]

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

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

This is half-way there but we also want to display a note when signature checking is disabled. I think where you have the check right now is right for both cases so just check signedState then use appDisabled in there to differentiate. I agree with error for the case where the user can't enable, probably warning for the other though. For completeness please add checks in onPropertyChanged in the list item binding and gDetailView for signedState.

Should be straightforward to update browser_list.js and browser_details.js with simple tests for this.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Dave Townsend [:mossop] from comment #8)
> This is half-way there but we also want to display a note when signature
> checking is disabled.

What should the message be in that case? Should there still be a "More Information" link and should it also point to <https://wiki.mozilla.org/Addons/Extension_Signing>?

Comment 10

[Dave Townsend [:mossop]]

(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Dave Townsend [:mossop] from comment #8)
> > This is half-way there but we also want to display a note when signature
> > checking is disabled.
> 
> What should the message be in that case? Should there still be a "More
> Information" link and should it also point to
> <https://wiki.mozilla.org/Addons/Extension_Signing>?

The UX mockup at https://bug1047247.bugzilla.mozilla.org/attachment.cgi?id=8596160 has "[add-on name] is not verified to use in Firefox. Use with caution". That doesn't sound entirely right to me though. Markus did this get run past Matej?

Comment 11

[Markus Jaritz [:designakt] (UX)]

Thank you for checking. It has been run past Matej. We might have overlooked that there are 2 similar warnings. Deriving it from the other warning I assume this should read:
"[add-on name] could not be verified for use in Firefox. Use with caution."
Matej, can you please check if this sound ok. I also modified it in our String-Doc:
https://docs.google.com/document/d/1r_KdSfOms3uwNJjWykSBusMjUHwqR1uw0r5H-gG_4k8/edit

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 13

[Matej Novak [:matej]]

(In reply to Markus Jaritz [:maritz] from comment #11)
> Thank you for checking. It has been run past Matej. We might have overlooked
> that there are 2 similar warnings. Deriving it from the other warning I
> assume this should read:
> "[add-on name] could not be verified for use in Firefox. Use with caution."
> Matej, can you please check if this sound ok. I also modified it in our
> String-Doc:
> https://docs.google.com/document/d/1r_KdSfOms3uwNJjWykSBusMjUHwqR1uw0r5H-
> gG_4k8/edit

I would say "Proceed with caution." so we're not repeating "use."

Otherwise, looks good to me.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8599817 [details] [diff] [review]
patch v2

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

Awesome, thanks for the tests. r+ with the final tweak to the text that's in the bug.

::: toolkit/mozapps/extensions/test/browser/browser_list.js
@@ +148,5 @@
>  // changes
>  add_test(function() {
>    gCategoryUtilities.openType("extension", function() {
>      let items = get_test_items();
> +    is(Object.keys(items).length, 11, "Should be nine add-ons installed");

Change the text to just be "Should be the right number of add-ons installed" here and elsewhere in this patch so we don't have to forever change the text.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/65afb97fc399

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65afb97fc399

Comment 17

[DJ-Leith]

Attachment: Fx-40-Add-ons-2015-05-02.png

Windows 7 64bit
64bit Nightly 40.0a1 (2015-05-02)

In "about:config"
the "xpinstall.signatures.required" is set to the default, "false"

See attached Screenshot "Fx-40-Add-ons-2015-05-02.png".


Actual Result:
Warnings on all Add-ons.

[Add-on] could not be verified for use in Nightly. Proceed with caution.

https://bug1149702.bugzilla.mozilla.org/attachment.cgi?id=8599817
> +notification.unsigned=%1$S could not be verified for use in %2$S. Use with caution.

Similar wording; "Proceed with caution." vs "Use with caution."

So, it might not be this bug.

However, comment # 13 has
> I would say "Proceed with caution." so we're not repeating "use."


FYI, the "More Information" Link does go to
https://wiki.mozilla.org/Addons/Extension_Signing

So the
"xpinstall.signatures.infoURL"
which is set to "https://wiki.mozilla.org/Addons/Extension_Signing
is working.



All Profiles were Updated today.
Other profiles, with Release (Fx 37.0.2) or Dev Ed (Fx 39), and with similar Add-ons,
have no 'Warnings'.

Expected Result:
No Warnings

DJ-Leith

Comment 18

[Dão Gottwald [:dao]]

(In reply to DJ-Leith from comment #17)
> the "xpinstall.signatures.required" is set to the default, "false"

> Actual Result:
> Warnings on all Add-ons.

This is expected behavior as per comment 8. If xpinstall.signatures.required were true, your add-ons would be disabled. But since the pref is set to false, you only get warnings.

Comment 19

[Dave Townsend [:mossop]]

Yeah this looks working as intended (except for themes which should clear up in a day or so). AMO based extensions should become verified early next week.

Comment 20

[Philip Chee]

(In reply to Dão Gottwald [:dao] from comment #18)
> (In reply to DJ-Leith from comment #17)
> > the "xpinstall.signatures.required" is set to the default, "false"
> 
> > Actual Result:
> > Warnings on all Add-ons.
> 
> This is expected behavior as per comment 8. If xpinstall.signatures.required
> were true, your add-ons would be disabled. But since the pref is set to
> false, you only get warnings.

Can you turn off the warnings for SeaMonkey and Thunderbird? This is causing some angst with our nightly tester community. An #ifdef for - I think - MOZ_SIGNING or MOZ_SIGN_CMD might be appropriate?

Comment 21

[Dave Townsend [:mossop]]

(In reply to Philip Chee from comment #20)
> (In reply to Dão Gottwald [:dao] from comment #18)
> > (In reply to DJ-Leith from comment #17)
> > > the "xpinstall.signatures.required" is set to the default, "false"
> > 
> > > Actual Result:
> > > Warnings on all Add-ons.
> > 
> > This is expected behavior as per comment 8. If xpinstall.signatures.required
> > were true, your add-ons would be disabled. But since the pref is set to
> > false, you only get warnings.
> 
> Can you turn off the warnings for SeaMonkey and Thunderbird? This is causing
> some angst with our nightly tester community. An #ifdef for - I think -
> MOZ_SIGNING or MOZ_SIGN_CMD might be appropriate?

Can you file a bug please?

Comment 22

[Philip Chee]

Kevin filed Bug 1164168

Comment 23

[Vasilica Tamas, QA [:vtamas]]

Confirmed fixed on Firefox 41.0a1 (2015-06-07) and Firefox 40.0a2 (2015-06-07) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.9.5. 

"[add-on name] could not be verified for use in Firefox. Proceed with caution." message is displayed for add-ons that are not properly signed.