-
-
Notifications
You must be signed in to change notification settings - Fork 15.5k
Firefox configurable #58195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Firefox configurable #58195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this! 👍
Only had a brief look so far.
I do not like the fact that we can not remove extensions and that (as you wrote) is still a lot in flux regarding that. Is it maybe a bit too early to add the extensions feature?
I really like the fact that we will be able to declare policies (finally). Since it allows users to disable things like DNS over HTTPs per default etc…
Every FireFox release we have to update ~3 branches with at least 2 versions at any time. It would be great if the patches could be upstreamed to reduce the maintenance on them. Very often there is some subtile reformatting/restyling going on that breaks the patches.
I already dropped the extensions option that was part of the original pr. The docs still mention, that this can be used for extensions, but I attempted to phrase it very carefully (I can drop it completely if you insist). |
I would seek advice about communicating with Mozilla from @nbp With Mozilla there is always a dual question — are the changes large enough to require asking again about official branding… |
I agree with @7c6f434c as I had the same concerns but didn't want to raise them as such just yet. It feels like it is a point of no return (even for trivial changes). Once that topic is on the table we have no way back ;-) In any way, if we decide that this is a desirable feature we can still include it whenever the official branding is disabled. |
Not sure if negotiating a command-line option with upstream is easier. |
397120f
to
e3ead5d
Compare
@xaverdh you might want to adjsut the commit messages to fit our guidelines. |
Yes, while doing that, should I squash everything or leave the commits separate? |
e3ead5d
to
bd23c9d
Compare
policiesJson = builtins.toFile "policies.json" | ||
(builtins.toJSON enterprisePolicies); | ||
|
||
mozillaCfg = builtins.toFile "mozilla.cfg" '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use pkgs.writeText
for these, builtins.toFile
is an evaluation time thing, which means it can't depend on any derivations, which might be wanted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I will change that.
@@ -81,6 +83,25 @@ let | |||
++ lib.optional (config.pulseaudio or true) libpulseaudio; | |||
gtk_modules = [ libcanberra-gtk2 ]; | |||
|
|||
enterprisePolicies = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this named enterprisePolicies
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because that's what upstream calls it.
firefox-user-config = runCommand "firefox-user-config" {} '' | ||
mkdir $out | ||
cat > "$out/policies.json" < ${policiesJson} | ||
cat > "$out/mozilla.cfg" < ${mozillaCfg} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just cp ${policiesJson} $out/policies.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For no reason in particular. The two are mostly interchangeable.
If cp
is preferred over cat
, I can change it.
Depending on your viewpoint cp
is the more complex tool I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use cp
to copy files :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok :-)
mkdir -p "$out/lib/firefox/defaults/pref" | ||
cat > "$out/lib/firefox/defaults/pref/autoconfig.js" <<EOF | ||
pref("general.config.filename", "mozilla.cfg"); | ||
pref("general.config.obscure_value", 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of this? Why these two options and why use autoconfig when this file won't ever change anyways (read-only nix store)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think «prepare» is in terms of Nixpkgs-time, i.e. check that autoconfig works so that another change can actually introduce customisable autoconfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the crazy way mozillas AutoConfig system works. The actual configuration file is added in the wrapper, but this file (autoconfig.js) has to be there in order for the AutoConfig machinery to do anything at all. Without it, firefox will just ignore the configuration file proper.
The first line tells firefox, that the name of the real configuration file is mozilla.cfg. This also has to be there, because some (enterprise) parties used it as a "lock down" mechanism, to prevent users from renaming it. In the same spirit, the configuration file (mozilla.cfg) is by default expected to be obfuscated by byte shifting. So the second line tells firefox that we use an non-obfuscated config file.
All of this is documented here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, can you add this link to the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
bd23c9d
to
48251e6
Compare
This adds an option installAutoConfig, which installs an autoconfig.js file in the appropriate subdirectory of the firefox runtime.
This adds patches to make the paths where firefox looks for the enterprise policies file (policies.json) and the autoconfig file (mozilla.cfg) configurable by setting the environment variables MOZ_POLICIES_DIR and MOZ_AUTOCONFIG_DIR respectively.
48251e6
to
38cd961
Compare
This uses MOZ_POLICIES_DIR and MOZ_AUTOCONFIG_DIR to point firefox to the users configuration.
This documents configuring firefox by passing options to the wrapper.
38cd961
to
4509e3d
Compare
I would have suggested to upstream these patches before accepting this changes in Nixpkgs. Unfortunately I do not think these patches would be accepted upstream because they are adding the option for a user to work-around the enterprise settings, using environment variables, which are meant to be controlled by the administrator. Knowing that these extra patches are unlikely to make it upstream, this implies that firefox-wrappers will not work as expected with pre-built versions of Firefox. Thus I wonder, is there any reason why our firefox-wrapper does not clone the content of the firefox directory and wrap the copied binary, in which case you can easily place the No? Apart from these remarks, I love the intent and I wanted these features for a while! |
That's essentially what the original pull request did (#57554). Actually it used symlinks mostly instead of copying. |
I personally don't really understand the issue, since a user can always execute his own private version of firefox, or copy the entire runtime and replace the config file, or rewrite parts of the binary before executing it. |
So, what's the better way out of the two? I'm a bit confused as to why there are two PR's that seemingly do the same. Edit: Ah you mention that in the description of this PR, already forgot :) So, I personally prefer this way, it seems less hacky. |
This feature is meant for enterprise installation, where users might not have as much freedom. This can also be the case on NixOS, if you forbid execution flags on partitions others than
Yes, but it would not be enabled for the binary distribution of Firefox. Which implies that this configuration toggle is at the mercy of anyone who might want to do a bit of clean-ups. I will give look later at the previous PR using external patching mechanism. |
I just came across this Bugzilla Entry: https://bugzilla.mozilla.org/show_bug.cgi?id=1170092 It has a patch (https://src.fedoraproject.org/rpms/firefox/blob/master/f/mozilla-1170092.patch) that allows putting configuration into |
The feature is very desirable. The firefox patch looks small, is there no way to get some feedback from them ? no firendly insider :) ? |
Is this PR stall? I would like to have it |
closing since #91724 was merged |
Motivation for this change
This is the same as #57554, except for the implementation, which differs. This pull request patches the browser to take the configuration directories from environment variables, which can then be set in e.g. the wrapper (the original pr did some symlink trickery).
Things done
I tested this with firefox on NixOS, but I did not, and probably cannot test it on darwin.
Also as is, this also patches all the variants of firefox, we may want to limit that to just firefox?I pushed changes to only patch firefox proper.The tor-browser-bundle in particular already has some code that deals with AutoConfig in its derivation, I am not sure how this fits together.