From 52fc54232931eb076ef070715749cc468946a911 Mon Sep 17 00:00:00 2001
From: Morgan Jones <me@numin.it>
Date: Mon, 21 Oct 2024 23:05:06 -0700
Subject: [PATCH 1/3] nixos/mattermost: modernize

Based on #198040. Prioritizes backwards compatibility, including
database and plugin compatibility, while adding more sensible
defaults like database peer auth.
---
 .../modules/services/web-apps/mattermost.nix  | 498 +++++++++++++-----
 nixos/tests/mattermost.nix                    | 174 ++++--
 2 files changed, 494 insertions(+), 178 deletions(-)

diff --git a/nixos/modules/services/web-apps/mattermost.nix b/nixos/modules/services/web-apps/mattermost.nix
index fee0ec2d641d2..7ea4aa943906f 100644
--- a/nixos/modules/services/web-apps/mattermost.nix
+++ b/nixos/modules/services/web-apps/mattermost.nix
@@ -1,32 +1,87 @@
 { config, pkgs, lib, ... }:
 
-with lib;
-
 let
+  inherit (lib.strings)
+    hasInfix hasSuffix escapeURL concatStringsSep escapeShellArgs;
+
+  inherit (lib.attrsets)
+    mapAttrsToList recursiveUpdate;
+
+  inherit (lib.options)
+    mkOption mkPackageOption mkEnableOption;
+
+  inherit (lib.modules)
+    mkRenamedOptionModule mkMerge mkIf;
+
+  inherit (lib.trivial)
+    warnIf throwIf;
+
+  inherit (lib) types;
 
   cfg = config.services.mattermost;
 
