Closed Bug 1368545 Opened 7 years ago Closed 7 years ago

Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference

Categories

(WebExtensions :: General, enhancement, P2)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: [chrome_settings_overrides] triaged)

Attachments

(1 file)

Bug 1368545 - Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference,
59 bytes, text/x-review-board-request
aswan
: review+
Details 
chrome_settings_overrides uses the ExtensionPreferencesManager to manage changes to the browser.startup.homepage pref, but that is also exposed to users via the UI. If a user makes changes to this manually after an extension has set the value unexpected things can occur when an extension is uninstalled or disabled. This needs to be addressed, and it will be addressable once a solution for bug 1368537 lands.
See Also: → 1354344
As discussed at [1], there are some issues to be addressed when coming up with a solution for this.

1. Should an extension be able to change the homepage if a user has changed the homepage themselves manually via about:preferences?

2. Does the answer to #1 differ whether the user has recorded a specific value that differs from the default, or if the user has recorded a value that is the same as the default?

3. If the answer to #1 is "yes", should there be any sort of mechanism to let a user to allow an extension to make such a change? If so, what might this look like?

4. Regardless of the answer to 1, should there be an indication that an extension has set the value of the homepage on the about:preferences screen? I think the answer to this is "yes, ideally", which is why bug 1354344 exists, so I suppose the question would be better worded as "Should changes to about:preferences to support indicating that an extension has set the home page be implemented as part of this bug, or can they be treated as separate issues?"

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1368537#c3
Flags: needinfo?(aswan)
Andy, I'd be interested to hear your answers to the above questions as well.
Flags: needinfo?(amckay)
There are also backwards compat issues for this, as this feature has been available since 54. If our code to address this won't land until at least 55, and we do want to prevent extensions from overriding user-set values, how do we deal with installations where an extension has already overridden the home page?
(In reply to Bob Silverberg [:bsilverberg] from comment #1)
> As discussed at [1], there are some issues to be addressed when coming up
> with a solution for this.
> 
> 1. Should an extension be able to change the homepage if a user has changed
> the homepage themselves manually via about:preferences?

Yes, as long as its clear and easy to change back.

> 2. Does the answer to #1 differ whether the user has recorded a specific
> value that differs from the default, or if the user has recorded a value
> that is the same as the default?

Don't see why.

> 3. If the answer to #1 is "yes", should there be any sort of mechanism to
> let a user to allow an extension to make such a change? If so, what might
> this look like?

That's bug 1354344.

> 4. Regardless of the answer to 1, should there be an indication that an
> extension has set the value of the homepage on the about:preferences screen?
> I think the answer to this is "yes, ideally", which is why bug 1354344
> exists, so I suppose the question would be better worded as "Should changes
> to about:preferences to support indicating that an extension has set the
> home page be implemented as part of this bug, or can they be treated as
> separate issues?"

They are related, but should be seperate issues.
 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1368537#c3
Flags: needinfo?(amckay)
Maybe I'm not thinking about this creatively enough but I think the first and last parts of comment 4 are in direct contradiction with each other.  That is, I can't see a reasonable way to handle all the different cases here without about:preferences explicitly showing when the setting is controlled by an extension.
Flags: needinfo?(aswan)
(In reply to Andrew Swan [:aswan] from comment #5)
> Maybe I'm not thinking about this creatively enough but I think the first
> and last parts of comment 4 are in direct contradiction with each other. 
> That is, I can't see a reasonable way to handle all the different cases here
> without about:preferences explicitly showing when the setting is controlled
> by an extension.

Well, even if about:preferences doesn't indicate that an extension is in control of the setting, a user can still use about:preferences to change it back to whatever they like. So while that may be somewhat confusing to a user, and is definitely less than ideal, I don't think it precludes us from moving forward with back end changes to address the issue of making sure that when a user does make a manual change it doesn't cause problems later because of the precedence chain. It wouldn't be both "clear and easy to change back", but it would be "easy to change back".

I agree that ideally we'd implement both fixes at the same time (back end and UI changes), but I don't think it's an absolute requirement. There isn't a huge rush to get this done, so maybe it's fine to wait, but I am a bit concerned that this is a case that is in the wild and is not currently being handled properly.
To be clear, are you talking about the scenario:
1. Setting has value V1
2. Extension changes setting to V2, remembering the previous value V1
3. User manually changes setting to V3
4. Extension is disabled and setting is changed to V1

I agree that should be fixed, and its a small change, but the subject of this bug makes it sound like it is meant to be the bigger and more general issue.
(In reply to Andrew Swan [:aswan] from comment #7)
> To be clear, are you talking about the scenario:
> 1. Setting has value V1
> 2. Extension changes setting to V2, remembering the previous value V1
> 3. User manually changes setting to V3
> 4. Extension is disabled and setting is changed to V1
> 

Yes.

> I agree that should be fixed, and its a small change, but the subject of
> this bug makes it sound like it is meant to be the bigger and more general
> issue.

The above issue is what I had in my mind when I opened the bug, but I can see how the title makes it sound like it's about addressing all of the issues that this situation causes. I don't think we need to conflate the two major issues though which are:

1. The problem described in comment #7 above, in which values can be rolled back to something invalid.
2. Users cannot see that an extension is currently overriding the home page when they go to about:preferences.

I figured that whatever solution we come up for #1 would be somewhat general in that it could be used in other cases where settings that can be adjusted by WebExtensions are also available to be changed via the UI. There are a number of open bugs which would cause this situation if they are implemented, and I think we do want to implement at least some of them.
We already have bug 1354344 which covers #2.

Assuming we're all in agreement about my original question #1 (i.e., we all agree that an extension should not be prevented from changing a setting just because a user has previously set it), then I think we can move forward with a solution to #1, that is, making sure that the scenario described in comment #7 cannot happen. Are we all okay with that?
To address the specific problem as described in comment #7, we can change the routines that do the rolling back of preferences when an extension is disabled or uninstalled to check to see if the current value of the pref is what is expected. If it is not, then no rolling back would occur (i.e., the pref would be unchanged). This will also have to make sure that things are not rolled forward when an extension is re-enabled, if the current pref value is not what we expect.

This is not a perfect solution and is considered to be short-term, with the long-term solution involving changes to about:preferences to support various use cases that have been discussed.
Summary: Address the issue that chrome_settings_overrides for the home page can also be set via Preferences → Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference
If we are going to address the issue of preferences exposed via about:preferences using the technique we discussed yesterday and is presented in bug 1354344, which is to prevent a user from making manual changes when an extension has control, and only giving them the option to disable the extension, then I'm not sure it's worth the time and effort it would take to fix this bug.

This fix will not be required if we go that route, and if we proceed with this fix it is unlikely to land before 56 anyway, so maybe it makes sense to just wait for bug 1354344 to "fix" this issue.

What do you think, Andrew?
Flags: needinfo?(aswan)
I'm still trying to get over not liking the proposed solution to bug 1354344 :)

Seriously, I think this is a small change and it would also make the (admittedly uncommon) scenario where a user changes something in about:config slightly less annoying.  But I don't think there's any urgency to get it done now.
Flags: needinfo?(aswan)
Comment on attachment 8874931 [details]
Bug 1368545 - Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference,

https://reviewboard.mozilla.org/r/146304/#review150828

Since every caller of `processItem()` uses identical logic to get the expectedItem argument, why don't you just move that inside `processItem()`?  Also, for something that affects multiple preferences, this can cause some but not all to be set.  I think it would be better to check that all the preferences have expected values before setting anything so that when you return `false` that means "nothing was changed", not "some unknown number of things were changed"
Attachment #8874931 - Flags: review?(aswan) → review-
Comment on attachment 8874931 [details]
Bug 1368545 - Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference,

https://reviewboard.mozilla.org/r/146304/#review150828

Thanks aswan. I refactored the code to remove some of the duplication, and have changed the behaviour so that if one of the prefs doesn't match the expected value none of them will be changed.
Comment on attachment 8874931 [details]
Bug 1368545 - Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference,

https://reviewboard.mozilla.org/r/146304/#review151528

I don't understand why all the changes to test_privacy_disable.js are needed?

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:93
(Diff revision 2)
> - * @returns {Promise}
> - *          Resolves to true if the preferences were changed and to false if
> - *          the preferences were not changed.
>  */
> -async function processItem(name, item) {
> -  if (item) {
> +async function setPrefs(setting, item) {
> +  let prefs = item.initialValue || await setting.setCallback(item.value);

why do we await the setCallback?

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:104
(Diff revision 2)
> -      }
> +    }
> -    }
> +  }
> +}
> +
> +/**
> + * Commits a change to a setting and conditionally sets preferences as required.

Please clarify this -- "as required" will not mean anything to new readers of this code.

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:107
(Diff revision 2)
> +
> +/**
> + * Commits a change to a setting and conditionally sets preferences as required.
> + *
> + * @param {Extension} extension
> + *        The extension for which a setting is being set.

nit: "being set" -> "being modified"

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:113
(Diff revision 2)
> + * @param {Object} expectedItem
> + *        An object with a value property which indicates what the expected
> + *        value is for the setting before this change was made. This is
> + *        used to generate a list of expected preference values.

this is not a parameter

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:119
(Diff revision 2)
> + *          Resolves to true if all of the preferences were changed and to
> + *          false if any of the preferences were not changed.

this is misleading, it either sets all or nothing.  if actually setting fails for some reason, this code doesn't attempt to detect that.

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:129
(Diff revision 2)
> +  let item = await ExtensionSettingsStore[action](extension, STORE_TYPE, name);
> +  if (item) {
> +    let setting = settingsMap.get(name);
> +    let expectedPrefs = expectedItem.initialValue
> +      || await setting.setCallback(expectedItem.value);
> +    for (let pref in expectedPrefs) {

nit: this could also be `if (expectedPrefs.some(pref => Preferences.get(pref) != epectePrefs[pref]))`

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:207
(Diff revision 2)
>    async disableSetting(extension, name) {
> -    let item = await ExtensionSettingsStore.disable(
> +    return await processSetting(extension, name, "disable");
> -      extension, STORE_TYPE, name);
> -    return await processItem(name, item);
>    },

This should be a regular (non-async) function that just does `return processSeting(...);`

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:223
(Diff revision 2)
>    async enableSetting(extension, name) {
> -    let item = await ExtensionSettingsStore.enable(extension, STORE_TYPE, name);
> +    return await processSetting(extension, name, "enable");
> -    return await processItem(name, item);
>    },

same as above

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:239
(Diff revision 2)
>    async removeSetting(extension, name) {
> -    let item = await ExtensionSettingsStore.removeSetting(
> +    return await processSetting(extension, name, "removeSetting");
> -      extension, STORE_TYPE, name);
> -    return await processItem(name, item);
>    },

and again
Attachment #8874931 - Flags: review?(aswan) → review-
Comment on attachment 8874931 [details]
Bug 1368545 - Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference,

https://reviewboard.mozilla.org/r/146304/#review151528

Good call. I thought the changes were needed because of the data I was using in the test, and the fact that I am setting the initial values of one of the prefs to something other than the default, but it turns out that was actually a bug in my code that I have now fixed.

> why do we await the setCallback?

Because we don't know what the `setCallback` function will actually be, I thought it might be possible for a developer to define a `setCallback` on the settings object that is `async`, and if that were the case then we'd need to await it. I think perhaps at one point in my development of this a setCallback _was_ async, and that's why I did it initially. Currently none of the values of setCallback that are used anywhere are async, so I guess we can remove this, but it seems like a good safety net, and it allows a setCallback to be async if that's ever needed. Or am I misunderstanding how that would work? If a developer created a setCallback method that _is_ async, will everything work fine if we don't await it? My understanding of that is "no".

> nit: this could also be `if (expectedPrefs.some(pref => Preferences.get(pref) != epectePrefs[pref]))`

`expectedPrefs` is an object, not an array, so I don't think that will work.

> This should be a regular (non-async) function that just does `return processSeting(...);`

I don't understand why. `processSetting` is an async function, because it contains awaits which call other async functions. So doesn't that mean we have to await processSetting here?
Comment on attachment 8874931 [details]
Bug 1368545 - Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference,

https://reviewboard.mozilla.org/r/146304/#review151528

> Because we don't know what the `setCallback` function will actually be, I thought it might be possible for a developer to define a `setCallback` on the settings object that is `async`, and if that were the case then we'd need to await it. I think perhaps at one point in my development of this a setCallback _was_ async, and that's why I did it initially. Currently none of the values of setCallback that are used anywhere are async, so I guess we can remove this, but it seems like a good safety net, and it allows a setCallback to be async if that's ever needed. Or am I misunderstanding how that would work? If a developer created a setCallback method that _is_ async, will everything work fine if we don't await it? My understanding of that is "no".

This whole module assumes that `setCallback()` does a deterministic mapping from the extension-visible value of a setting to individual preference values.  An asynchronous `setCallback()` implies accessing some external source of information, which breaks that assumption.
I think it should be removed.  At that point, if a developer does write an asynchronous callback, then you'll get a Promise object here instead of an object containing preference values, so it should break in a pretty immediate and obvious way.

> `expectedPrefs` is an object, not an array, so I don't think that will work.

`Object.keys(expectedPrefs).some(...)`
I think it would be neater but feel free to ignore if you like the longer version.

> I don't understand why. `processSetting` is an async function, because it contains awaits which call other async functions. So doesn't that mean we have to await processSetting here?

No, I think you might be overthinking async functions.  They just return promises.  `processSetting()` is async so it returns a promise.  You want `disableSetting()` to return a promise so just writing `return processSetting(...);` will suffice.
Comment on attachment 8874931 [details]
Bug 1368545 - Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference,

https://reviewboard.mozilla.org/r/146304/#review152452

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:109
(Diff revision 4)
> + * Commits a change to a setting and conditionally sets preferences.
> + *
> + * If the change to the setting causes a different extension to gain
> + * control of the pref (or removes all extensions with control over the pref)
> + * then the prefs should be updated, otherwise they should not be.
> + * In addition, if the current value of any of theprefs does not

nit: need a space in "theprefs"

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:210
(Diff revision 4)
>     *
>     * @returns {Promise}
>     *          Resolves to true if the preferences were changed and to false if
>     *          the preferences were not changed.
>     */
>    async disableSetting(extension, name) {

this doesn't need to be async

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:226
(Diff revision 4)
>     *
>     * @returns {Promise}
>     *          Resolves to true if the preferences were changed and to false if
>     *          the preferences were not changed.
>     */
>    async enableSetting(extension, name) {

neither does this
Attachment #8874931 - Flags: review?(aswan) → review+
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5583ea8ddb57
Prevent the ExtensionPreferencesManager from mistakenly overriding a user set preference, r=aswan
https://hg.mozilla.org/mozilla-central/rev/5583ea8ddb57
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: