From 01acea68381bc79e7f89e6b11d07522c7ec4f3bb Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 7 May 2026 10:01:19 +0200 Subject: [PATCH] fix: make sure smtp can be set either via env or ui --- CODE_GUIDELINES.md | 3 +- docs/smtp-configuration-concept.md | 13 ++- lib/mv/config.ex | 107 ++++++++++++++---- lib/mv_web/live/global_settings_live.ex | 40 +++++-- priv/gettext/de/LC_MESSAGES/default.po | 10 ++ priv/gettext/default.pot | 10 ++ priv/gettext/en/LC_MESSAGES/default.po | 10 ++ test/mv/config_smtp_test.exs | 22 +++- .../mv_web/live/global_settings_live_test.exs | 65 +++++++++++ 9 files changed, 238 insertions(+), 42 deletions(-) diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 0d478f9..d721a3a 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -1277,7 +1277,8 @@ mix hex.outdated **SMTP configuration:** -- SMTP can be configured via **ENV variables** (`SMTP_HOST`, `SMTP_PORT`, `SMTP_USERNAME`, `SMTP_PASSWORD`, `SMTP_PASSWORD_FILE`, `SMTP_SSL`) or via **Admin Settings** (database: `smtp_host`, `smtp_port`, `smtp_username`, `smtp_password`, `smtp_ssl`). ENV takes priority (same pattern as OIDC/Vereinfacht). +- SMTP can be configured via **ENV variables** (`SMTP_HOST`, `SMTP_PORT`, `SMTP_USERNAME`, `SMTP_PASSWORD`, `SMTP_PASSWORD_FILE`, `SMTP_SSL`) or via **Admin Settings** (database: `smtp_host`, `smtp_port`, `smtp_username`, `smtp_password`, `smtp_ssl`). +- **ENV-only policy:** If `SMTP_HOST` is set, SMTP is treated as environment-managed only. All SMTP fields in Settings are read-only, SMTP save action is hidden, and the UI shows a warning when required ENV values are missing (`SMTP_USERNAME`, and `SMTP_PASSWORD` or `SMTP_PASSWORD_FILE`). - **Sensitive settings in DB:** `smtp_password` and `oidc_client_secret` are excluded from the default read of the Setting resource; they are loaded only via explicit select when needed (e.g. `Mv.Config.smtp_password/0`, `Mv.Config.oidc_client_secret/0`). This avoids exposing secrets through `get_settings()`. - **Settings cache:** `Mv.Membership.get_settings/0` uses `Mv.Membership.SettingsCache` when the cache process is running (not in test). Cache has a short TTL and is invalidated on every settings update. This avoids repeated DB reads on hot paths (e.g. `RegistrationEnabled` validation, `Layouts.public_page`). In test, the cache is not started so all callers use `get_settings_uncached/0` in the test process (Ecto Sandbox). - **Join emails (domain → web):** The domain calls `Mv.Membership.JoinNotifier` (config `:join_notifier`, default `MvWeb.JoinNotifierImpl`) for sending join confirmation, already-member, and already-pending emails. This keeps the domain independent of the web layer; tests can override the notifier. diff --git a/docs/smtp-configuration-concept.md b/docs/smtp-configuration-concept.md index 4ae7760..6668485 100644 --- a/docs/smtp-configuration-concept.md +++ b/docs/smtp-configuration-concept.md @@ -25,7 +25,10 @@ Enable configurable SMTP for sending transactional emails (join confirmation, us | ENV | 1 | Production, Docker, 12-factor | | Settings | 2 | Admin UI, dev without ENV | -When an ENV variable is set, the corresponding Settings field is read-only in the UI (with hint "Set by environment"). +When `SMTP_HOST` is set, SMTP runs in **ENV-only mode**: +- all SMTP fields in Settings are read-only, +- saving SMTP settings in the UI is disabled, +- and the UI shows a warning block if required SMTP ENV values are missing. --- @@ -63,6 +66,14 @@ Support **SMTP_PASSWORD_FILE** (path to file containing the password), same patt - Show a warning in the Settings UI. - Delivery attempts silently fall back to the Local adapter (no crash). +### 6.1 Behaviour in ENV-only mode (`SMTP_HOST` set) + +- The SMTP source of truth is environment variables only. +- The UI does not allow editing SMTP fields in this mode. +- The Settings page shows a warning block when required values are missing: + - `SMTP_USERNAME` + - `SMTP_PASSWORD` or `SMTP_PASSWORD_FILE` + --- ## 7. Test Email (Settings UI) diff --git a/lib/mv/config.ex b/lib/mv/config.ex index 3494937..c807193 100644 --- a/lib/mv/config.ex +++ b/lib/mv/config.ex @@ -478,48 +478,61 @@ defmodule Mv.Config do end @doc """ - Returns SMTP port as integer. ENV `SMTP_PORT` (parsed) overrides Settings. - Returns nil when neither ENV nor Settings provide a valid port. + Returns SMTP port as integer. + + Policy: + - ENV-only mode (`SMTP_HOST` set): read from ENV `SMTP_PORT` + - Settings mode: read from Settings only """ @spec smtp_port() :: non_neg_integer() | nil def smtp_port do - case System.get_env("SMTP_PORT") do - nil -> - get_from_settings_integer(:smtp_port) - - value when is_binary(value) -> - case Integer.parse(String.trim(value)) do - {port, _} when port > 0 -> port - _ -> nil - end + if smtp_env_mode?() do + parse_smtp_port_env(System.get_env("SMTP_PORT")) + else + get_from_settings_integer(:smtp_port) end end @doc """ - Returns SMTP username. ENV `SMTP_USERNAME` overrides Settings. + Returns SMTP username. + + Policy: + - ENV-only mode (`SMTP_HOST` set): read from ENV `SMTP_USERNAME` + - Settings mode: read from Settings only """ @spec smtp_username() :: String.t() | nil def smtp_username do - smtp_env_or_setting("SMTP_USERNAME", :smtp_username) + if smtp_env_mode?() do + System.get_env("SMTP_USERNAME") |> trim_nil() + else + get_from_settings(:smtp_username) + end end @doc """ Returns SMTP password. - Priority: `SMTP_PASSWORD` ENV > `SMTP_PASSWORD_FILE` (file contents) > Settings. + Policy: + - ENV-only mode (`SMTP_HOST` set): `SMTP_PASSWORD` > `SMTP_PASSWORD_FILE` + - Settings mode: read from Settings only + Strips trailing whitespace/newlines from file contents. """ @spec smtp_password() :: String.t() | nil def smtp_password do - case System.get_env("SMTP_PASSWORD") do - nil -> smtp_password_from_file_or_settings() - value -> trim_nil(value) + if smtp_env_mode?() do + case System.get_env("SMTP_PASSWORD") do + nil -> smtp_password_from_file_or_settings() + value -> trim_nil(value) + end + else + get_smtp_password_from_settings() end end defp smtp_password_from_file_or_settings do case System.get_env("SMTP_PASSWORD_FILE") do - nil -> get_smtp_password_from_settings() + nil -> nil path -> read_smtp_password_file(path) end end @@ -533,11 +546,18 @@ defmodule Mv.Config do @doc """ Returns SMTP TLS/SSL mode string (e.g. 'tls', 'ssl', 'none'). - ENV `SMTP_SSL` overrides Settings. + + Policy: + - ENV-only mode (`SMTP_HOST` set): read from ENV `SMTP_SSL` + - Settings mode: read from Settings only """ @spec smtp_ssl() :: String.t() | nil def smtp_ssl do - smtp_env_or_setting("SMTP_SSL", :smtp_ssl) + if smtp_env_mode?() do + System.get_env("SMTP_SSL") |> trim_nil() + else + get_from_settings(:smtp_ssl) + end end @doc """ @@ -549,12 +569,39 @@ defmodule Mv.Config do end @doc """ - Returns true when any SMTP ENV variable is set (used in Settings UI for hints). + Returns true when SMTP ENV mode is active. """ @spec smtp_env_configured?() :: boolean() def smtp_env_configured? do - smtp_host_env_set?() or smtp_port_env_set?() or smtp_username_env_set?() or - smtp_password_env_set?() or smtp_ssl_env_set?() + smtp_env_mode?() + end + + @doc """ + Returns true when SMTP is managed by environment variables. + + Policy: if `SMTP_HOST` is set, SMTP is treated as ENV-only. + """ + @spec smtp_env_mode?() :: boolean() + def smtp_env_mode? do + smtp_host_env_set?() + end + + @doc """ + Returns missing required SMTP ENV keys for ENV-only mode warnings. + + Required in ENV-only mode: + - `SMTP_USERNAME` + - one of `SMTP_PASSWORD` or `SMTP_PASSWORD_FILE` + """ + @spec smtp_missing_required_env_keys() :: [String.t()] + def smtp_missing_required_env_keys do + if smtp_env_mode?() do + [] + |> maybe_add_missing("SMTP_USERNAME", smtp_username_env_set?()) + |> maybe_add_missing("SMTP_PASSWORD/SMTP_PASSWORD_FILE", smtp_password_env_set?()) + else + [] + end end @doc "Returns true if SMTP_HOST ENV is set." @@ -618,6 +665,17 @@ defmodule Mv.Config do @spec mail_from_email_env_set?() :: boolean() def mail_from_email_env_set?, do: env_set?("MAIL_FROM_EMAIL") + defp parse_smtp_port_env(nil), do: nil + + defp parse_smtp_port_env(value) when is_binary(value) do + case Integer.parse(String.trim(value)) do + {port, _} when port > 0 -> port + _ -> nil + end + end + + defp parse_smtp_port_env(_), do: nil + # Reads a plain string SMTP setting: ENV first, then Settings. defp smtp_env_or_setting(env_key, setting_key) do case System.get_env(env_key) do @@ -626,6 +684,9 @@ defmodule Mv.Config do end end + defp maybe_add_missing(acc, _label, true), do: acc + defp maybe_add_missing(acc, label, false), do: acc ++ [label] + # Reads an integer setting attribute from Settings. defp get_from_settings_integer(key) do case Mv.Membership.get_settings() do diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index 43851db..983f075 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -86,6 +86,8 @@ defmodule MvWeb.GlobalSettingsLive do |> assign(:oidc_client_secret_set, Mv.Config.oidc_client_secret_set?()) |> assign(:registration_enabled, settings.registration_enabled != false) |> assign(:smtp_env_configured, Mv.Config.smtp_env_configured?()) + |> assign(:smtp_env_mode, Mv.Config.smtp_env_mode?()) + |> assign(:smtp_missing_required_env_keys, Mv.Config.smtp_missing_required_env_keys()) |> assign(:smtp_host_env_set, Mv.Config.smtp_host_env_set?()) |> assign(:smtp_port_env_set, Mv.Config.smtp_port_env_set?()) |> assign(:smtp_username_env_set, Mv.Config.smtp_username_env_set?()) @@ -321,12 +323,25 @@ defmodule MvWeb.GlobalSettingsLive do <%!-- SMTP / E-Mail Section --%> <.form_section title={gettext("SMTP / E-Mail")}> - <%= if @smtp_env_configured do %> + <%= if @smtp_env_mode do %>

- {gettext("Some values are set via environment variables. Those fields are read-only.")} + {gettext( + "SMTP is fully managed via environment variables. All SMTP fields are read-only." + )}

<% end %> + <%= if @smtp_env_mode and @smtp_missing_required_env_keys != [] do %> +
+ <.icon name="hero-exclamation-triangle" class="size-5 shrink-0 mt-0.5" /> + + {gettext("SMTP environment configuration appears incomplete. Missing: %{keys}", + keys: Enum.join(@smtp_missing_required_env_keys, ", ") + )} + +
+ <% end %> + <%= if @environment == :prod and not @smtp_configured do %>
<.icon name="hero-exclamation-triangle" class="size-5 shrink-0 mt-0.5" /> @@ -345,7 +360,7 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_host]} type="text" label={gettext("Host")} - disabled={@smtp_host_env_set} + disabled={@smtp_env_mode} placeholder={ if(@smtp_host_env_set, do: gettext("From SMTP_HOST"), @@ -357,14 +372,14 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_port]} type="number" label={gettext("Port")} - disabled={@smtp_port_env_set} + disabled={@smtp_env_mode} placeholder={if(@smtp_port_env_set, do: gettext("From SMTP_PORT"), else: "587")} /> <.input field={@form[:smtp_ssl]} type="select" label={gettext("TLS/SSL")} - disabled={@smtp_ssl_env_set} + disabled={@smtp_env_mode} options={[ {gettext("TLS (port 587, recommended)"), "tls"}, {gettext("SSL (port 465)"), "ssl"}, @@ -379,7 +394,7 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_username]} type="text" label={gettext("Username")} - disabled={@smtp_username_env_set} + disabled={@smtp_env_mode} placeholder={ if(@smtp_username_env_set, do: gettext("From SMTP_USERNAME"), @@ -391,7 +406,7 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_password]} type="password" label={gettext("Password")} - disabled={@smtp_password_env_set} + disabled={@smtp_env_mode} placeholder={ if(@smtp_password_env_set, do: gettext("From SMTP_PASSWORD"), @@ -410,7 +425,7 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_from_email]} type="email" label={gettext("Sender email (From)")} - disabled={@smtp_from_email_env_set} + disabled={@smtp_env_mode} placeholder={ if(@smtp_from_email_env_set, do: gettext("From MAIL_FROM_EMAIL"), @@ -422,7 +437,7 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_from_name]} type="text" label={gettext("Sender name (From)")} - disabled={@smtp_from_name_env_set} + disabled={@smtp_env_mode} placeholder={ if(@smtp_from_name_env_set, do: gettext("From MAIL_FROM_NAME"), else: "Mila") } @@ -436,9 +451,10 @@ defmodule MvWeb.GlobalSettingsLive do