-  database = "postgres://${cfg.localDatabaseUser}:${cfg.localDatabasePassword}@localhost:5432/${cfg.localDatabaseName}?sslmode=disable&connect_timeout=10";
-
-  postgresPackage = config.services.postgresql.package;
-
-  createDb = {
-    statePath ? cfg.statePath,
-    localDatabaseUser ? cfg.localDatabaseUser,
-    localDatabasePassword ? cfg.localDatabasePassword,
-    localDatabaseName ? cfg.localDatabaseName,
-    useSudo ? true
-  }: ''
-    if ! test -e ${escapeShellArg "${statePath}/.db-created"}; then
-      ${lib.optionalString useSudo "${pkgs.sudo}/bin/sudo -u ${escapeShellArg config.services.postgresql.superUser} \\"}
-        ${postgresPackage}/bin/psql postgres -c \
-          "CREATE ROLE ${localDatabaseUser} WITH LOGIN NOCREATEDB NOCREATEROLE ENCRYPTED PASSWORD '${localDatabasePassword}'"
-      ${lib.optionalString useSudo "${pkgs.sudo}/bin/sudo -u ${escapeShellArg config.services.postgresql.superUser} \\"}
-        ${postgresPackage}/bin/createdb \
-          --owner ${escapeShellArg localDatabaseUser} ${escapeShellArg localDatabaseName}
-      touch ${escapeShellArg "${statePath}/.db-created"}
-    fi
-  '';
+  # The directory to store mutable data within dataDir.
+  mutableDataDir = "${cfg.dataDir}/data";
+
+  # The plugin directory. Note that this is the *post-unpack* plugin directory,
+  # since Mattermost unpacks plugins to put them there. (Hence, mutable data.)
+  pluginDir = "${mutableDataDir}/plugins";
+
+  # Mattermost uses this as a staging directory to unpack plugins, among possibly other things.
+  # Ensure that it's inside mutableDataDir since it can get rather large.
+  tempDir = "${mutableDataDir}/tmp";
+
+  mkDatabaseUri = {
+    user ? null,
+    password ? null,
+    host ? null,
+    port ? null,
+    path ? null,
+    query ? {}
+  }: let
+    nullToEmpty = val: if val == null then "" else toString val;
+
+    # Converts a list of URI attrs to a query string.
+    toQuery = mapAttrsToList (name: value:
+      if value == null then null else (escapeURL name) + "=" + (escapeURL (toString value))
+    );
+
+    userPart =
+      if user == null && password == null then
+        ""
+      else if user != null && password != null then
+        escapeURL user + ":" + escapeURL password
+      else
+        escapeURL (if password != null then password else user);
+    hostPart =
+      if userPart == "" then
+        escapeURL (nullToEmpty host)
+      else
+        "@" + escapeURL (nullToEmpty host);
+    portPart =
+      if port == null then ""
+      else ":" + (toString port);
+    pathPart =
+      if path == null then ""
+      else "/" + (escapeURL path);
+    queryPart =
+      if query == {} then ""
+      else "?" + concatStringsSep "&" (toQuery query);
+  in
+    "postgres://" + userPart + hostPart + portPart + pathPart + queryPart;
+
+  database =
+    if cfg.database.peerAuth then
+      mkDatabaseUri {
+        path = cfg.database.name;
+        query = { host = cfg.database.socketPath; } // cfg.database.extraConnectionOptions;
+      }
+    else
+      mkDatabaseUri {
+        inherit (cfg.database) user password host port;
+        path = cfg.database.name;
+        query = cfg.database.extraConnectionOptions;
+      };
 
   mattermostPluginDerivations = with pkgs;
     map (plugin: stdenv.mkDerivation {
@@ -53,14 +108,14 @@ let
         cfg.package
       ];
       installPhase = ''
-        mkdir -p $out/data/plugins
+        mkdir -p $out
         plugins=(${escapeShellArgs (map (plugin: "${plugin}/share/plugin.tar.gz") mattermostPluginDerivations)})
         for plugin in "''${plugins[@]}"; do
-          hash="$(sha256sum "$plugin" | cut -d' ' -f1)"
+          hash="$(sha256sum "$plugin" | awk '{print $1}')"
           mkdir -p "$hash"
           tar -C "$hash" -xzf "$plugin"
           autoPatchelf "$hash"
-          GZIP_OPT=-9 tar -C "$hash" -cvzf "$out/data/plugins/$hash.tar.gz" .
+          GZIP_OPT=-9 tar -C "$hash" -cvzf "$out/$hash.tar.gz" .
           rm -rf "$hash"
         done
       '';
@@ -74,19 +129,38 @@ let
 
   mattermostConfWithoutPlugins = recursiveUpdate
     { ServiceSettings.SiteURL = cfg.siteUrl;
-      ServiceSettings.ListenAddress = cfg.listenAddress;
+      ServiceSettings.ListenAddress = "${cfg.host}:${toString cfg.port}";
       TeamSettings.SiteName = cfg.siteName;
       SqlSettings.DriverName = "postgres";
-      SqlSettings.DataSource = database;
-      PluginSettings.Directory = "${cfg.statePath}/plugins/server";
-      PluginSettings.ClientDirectory = "${cfg.statePath}/plugins/client";
+      SqlSettings.DataSource =
+        if cfg.database.fromEnvironment then
+          null
+        else
+          warnIf (!cfg.database.peerAuth && cfg.database.password != null) ''
+            Database password is set in Mattermost config! This password will end up in the Nix store.
+            Write the following to ${if cfg.environmentFile == null then "your environment file" else cfg.environmentFile}:
+
+            MM_SQLSETTINGS_DATASOURCE=${database}
+
+            Then set the following options:
+            services.mattermost.environmentFile = "<your environment file>";
+            services.mattermost.database.fromEnvironment = true;
+
+            Alternatively, you may be able to simply set the following, if the database is on the same host:
+            services.mattermost.database.peerAuth = true;
+          '' database;
+      FileSettings.Directory = cfg.dataDir;
+      PluginSettings.Directory = "${pluginDir}/server";
+      PluginSettings.ClientDirectory = "${pluginDir}/client";
+      LogSettings.FileLocation = cfg.logDir;
     }
-    cfg.extraConfig;
+    cfg.settings;
 
   mattermostConf = recursiveUpdate
     mattermostConfWithoutPlugins
     (
-      lib.optionalAttrs (mattermostPlugins != null) {
+      if mattermostPlugins == null then {}
+      else {
         PluginSettings = {
           Enable = true;
         };
@@ -96,20 +170,23 @@ let
   mattermostConfJSON = pkgs.writeText "mattermost-config.json" (builtins.toJSON mattermostConf);
 
 in
-
 {
+  imports = [
+    (mkRenamedOptionModule [ "services" "mattermost" "listenAddress" ] [ "services" "mattermost" "host" ])
+    (mkRenamedOptionModule [ "services" "mattermost" "localDatabaseCreate" ] [ "services" "mattermost" "database" "create" ])
+    (mkRenamedOptionModule [ "services" "mattermost" "localDatabasePassword" ] [ "services" "mattermost" "database" "password" ])
+    (mkRenamedOptionModule [ "services" "mattermost" "localDatabaseUser" ] [ "services" "mattermost" "database" "user" ])
+    (mkRenamedOptionModule [ "services" "mattermost" "localDatabaseName" ] [ "services" "mattermost" "database" "name" ])
+    (mkRenamedOptionModule [ "services" "mattermost" "extraConfig" ] [ "services" "mattermost" "settings" ])
+    (mkRenamedOptionModule [ "services" "mattermost" "statePath" ] [ "services" "mattermost" "dataDir" ])
+  ];
+
   options = {
     services.mattermost = {
       enable = mkEnableOption "Mattermost chat server";
 
       package = mkPackageOption pkgs "mattermost" { };
 
-      statePath = mkOption {
-        type = types.str;
-        default = "/var/lib/mattermost";
-        description = "Mattermost working directory";
-      };
-
       siteUrl = mkOption {
         type = types.str;
         example = "https://chat.example.com";
@@ -124,12 +201,44 @@ in
         description = "Name of this Mattermost site.";
       };
 
-      listenAddress = mkOption {
+      host = mkOption {
+        type = types.str;
+        default = "127.0.0.1";
+        example = "0.0.0.0";
+        description = ''
+          Host or address that this Mattermost instance listens on.
+        '';
+      };
+
+      port = mkOption {
+        type = types.port;
+        default = 8065;
+        description = ''
+          Port for Mattermost server to listen on.
+        '';
+      };
+
+      dataDir = mkOption {
+        type = types.str;
+        default = "/var/lib/mattermost";
+        description = ''
+          Mattermost working directory.
+        '';
+      };
+
+      logDir = mkOption {
+        type = types.str;
+        default = "/var/log/mattermost";
+        description = ''
+          Mattermost log directory.
+        '';
+      };
+
+      configDir = mkOption {
         type = types.str;
-        default = ":8065";
-        example = "[::1]:8065";
+        default = "/etc/mattermost";
         description = ''
-          Address and port this Mattermost instance listens to.
+          Mattermost config directory.
         '';
       };
 
@@ -160,16 +269,8 @@ in
         '';
       };
 
-      extraConfig = mkOption {
-        type = types.attrs;
-        default = { };
-        description = ''
-          Additional configuration options as Nix attribute set in config.json schema.
-        '';
-      };
-
       plugins = mkOption {
-        type = types.listOf (types.oneOf [types.path types.package]);
+        type = with types; listOf (either path package);
         default = [];
         example = "[ ./com.github.moussetc.mattermost.plugin.giphy-2.0.0.tar.gz ]";
         description = ''
@@ -179,7 +280,7 @@ in
         '';
       };
       environmentFile = mkOption {
-        type = types.nullOr types.path;
+        type = with types; nullOr path;
         default = null;
         description = ''
           Environment file (see {manpage}`systemd.exec(5)`
@@ -195,36 +296,94 @@ in
         '';
       };
 
-      localDatabaseCreate = mkOption {
-        type = types.bool;
-        default = true;
-        description = ''
-          Create a local PostgreSQL database for Mattermost automatically.
-        '';
-      };
+      database = {
+        create = mkOption {
+          type = types.bool;
+          default = true;
+          description = ''
+            Create a local PostgreSQL database for Mattermost automatically.
+          '';
+        };
 
-      localDatabaseName = mkOption {
-        type = types.str;
-        default = "mattermost";
-        description = ''
-          Local Mattermost database name.
-        '';
-      };
+        peerAuth = mkOption {
+          type = types.bool;
+          default = false;
+          description = ''
+            If set, will use peer auth instead of connecting to a Postgres server.
+            Use services.mattermost.database.socketPath to configure the socket path.
+          '';
+        };
 
-      localDatabaseUser = mkOption {
-        type = types.str;
-        default = "mattermost";
-        description = ''
-          Local Mattermost database username.
-        '';
-      };
+        socketPath = mkOption {
+          type = types.path;
+          default = "/run/postgresql";
+          description = ''
+            The Postgres socket path.
+          '';
+        };
 
-      localDatabasePassword = mkOption {
-        type = types.str;
-        default = "mmpgsecret";
-        description = ''
-          Password for local Mattermost database user.
-        '';
+        fromEnvironment = mkOption {
+          type = types.bool;
+          default = false;
+          description = ''
+            Use services.mattermost.environmentFile to configure the database instead of writing the database URI
+            to the Nix store. Useful if you use password authentication with peerAuth set to false.
+          '';
+        };
+
+        name = mkOption {
+          type = types.str;
+          default = "mattermost";
+          description = ''
+            Local Mattermost database name.
+          '';
+        };
+
+        host = mkOption {
+          type = types.str;
+          default = "localhost";
+          example = "127.0.0.1";
+          description = ''
+            Host to use for the Postgres database. If this is a path, this
+            will use socket authentication and ignore the port.
+          '';
+        };
+
+        port = mkOption {
+          type = types.port;
+          default = 5432;
+          description = ''
+            Port to use for the Postgres database.
+          '';
+        };
+
+        user = mkOption {
+          type = types.str;
+          default = "mattermost";
+          description = ''
+            Local Mattermost database username.
+          '';
+        };
+
+        password = mkOption {
+          type = types.str;
+          default = "mmpgsecret";
+          description = ''
+            Password for local Mattermost database user. If set and `host` is not a path,
+            will cause a warning nagging you to use environmentFile instead.
+          '';
+        };
+
+        extraConnectionOptions = mkOption {
+          type = with types; attrsOf (either int str);
+          default = {
+            sslmode = "disable";
+            connect_timeout = 10;
+          };
+          description = ''
+            Extra options that are placed in the connection URI's query parameters.
+          '';
+        };
       };
 
       user = mkOption {
@@ -243,6 +402,14 @@ in
         '';
       };
 
+      settings = mkOption {
+        type = types.attrs;
+        default = { };
+        description = ''
+          Additional configuration options as Nix attribute set in config.json schema.
+        '';
+      };
+
       matterircd = {
         enable = mkEnableOption "Mattermost IRC bridge";
         package = mkPackageOption pkgs "matterircd" { };
@@ -261,73 +428,148 @@ in
 
   config = mkMerge [
     (mkIf cfg.enable {
-      users.users = optionalAttrs (cfg.user == "mattermost") {
-        mattermost = {
-          group = cfg.group;
-          uid = config.ids.uids.mattermost;
-          home = cfg.statePath;
-        };
+      users.users.${cfg.user} = {
+        group = cfg.group;
+        uid = config.ids.uids.mattermost;
+        home = cfg.dataDir;
       };
 
-      users.groups = optionalAttrs (cfg.group == "mattermost") {
-        mattermost.gid = config.ids.gids.mattermost;
+      users.groups.${cfg.group} = {
+        gid = config.ids.gids.mattermost;
       };
 
-      services.postgresql.enable = cfg.localDatabaseCreate;
+      services.postgresql = mkIf cfg.database.create {
+        enable = true;
+        ensureDatabases = [ cfg.database.name ];
+        ensureUsers = [{
+          name = throwIf (cfg.database.peerAuth && (cfg.database.user != cfg.user || cfg.database.name != cfg.database.user)) ''
+            Mattermost database peer auth is enabled and the user, database user, or database name mismatch.
+            Peer authentication will not work.
+          '' cfg.database.user;
+          ensureDBOwnership = true;
+        }];
+      };
 
-      # The systemd service will fail to execute the preStart hook
-      # if the WorkingDirectory does not exist
-      systemd.tmpfiles.settings."10-mattermost".${cfg.statePath}.d = { };
+      systemd.tmpfiles.rules = [
+        "d ${cfg.dataDir} 0750 ${cfg.user} ${cfg.group} - -"
+        "d ${cfg.logDir} 0750 ${cfg.user} ${cfg.group} - -"
+        "d ${cfg.configDir} 0750 ${cfg.user} ${cfg.group} - -"
+        "d ${mutableDataDir} 0750 ${cfg.user} ${cfg.group} - -"
+
+        # Remove and recreate tempDir.
+        "R ${tempDir} - - - - -"
+        "d ${tempDir} 0750 ${cfg.user} ${cfg.group} - -"
+
+        # Ensure that pluginDir is a directory, as it could be a symlink on prior versions.
+        "r ${pluginDir} - - - - -"
+        "d ${pluginDir} 0750 ${cfg.user} ${cfg.group} - -"
+        "d ${mattermostConf.PluginSettings.Directory} 0750 ${cfg.user} ${cfg.group} - -"
+        "d ${mattermostConf.PluginSettings.ClientDirectory} 0750 ${cfg.user} ${cfg.group} - -"
+
+        "L+ ${cfg.dataDir}/fonts - - - - ${cfg.package}/fonts"
+        "L+ ${cfg.dataDir}/i18n - - - - ${cfg.package}/i18n"
+        "L+ ${cfg.dataDir}/templates - - - - ${cfg.package}/templates"
+        "L+ ${cfg.dataDir}/client - - - - ${cfg.package}/client"
+      ] ++ (
+        if mattermostPlugins == null then
+          # Create the plugin tarball directory if it's a symlink.
+          [
+            "r ${cfg.dataDir}/plugins - - - - -"
+            "d ${cfg.dataDir}/plugins 0750 ${cfg.user} ${cfg.group} - -"
+          ]
+        else
+          # Symlink the plugin tarball directory.
+          ["L+ ${cfg.dataDir}/plugins - - - - ${mattermostPlugins}"]
+      );
 
       systemd.services.mattermost = {
         description = "Mattermost chat service";
         wantedBy = [ "multi-user.target" ];
         after = [ "network.target" "postgresql.service" ];
+        requires = [ "network.target" "postgresql.service" ];
+
+        # Use tempDir as this can get rather large, especially if Mattermost unpacks a large number of plugins.
+        environment.TMPDIR = tempDir;
 
         preStart = ''
-          mkdir -p "${cfg.statePath}"/{data,config,logs,plugins}
-          mkdir -p "${cfg.statePath}/plugins"/{client,server}
-          ln -sf ${cfg.package}/{bin,fonts,i18n,templates,client} "${cfg.statePath}"
-        '' + lib.optionalString (mattermostPlugins != null) ''
-          rm -rf "${cfg.statePath}/data/plugins"
-          ln -sf ${mattermostPlugins}/data/plugins "${cfg.statePath}/data"
+          if [ -f ${cfg.dataDir}/config/config.json ] && [ ! -f ${cfg.configDir}/config.json ]; then
+            # Migrate the old config location to the new config location
+            cp ${cfg.dataDir}/config/config.json ${cfg.configDir}/config.json
+            touch "${cfg.configDir}/.initial-created"
+          fi
         '' + lib.optionalString (!cfg.mutableConfig) ''
-          rm -f "${cfg.statePath}/config/config.json"
-          ${pkgs.jq}/bin/jq -s '.[0] * .[1]' ${cfg.package}/config/config.json ${mattermostConfJSON} > "${cfg.statePath}/config/config.json"
+          ${pkgs.jq}/bin/jq -s '.[0] * .[1]' ${cfg.package}/config/config.json ${mattermostConfJSON} > "${cfg.configDir}/config.json"
         '' + lib.optionalString cfg.mutableConfig ''
-          if ! test -e "${cfg.statePath}/config/.initial-created"; then
-            rm -f ${cfg.statePath}/config/config.json
-            ${pkgs.jq}/bin/jq -s '.[0] * .[1]' ${cfg.package}/config/config.json ${mattermostConfJSON} > "${cfg.statePath}/config/config.json"
-            touch "${cfg.statePath}/config/.initial-created"
+          if [ ! -e "${cfg.configDir}/.initial-created" ]; then
+            ${pkgs.jq}/bin/jq -s '.[0] * .[1]' ${cfg.package}/config/config.json ${mattermostConfJSON} > "${cfg.configDir}/config.json"
+            touch "${cfg.configDir}/.initial-created"
           fi
         '' + lib.optionalString (cfg.mutableConfig && cfg.preferNixConfig) ''
-          new_config="$(${pkgs.jq}/bin/jq -s '.[0] * .[1]' "${cfg.statePath}/config/config.json" ${mattermostConfJSON})"
-
-          rm -f "${cfg.statePath}/config/config.json"
-          echo "$new_config" > "${cfg.statePath}/config/config.json"
-        '' + lib.optionalString cfg.localDatabaseCreate (createDb {}) + ''
-          # Don't change permissions recursively on the data, current, and symlinked directories (see ln -sf command above).
-          # This dramatically decreases startup times for installations with a lot of files.
-          find . -maxdepth 1 -not -name data -not -name client -not -name templates -not -name i18n -not -name fonts -not -name bin -not -name . \
-            -exec chown "${cfg.user}:${cfg.group}" -R {} \; -exec chmod u+rw,g+r,o-rwx -R {} \;
-
-          chown "${cfg.user}:${cfg.group}" "${cfg.statePath}/data" .
-          chmod u+rw,g+r,o-rwx "${cfg.statePath}/data" .
+          echo "$(${pkgs.jq}/bin/jq -s '.[0] * .[1]' "${cfg.configDir}/config.json" ${mattermostConfJSON})" > "${cfg.configDir}/config.json"
         '';
 
-        serviceConfig = {
-          PermissionsStartOnly = true;
-          User = cfg.user;
-          Group = cfg.group;
-          ExecStart = "${cfg.package}/bin/mattermost";
-          WorkingDirectory = "${cfg.statePath}";
-          Restart = "always";
-          RestartSec = "10";
-          LimitNOFILE = "49152";
-          EnvironmentFile = cfg.environmentFile;
-        };
-        unitConfig.JoinsNamespaceOf = mkIf cfg.localDatabaseCreate "postgresql.service";
+        serviceConfig = mkMerge [
+          {
+            User = cfg.user;
+            Group = cfg.group;
+            ExecStart = "${cfg.package}/bin/mattermost --config ${cfg.configDir}/config.json";
+            ReadWritePaths = [ cfg.dataDir cfg.logDir cfg.configDir ];
+            UMask = "0027";
+            Restart = "always";
+            RestartSec = 10;
+            LimitNOFILE = 49152;
+            LockPersonality = true;
+            NoNewPrivileges = true;
+            PrivateDevices = true;
+            PrivateTmp = true;
+            PrivateUsers = true;
+            ProtectClock = true;
+            ProtectControlGroups = true;
+            ProtectHome = true;
+            ProtectHostname = true;
+            ProtectKernelLogs = true;
+            ProtectKernelModules = true;
+            ProtectKernelTunables = true;
+            ProtectProc = "invisible";
+            ProtectSystem = "strict";
+            RestrictNamespaces = true;
+            RestrictSUIDSGID = true;
+            EnvironmentFile = cfg.environmentFile;
+            WorkingDirectory = cfg.dataDir;
+          }
+          (mkIf (cfg.dataDir == "/var/lib/mattermost") {
+            StateDirectory = baseNameOf cfg.dataDir;
+            StateDirectoryMode = "0750";
+          })
+          (mkIf (cfg.logDir == "/var/log/mattermost") {
+            LogsDirectory = baseNameOf cfg.logDir;
+            LogsDirectoryMode = "0750";
+          })
+          (mkIf (cfg.configDir == "/etc/mattermost") {
+            ConfigurationDirectory = baseNameOf cfg.configDir;
+            ConfigurationDirectoryMode = "0750";
+          })
+        ];
+
+        unitConfig.JoinsNamespaceOf = mkIf cfg.database.create "postgresql.service";
       };
+
+      assertions = [
+        {
+          # Make sure the URL doesn't have a trailing slash
+          assertion = !(hasSuffix "/" cfg.siteUrl);
+          message = ''
+            host or listenAddress should not include a port, use services.mattermost.host and services.mattermost.port to specify the port.
+          '';
+        }
+        {
+          # Make sure this isn't a host/port pair
+          assertion = !(hasInfix ":" cfg.host && !(hasInfix "[" cfg.host) && !(hasInfix "]" cfg.host));
+          message = ''
+            host or listenAddress should not include a port, use services.mattermost.host and services.mattermost.port to specify the port.
+          '';
+        }
+      ];
     })
     (mkIf cfg.matterircd.enable {
       systemd.services.matterircd = {
diff --git a/nixos/tests/mattermost.nix b/nixos/tests/mattermost.nix
index e11201f05357d..63dad310cc2ad 100644
--- a/nixos/tests/mattermost.nix
+++ b/nixos/tests/mattermost.nix
@@ -1,8 +1,8 @@
 import ./make-test-python.nix ({ pkgs, lib, ... }:
 let
   host = "smoke.test";
-  port = "8065";
-  url = "http://${host}:${port}";
+  port = 8065;
+  url = "http://${host}:${toString port}";
   siteName = "NixOS Smoke Tests, Inc.";
 
   makeMattermost = mattermostConfig:
@@ -15,13 +15,21 @@ let
       networking.hosts = {
         "127.0.0.1" = [ host ];
       };
+      services.postgresql.initialScript = lib.mkIf (!config.services.mattermost.database.peerAuth) (pkgs.writeText "init.sql" ''
+        create role ${config.services.mattermost.database.user} with login nocreatedb nocreaterole encrypted password '${config.services.mattermost.database.password}'
+      '');
       services.mattermost = lib.recursiveUpdate {
         enable = true;
         inherit siteName;
-        listenAddress = "0.0.0.0:${port}";
+        host = "0.0.0.0";
+        inherit port;
         siteUrl = url;
-        extraConfig = {
+        database = {
+          peerAuth = true;
+        };
+        settings = {
           SupportSettings.AboutLink = "https://nixos.org";
+          PluginSettings.AutomaticPrepackagedPlugins = false;
         };
       } mattermostConfig;
     };
@@ -32,109 +40,175 @@ in
   nodes = {
     mutable = makeMattermost {
       mutableConfig = true;
-      extraConfig.SupportSettings.HelpLink = "https://search.nixos.org";
+      settings.SupportSettings.HelpLink = "https://search.nixos.org";
     };
     mostlyMutable = makeMattermost {
       mutableConfig = true;
       preferNixConfig = true;
-      plugins = let
-        mattermostDemoPlugin = pkgs.fetchurl {
-          url = "https://github.com/mattermost/mattermost-plugin-demo/releases/download/v0.9.0/com.mattermost.demo-plugin-0.9.0.tar.gz";
-          sha256 = "1h4qi34gcxcx63z8wiqcf2aaywmvv8lys5g8gvsk13kkqhlmag25";
-        };
-      in [
-        mattermostDemoPlugin
+      plugins = [
+        (pkgs.fetchurl {
+          url = "https://github.com/mattermost-community/mattermost-plugin-todo/releases/download/v0.7.1/com.mattermost.plugin-todo-0.7.1.tar.gz";
+          sha256 = "1ki7vsvhjl2xgw4mfmnvn9s5hkn98l7nf4v9fdk59v44zbm7mriz";
+        })
       ];
     };
     immutable = makeMattermost {
       mutableConfig = false;
-      extraConfig.SupportSettings.HelpLink = "https://search.nixos.org";
+
+      # Make sure something other than the default works.
+      user = "mmuser";
+      group = "mmgroup";
+
+      database = {
+        peerAuth = false;
+      };
+      settings.SupportSettings.HelpLink = "https://search.nixos.org";
     };
     environmentFile = makeMattermost {
       mutableConfig = false;
-      extraConfig.SupportSettings.AboutLink = "https://example.org";
+      database.fromEnvironment = true;
+      settings.SupportSettings.AboutLink = "https://example.org";
       environmentFile = pkgs.writeText "mattermost-env" ''
+        MM_SQLSETTINGS_DATASOURCE=postgres:///mattermost?host=/run/postgresql
         MM_SUPPORTSETTINGS_ABOUTLINK=https://nixos.org
       '';
     };
   };
 
   testScript = let
-    expectConfig = jqExpression: pkgs.writeShellScript "expect-config" ''
+    expectMattermostUp = pkgs.writeShellScript "expect-mattermost-up" ''
       set -euo pipefail
-      echo "Expecting config to match: "${lib.escapeShellArg jqExpression} >&2
       curl ${lib.escapeShellArg url} >/dev/null
+    '';
+
+    expectConfig = pkgs.writeShellScript "expect-config" ''
+      set -euo pipefail
       config="$(curl ${lib.escapeShellArg "${url}/api/v4/config/client?format=old"})"
       echo "Config: $(echo "$config" | ${pkgs.jq}/bin/jq)" >&2
-      [[ "$(echo "$config" | ${pkgs.jq}/bin/jq -r ${lib.escapeShellArg ".SiteName == $siteName and .Version == ($mattermostName / $sep)[-1] and (${jqExpression})"} --arg siteName ${lib.escapeShellArg siteName} --arg mattermostName ${lib.escapeShellArg pkgs.mattermost.name} --arg sep '-')" = "true" ]]
+      [[ "$(echo "$config" | ${pkgs.jq}/bin/jq -r ${lib.escapeShellArg ".SiteName == $siteName and .Version == ($mattermostName / $sep)[-1] and "}"($1)" --arg siteName ${lib.escapeShellArg siteName} --arg mattermostName ${lib.escapeShellArg pkgs.mattermost.name} --arg sep '-')" = "true" ]]
     '';
 
-    setConfig = jqExpression: pkgs.writeShellScript "set-config" ''
+    setConfig = pkgs.writeShellScript "set-config" ''
       set -euo pipefail
-      mattermostConfig=/var/lib/mattermost/config/config.json
-      newConfig="$(${pkgs.jq}/bin/jq -r ${lib.escapeShellArg jqExpression} $mattermostConfig)"
-      rm -f $mattermostConfig
-      echo "$newConfig" > "$mattermostConfig"
+      mattermostConfig=/etc/mattermost/config.json
+      echo "Old config: $(echo "$config" | ${pkgs.jq}/bin/jq)" >&2
+      newConfig="$(${pkgs.jq}/bin/jq -r "$1" $mattermostConfig)"
+      echo "New config: $(echo "$newConfig" | ${pkgs.jq}/bin/jq)" >&2
+      truncate -s 0 "$mattermostConfig"
+      echo "$newConfig" >> "$mattermostConfig"
     '';
 
+    expectPlugins = pkgs.writeShellScript "expect-plugins" ''
+      set -euo pipefail
+      case "$1" in
+        ""|*[!0-9]*)
+          plugins="$(curl ${lib.escapeShellArg "${url}/api/v4/plugins/webapp"})"
+          echo "Plugins: $(echo "$plugins" | ${pkgs.jq}/bin/jq)" >&2
+          [[ "$(echo "$plugins" | ${pkgs.jq}/bin/jq -r "$1")" == "true" ]]
+          ;;
+        *)
+          code="$(curl -s -o /dev/null -w "%{http_code}" ${lib.escapeShellArg "${url}/api/v4/plugins/webapp"})"
+          [[ "$code" == "$1" ]]
+          ;;
+      esac
+    '';
   in
   ''
-    start_all()
+    import shlex
+
+    def wait_mattermost_up(node):
+      node.wait_for_unit("mattermost.service")
+      node.wait_for_open_port(8065)
+      node.succeed(f"curl {shlex.quote('${url}')} >/dev/null")
+
+    def restart_mattermost(node):
+      node.systemctl("restart mattermost.service")
+      wait_mattermost_up(node)
+
+    def expect_config(node, *configs):
+      for config in configs:
+        node.succeed(f"${expectConfig} {shlex.quote(config)}")
+
+    def expect_plugins(node, jq_or_code):
+      node.succeed(f"${expectPlugins} {shlex.quote(str(jq_or_code))}")
+
+    def set_config(node, *configs):
+      for config in configs:
+        node.succeed(f"${setConfig} {shlex.quote(config)}")
 
     ## Mutable node tests ##
-    mutable.wait_for_unit("mattermost.service")
-    mutable.wait_for_open_port(8065)
+    mutable.start()
+    wait_mattermost_up(mutable)
 
     # Get the initial config
-    mutable.succeed("${expectConfig ''.AboutLink == "https://nixos.org" and .HelpLink == "https://search.nixos.org"''}")
+    expect_config(mutable, '.AboutLink == "https://nixos.org" and .HelpLink == "https://search.nixos.org"')
 
     # Edit the config
-    mutable.succeed("${setConfig ''.SupportSettings.AboutLink = "https://mattermost.com"''}")
-    mutable.succeed("${setConfig ''.SupportSettings.HelpLink = "https://nixos.org/nixos/manual"''}")
-    mutable.systemctl("restart mattermost.service")
-    mutable.wait_for_open_port(8065)
+    set_config(
+      mutable,
+      '.SupportSettings.AboutLink = "https://mattermost.com"',
+      '.SupportSettings.HelpLink = "https://nixos.org/nixos/manual"'
+    )
+    restart_mattermost(mutable)
 
     # AboutLink and HelpLink should be changed
-    mutable.succeed("${expectConfig ''.AboutLink == "https://mattermost.com" and .HelpLink == "https://nixos.org/nixos/manual"''}")
+    expect_config(mutable, '.AboutLink == "https://mattermost.com" and .HelpLink == "https://nixos.org/nixos/manual"')
+    mutable.shutdown()
 
     ## Mostly mutable node tests ##
-    mostlyMutable.wait_for_unit("mattermost.service")
-    mostlyMutable.wait_for_open_port(8065)
+    mostlyMutable.start()
+    wait_mattermost_up(mostlyMutable)
 
     # Get the initial config
-    mostlyMutable.succeed("${expectConfig ''.AboutLink == "https://nixos.org"''}")
+    expect_config(mostlyMutable, '.AboutLink == "https://nixos.org"')
+
+    # No plugins.
+    expect_plugins(mostlyMutable, 'length == 0')
 
     # Edit the config
-    mostlyMutable.succeed("${setConfig ''.SupportSettings.AboutLink = "https://mattermost.com"''}")
-    mostlyMutable.succeed("${setConfig ''.SupportSettings.HelpLink = "https://nixos.org/nixos/manual"''}")
-    mostlyMutable.systemctl("restart mattermost.service")
-    mostlyMutable.wait_for_open_port(8065)
+    set_config(
+      mostlyMutable,
+      '.SupportSettings.AboutLink = "https://mattermost.com"',
+      '.SupportSettings.HelpLink = "https://nixos.org/nixos/manual"',
+      '.PluginSettings.PluginStates."com.mattermost.plugin-todo".Enable = true'
+    )
+    restart_mattermost(mostlyMutable)
 
     # AboutLink should be overridden by NixOS configuration; HelpLink should be what we set above
-    mostlyMutable.succeed("${expectConfig ''.AboutLink == "https://nixos.org" and .HelpLink == "https://nixos.org/nixos/manual"''}")
+    expect_config(mostlyMutable, '.AboutLink == "https://nixos.org" and .HelpLink == "https://nixos.org/nixos/manual"')
+
+    # Single plugin that's now enabled.
+    expect_plugins(mostlyMutable, 'length == 1')
+    mostlyMutable.shutdown()
 
     ## Immutable node tests ##
-    immutable.wait_for_unit("mattermost.service")
-    immutable.wait_for_open_port(8065)
+    immutable.start()
+    wait_mattermost_up(immutable)
 
     # Get the initial config
-    immutable.succeed("${expectConfig ''.AboutLink == "https://nixos.org" and .HelpLink == "https://search.nixos.org"''}")
+    expect_config(immutable, '.AboutLink == "https://nixos.org" and .HelpLink == "https://search.nixos.org"')
 
     # Edit the config
-    immutable.succeed("${setConfig ''.SupportSettings.AboutLink = "https://mattermost.com"''}")
-    immutable.succeed("${setConfig ''.SupportSettings.HelpLink = "https://nixos.org/nixos/manual"''}")
-    immutable.systemctl("restart mattermost.service")
-    immutable.wait_for_open_port(8065)
+    set_config(
+      immutable,
+      '.SupportSettings.AboutLink = "https://mattermost.com"',
+      '.SupportSettings.HelpLink = "https://nixos.org/nixos/manual"'
+    )
+    restart_mattermost(immutable)
 
     # Our edits should be ignored on restart
-    immutable.succeed("${expectConfig ''.AboutLink == "https://nixos.org" and .HelpLink == "https://search.nixos.org"''}")
+    expect_config(immutable, '.AboutLink == "https://nixos.org" and .HelpLink == "https://search.nixos.org"')
 
+    # No plugins.
+    expect_plugins(immutable, 'length == 0')
+    immutable.shutdown()
 
     ## Environment File node tests ##
-    environmentFile.wait_for_unit("mattermost.service")
-    environmentFile.wait_for_open_port(8065)
+    environmentFile.start()
+    wait_mattermost_up(environmentFile)
 
     # Settings in the environment file should override settings set otherwise
-    environmentFile.succeed("${expectConfig ''.AboutLink == "https://nixos.org"''}")
+    expect_config(environmentFile, '.AboutLink == "https://nixos.org"')
+    environmentFile.shutdown()
   '';
 })

From 8be164076fdd0f4e9091062b1a6316aa7c995e45 Mon Sep 17 00:00:00 2001
From: Morgan Jones <me@numin.it>
Date: Sat, 26 Oct 2024 15:57:57 -0700
Subject: [PATCH 2/3] nixos/matermost: address review feedback; add regression
 test

---
 .../modules/services/web-apps/mattermost.nix  | 105 ++++++++++------
 nixos/tests/mattermost.nix                    | 112 +++++++++++-------
 2 files changed, 140 insertions(+), 77 deletions(-)

diff --git a/nixos/modules/services/web-apps/mattermost.nix b/nixos/modules/services/web-apps/mattermost.nix
index 7ea4aa943906f..7845c6990f072 100644
--- a/nixos/modules/services/web-apps/mattermost.nix
+++ b/nixos/modules/services/web-apps/mattermost.nix
@@ -2,7 +2,7 @@
 
 let
   inherit (lib.strings)
-    hasInfix hasSuffix escapeURL concatStringsSep escapeShellArgs;
+    hasInfix hasSuffix escapeURL concatStringsSep escapeShellArg escapeShellArgs versionAtLeast;
 
   inherit (lib.attrsets)
     mapAttrsToList recursiveUpdate;
@@ -51,8 +51,10 @@ let
         ""
       else if user != null && password != null then
         escapeURL user + ":" + escapeURL password
+      else if user != null then
+        escapeURL user
       else
-        escapeURL (if password != null then password else user);
+        throw "Either user or username and password must be provided";
     hostPart =
       if userPart == "" then
         escapeURL (nullToEmpty host)
@@ -63,7 +65,7 @@ let
       else ":" + (toString port);
     pathPart =
       if path == null then ""
-      else "/" + (escapeURL path);
+      else "/" + path;
     queryPart =
       if query == {} then ""
       else "?" + concatStringsSep "&" (toQuery query);
@@ -73,13 +75,13 @@ let
   database =
     if cfg.database.peerAuth then
       mkDatabaseUri {
-        path = cfg.database.name;
+        path = escapeURL cfg.database.name;
         query = { host = cfg.database.socketPath; } // cfg.database.extraConnectionOptions;
       }
     else
       mkDatabaseUri {
         inherit (cfg.database) user password host port;
-        path = cfg.database.name;
+        path = escapeURL cfg.database.name;
         query = cfg.database.extraConnectionOptions;
       };
 
@@ -228,7 +230,8 @@ in
 
       logDir = mkOption {
         type = types.str;
-        default = "/var/log/mattermost";
+        default = if versionAtLeast config.system.stateVersion "24.11" then "/var/log/mattermost"
+                  else "${cfg.dataDir}/logs";
         description = ''
           Mattermost log directory.
         '';
@@ -236,7 +239,8 @@ in
 
       configDir = mkOption {
         type = types.str;
-        default = "/etc/mattermost";
+        default = if versionAtLeast config.system.stateVersion "24.11" then "/etc/mattermost"
+                  else "${cfg.dataDir}/config";
         description = ''
           Mattermost config directory.
         '';
@@ -307,7 +311,7 @@ in
 
         peerAuth = mkOption {
           type = types.bool;
-          default = false;
+          default = versionAtLeast config.system.stateVersion "24.11" && cfg.database.host == "localhost";
           description = ''
             If set, will use peer auth instead of connecting to a Postgres server.
             Use services.mattermost.database.socketPath to configure the socket path.
@@ -428,14 +432,19 @@ in
 
   config = mkMerge [
     (mkIf cfg.enable {
-      users.users.${cfg.user} = {
-        group = cfg.group;
-        uid = config.ids.uids.mattermost;
-        home = cfg.dataDir;
+      users.users = {
+        ${cfg.user} = {
+          group = cfg.group;
+          uid = mkIf (cfg.user == "mattermost") config.ids.uids.mattermost;
+          home = cfg.dataDir;
+          isSystemUser = true;
+        };
       };
 
-      users.groups.${cfg.group} = {
-        gid = config.ids.gids.mattermost;
+      users.groups = {
+        ${cfg.group} = {
+          gid = mkIf (cfg.group == "mattermost") config.ids.gids.mattermost;
+        };
       };
 
       services.postgresql = mkIf cfg.database.create {
@@ -456,16 +465,20 @@ in
         "d ${cfg.configDir} 0750 ${cfg.user} ${cfg.group} - -"
         "d ${mutableDataDir} 0750 ${cfg.user} ${cfg.group} - -"
 
-        # Remove and recreate tempDir.
-        "R ${tempDir} - - - - -"
-        "d ${tempDir} 0750 ${cfg.user} ${cfg.group} - -"
+        # Make sure tempDir exists and is not a symlink.
+        "R- ${tempDir} - - - - -"
+        "d= ${tempDir} 0750 ${cfg.user} ${cfg.group} - -"
 
         # Ensure that pluginDir is a directory, as it could be a symlink on prior versions.
-        "r ${pluginDir} - - - - -"
-        "d ${pluginDir} 0750 ${cfg.user} ${cfg.group} - -"
-        "d ${mattermostConf.PluginSettings.Directory} 0750 ${cfg.user} ${cfg.group} - -"
-        "d ${mattermostConf.PluginSettings.ClientDirectory} 0750 ${cfg.user} ${cfg.group} - -"
+        "r- ${pluginDir} - - - - -"
+        "d= ${pluginDir} 0750 ${cfg.user} ${cfg.group} - -"
+
+        # Ensure that the plugin directories exist.
+        "d= ${mattermostConf.PluginSettings.Directory} 0750 ${cfg.user} ${cfg.group} - -"
+        "d= ${mattermostConf.PluginSettings.ClientDirectory} 0750 ${cfg.user} ${cfg.group} - -"
 
+        # Link in some of the immutable data directories.
+        "L+ ${cfg.dataDir}/bin - - - - ${cfg.package}/bin"
         "L+ ${cfg.dataDir}/fonts - - - - ${cfg.package}/fonts"
         "L+ ${cfg.dataDir}/i18n - - - - ${cfg.package}/i18n"
         "L+ ${cfg.dataDir}/templates - - - - ${cfg.package}/templates"
@@ -474,11 +487,11 @@ in
         if mattermostPlugins == null then
           # Create the plugin tarball directory if it's a symlink.
           [
-            "r ${cfg.dataDir}/plugins - - - - -"
-            "d ${cfg.dataDir}/plugins 0750 ${cfg.user} ${cfg.group} - -"
+            "r- ${cfg.dataDir}/plugins - - - - -"
+            "d= ${cfg.dataDir}/plugins 0750 ${cfg.user} ${cfg.group} - -"
           ]
         else
-          # Symlink the plugin tarball directory.
+          # Symlink the plugin tarball directory, removing anything existing.
           ["L+ ${cfg.dataDir}/plugins - - - - ${mattermostPlugins}"]
       );
 
@@ -492,20 +505,42 @@ in
         environment.TMPDIR = tempDir;
 
         preStart = ''
-          if [ -f ${cfg.dataDir}/config/config.json ] && [ ! -f ${cfg.configDir}/config.json ]; then
-            # Migrate the old config location to the new config location
-            cp ${cfg.dataDir}/config/config.json ${cfg.configDir}/config.json
-            touch "${cfg.configDir}/.initial-created"
+          dataDir=${escapeShellArg cfg.dataDir}
+          configDir=${escapeShellArg cfg.configDir}
+          logDir=${escapeShellArg cfg.logDir}
+          package=${escapeShellArg cfg.package}
+          nixConfig=${escapeShellArg mattermostConfJSON}
+        '' + lib.optionalString (versionAtLeast config.system.stateVersion "24.11") ''
+          # Migrate configs in the pre-24.11 directory structure.
+          oldConfig="$dataDir/config/config.json"
+          newConfig="$configDir/config.json"
+          if [ "$oldConfig" != "$newConfig" ] && [ -f "$oldConfig" ] && [ ! -f "$newConfig" ]; then
+            # Migrate the legacy config location to the new config location
+            echo "Moving legacy config at $oldConfig to $newConfig" >&2
+            mkdir -p "$configDir"
+            mv "$oldConfig" "$newConfig"
+            touch "$configDir/.initial-created"
+          fi
+
+          # Logs too.
+          oldLogs="$dataDir/logs"
+          newLogs="$logDir"
+          if [ "$oldLogs" != "$newLogs" ] && [ -d "$oldLogs" ]; then
+            # Migrate the legacy log location to the new log location.
+            # Allow this to fail if there aren't any logs to move.
+            echo "Moving legacy logs at $oldLogs to $newLogs" >&2
+            mkdir -p "$newLogs"
+            mv "$oldLogs"/* "$newLogs" || true
           fi
         '' + lib.optionalString (!cfg.mutableConfig) ''
-          ${pkgs.jq}/bin/jq -s '.[0] * .[1]' ${cfg.package}/config/config.json ${mattermostConfJSON} > "${cfg.configDir}/config.json"
+          ${pkgs.jq}/bin/jq -s '.[0] * .[1]' "$package/config/config.json" "$nixConfig" > "$configDir/config.json"
         '' + lib.optionalString cfg.mutableConfig ''
-          if [ ! -e "${cfg.configDir}/.initial-created" ]; then
-            ${pkgs.jq}/bin/jq -s '.[0] * .[1]' ${cfg.package}/config/config.json ${mattermostConfJSON} > "${cfg.configDir}/config.json"
-            touch "${cfg.configDir}/.initial-created"
+          if [ ! -e "$configDir/.initial-created" ]; then
+            ${pkgs.jq}/bin/jq -s '.[0] * .[1]' "$package/config/config.json" "$nixConfig" > "$configDir/config.json"
+            touch "$configDir/.initial-created"
           fi
         '' + lib.optionalString (cfg.mutableConfig && cfg.preferNixConfig) ''
-          echo "$(${pkgs.jq}/bin/jq -s '.[0] * .[1]' "${cfg.configDir}/config.json" ${mattermostConfJSON})" > "${cfg.configDir}/config.json"
+          echo "$(${pkgs.jq}/bin/jq -s '.[0] * .[1]' "$configDir/config.json" "$nixConfig")" > "$configDir/config.json"
         '';
 
         serviceConfig = mkMerge [
@@ -559,7 +594,7 @@ in
           # Make sure the URL doesn't have a trailing slash
           assertion = !(hasSuffix "/" cfg.siteUrl);
           message = ''
-            host or listenAddress should not include a port, use services.mattermost.host and services.mattermost.port to specify the port.
+            siteUrl should not have a trailing "/"
           '';
         }
         {
@@ -587,4 +622,6 @@ in
       };
     })
   ];
+
+  meta.maintainers = with lib.maintainers; [ numinit ];
 }
diff --git a/nixos/tests/mattermost.nix b/nixos/tests/mattermost.nix
index 63dad310cc2ad..9dadf01fde08b 100644
--- a/nixos/tests/mattermost.nix
+++ b/nixos/tests/mattermost.nix
@@ -5,34 +5,38 @@ let
   url = "http://${host}:${toString port}";
   siteName = "NixOS Smoke Tests, Inc.";
 
-  makeMattermost = mattermostConfig:
-    { config, ... }: {
-      environment.systemPackages = [
-        pkgs.mattermost
-        pkgs.curl
-        pkgs.jq
-      ];
-      networking.hosts = {
-        "127.0.0.1" = [ host ];
-      };
-      services.postgresql.initialScript = lib.mkIf (!config.services.mattermost.database.peerAuth) (pkgs.writeText "init.sql" ''
-        create role ${config.services.mattermost.database.user} with login nocreatedb nocreaterole encrypted password '${config.services.mattermost.database.password}'
-      '');
-      services.mattermost = lib.recursiveUpdate {
-        enable = true;
-        inherit siteName;
-        host = "0.0.0.0";
-        inherit port;
-        siteUrl = url;
-        database = {
-          peerAuth = true;
-        };
-        settings = {
-          SupportSettings.AboutLink = "https://nixos.org";
-          PluginSettings.AutomaticPrepackagedPlugins = false;
+  makeMattermost = mattermostConfig: extraConfig:
+    lib.mkMerge [
+      ({ config, ... }: {
+        environment.systemPackages = [
+          pkgs.mattermost
+          pkgs.curl
+          pkgs.jq
+        ];
+        networking.hosts = {
+          "127.0.0.1" = [ host ];
         };
-      } mattermostConfig;
-    };
+        services.postgresql.initialScript = lib.mkIf (!config.services.mattermost.database.peerAuth) (pkgs.writeText "init.sql" ''
+          create role ${config.services.mattermost.database.user} with login nocreatedb nocreaterole encrypted password '${config.services.mattermost.database.password}'
+        '');
+        services.mattermost = lib.recursiveUpdate {
+          enable = true;
+          inherit siteName;
+          host = "0.0.0.0";
+          inherit port;
+          siteUrl = url;
+          database = {
+            peerAuth = true;
+          };
+          settings = {
+            SupportSettings.AboutLink = "https://nixos.org";
+            PluginSettings.AutomaticPrepackagedPlugins = false;
+          };
+        } mattermostConfig;
+      }
+    )
+    extraConfig
+  ];
 in
 {
   name = "mattermost";
@@ -41,6 +45,12 @@ in
     mutable = makeMattermost {
       mutableConfig = true;
       settings.SupportSettings.HelpLink = "https://search.nixos.org";
+    } {
+      # Last version to support the "old" config layout.
+      system.stateVersion = "24.05";
+
+      # First version to support the "new" config layout.
+      specialisation.upgrade.configuration.system.stateVersion = lib.mkForce "24.11";
     };
     mostlyMutable = makeMattermost {
       mutableConfig = true;
@@ -51,7 +61,7 @@ in
           sha256 = "1ki7vsvhjl2xgw4mfmnvn9s5hkn98l7nf4v9fdk59v44zbm7mriz";
         })
       ];
-    };
+    } {};
     immutable = makeMattermost {
       mutableConfig = false;
 
@@ -63,7 +73,7 @@ in
         peerAuth = false;
       };
       settings.SupportSettings.HelpLink = "https://search.nixos.org";
-    };
+    } {};
     environmentFile = makeMattermost {
       mutableConfig = false;
       database.fromEnvironment = true;
@@ -72,15 +82,10 @@ in
         MM_SQLSETTINGS_DATASOURCE=postgres:///mattermost?host=/run/postgresql
         MM_SUPPORTSETTINGS_ABOUTLINK=https://nixos.org
       '';
-    };
+    } {};
   };
 
-  testScript = let
-    expectMattermostUp = pkgs.writeShellScript "expect-mattermost-up" ''
-      set -euo pipefail
-      curl ${lib.escapeShellArg url} >/dev/null
-    '';
-
+  testScript = { nodes, ... }: let
     expectConfig = pkgs.writeShellScript "expect-config" ''
       set -euo pipefail
       config="$(curl ${lib.escapeShellArg "${url}/api/v4/config/client?format=old"})"
@@ -89,11 +94,19 @@ in
     '';
 
     setConfig = pkgs.writeShellScript "set-config" ''
-      set -euo pipefail
+      set -eo pipefail
       mattermostConfig=/etc/mattermost/config.json
-      echo "Old config: $(echo "$config" | ${pkgs.jq}/bin/jq)" >&2
-      newConfig="$(${pkgs.jq}/bin/jq -r "$1" $mattermostConfig)"
-      echo "New config: $(echo "$newConfig" | ${pkgs.jq}/bin/jq)" >&2
+      nixosVersion="$2"
+      if [ -z "$nixosVersion" ]; then
+        nixosVersion="$(nixos-version)"
+      fi
+      nixosVersion="$(echo "$nixosVersion" | sed -nr 's/^([0-9]{2})\.([0-9]{2}).*/\1\2/p')"
+      echo "NixOS version: $nixosVersion" >&2
+      if [ "$nixosVersion" -lt 2411 ]; then
+        mattermostConfig=/var/lib/mattermost/config/config.json
+      fi
+      newConfig="$(${pkgs.jq}/bin/jq -r "$1" "$mattermostConfig")"
+      echo "New config @ $mattermostConfig: $(echo "$newConfig" | ${pkgs.jq}/bin/jq)" >&2
       truncate -s 0 "$mattermostConfig"
       echo "$newConfig" >> "$mattermostConfig"
     '';
@@ -132,9 +145,13 @@ in
     def expect_plugins(node, jq_or_code):
       node.succeed(f"${expectPlugins} {shlex.quote(str(jq_or_code))}")
 
-    def set_config(node, *configs):
+    def set_config(node, *configs, nixos_version=None):
       for config in configs:
-        node.succeed(f"${setConfig} {shlex.quote(config)}")
+        args = [shlex.quote("${setConfig}")]
+        args.append(shlex.quote(config))
+        if nixos_version:
+          args.append(shlex.quote(str(nixos_version)))
+        node.succeed(' '.join(args))
 
     ## Mutable node tests ##
     mutable.start()
@@ -147,12 +164,21 @@ in
     set_config(
       mutable,
       '.SupportSettings.AboutLink = "https://mattermost.com"',
-      '.SupportSettings.HelpLink = "https://nixos.org/nixos/manual"'
+      '.SupportSettings.HelpLink = "https://nixos.org/nixos/manual"',
+      nixos_version='24.05' # Default 'mutable' config is an old version
     )
     restart_mattermost(mutable)
 
     # AboutLink and HelpLink should be changed
     expect_config(mutable, '.AboutLink == "https://mattermost.com" and .HelpLink == "https://nixos.org/nixos/manual"')
+
+    # Switch to the newer config
+    mutable.succeed("${nodes.mutable.system.build.toplevel}/specialisation/upgrade/bin/switch-to-configuration switch")
+    wait_mattermost_up(mutable)
+
+    # AboutLink and HelpLink should be changed, still
+    expect_config(mutable, '.AboutLink == "https://mattermost.com" and .HelpLink == "https://nixos.org/nixos/manual"')
+
     mutable.shutdown()
 
     ## Mostly mutable node tests ##

From b313008f5c173a1aa8605c626a29a633ad4e4153 Mon Sep 17 00:00:00 2001
From: Morgan Jones <me@numin.it>
Date: Sat, 26 Oct 2024 16:08:55 -0700
Subject: [PATCH 3/3] nixos/mattermost: Add various defaultTexts

---
 nixos/modules/services/web-apps/mattermost.nix | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/nixos/modules/services/web-apps/mattermost.nix b/nixos/modules/services/web-apps/mattermost.nix
index 7845c6990f072..043798c2f8503 100644
--- a/nixos/modules/services/web-apps/mattermost.nix
+++ b/nixos/modules/services/web-apps/mattermost.nix
@@ -232,6 +232,10 @@ in
         type = types.str;
         default = if versionAtLeast config.system.stateVersion "24.11" then "/var/log/mattermost"
                   else "${cfg.dataDir}/logs";
+        defaultText = ''
+          if versionAtLeast config.system.stateVersion "24.11" then "/var/log/mattermost"
+          else "''${cfg.dataDir}/logs";
+        '';
         description = ''
           Mattermost log directory.
         '';
@@ -241,6 +245,10 @@ in
         type = types.str;
         default = if versionAtLeast config.system.stateVersion "24.11" then "/etc/mattermost"
                   else "${cfg.dataDir}/config";
+        defaultText = ''
+          if versionAtLeast config.system.stateVersion "24.11" then "/etc/mattermost"
+          else "''${cfg.dataDir}/config";
+        '';
         description = ''
           Mattermost config directory.
         '';
@@ -312,6 +320,7 @@ in
         peerAuth = mkOption {
           type = types.bool;
           default = versionAtLeast config.system.stateVersion "24.11" && cfg.database.host == "localhost";
+          defaultText = ''versionAtLeast config.system.stateVersion "24.11" && cfg.database.host == "localhost"'';
           description = ''
             If set, will use peer auth instead of connecting to a Postgres server.
             Use services.mattermost.database.socketPath to configure the socket path.
@@ -374,7 +383,8 @@ in
           default = "mmpgsecret";
           description = ''
             Password for local Mattermost database user. If set and `host` is not a path,
-            will cause a warning nagging you to use environmentFile instead.
+            will cause a warning nagging you to use environmentFile instead since it will
+            end up in the Nix store.
           '';
         };