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.

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

nixos/nextcloud: Add ensureUsers option #248334

Closed
wants to merge 1 commit into from

Conversation

onny
Copy link
Contributor

@onny onny commented Aug 10, 2023

Description of changes

Aiming to get rid of the config option in the Nextcloud module, having them all in the free form type extraOptions which can be renamed to settings in accordance to RFC42 in a later PR.

Instead of defining a single admin user with config.adminuser and config.adminpassFile switch to a more generic approach which is used by other modules, having a ensureUsers attribute set.

services.nextcloud = {
  enable = true;
  package = pkgs.nextcloud27;
  hostName = "localhost";
  config = {
    adminpassFile = "${pkgs.writeText "adminpass" "hunter2"}";
  };
  database.createLocally = true;
  ensureUsers = {
    user1 = {
      email = "user1@localhost";
      passwordFile = /secrets/user1.secret;
    };
    user2 = {
      email = "user1@localhost";
      passwordFile = /secrets/user1.secret;
    };
  };
};

For now I would like to only add the ensureUsers option to add additional users. In a later PR I would like to manage admin user configuration with this option.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 10, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Aug 10, 2023
Comment on lines +710 to +711
exist yet. This option does not delete accounts which are not listed
anymore.
Copy link
Contributor

Choose a reason for hiding this comment

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

When designing new options, why not make them fully declarative? Dump some state about which users are managed with by NixOS so that they can be removed again when they are removed from the config?

'';
example = {
user1 = {
passwordFile = /secrets/user1-localhost;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think passwordFile options must be set with quoted strings, or else Nix will copy the secret into the world-readable Nix store at eval time (not so secret anymore).

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Very strong 👎

As pointed out by @bjornfor this is not declarative at all - except for dumping some state somewhere and as a result this strictly contradicts with what you can expect from NixOS in general. Granted, there are many other cases, but usually it was some necessary evil (e.g. you need to provide root credentials to set up Nextcloud initially), but in that case, there's no good reason!

@onny
Copy link
Contributor Author

onny commented Aug 15, 2023

@Ma27 Good point, I agree on this. But this argument somehow also applies to the current implementation of nextcloud.config.adminuser and nextcloud.config.adminpasswordFile.

As pointed out in the PR description, it would be nice to move all config options into the freeForm type extraOptions which could be renamed to settings.

So if I manage to migrate the adminuser into ensureUsers we'll still stick to a non-declerative approach as before but then with a more generic interface?

Being able to add additional users is helpful with bootstrapping test and dev environments using the NixOS Nextcloud module :)

But nevertheless I understand your concern :)

@Ma27
Copy link
Member

Ma27 commented Aug 16, 2023

But this argument somehow also applies to the current implementation of nextcloud.config.adminuser and nextcloud.config.adminpasswordFile.

Yeah, that's precisely what I meant with "necessary evil" in my previous comment ;-)
You have to create an admin user on setup, that's why it exists.

I don't even see any value in this "feature": after the initial setup, the declaration of additional users is effectively worthless. As soon as such a user touches their email, the data in your deployment is outdated with no way to fix it (well, except for manual fixing up the deployment, but none of that will have any effect on the system you're deploying).

Being able to add additional users is helpful with bootstrapping test and dev environments using the NixOS Nextcloud module :)

OK, if that's the only motivation behind that, then let's close this right away, please. NixOS isn't tooling to bootstrap dev environments, but a Linux distribution with modules that are supposed to be used in production. By the same argument we could enable XDebug by default in Nextcloud because it makes dev work way easier.

For dev-only features, nothing stops you to write your own module for the nice interface. You can even add options from outside into services.nextcloud (and even into submodules). Just put the occ-commands into a service that gets executed after nextcloud-setup.service has started up and that's it.

@onny onny closed this Aug 17, 2023
@onny
Copy link
Contributor Author

onny commented Aug 17, 2023

Thank you @Ma27 for the feedback, sounds reasonable! Going to look on how to integrate this into an external flake module in case it's useful for someone

@onny
Copy link
Contributor Author

onny commented Mar 30, 2024

Is someone still interested in using the ensureUsers option, I added a practical configuration example to the NixOS wiki https://nixos.wiki/wiki/Nextcloud#Add_users_declaratively

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants