Skip to content

Conversation

@provokateurin
Copy link
Member

@provokateurin provokateurin commented Nov 11, 2025

Replaces #413728. I started from scratch and modeled it as closely as possible to the docs: https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/config_sample_php_parameters.html#mail-parameters. The config options are super confusing and I have to admit that I had to look up a bunch of logic in the server to understand when and how they are used.

CC @6543

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 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 Nov 11, 2025
@6543

This comment was marked as resolved.

@6543
Copy link
Member

6543 commented Nov 11, 2025

... The config options are super confusing ...

yes and we should aim to have it simpler within our nix options so users dont shoot themselfes int he foot :D ...

PS: i like the nixos test :D

@6543

This comment was marked as resolved.

@6543
Copy link
Member

6543 commented Nov 11, 2025

I have tried to create a test for sendmail based on your test ...

          environment.systemPackages = [
            pkgs.msmtp
          ];
          environment.etc."msmtprc".text = ''
            defaults
            auth on
            tls on
            tls_starttls on
            account default
            host ${domain}
            port 587
            from alice@${domain}
            user alice
            password foobar
          '';

let me send via sendmail inside the test vms from nextcloud to stalward manually ... BUT nextcloud fails to do so :/

click to expand
From eb766e6d0af65510f951334598f1ced6e9451e2e Mon Sep 17 00:00:00 2001
From: 6543 <[email protected]>
Date: Tue, 11 Nov 2025 04:31:46 +0100
Subject: [PATCH] nixos/nextcloud: add test for mail config via sendmail

---
 nixos/tests/nextcloud/default.nix       |   1 +
 nixos/tests/nextcloud/with-sendmail.nix | 109 ++++++++++++++++++++++++
 2 files changed, 110 insertions(+)
 create mode 100644 nixos/tests/nextcloud/with-sendmail.nix

diff --git a/nixos/tests/nextcloud/default.nix b/nixos/tests/nextcloud/default.nix
index 72f7a28070b5..e4b71f1d4385 100644
--- a/nixos/tests/nextcloud/default.nix
+++ b/nixos/tests/nextcloud/default.nix
@@ -136,6 +136,7 @@ let
         ./with-postgresql-and-redis.nix
         ./with-objectstore.nix
         ./with-mail.nix
+        ./with-sendmail.nix
       ]
       ++ (pkgs.lib.optional (version >= 32) ./without-admin-user.nix)
     );
