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)
Tracking
()
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.
Assignee | ||
Updated•9 years ago
|
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
Assignee | ||
Comment 1•9 years ago
|
||
The mockup from bug 1120996
Assignee | ||
Comment 2•9 years ago
|
||
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..."
Assignee | ||
Comment 3•9 years ago
|
||
[Tracking Requested - why for this release]: First two stages of add-ons signing work are targeted at Firefox 39.
tracking-firefox39:
--- → ?
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
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.
Updated•9 years ago
|
tracking-firefox39:
? → ---
Assignee | ||
Updated•9 years ago
|
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
Assignee | ||
Updated•9 years ago
|
Whiteboard: [fxsearch][searchhijacking]
Assignee | ||
Updated•9 years ago
|
Priority: -- → P1
Assignee | ||
Comment 6•9 years ago
|
||
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)
Updated•9 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
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)
Comment 9•9 years ago
|
||
(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)
Assignee | ||
Comment 10•9 years ago
|
||
(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)
Comment 11•9 years ago
|
||
(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)
Comment 12•9 years ago
|
||
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)
Assignee | ||
Comment 13•9 years ago
|
||
(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.
Assignee | ||
Comment 14•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8602176 -
Flags: review?(dao)
Comment 17•9 years ago
|
||
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+
Assignee | ||
Comment 18•9 years ago
|
||
Made sense to me to bold the (unverified) in the final patch, which would you prefer Markus?
Flags: needinfo?(mjaritz)
Comment 19•9 years ago
|
||
Not having it bold makes it more obvious that it's an annotation rather than part of the add-on name, I think.
Assignee | ||
Comment 21•9 years ago
|
||
Landed without the theme changes: https://hg.mozilla.org/integration/fx-team/rev/b3b825c06914
https://hg.mozilla.org/mozilla-central/rev/b3b825c06914
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•