853584 - Don't show about:newaddon for blocklisted add-ons.

Status: RESOLVED INVALID

(Toolkit :: Add-ons Manager, defect)

Description

[Kris Maglione [:kmag]]

It seems that soft blocks are ineffective for add-ons installed via about:newaddon. They start off disabled, but about:newaddon opens as normal, with no warning, and can be used to enable the add-on as usual. The only place the warning is shown is in the add-on manager, which users are unlikely to open before accepting the installation.

I think that instead we should show the normal blocked add-on dialog, and skip about:newaddon entirely, in these cases.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

This is pretty important, since we're currently blocking lots of side-installed add-ons, and these blocks aren't working correctly.

Comment 2

[riadh.chtara]

Attachment: patch for newaddon.js

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 728764 [details] [diff] [review]
patch for newaddon.js

>+        document.getElementById("warning-soft-msg").value =  gStrings.ext.formatStringFromName(
>+            "details.notification.softblocked",
>+          [aAddon.name], 1

Indent is off.

>+        );

Same line as above, please.

It would be better to keep to the style of similar code in this file:

let warning = gStrings.ext.formatStringFromName("details.notification.softblocked",
						[aAddon.name], 1);
document.getElementById("warning-soft-msg").value = warning;

>+      <hbox id="warning-soft" hidden="true">
>+        <image id="warning-soft-icon"/>
>+        <description id="warning-soft-msg" flex="1"></description>
>+      </hbox>
...
>+#warning-soft {
>+  margin-bottom: 25px;
>+  -moz-box-align: start;
>+}
>+
>+#warning-soft-icon {
>+  list-style-image: url("chrome://mozapps/skin/extensions/alerticon-warning.png");
>+  width: 16px;
>+  height: 15px;
>+  -moz-margin-end: 5px;
>+}

UX will have to have a look at this. I don't think this warning is strong enough. We'll also need a more info link like we have in the other places where this warning appears.

Blair, can you have a look at this?

Comment 4

[riadh.chtara]

Attachment: addons with indention

Comment 5

[riadh.chtara]

Hey Kris,
I updated the patch. Sorry for these mistake

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 728770 [details] [diff] [review]
addons with indention

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

I like this better than comment 0. Comment 0 describes using the blocklist UI for this, but a modal dialog doesn't feel like the right fit for something that won't suddenly disable an add-on that a person has opted to use (since this is a new add-on). So contrary to comment 0, modifying about:newaddon as this patch does seems more appropriate.

But I think if we're going to add such a warning to about:newaddon, it's going to need to be a lot more visually stronger than this. It should also re-use the existing convention used in about:addons, which is:
* Red stripes at the top of the .addon-container element, using stripes-error.png
* Warning, in bold red, at the top
* Warning should include a link to get more information as to why the add-on is blocked

Screenshots of existing UI:
* http://i.imgur.com/a2YoVof.png
* http://i.imgur.com/gQ3zg4D.png

