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

nixos/users: Allow mutable shells for declarative users #41966

Merged
merged 1 commit into from Dec 31, 2020
Merged

nixos/users: Allow mutable shells for declarative users #41966

merged 1 commit into from Dec 31, 2020

Conversation

aneeshusa
Copy link
Contributor

Motivation for this change

I want to manage users centrally via declarativeUsers,
but allow users to change their shell as they please,
similar to how they can change passwords at will
if none of the password-related NixOS settings are set for their user.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

aneeshusa added a commit to aneeshusa/nixpkgs that referenced this pull request Apr 7, 2019
@aneeshusa
nixos/shadow: create setuid wrapper for chsh
925b028 
This allows non-declarative users to change their login shells.
NixOS#41966 will make this possible
for declarative users as well if the system config explicitly allows it.
@aneeshusa aneeshusa mentioned this pull request Apr 7, 2019
Merged
10 tasks
@aneeshusa
nixos/users: Allow mutable shells for declarative users
a709b1a 
I want to manage users centrally via declarativeUsers,
but allow users to change their shell as they please,
similar to how they can change passwords at will
if none of the password-related NixOS settings are set for their user.
@aneeshusa aneeshusa force-pushed the allow-mutable-shells-for-declarative-users branch from f465eea to a709b1a Compare April 7, 2019 22:16
@typetetris
Copy link
Contributor

This only works together with #51270 ?

I assume, I can get both behaviours? Declarative users, who can't change their login shell and declarative users, who can change their login shell? How to select between those behaviours (if both are present.)

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
Mic92
Mic92 reviewed Jun 2, 2020
View reviewed changes
nixos/modules/config/users-groups.nix
@@ -119,7 +119,7 @@ let
};

shell = mkOption {
type = types.either types.shellPackage types.path;
type = types.nullOr (types.either types.shellPackage types.path);
Copy link
Member

Choose a reason for hiding this comment

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

Question: If I set a shell in nixos, does this override the user shell on every update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding of nixos/modules/config/update-users-groups.pl is that:

  • it always sets the shell
  • if a given user has a shell set in the NixOS config, that will always be used, overwriting any existing shell

Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, this isn't changing the existing behaviour at all, right? It just allows you to additionally explicitly set a user's shell to null which then means NixOS won't touch it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's correct!

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 2, 2020
@aneeshusa
Copy link
Contributor Author

@typetetris #51270 was mainly for non-declarative users. You can get both behaviors:

  • if you set the shell for a user in the NixOS config to some actual shell, that shell should be applied on every nixos-rebuild
  • if you set a null shell for a user in the NixOS config, NixOS won't enforce any given shell for them and they can change it as they wish

lukegb
lukegb approved these changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@lukegb lukegb left a comment

Choose a reason for hiding this comment

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

LG.

nixos/modules/config/users-groups.nix
@@ -119,7 +119,7 @@ let
};

shell = mkOption {
type = types.either types.shellPackage types.path;
type = types.nullOr (types.either types.shellPackage types.path);
Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, this isn't changing the existing behaviour at all, right? It just allows you to additionally explicitly set a user's shell to null which then means NixOS won't touch it.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/313

@nh2
Copy link
Contributor

nh2 commented Dec 31, 2020

Merging based on multiple reviews.

@nh2 nh2 merged commit 9206c0d into NixOS:master Dec 31, 2020
@aneeshusa aneeshusa deleted the allow-mutable-shells-for-declarative-users branch January 7, 2022 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@lukegb lukegb lukegb approved these changes

@thiagokokada thiagokokada thiagokokada approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@aneeshusa @typetetris @nixos-discourse @nh2 @Mic92 @lukegb @thiagokokada @GrahamcOfBorg