Closed Bug 1149702 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect
Points:
3

Tracking

()

VERIFIED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 + verified
firefox41 --- verified

People

(Reporter: mossop, Assigned: dao)

References

Details

(Whiteboard: [fxsearch][searchhijacking])

Attachments

(2 files, 2 obsolete files)

      No description provided.
Depends on: 1047247
[Tracking Requested - why for this release]:

First two stages of add-ons signing work are targeted at Firefox 39.
Tracking this, along with its friends, for 39+
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.
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.
Flags: qe-verify+
Points: --- → 3
Flags: firefox-backlog+
Assignee: nobody → dao
Status: NEW → ASSIGNED
Iteration: --- → 40.2 - 27 Apr
Whiteboard: [fxsearch][searchhijacking]
Priority: -- → P1
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8599332 - Flags: review?(dtownsend)
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...
Attached patch patch (obsolete) — Splinter Review
Attachment #8599332 - Attachment is obsolete: true
Attachment #8599332 - Flags: review?(dtownsend)
Attachment #8599335 - Flags: review?(dtownsend)
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.
Attachment #8599335 - Flags: review?(dtownsend) → review-
(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>?
Flags: needinfo?(mjaritz)
Flags: needinfo?(dtownsend)
(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?
Flags: needinfo?(dtownsend)
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
Flags: needinfo?(mjaritz) → needinfo?(matej)
Attached patch patch v2Splinter Review
Attachment #8599335 - Attachment is obsolete: true
Attachment #8599817 - Flags: review?(dtownsend)
(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.
Flags: needinfo?(matej)
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.
Attachment #8599817 - Flags: review?(dtownsend) → review+
Depends on: 1160340
https://hg.mozilla.org/mozilla-central/rev/65afb97fc399
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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
(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.
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.
QA Contact: vasilica.mihasca
(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?
(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?
Kevin filed Bug 1164168
Depends on: 1164168
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.
Status: RESOLVED → VERIFIED
Depends on: 1178127
You need to log in before you can comment on or make changes to this bug.