diff --git a/nixos/tests/nextcloud/with-sendmail.nix b/nixos/tests/nextcloud/with-sendmail.nix
new file mode 100644
index 000000000000..a41b55f148c1
--- /dev/null
+++ b/nixos/tests/nextcloud/with-sendmail.nix
@@ -0,0 +1,109 @@
+{
+  name,
+  pkgs,
+  testBase,
+  system,
+  ...
+}:
+with import ../../lib/testing-python.nix { inherit system pkgs; };
+runTest (
+  { config, lib, ... }:
+  let
+    certs = import ../common/acme/server/snakeoil-certs.nix;
+    domain = certs.domain;
+  in
+  {
+    inherit name;
+
+    meta.maintainers = lib.teams.nextcloud.members;
+
+    imports = [ testBase ];
+
+    nodes = {
+      nextcloud =
+        {
+          config,
+          pkgs,
+          nodes,
+          ...
+        }:
+        {
+          security.pki.certificateFiles = [ certs.ca.cert ];
+
+          networking.extraHosts = ''
+            ${nodes.stalwart.networking.primaryIPAddress} ${domain}
+          '';
+
+          environment.etc."nextcloud/mail_smtppassword".text = "foobar";
+          environment.systemPackages = [
+            pkgs.msmtp
+          ];
+          environment.etc."msmtprc".text = ''
+            defaults
+            auth on
+            tls on
+            tls_starttls on
+            account default
+            host ${domain}
+            port 587
+            from alice@${domain}
+            user alice
+            password foobar
+          '';
+
+          services.nextcloud.config = {
+            dbtype = "sqlite";
+
+            mail = {
+              from_address = "alice";
+              inherit domain;
+              smtpmode = "sendmail";
+              send_plaintext_only = true;
+            };
+          };
+        };
+
+      stalwart =
+        { pkgs, ... }:
+        {
+          imports = [ ../stalwart/stalwart-mail-config.nix ];
+
+          networking.firewall.allowedTCPPorts = [ 587 ];
+
+          environment.systemPackages = [
+            (pkgs.writers.writePython3Bin "test-imap-read" { } ''
+              from imaplib import IMAP4
+
+              with IMAP4('localhost') as imap:
+                  imap.starttls()
+                  status, [caps] = imap.login('bob', 'foobar')
+                  assert status == 'OK'
+                  imap.select()
+                  status, [ref] = imap.search(None, 'ALL')
+                  assert status == 'OK'
+                  [msgId] = ref.split()
+                  status, msg = imap.fetch(msgId, 'BODY[TEXT]')
+                  assert status == 'OK'
+                  assert (msg[0][1].strip()
+                          == (b'Well done, ${config.adminuser}!\r\n\r\n'
+                              b'If you received this email, the email configuration '
+                              b's=\r\neems to be correct.\r\n\r\n\r\n--=20\r\n'
+                              b'Nextcloud - a safe home for all your data=\r\n\r\n'
+                              b'This is an automatically sent email, please do not reply.'))
+            '')
+          ];
+        };
+    };
+
+    test-helpers.init = ''
+      stalwart.wait_for_unit("multi-user.target")
+      stalwart.wait_until_succeeds("nc -vzw 2 localhost 587")
+
+      nextcloud.succeed("nc -vzw 2 ${domain} 587")
+      nextcloud.succeed("curl -sS --fail-with-body -u ${config.adminuser}:${config.adminpass} -H 'OCS-APIRequest: true' -X PUT http://nextcloud/ocs/v2.php/cloud/users/${config.adminuser} -H 'Content-Type: application/json' --data-raw '{\"key\":\"email\",\"value\":\"bob@${domain}\"}'")
+      nextcloud.succeed("curl -sS --fail-with-body -u ${config.adminuser}:${config.adminpass} -H 'OCS-APIRequest: true' -X POST http://nextcloud/settings/admin/mailtest")
+
+      stalwart.succeed("test-imap-read")
+    '';
+  }
+)
-- 
2.50.1

@Ma27
Copy link
Member

Ma27 commented Nov 11, 2025

Any reason to do this in config rather than reusing settings? That way, we don't have to set most of these options. I essentially have

{
  services.nextcloud.settings = {
      mail_smtpmode = "smtp";
      mail_smtphost = "XXX";
      mail_smtpport = 465;
      mail_smtptimeout = 60;
      mail_smtpsecure = "tls";
      mail_from_address = "nextcloud";
      mail_domain = "XXX";
      mail_smtpauth = true;
      mail_smtpauthtype = "LOGIN";
  };
}

Historically we've been trying to get rid of config in favor of going the rfc42-style approach. This unfortunately stalled at some point, but I don't think it's worth re-introducing this pattern for new stuff.

For the password we have two options:

  • use the secret file as for everything else
  • go for the services.netcloud.secretSttings (or whatever this was called) PR.

@provokateurin
Copy link
Member Author

@6543

and it is one less config option to set

I get your point, but I feel like it's easier to understand by just following the docs. This is also easier for admins who port their config/knowledge from a different method of deployment.

I'm not sure if we should call it smtp* as e.g. smtpmode=sendmail is clearly not an smtp thing

Well it's how the options are called, but the reason is mostly that even if you use sendmail, the smtp options can apply if you use smtp with sendmail. Like I said, really confusing -_-

I have tried to create a test for sendmail based on your test

While I feel like it's not the job of the test to check that sendmail actually works, it certainly won't hurt either having this as a test and example for anyone who wants to use it.

@provokateurin
Copy link
Member Author

@Ma27

Any reason to do this in config rather than reusing settings

TBH I didn't put much though into that part, so I'm happy to change it how you see fit.

That way, we don't have to set most of these options

I would still like to define them, as it makes it easier to spot problems early.

For the password we have two options:

I'm not sure I follow. Does this require changes besides moving from config to settings?

@provokateurin provokateurin force-pushed the nixos-nextcloud-mail-options branch from 78da731 to e063852 Compare November 11, 2025 10:14
@provokateurin
Copy link
Member Author

