Allow .js preference files to set locked prefs

ASSIGNED
Assigned to

Status

()

Core
Preferences: Backend
ASSIGNED
10 years ago
3 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 326060 [details] [diff] [review]
patch

The attached patch allows to use lockPref() in any .js preference file (prefs.js, files in default/preferences...)
Attachment #326060 - Flags: review?(darin.moz)
(Assignee)

Comment 1

10 years ago
Created attachment 326061 [details] [diff] [review]
patch

Obviously, neither the debian changelog nor the fprintf should be in the patch. The first patch doesn't exist. You haven't seen it. The only patch existing is this one.
Assignee: nobody → mh+mozilla
Attachment #326060 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #326061 - Flags: review?(darin.moz)
Attachment #326060 - Flags: review?(darin.moz)
Attachment #326061 - Flags: review?(darin.moz) → review?(benjamin)

Comment 2

10 years ago
Because I apparently inherited ownership of this code by default, I'm going to need some more summary here:

* what is the motivation for this change?
* where did lockPref use to work? In what new cases are we making it work?
* I'm going to need tests. I think writing an xpcshell test for this shouldn't be too hard.

Comment 3

10 years ago
Comment on attachment 326061 [details] [diff] [review]
patch

canceling review request until questions are answered
Attachment #326061 - Flags: review?(benjamin)
(Assignee)

Comment 4

