system add-ons should not be disabled by users

RESOLVED FIXED in Firefox 50

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: rhelmer, Assigned: rhelmer)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Add-ons have a userDisabled state, however system add-ons should not be disabled by users.

AddonManager should ignore this state when addon.isSystem is true.

Updated

2 years ago
Whiteboard: triaged
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
See Also: → bug 1260450
(Assignee)

Comment 1

2 years ago
Created attachment 8766595 [details]
Bug 1281077 - do not allow hidden add-ons to be user disabled.

A few reviewer notes about this:

I looked into using the PERM_CAN_DISABLE permission instead, but that seems mostly advisory at this point. I could do this instead if you'd prefer.

This throws an exception to be consistent with what we do if someone attempts to disable the default theme.

Generally AddonManager seems to prefer to log and return a value for the client to
check. I made sure that about:performance doesn't break at least (about:perf is
the driving reason behind this change, see bug 1260450 for context).

Review commit: https://reviewboard.mozilla.org/r/61428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61428/
Attachment #8766595 - Flags: review?(aswan)
(Assignee)

Comment 2

2 years ago
Comment on attachment 8766595 [details]
Bug 1281077 - do not allow hidden add-ons to be user disabled.

I see a mistake in the test, let me fix that before you bother to review.
Attachment #8766595 - Flags: review?(aswan)
(Assignee)

Comment 3

2 years ago
Comment on attachment 8766595 [details]
Bug 1281077 - do not allow hidden add-ons to be user disabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61428/diff/1-2/
Attachment #8766595 - Flags: review?(aswan)

Updated

2 years ago
Attachment #8766595 - Flags: review?(aswan) → review+

Comment 4

2 years ago
Comment on attachment 8766595 [details]
Bug 1281077 - do not allow hidden add-ons to be user disabled.

https://reviewboard.mozilla.org/r/61428/#review58338

The change itself looks good and I think you have the essential tests.  I'm not sure about the purpose of some of the other tests (see line comments below) but I think you cover the important parts.

::: toolkit/mozapps/extensions/test/xpcshell/test_nodisable_hidden.js:13
(Diff revision 2)
> +const NORMAL_ID = "normal@tests.mozilla.org";
> +const NORMAL_ID2 = "normal@tests.mozilla.org";

did you mean for these to be the same?

::: toolkit/mozapps/extensions/test/xpcshell/test_nodisable_hidden.js:20
(Diff revision 2)
> +const SYSTEM_ID = "system@tests.mozilla.org";
> +const SYSTEM_ID2 = "system2@tests.mozilla.org";
> +
> +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "42");
> +
> +function* createNormalAddon(id, version, targetApp) {

nitpick, the only real difference between these two functions is the directory they write to, you could just have a single createAddon() that also takes dir as a parameter.

::: toolkit/mozapps/extensions/test/xpcshell/test_nodisable_hidden.js:83
(Diff revision 2)
> +  // install a new, normal add-on that is incompatible to ensure that appDisabled
> +  // state works correctly.
> +  let incompatAddon = yield promiseAddonByID(NORMAL_ID2);
> +  do_check_neq(incompatAddon, null);
> +  do_check_eq(incompatAddon.version, "2.0");
> +  do_check_eq(incompatAddon.name, "Test disabling hidden add-ons, non-hidden add-on case.");
> +  do_check_false(incompatAddon.isCompatible);
> +  do_check_true(incompatAddon.appDisabled);
> +  do_check_false(incompatAddon.userDisabled);
> +  do_check_false(incompatAddon.isActive);
> +  do_check_eq(incompatAddon.type, "extension");

I'm not sure what's being tested here?  You're upgrading the userDisabled add-on to one that is incompatible and checking that that reset userDisabled to false but did get marked with appDisabled true?  Seems like a reasonable test, but I'm not sure what it has to do with system addons?

::: toolkit/mozapps/extensions/test/xpcshell/test_nodisable_hidden.js:136
(Diff revision 2)
> +  // install a new system add-on that is incompatible to ensure that appDisabled
> +  // state works correctly.
> +  let incompatAddon = yield promiseAddonByID(SYSTEM_ID2);
> +  do_check_neq(incompatAddon, null);
> +  do_check_eq(incompatAddon.version, "2.0");
> +  do_check_eq(incompatAddon.name, "Test disabling hidden add-ons, hidden system add-on case.");
> +  do_check_false(incompatAddon.isCompatible);
> +  do_check_true(incompatAddon.appDisabled);
> +  do_check_false(incompatAddon.userDisabled);
> +  do_check_false(incompatAddon.isActive);
> +  do_check_eq(incompatAddon.type, "extension");

I don't understand how this test is related to the changes in this bug?
(Assignee)

Comment 5

2 years ago
Comment on attachment 8766595 [details]
Bug 1281077 - do not allow hidden add-ons to be user disabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61428/diff/2-3/
(Assignee)

Comment 6

2 years ago
https://reviewboard.mozilla.org/r/61428/#review58338

> I'm not sure what's being tested here?  You're upgrading the userDisabled add-on to one that is incompatible and checking that that reset userDisabled to false but did get marked with appDisabled true?  Seems like a reasonable test, but I'm not sure what it has to do with system addons?

You're right, this should just be removed.

This (and the one below) were intended to show that `appDisabled` still works, but we have other tests that cover that.

Comment 7

2 years ago
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62e6d24988ec
do not allow hidden add-ons to be user disabled. r=aswan

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/62e6d24988ec
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.