::: toolkit/mozapps/extensions/content/newaddon.js
@@ +79,5 @@
>        document.getElementById("location").hidden = true;
>      }
>  
> +    if (aAddon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED) {            
> +      let warning = gStrings.ext.formatStringFromName("details.notification.softblocked",

gStrings doesn't actually exist in newaddon.js - only in extensions.js.

Also, we can't re-use the details.notification.softblocked string for this, as we're using a string in a different context. It needs to be a separate string, even though it'll be the exact same wording.

So add a new string to newaddon.properties, which is already loaded earlier in this function.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #6)
> I like this better than comment 0. Comment 0 describes using the blocklist
> UI for this, but a modal dialog doesn't feel like the right fit for
> something that won't suddenly disable an add-on that a person has opted to
> use (since this is a new add-on). So contrary to comment 0, modifying
> about:newaddon as this patch does seems more appropriate.

Sorry, ignore all of comment 6 - just talked this over with Dave and Kris.

We don't show any UI for newly detected add-ons that are hard-blocked, and before we had about:newaddon we showed no UI for newly detected add-ons that are soft-blocked. We generally want to discourage enabling soft-blocked add-ons (scary warning + not super easy to do). Neither a warning in about:newaddon or the blocklist UI fit with this.

Therefore, lets make it so we just don't show any UI for soft-blocked add-ons. We figure out whether to open about:newaddon here:
https://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#559

Comment 8

[riadh.chtara]

Attachment: simple new addons

Comment 9

[riadh.chtara]

hey
I changed.
By the way,I have a stupid problem, I use ubuntu, and I could'not find an app that installs softblocked extension or addons to my browser to test if the changes I made works.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 729297 [details] [diff] [review]
simple new addons

If you know who should review a patch, you should generally flag it for review.

>+          if (!aAddon.userDisabled || !(aAddon.permissions & AddonManager.PERM_CAN_ENABLE) ||
>+              (aAddon.blocklistState == Ci.nsIBlocklistService.STATE_SOFTBLOCKED))

I wonder if we should be doing blocklistState != STATE_NOT_BLOCKED instead?

Comment 11

[riadh.chtara]

If, I'm not wrong, we need to hide the about:newaddon if the addons is SOFTBLOCKED.

Thank you for the hint (about reviewing), I never know that :)

Comment 12

[Kris Maglione [:kmag]]

Yes, but I think it might make sense to err on the side of caution and not show it if the add-on is any-kind-of-blocked.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Kris Maglione [:kmag] from comment #10)
> I wonder if we should be doing blocklistState != STATE_NOT_BLOCKED instead?

Nah, just testing STATE_SOFTBLOCKED is fine. And anyway, we've added a few blocklist states that don't disable add-ons - they're currently only used for plugins, but theoretically any add-on type can use them, and about:newaddon (theoretically) applies to any add-on type.

(In reply to riadh.chtara from comment #9)
> By the way,I have a stupid problem, I use ubuntu, and I could'not find an
> app that installs softblocked extension or addons to my browser to test if
> the changes I made works.

Yea, that can often be difficult. Your best bet is to use our test framework to install a mock add-on - we should have an automated test cover this change anyway. 

This test covers about:newaddon:
https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser_newaddon.js

You should be able to use a 'blocklistState' property on an object passed into gProvider.createAddons() to mimic a softblocked add-on.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 729297 [details] [diff] [review]
simple new addons

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

Looks good :) Just needs a test, as mentioned in comment 13.

Comment 15

[Kris Maglione [:kmag]]

(In reply to riadh.chtara from comment #9)
> hey
> I changed.
> By the way,I have a stupid problem, I use ubuntu, and I could'not find an
> app that installs softblocked extension or addons to my browser to test if
> the changes I made works.

No need to find software that side-installs an add-on. Just make sure extensions.autoDisableScopes & 1 is true (the default value of 15 works) and drop this (do-nothing) XPI in your profile's extensions directory: https://people.mozilla.com/~kmaglione/[e-mail]

Comment 16

[Kris Maglione [:kmag]]

(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #13)
> (In reply to Kris Maglione [:kmag] from comment #10)
> > I wonder if we should be doing blocklistState != STATE_NOT_BLOCKED instead?
> 
> Nah, just testing STATE_SOFTBLOCKED is fine. And anyway, we've added a few
> blocklist states that don't disable add-ons - they're currently only used
> for plugins, but theoretically any add-on type can use them, and
> about:newaddon (theoretically) applies to any add-on type.

In that case, we should probably show a warning in about:newaddon a la the initial patch for add-ons with other types of blocks, but that can be a different bug. It's probably not relevant until we have other types of blocks that apply to non-plugin add-ons anyway.

Comment 17

[riadh.chtara]

hey,
I tried the Kris's extension and it works fine. 
I'm very glad for that. (it's my first bug for the addons manager)

Comment 18

[Andy McKay]

Thank you Riadh for working on this bug. Unfortunately about:newaddon no longer exists with the new permissions model and WebExtensions. It's been 4 years since this bug was last commented on. I'd like to close this bug since I suspect its no longer valid. If that's incorrect, I'll reopen.

Thanks for your help Riadh.