provokateurin commented Nov 11, 2025

@Ma27 I did what I think you meant, but please tell me if I got it wrong. I'm not super happy with the way it works now, but at least it works 🤷‍♀️

@provokateurin provokateurin force-pushed the nixos-nextcloud-mail-options branch from e063852 to acd0bcd Compare November 11, 2025 12:02
@provokateurin
Copy link
Member Author

I based this on #394136 and now I'm happy with the way it works :)

@provokateurin provokateurin changed the title nixos/nextcloud: Add services.nextcloud.config.mail.* options nixos/nextcloud: Add services.nextcloud.settings.mail_* options Nov 11, 2025
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.

Looks good.

Explicitly blocking merge until #394136 is through.

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

tested nix run .#nixosTests.nextcloud.with-mail32.driver: PASS

code looks nice

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Nov 12, 2025
@provokateurin provokateurin force-pushed the nixos-nextcloud-mail-options branch from acd0bcd to 3fc5278 Compare November 13, 2025 11:06
@provokateurin
Copy link
Member Author

To move this forward I implemented all the feedback from #394136 and add a test for reading back the secret.
@Ma27 could we merge this one and subsequently close the other PR?

@kraftnix
Copy link
Contributor

Thanks for continuing on the work from #394136, especially the nixos test.

I checked over changes and ran this PR against my current nextcloud server and it all still works as expected.

I am happy to close #394136 in favour of this PR since mine is now a subset of the changes here.

@6543 6543 requested a review from Ma27 November 20, 2025 22:43
@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 21, 2025
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.

Now that I'm reading the NC sources, does Nextcloud always send emails? Asking because I wondered if we'd accidentally turn this on by default for everyone.

type = lib.types.str;
default = "127.0.0.1";
description = ''
This depends on `mail_smtpmode`. Specify the IP address of your mail server host. This may contain multiple hosts separated by a semicolon. If you need to specify the port number, append it to the IP address separated by a colon, like this: `127.0.0.1:24`.
Copy link
Member

Choose a reason for hiding this comment

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

Uhm, what will happen if you specify both mail_smtpport and 127.0.0.1:24 as smtphost?

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH I have no idea, this is just copied from the sample config docs. I suppose the mail_smtpport would act as a default and with the colon it would be possible to override it for each server.

Copy link
Member

Choose a reason for hiding this comment

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

OK I see.

We don't have much of a choice anyways.

@provokateurin
Copy link
Member Author

provokateurin commented Nov 21, 2025

Now that I'm reading the NC sources, does Nextcloud always send emails?

Yes it does, while looking into this I found it doesn't even check the config and just blindly sends emails (and the errors are hidden away until nextcloud/server#56090). I only realized this later on, while doing other Nextcloud development.

I think adding these configs doesn't change anything, as they are just the default values from the server.

What we could do instead is just add the assertion to use services.nextcloud.secrets and not add any of the options in services.nextcloud.settings. Then we definitely don't interfere with anything that isn't happening upstream anyway.

@Ma27
Copy link
Member

Ma27 commented Nov 21, 2025

Yes it does, while looking into this I found it doesn't even check the config and just blindly sends emails (and the errors are hidden away until nextcloud/server#56090)

OK, in that case we're good.
I'd kinda hope that we'd end up with an explicit enable/disable flag though that ignores the rest of the settings, this seems kinda odd currently.

kraftnix and others added 2 commits November 21, 2025 15:33
Uses existing `nix_read_secret` and LoadCredential to read contents of a
file into an entry in `config.php`
@provokateurin provokateurin force-pushed the nixos-nextcloud-mail-options branch from 3fc5278 to 2718336 Compare November 21, 2025 14:48
@provokateurin provokateurin requested a review from Ma27 November 21, 2025 14:48
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 21, 2025
@Ma27 Ma27 added this pull request to the merge queue Nov 21, 2025
@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Nov 21, 2025
Merged via the queue into NixOS:master with commit 4065193 Nov 21, 2025
29 of 32 checks passed
@provokateurin provokateurin deleted the nixos-nextcloud-mail-options branch November 21, 2025 15:44
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: 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 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants