Skip to content
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.

Sign up for GitHub

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

Jump to bottom

firefox: add firefox-policies w/ patch #94898

Closed
wants to merge 1 commit into from
Closed

firefox: add firefox-policies w/ patch #94898

wants to merge 1 commit into from

Conversation

eyJhb
Copy link
Member

@eyJhb eyJhb commented Aug 7, 2020 •
edited

Motivation for this change

Allows for enterprise policies policies.json to be specified on a pr.
profile basis, instead of pr. user/global. This could pose a security
rist IF NOT because it is already possible to do as it can be placed in
/run/user/:uid.

This is heavily for use in home-manager, where configuring on a pr.
profile basis is nice/needed. This is placed in nixpkgs, as we would
like to cache the firefox-policies, as it is quite heavy to compile
yourself each time.

There has been some questions regarding if this is a sane commit, regards to branding which was described here - #31843 (comment)

I have asked Sylvestre in chat.mozilla.org and has said it is a portability modification.
2020_08_07-15:04:11

Besides this, the legal department has said OK for it, if Sylvestre said OK.

Hi <removed>,

At a high-level that sounds okay though I would usually check-in with Sylvestre to make sure he is comfortable with that the changes really are very minor and don't impact the experience of the product (since I am a lawyer and not an engineer). Sounds like you were able to connect but let me know if you need anything further.

Best,
Daniel

The patch is fairly trivial in general, but I am unsure if it should be placed elsewhere?

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested review from andir and edolstra August 7, 2020 23:21
@eyJhb eyJhb mentioned this pull request Aug 11, 2020
Closed
7 tasks
@eyJhb eyJhb force-pushed the firefox-policies branch 2 times, most recently from 490e877 to 3a8f97b Compare September 19, 2020 09:57
@andir
Copy link
Member

andir commented Oct 24, 2020

Sorry for the late response. Just catching up with all the reviews that are awaiting. Gonna have a look at this finally.

andir
andir requested changes Oct 24, 2020
View reviewed changes
Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks actually fine. I wonder if we should probably have this in the default firefox package. I also noted some other nits.

The commit that introduces this should also carry the information that is given in the PR description so it becomes obvious from git blame why this change was done.

pkgs/applications/networking/browsers/firefox/packages.nix Outdated
@@ -61,4 +61,37 @@ rec {
versionKey = "ffversion";
};
};

firefox-esr-68 = (common rec {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we reintroducing firefox 68? Was this a side-effect of rebasing?

pkgs/applications/networking/browsers/firefox/packages.nix

# apply enterprise-policies.patch so that we can have enterprise policies
# on a pr. profile basis, instead of pr. user or global for the installation.
firefox-policies = firefox.overrideAttrs(old: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any downside from having this in our default Firefox build? From the patch above I can see that we can select the mode during runtime anyway.

pkgs/applications/networking/browsers/firefox/enterprise-policies.patch
@@ -0,0 +1,29 @@
diff --git a/toolkit/components/enterprisepolicies/EnterprisePolicies.js b/toolkit/components/enterprisepolicies/EnterprisePolicies.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to see a bit more description as a comment before the actual diff. In a year from now it should still be crystal clear what this does. Reading in the diff is possible but probably harder than it should be.

@eyJhb
Copy link
Member Author

eyJhb commented Oct 25, 2020

Adding this, from a discussion on IRC about this PR - https://logs.nix.samueldr.com/nixos-dev/2020-10-25#1603623426-1603626184;

@ofborg ofborg bot requested a review from andir October 28, 2020 07:35
@eyJhb
Copy link
Member Author

eyJhb commented Dec 3, 2020

Closing this in favor of #91724 for now.
The IRC logs state how one could have multiple firefox bins, e.g.

firefox-work
firefox-uni
firefox -> default

@eyJhb eyJhb closed this Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra Awaiting requested review from edolstra

@andir andir Awaiting requested review from andir

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@eyJhb @andir