10 years ago
(In reply to comment #2)
> Because I apparently inherited ownership of this code by default, I'm going to
> need some more summary here:
> 
> * what is the motivation for this change?
> * where did lockPref use to work? In what new cases are we making it work?

lockPref is an exclusive feature of the autoconfig stuff right now. Autoconfig uses a single file, located in the application directory, and which name is given by the general.config.filename preference, with pseudo security featured by a general.config.obscure_value preference.

Autoconfig also allows to use javascript constructs, contrary to standard pref files that are limited to pref() and user_pref().

The problem we had in debian is that we need to lock some preferences, to avoid users to harm themselves with bad preferences. app.update.enable, for instance. The first solution was to use the general.config.filename/obscure_value pair to just do that. The problem is that this file being provided by the package, it is also overwritten on any package upgrade, which means admins needing to use autoconfig just can't.

I found it a convenient way to solve this by allowing standard preferences to lock preferences.

I'm pretty sure you can find some other uses to this feature.

> * I'm going to need tests. I think writing an xpcshell test for this shouldn't
> be too hard.

Can you still review the patch without the tests ? 
Attachment #326061 - Flags: review?(benjamin)

Comment 5

9 years ago
Frankly, to solve the specific case of Debian users trying to enable updates, I think the best course of action is to get rid of the app.update.url pref from the Debian builds: then even if the user were to turn updates on, nothing bad could happen.

I'm trying to find somebody to help walk through the twisted jungle here; this is not likely to make it for FF3.1 because it touches something close to PAC, which is some of our most fragile code.
(Assignee)

Comment 6

9 years ago
This has nothing to do with app.update.url, but with admins wanting to lock some prefs to their users without fidling with general.config.filename.

Comment 7

9 years ago
Which may be a worthwhile use case, but I'm pretty sure we don't want extensions to be able to lock preferences (for example), and it's not clear to me that we distinguish between admins and extensions very well.
(Assignee)

Comment 8

9 years ago
Actually, AFAIK, an extension can already lock preferences...

Comment 9

9 years ago
Comment on attachment 326061 [details] [diff] [review]
patch

At least extensions should not be allowed to set locked prefs, so r-. I'm not sure what the right design is.
Attachment #326061 - Flags: review?(benjamin) → review-
(Assignee)

Comment 10

9 years ago
As I said, extensions can already lock preferences...
Comment on attachment 326061 [details] [diff] [review]
patch

Re-requesting review due to glandium's overlooked comment.
Attachment #326061 - Flags: review- → review?(benjamin)

Comment 12

9 years ago
How can an extension lock preferences?
(Assignee)

Comment 13

9 years ago
Created attachment 347810 [details]
extension locking a pref

This is one way i was thinking of for an extension to lock a preference. There are, I guess, many others.

Updated

9 years ago
Duplicate of this bug: 467738

Comment 15

9 years ago
Comment on attachment 326061 [details] [diff] [review]
patch

ok, yes, extensions can do anything. I don't think we want them doing this, and this will be an attractive nuisance.
Attachment #326061 - Flags: review?(benjamin) → review-
QA Contact: preferences → preferences-backend
(Assignee)

Comment 16

8 years ago
The point is, extensions already can lock prefs, so why not allow this patch, which doesn't change that, and file another bug to prevent extensions from locking prefs ?
(Assignee)

Comment 17

8 years ago
Comment on attachment 326061 [details] [diff] [review]
patch

Re-requesting review per comment #16
Attachment #326061 - Flags: review- → review?(benjamin)

Updated

8 years ago

Updated

7 years ago

Comment 18

6 years ago
Comment on attachment 326061 [details] [diff] [review]
patch

I cannot review this without spending boatloads of time understanding the use cases and code, which I don't think is worth my time at this point. Sorry :-(
Attachment #326061 - Flags: review?(benjamin)

Comment 19

6 years ago
Just voted for this bug. We are locking certain prefs in the default installation (for example browser and extension updates are done via packages, so we diable eg "app.update.enabled").

Debian is shipping a patch in their packages, Ubuntu isn't (see https://bugs.launchpad.net/ubuntu/+source/thunderbird/+bug/623844).
(Assignee)

Comment 20

6 years ago
Created attachment 572068 [details] [diff] [review]
Allow .js preference files to set locked prefs with lockPref()

Refreshed after the PRBool/bool change.

Now, as bsmedberg won't review it, and dwitte is essentially not around anymore, who can review this?
(Assignee)

Comment 21

6 years ago
Created attachment 576542 [details] [diff] [review]
Allow .js preference files to set locked prefs with lockPref()

Sorry for the noise, just testing "hg bzexport". I'm still interested to know who could review this, though.
(Assignee)

Updated

6 years ago
Attachment #572068 - Attachment is obsolete: true
This is a fly by night review.

I don't think it should be "lockPref" like autoconfig. With autoconfig, it's a function call.

With the default pref files, you're specifying a preference.

so:

locked_pref("foo", "bar")

makes more sense it that context (the same as user_pref)

Looking through the code, it looks like you've assumed that if a pref is locked, it becomes a default as well, is that correct?

The code looks good to me, but I'm not the right person to do this.

I think this is a good solution to the problem you are seeing and makes it a lot easier for enterprises to lock prefs, so this would be a good thing to have.

Updated

6 years ago
Blocks: 720362

Updated

4 years ago
Duplicate of this bug: 788481

Comment 24

4 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] (Away 24-25 October) from comment #18)
> Comment on attachment 326061 [details] [diff] [review]
> patch
> 
> I cannot review this without spending boatloads of time understanding the
> use cases and code, which I don't think is worth my time at this point.
> Sorry :-(

In that case, can you suggest someone that could review it?
Flags: needinfo?(benjamin)

Comment 25

4 years ago
No, at this point I don't think there is anyone who can own and review this.
Flags: needinfo?(benjamin)

Comment 26

3 years ago
seems like there is some interest in this bug over in 

	https://bugzilla.mozilla.org/show_bug.cgi?id=984012#c15
	https://bugzilla.mozilla.org/show_bug.cgi?id=984012#c17

to help protect international users from getting phished into changing the pref named security.turn_off_all_security_so_that_viruses_can_take_over_this_computer

and not necessarily understanding the ramifications of that action if they don't understand English.

Updated

3 years ago
See Also: → bug 984012
(Assignee)

Comment 27

3 years ago
Created attachment 8613433 [details] [diff] [review]
Allow .js preference files to set locked prefs with lockPref()

Refreshed after bug 1098343. Benjamin, would you like to review this, this time, considering comment 26?
Attachment #576542 - Attachment is obsolete: true
Attachment #8613433 - Flags: review?(benjamin)
You didn't address my comments.

I really don't want it being called lockPref. That will confuse it with the autoconfig "lockPref" function.

There is already enough confusion with pref()
(Assignee)

Comment 29

3 years ago
(In reply to Mike Kaply [:mkaply] from comment #28)
> I really don't want it being called lockPref. That will confuse it with the
> autoconfig "lockPref" function.

What is confusing? It's the same syntax. The only difference aiui, is the execution environment, which means autoconf can do more javascript-y things, which, as you point out, is already a source of confusion. I'd argue that having two different names is what would be confusing.
It's not the same syntax, that's my point.

pref() in a prefs.js file is NOT the same as pref() in an AutoConfig.js file.

pref() in prefs.js sets the default pref, defaultPref() in an Autoconfig sets a default pref.

user_pref() in prefs.js sets a user pref, pref() in an AutoConfig file sets a user preference.

So what you're doing is making it look like autoconfig.js and prefs.js use the same syntax, when they do not.
(In reply to Mike Kaply [:mkaply] from comment #30)
> It's not the same syntax, that's my point.

I tend to agree here.  I think this could be a useful patch (I'd use it instead of doing autoconfig stuff) but it's kind of silly to be hung up on naming...

I do think that "lockPref" sticks out relative to the calls for "default_pref", and "sticky_pref".  I'm a +1 for calling it "locked_pref".
(Assignee)

Comment 32

2 years ago
The syntax is a triviality of the patch. The core problem here is that there's no one to review the patch so that it possibly lands.
:(

Ideally, I'd like to favor this patch over the one I proposed at https://bugzilla.mozilla.org/show_bug.cgi?id=976334 - since it has been tried and tested for so long...and that just adds another use case for this: ease of locking preferences for android distributions (which aren't as straightforward when it comes to specifying the autoconfig file in the install directory)

Is it just a matter of having someone sign off on it, or were there outstanding concerns outside of the syntax?  It seemed like it kind of just got dropped due to time constraints 2+ years ago, but that there now are actually some compelling use cases for it. 

Can't the fact that it's been needed and used by Debian for so long be an indication of the stability and/or utility of the patch?  just an appeal to *someone* to perhaps move this along...

Aside from that, :glandium - how often does this patch "break"...that is, how many times have you had to rework it because of changes in the upstream code?  I ask because I actually would consider applying this patch and using it in my own distribution - even if it never lands.
(Assignee)

Comment 34

2 years ago
From a quick glance at the history of the patch in Debian, in the past 5 years, I had to do actual but trivial code changes twice, and adapt to context changes twice (one of them having been the change from PRBool to bool)

Comment 35

2 years ago
(In reply to Mike Hommey [:glandium] (VAC: 31 Dec - 11 Jan) from comment #32)
> The syntax is a triviality of the patch. The core problem here is that
> there's no one to review the patch so that it possibly lands.

Can I help review?

Comment 36

2 years ago
Comment on attachment 8613433 [details] [diff] [review]
Allow .js preference files to set locked prefs with lockPref()

Because this came up on the mailing list, I'll try to be explicit about the decision here. I don't think we should allow extensions to lock preferences, and we don't even have a clear description (other than this code) of what it means to "lock" a preference: whether changes to the default value are still allowed, or the user value, etc.

The code is the incidental part of this; it's the basic problem and solution space that's totally unclear.
Attachment #8613433 - Flags: review?(benjamin) → review-
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #36)
> Because this came up on the mailing list

Which mailing list is that? (just so I can make sure I'm following the right one)
(Assignee)

Comment 38

2 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #36)
> Comment on attachment 8613433 [details] [diff] [review]
> Allow .js preference files to set locked prefs with lockPref()
> 
> Because this came up on the mailing list, I'll try to be explicit about the
> decision here. I don't think we should allow extensions to lock preferences,

As mentioned in the long history of this bug, extensions can already lock preferences. See the attached xpi.
To expound on what :glandium said:

Extensions can lock preferences with:

  Components.classes["@mozilla.org/preferences-service;1"]
            .getService(Components.interfaces.nsIPrefService)
            .getBranch(null)
            .lockPref("name_of_pref");

Locking a preference doesn't affect the value, it just locks the preference to the user. Obviously any extension could unlock a preference and do what they want with that value.

Locking preferences is not for the benefit of preventing other extensions from messing with things, it's to keep users from messing with things.

Oddly, this document:

https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/A_brief_guide_to_Mozilla_preferences

Says we already support lockPref in JS files?
(Assignee)

Comment 40

2 years ago
(In reply to Mike Kaply [:mkaply] from comment #39)
> Oddly, this document:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/
> A_brief_guide_to_Mozilla_preferences
> 
> Says we already support lockPref in JS files?

It says: "All preferences files may call pref(), user_pref() and sticky_pref(), while the config file in addition may call lockPref()."
which is about autoconf.
(In reply to Mike Hommey (VAC: 31 Dec - 11 Jan) [:glandium] from comment #40)
> (In reply to Mike Kaply [:mkaply] from comment #39)
> > Oddly, this document:
> > 
> > https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/
> > A_brief_guide_to_Mozilla_preferences
> > 
> > Says we already support lockPref in JS files?
> 
> It says: "All preferences files may call pref(), user_pref() and
> sticky_pref(), while the config file in addition may call lockPref()."
> which is about autoconf.

You'd think I would have noticed that. I really don't like a document that combines the two. pref behaves differently on both, user_pref doesn't work in autoconfig and sticky_pref doesn't work in autoconfig. This shouldn't reference autoconfig at all.
You need to log in before you can comment on or make changes to this bug.