<.button :if={ - not (@smtp_host_env_set and @smtp_port_env_set and @smtp_username_env_set and - @smtp_password_env_set and @smtp_ssl_env_set and @smtp_from_email_env_set and - @smtp_from_name_env_set) + not @smtp_env_mode and + not (@smtp_host_env_set and @smtp_port_env_set and @smtp_username_env_set and + @smtp_password_env_set and @smtp_ssl_env_set and @smtp_from_email_env_set and + @smtp_from_name_env_set) } phx-disable-with={gettext("Saving...")} variant="primary" diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 52270cc..1482490 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -3917,3 +3917,13 @@ msgstr "Offen" #, elixir-autogen, elixir-format msgid "join page URL in a new tab" msgstr "Beitrittslink in einem neuen Tab" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "SMTP environment configuration appears incomplete. Missing: %{keys}" +msgstr "Die SMTP-Umgebungs-Konfiguration ist unvollständig. Fehlend: %{keys}" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "SMTP is fully managed via environment variables. All SMTP fields are read-only." +msgstr "SMTP wird vollständig über Umgebungsvariablen verwaltet. Alle SMTP-Felder sind schreibgeschützt." diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 5d48691..21e7b16 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -3917,3 +3917,13 @@ msgstr "" #, elixir-autogen, elixir-format msgid "join page URL in a new tab" msgstr "" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "SMTP environment configuration appears incomplete. Missing: %{keys}" +msgstr "" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "SMTP is fully managed via environment variables. All SMTP fields are read-only." +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index ec6f305..7d94b7c 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -3917,3 +3917,13 @@ msgstr "Open" #, elixir-autogen, elixir-format msgid "join page URL in a new tab" msgstr "join page URL in a new tab" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "SMTP environment configuration appears incomplete. Missing: %{keys}" +msgstr "" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "SMTP is fully managed via environment variables. All SMTP fields are read-only." +msgstr "" diff --git a/test/mv/config_smtp_test.exs b/test/mv/config_smtp_test.exs index 5359366..7a1b895 100644 --- a/test/mv/config_smtp_test.exs +++ b/test/mv/config_smtp_test.exs @@ -23,7 +23,8 @@ defmodule Mv.ConfigSmtpTest do end describe "smtp_port/0" do - test "returns parsed integer when SMTP_PORT ENV is set" do + test "returns parsed integer when SMTP_PORT ENV is set in ENV-only mode" do + set_smtp_env("SMTP_HOST", "smtp.example.com") set_smtp_env("SMTP_PORT", "587") assert Mv.Config.smtp_port() == 587 after @@ -52,13 +53,21 @@ defmodule Mv.ConfigSmtpTest do end describe "smtp_env_configured?/0" do - test "returns true when any SMTP ENV variable is set" do + test "returns true when SMTP_HOST is set" do set_smtp_env("SMTP_HOST", "smtp.example.com") assert Mv.Config.smtp_env_configured?() == true after clear_smtp_env() end + test "returns false when SMTP_HOST is not set even if other SMTP ENV variables are set" do + set_smtp_env("SMTP_USERNAME", "user@example.com") + set_smtp_env("SMTP_PASSWORD", "secret") + refute Mv.Config.smtp_env_configured?() + after + clear_smtp_env() + end + test "returns false when no SMTP ENV variables are set" do clear_smtp_env() refute Mv.Config.smtp_env_configured?() @@ -66,15 +75,17 @@ defmodule Mv.ConfigSmtpTest do end describe "smtp_password/0 and SMTP_PASSWORD_FILE" do - test "returns value from SMTP_PASSWORD when set" do + test "returns value from SMTP_PASSWORD when set in ENV-only mode" do + set_smtp_env("SMTP_HOST", "smtp.example.com") set_smtp_env("SMTP_PASSWORD", "env-secret") assert Mv.Config.smtp_password() == "env-secret" after clear_smtp_env() end - test "returns content of file when SMTP_PASSWORD_FILE is set and SMTP_PASSWORD is not" do + test "returns content of file when SMTP_PASSWORD_FILE is set in ENV-only mode and SMTP_PASSWORD is not" do clear_smtp_env() + set_smtp_env("SMTP_HOST", "smtp.example.com") path = Path.join(System.tmp_dir!(), "mv_smtp_test_#{System.unique_integer([:positive])}") File.write!(path, "file-secret\n") Process.put(:smtp_password_file_path, path) @@ -85,7 +96,8 @@ defmodule Mv.ConfigSmtpTest do if path = Process.get(:smtp_password_file_path), do: File.rm(path) end - test "SMTP_PASSWORD overrides SMTP_PASSWORD_FILE when both are set" do + test "SMTP_PASSWORD overrides SMTP_PASSWORD_FILE in ENV-only mode when both are set" do + set_smtp_env("SMTP_HOST", "smtp.example.com") path = Path.join(System.tmp_dir!(), "mv_smtp_test_#{System.unique_integer([:positive])}") File.write!(path, "file-secret") Process.put(:smtp_password_file_path, path) diff --git a/test/mv_web/live/global_settings_live_test.exs b/test/mv_web/live/global_settings_live_test.exs index 2edaf74..f90802a 100644 --- a/test/mv_web/live/global_settings_live_test.exs +++ b/test/mv_web/live/global_settings_live_test.exs @@ -124,6 +124,71 @@ defmodule MvWeb.GlobalSettingsLiveTest do {:ok, _view, html} = live(conn, ~p"/settings") assert html =~ "SMTP" or html =~ "E-Mail" or html =~ "Settings" end + + @tag :ui + test "disables all SMTP inputs when SMTP_HOST is set", %{conn: conn} do + clear_smtp_env() + System.put_env("SMTP_HOST", "smtp.env-only.example") + on_exit(fn -> clear_smtp_env() end) + + {:ok, view, _html} = live(conn, ~p"/settings") + + assert has_element?(view, "#setting_smtp_host[disabled]") + assert has_element?(view, "#setting_smtp_port[disabled]") + assert has_element?(view, "#setting_smtp_ssl[disabled]") + assert has_element?(view, "#setting_smtp_username[disabled]") + assert has_element?(view, "#setting_smtp_password[disabled]") + assert has_element?(view, "#setting_smtp_from_email[disabled]") + assert has_element?(view, "#setting_smtp_from_name[disabled]") + end + + @tag :ui + test "does not render SMTP save action when SMTP_HOST is set", %{conn: conn} do + clear_smtp_env() + System.put_env("SMTP_HOST", "smtp.env-only.example") + on_exit(fn -> clear_smtp_env() end) + + {:ok, view, _html} = live(conn, ~p"/settings") + refute has_element?(view, "#smtp-form button", "Save SMTP Settings") + end + + @tag :ui + test "shows explicit ENV-only mode hint when SMTP_HOST is set", %{conn: conn} do + clear_smtp_env() + System.put_env("SMTP_HOST", "smtp.env-only.example") + on_exit(fn -> clear_smtp_env() end) + + {:ok, _view, html} = live(conn, ~p"/settings") + assert html =~ "SMTP is fully managed via environment variables" + end + + @tag :ui + test "shows warning block for missing required SMTP ENV values in ENV-only mode", %{ + conn: conn + } do + clear_smtp_env() + System.put_env("SMTP_HOST", "smtp.env-only.example") + on_exit(fn -> clear_smtp_env() end) + + {:ok, _view, html} = live(conn, ~p"/settings") + assert html =~ "SMTP environment configuration appears incomplete" + assert html =~ "SMTP_USERNAME" + assert html =~ "SMTP_PASSWORD/SMTP_PASSWORD_FILE" + end + + @tag :ui + test "does not enter ENV-only mode when SMTP_HOST is not set", %{conn: conn} do + clear_smtp_env() + System.put_env("SMTP_USERNAME", "leftover@example.com") + on_exit(fn -> clear_smtp_env() end) + + {:ok, view, html} = live(conn, ~p"/settings") + + refute html =~ "SMTP is fully managed via environment variables" + refute html =~ "SMTP environment configuration appears incomplete" + refute has_element?(view, "#setting_smtp_host[disabled]") + refute has_element?(view, "#setting_smtp_username[disabled]") + end end describe "Authentication section when OIDC-only is enabled" do