Closed Bug 1148021 Opened 9 years ago Closed 9 years ago

Warn users when installing an add-on not signed by Mozilla if the pref allows installation

Categories

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

x86
macOS
defect
Points:
2

Tracking

()

RESOLVED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Whiteboard: [fxsearch][searchhijacking])

Attachments

(3 files)

This should add the UI to tell the user that the add-on they are installing is not signed by Mozilla and so may be dangerous in some way. See the "click uncertified (current)" line here: https://bug1120996.bugzilla.mozilla.org/attachment.cgi?id=8560498

We need bug 1038068 before we can detect this case.
Depends on: 1148403
No longer depends on: 1148403
Blocks: 1149654
Summary: Warn users when installing an add-on not signed by Mozilla → Warn users when installing an add-on not signed by Mozilla if the pref allows installation
Attached image mockup
The mockup from bug 1120996
Looks like we need the following strings here:

"Caution: This site would like to install an uncertified add-on in %BRANDNAME%. Proceed at your own risk."
"Learn more..."
[Tracking Requested - why for this release]:

First two stages of add-ons signing work are targeted at Firefox 39.
Blocks: 1150122
There is a complication here when the site triggers multiple installs and some are unsigned and some are signed. To keep things simple I think for now we can just use the same text for that.
We should just be able to use install.addon.signedState == AddonManager.SIGNEDSTATE_MISSING to spot add-ons that need this warning once bug 1038068 lands.
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
Whiteboard: [fxsearch][searchhijacking]
Priority: -- → P1
Markus, the latest mockups for this (https://bug1120996.bugzilla.mozilla.org/attachment.cgi?id=8598797) show a learn more link. Should this just link to https://support.mozilla.org/en-US/kb/find-and-install-add-ons-add-features-to-firefox like the normal install confirmation?
Assignee: nobody → dtownsend
Flags: needinfo?(mjaritz)
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Dave, so far I would link all learn more to the same address. 
I have filed bug 1149506 for Joni to update the add-on SUMO to reflect verification.
(If with that we have a section we could link to, we might change that in future.)
Flags: needinfo?(mjaritz)
I think we need to tweak the string in the case where the user is installing a bundle of extensions and only some of them are unverified. The others may be either verified or don't need verification (like themes). Localisation of plural forms is going to make this very difficult but I think we can do a string like this:

Caution: This site would like to install #2 add-ons in #1. #3 of these add-ons are unverified. Proceed at your own risk.

Does this look like good text?
Flags: needinfo?(mjaritz)
Flags: needinfo?(matej)
(In reply to Dave Townsend [:mossop] from comment #8)
> I think we need to tweak the string in the case where the user is installing
> a bundle of extensions and only some of them are unverified. The others may
> be either verified or don't need verification (like themes). Localisation of
> plural forms is going to make this very difficult but I think we can do a
> string like this:
> 
> Caution: This site would like to install #2 add-ons in #1. #3 of these
> add-ons are unverified. Proceed at your own risk.
> 
> Does this look like good text?

The words look good, but can you give me examples of what would be in place of #1, #2 and #3?
Flags: needinfo?(matej)
(In reply to Matej Novak [:matej] from comment #9)
> (In reply to Dave Townsend [:mossop] from comment #8)
> > I think we need to tweak the string in the case where the user is installing
> > a bundle of extensions and only some of them are unverified. The others may
> > be either verified or don't need verification (like themes). Localisation of
> > plural forms is going to make this very difficult but I think we can do a
> > string like this:
> > 
> > Caution: This site would like to install #2 add-ons in #1. #3 of these
> > add-ons are unverified. Proceed at your own risk.
> > 
> > Does this look like good text?
> 
> The words look good, but can you give me examples of what would be in place
> of #1, #2 and #3?

Sorry yes, #2 is the total number of add-ons (will always be 2 or larger for this case), #1 is the application name, Firefox. #3 is the number of unverified add-ons, always 1 or larger.
Flags: needinfo?(matej)
(In reply to Dave Townsend [:mossop] from comment #10)
> (In reply to Matej Novak [:matej] from comment #9)
> > (In reply to Dave Townsend [:mossop] from comment #8)
> > > I think we need to tweak the string in the case where the user is installing
> > > a bundle of extensions and only some of them are unverified. The others may
> > > be either verified or don't need verification (like themes). Localisation of
> > > plural forms is going to make this very difficult but I think we can do a
> > > string like this:
> > > 
> > > Caution: This site would like to install #2 add-ons in #1. #3 of these
> > > add-ons are unverified. Proceed at your own risk.
> > > 
> > > Does this look like good text?
> > 
> > The words look good, but can you give me examples of what would be in place
> > of #1, #2 and #3?
> 
> Sorry yes, #2 is the total number of add-ons (will always be 2 or larger for
> this case), #1 is the application name, Firefox. #3 is the number of
> unverified add-ons, always 1 or larger.

The only case where it would be awkward is when both #2 and #3 are "2," but I guess that's unavoidable.

It could also be:

Caution: This site would like to install #2 add-ons in #1, #3 of which are unverified. Proceed at your own risk.

Would that work?
Flags: needinfo?(matej)
Does it make sens to install a bundle if one or more add-ons will not be installed as this might impact the function of the others? If so, we should tell the user,( or block the whole bundle. )
Flags: needinfo?(mjaritz)
(In reply to Matej Novak [:matej] from comment #11)
> (In reply to Dave Townsend [:mossop] from comment #10)
> > (In reply to Matej Novak [:matej] from comment #9)
> > > (In reply to Dave Townsend [:mossop] from comment #8)
> > > > I think we need to tweak the string in the case where the user is installing
> > > > a bundle of extensions and only some of them are unverified. The others may
> > > > be either verified or don't need verification (like themes). Localisation of
> > > > plural forms is going to make this very difficult but I think we can do a
> > > > string like this:
> > > > 
> > > > Caution: This site would like to install #2 add-ons in #1. #3 of these
> > > > add-ons are unverified. Proceed at your own risk.
> > > > 
> > > > Does this look like good text?
> > > 
> > > The words look good, but can you give me examples of what would be in place
> > > of #1, #2 and #3?
> > 
> > Sorry yes, #2 is the total number of add-ons (will always be 2 or larger for
> > this case), #1 is the application name, Firefox. #3 is the number of
> > unverified add-ons, always 1 or larger.
> 
> The only case where it would be awkward is when both #2 and #3 are "2," but
> I guess that's unavoidable.

In that case I'd be using the simpler string:

Caution: This site would like to install #2 unverified add-ons in #1. Proceed at your own risk.

> It could also be:
> 
> Caution: This site would like to install #2 add-ons in #1, #3 of which are
> unverified. Proceed at your own risk.
> 
> Would that work?

I'll use that.

(In reply to Markus Jaritz [:maritz] from comment #12)
> Does it make sens to install a bundle if one or more add-ons will not be
> installed as this might impact the function of the others? If so, we should
> tell the user,( or block the whole bundle. )

Yes you're right, that's a separate issue though. I've filed bug 1161645 to check on it.
Attached image screenshot
I realised that the string I was suggesting would be very difficult to localise correctly due to the presence of two numbers that could affect the plurality of various words in the text. Instead how about this string:

Caution: This site would like to install #2 add-ons in #1, some of which are unverified. Proceed at your own risk.

Then below in the list of add-ons we mark which are unverified as in the screenshot.
Flags: needinfo?(matej)
Looks perfect. Thanks.
Flags: needinfo?(matej)
Attached patch patchSplinter Review
Attachment #8602176 - Flags: review?(dao)
Comment on attachment 8602176 [details] [diff] [review]
patch

>     case "addon-install-confirmation": {
>+      let unsigned = installInfo.installs.filter(i => i.addon.signedState <= AddonManager.SIGNEDSTATE_MISSING);
>+      let someUnsigned = unsigned.length > 0 && unsigned.length < installInfo.installs.length;

&& unsigned.length < installInfo.installs.length is slightly confusing here. Might be worth a comment, or someUnsigned could use a better name to indicate that you want to exclude the case of all add-ons being unsigned.

>+      if (unsigned.length == installInfo.installs.length) {
>+        // None of the add-ons are verified
>+        messageString = gNavigatorBundle.getString("addonConfirmInstallUnsigned.message");
>+      }
>+      else if (unsigned.length == 0) {

nit: } else if (... on one line according to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

>+      is(container.childNodes[0].lastChild.getAttribute("class"),
>+         "addon-install-confirmation-unsigned", "Should have the unverified marker");

ok(container.childNodes[0].lastChild.classList.includes(...), ...); would be more robust

>-.addon-install-confirmation-name {
>+.addon-install-confirmation-name,
>+.addon-install-confirmation-unsigned {
>   font-weight: bold;
> }

Is this really wanted? The screenshot didn't show "(Unsigned)" in bold.
Attachment #8602176 - Flags: review?(dao) → review+
Made sense to me to bold the (unverified) in the final patch, which would you prefer Markus?
Flags: needinfo?(mjaritz)
Not having it bold makes it more obvious that it's an annotation rather than part of the add-on name, I think.
I agree with Dão. Not having unverified in bold.
Flags: needinfo?(mjaritz)
https://hg.mozilla.org/mozilla-central/rev/b3b825c06914
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.