diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index d721a3a..2b378ef 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -1278,7 +1278,7 @@ 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-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`). +- **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`). This keeps one source of truth for transport credentials and avoids mixed ENV/DB SMTP states. - **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 6668485..b19f6e4 100644 --- a/docs/smtp-configuration-concept.md +++ b/docs/smtp-configuration-concept.md @@ -29,6 +29,7 @@ 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. +- the UI displays the effective ENV-driven SMTP values in disabled fields so admins can verify what is active. --- diff --git a/lib/mv/config.ex b/lib/mv/config.ex index c807193..870d1d3 100644 --- a/lib/mv/config.ex +++ b/lib/mv/config.ex @@ -470,11 +470,19 @@ defmodule Mv.Config do # --------------------------------------------------------------------------- @doc """ - Returns SMTP host. ENV `SMTP_HOST` overrides Settings. + Returns SMTP host. + + Policy: + - ENV-only mode (`SMTP_HOST` set): read from ENV `SMTP_HOST` + - Settings mode: read from Settings only """ @spec smtp_host() :: String.t() | nil def smtp_host do - smtp_env_or_setting("SMTP_HOST", :smtp_host) + if smtp_env_mode?() do + System.get_env("SMTP_HOST") |> trim_nil() + else + get_from_settings(:smtp_host) + end end @doc """ @@ -522,7 +530,7 @@ defmodule Mv.Config do def smtp_password do if smtp_env_mode?() do case System.get_env("SMTP_PASSWORD") do - nil -> smtp_password_from_file_or_settings() + nil -> smtp_password_from_file() value -> trim_nil(value) end else @@ -530,7 +538,7 @@ defmodule Mv.Config do end end - defp smtp_password_from_file_or_settings do + defp smtp_password_from_file do case System.get_env("SMTP_PASSWORD_FILE") do nil -> nil path -> read_smtp_password_file(path) @@ -568,14 +576,6 @@ defmodule Mv.Config do present?(smtp_host()) end - @doc """ - Returns true when SMTP ENV mode is active. - """ - @spec smtp_env_configured?() :: boolean() - def smtp_env_configured? do - smtp_env_mode?() - end - @doc """ Returns true when SMTP is managed by environment variables. @@ -599,6 +599,7 @@ defmodule Mv.Config do [] |> maybe_add_missing("SMTP_USERNAME", smtp_username_env_set?()) |> maybe_add_missing("SMTP_PASSWORD/SMTP_PASSWORD_FILE", smtp_password_env_set?()) + |> Enum.reverse() else [] end @@ -665,8 +666,6 @@ 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 @@ -676,16 +675,8 @@ defmodule Mv.Config do 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 - nil -> get_from_settings(setting_key) - value -> trim_nil(value) - end - end - defp maybe_add_missing(acc, _label, true), do: acc - defp maybe_add_missing(acc, label, false), do: acc ++ [label] + defp maybe_add_missing(acc, label, false), do: [label | acc] # Reads an integer setting attribute from Settings. defp get_from_settings_integer(key) do diff --git a/lib/mv_web/components/core_components.ex b/lib/mv_web/components/core_components.ex index 4e55cf7..465d41a 100644 --- a/lib/mv_web/components/core_components.ex +++ b/lib/mv_web/components/core_components.ex @@ -1022,7 +1022,6 @@ defmodule MvWeb.CoreComponents do id={@row_id && @row_id.(row)} class={[ table_row_tr_class( - @row_click, table_row_selected?(assigns, row), @sticky_first_col ) @@ -1142,17 +1141,8 @@ defmodule MvWeb.CoreComponents do # Returns CSS classes for table row selection styles. # Hover/focus row highlighting is CSS-driven via [data-row-interactive] selectors in app.css. # Sticky-first-column zebra tables use CSS accents and omit selected row ring classes. - defp table_row_tr_class(_row_click, selected?, sticky_first_col) do - base = [] - - # Sticky-first-col tables: selection/hover accents are CSS-only (orange bar + fills). - base = - if selected? and not sticky_first_col, - do: base ++ ["ring-2", "ring-inset", "ring-primary"], - else: base - - Enum.join(base, " ") - end + defp table_row_tr_class(true, false), do: "ring-2 ring-inset ring-primary" + defp table_row_tr_class(_, _), do: "" defp table_th_aria_sort(col, sort_field, sort_order) do col_sort = Map.get(col, :sort_field) diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index 983f075..6a1c926 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -85,16 +85,8 @@ defmodule MvWeb.GlobalSettingsLive do |> assign(:oidc_configured, Mv.Config.oidc_configured?()) |> 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?()) - |> assign(:smtp_password_env_set, Mv.Config.smtp_password_env_set?()) - |> assign(:smtp_ssl_env_set, Mv.Config.smtp_ssl_env_set?()) - |> assign(:smtp_from_name_env_set, Mv.Config.mail_from_name_env_set?()) - |> assign(:smtp_from_email_env_set, Mv.Config.mail_from_email_env_set?()) |> assign(:smtp_password_set, present?(Mv.Config.smtp_password())) |> assign(:smtp_configured, Mv.Config.smtp_configured?()) |> assign(:smtp_test_result, nil) @@ -361,19 +353,14 @@ defmodule MvWeb.GlobalSettingsLive do type="text" label={gettext("Host")} disabled={@smtp_env_mode} - placeholder={ - if(@smtp_host_env_set, - do: gettext("From SMTP_HOST"), - else: "smtp.example.com" - ) - } + placeholder="smtp.example.com" /> <.input field={@form[:smtp_port]} type="number" label={gettext("Port")} disabled={@smtp_env_mode} - placeholder={if(@smtp_port_env_set, do: gettext("From SMTP_PORT"), else: "587")} + placeholder="587" /> <.input field={@form[:smtp_ssl]} @@ -385,7 +372,6 @@ defmodule MvWeb.GlobalSettingsLive do {gettext("SSL (port 465)"), "ssl"}, {gettext("None (port 25, insecure)"), "none"} ]} - placeholder={if(@smtp_ssl_env_set, do: gettext("From SMTP_SSL"), else: nil)} /> @@ -395,12 +381,7 @@ defmodule MvWeb.GlobalSettingsLive do type="text" label={gettext("Username")} disabled={@smtp_env_mode} - placeholder={ - if(@smtp_username_env_set, - do: gettext("From SMTP_USERNAME"), - else: "user@example.com" - ) - } + placeholder="user@example.com" /> <.input field={@form[:smtp_password]} @@ -408,14 +389,11 @@ defmodule MvWeb.GlobalSettingsLive do label={gettext("Password")} disabled={@smtp_env_mode} placeholder={ - if(@smtp_password_env_set, - do: gettext("From SMTP_PASSWORD"), - else: - if(@smtp_password_set, - do: gettext("Leave blank to keep current"), - else: nil - ) - ) + if @smtp_env_mode do + gettext("From SMTP_PASSWORD") + else + if @smtp_password_set, do: gettext("Leave blank to keep current"), else: nil + end } /> @@ -426,21 +404,14 @@ defmodule MvWeb.GlobalSettingsLive do type="email" label={gettext("Sender email (From)")} disabled={@smtp_env_mode} - placeholder={ - if(@smtp_from_email_env_set, - do: gettext("From MAIL_FROM_EMAIL"), - else: "noreply@example.com" - ) - } + placeholder="noreply@example.com" /> <.input field={@form[:smtp_from_name]} type="text" label={gettext("Sender name (From)")} disabled={@smtp_env_mode} - placeholder={ - if(@smtp_from_name_env_set, do: gettext("From MAIL_FROM_NAME"), else: "Mila") - } + placeholder="Mila" /> @@ -450,12 +421,7 @@ defmodule MvWeb.GlobalSettingsLive do )}

<.button - :if={ - 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) - } + :if={not @smtp_env_mode} phx-disable-with={gettext("Saving...")} variant="primary" class="mt-2" @@ -941,9 +907,9 @@ defmodule MvWeb.GlobalSettingsLive do |> assign(:oidc_only, Mv.Config.oidc_only?()) |> assign(:oidc_configured, Mv.Config.oidc_configured?()) |> assign(:smtp_configured, Mv.Config.smtp_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_password_set, present?(Mv.Config.smtp_password())) - |> assign(:smtp_from_name_env_set, Mv.Config.mail_from_name_env_set?()) - |> assign(:smtp_from_email_env_set, Mv.Config.mail_from_email_env_set?()) |> assign(:vereinfacht_test_result, test_result) |> put_flash(:success, gettext("Settings updated successfully")) |> assign_form() @@ -1283,25 +1249,17 @@ defmodule MvWeb.GlobalSettingsLive do end defp merge_smtp_env_values(s) do - s - |> put_if_env_set(:smtp_host, Mv.Config.smtp_host_env_set?(), Mv.Config.smtp_host()) - |> put_if_env_set(:smtp_port, Mv.Config.smtp_port_env_set?(), Mv.Config.smtp_port()) - |> put_if_env_set( - :smtp_username, - Mv.Config.smtp_username_env_set?(), - Mv.Config.smtp_username() - ) - |> put_if_env_set(:smtp_ssl, Mv.Config.smtp_ssl_env_set?(), Mv.Config.smtp_ssl()) - |> put_if_env_set( - :smtp_from_email, - Mv.Config.mail_from_email_env_set?(), - Mv.Config.mail_from_email() - ) - |> put_if_env_set( - :smtp_from_name, - Mv.Config.mail_from_name_env_set?(), - Mv.Config.mail_from_name() - ) + if Mv.Config.smtp_env_mode?() do + s + |> Map.put(:smtp_host, Mv.Config.smtp_host()) + |> Map.put(:smtp_port, Mv.Config.smtp_port()) + |> Map.put(:smtp_username, Mv.Config.smtp_username()) + |> Map.put(:smtp_ssl, Mv.Config.smtp_ssl()) + |> Map.put(:smtp_from_email, Mv.Config.mail_from_email()) + |> Map.put(:smtp_from_name, Mv.Config.mail_from_name()) + else + s + end end defp enrich_sync_errors([]), do: [] diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 1482490..a85b4cf 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -1384,16 +1384,6 @@ msgstr "Festgelegt nach der Erstellung. Mitglieder können nur zwischen Beitrags msgid "From %{first} to %{last} (relevant years with membership data)" msgstr "Von %{first} bis %{last} (Jahre mit Mitgliederdaten)" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From MAIL_FROM_EMAIL" -msgstr "Aus MAIL_FROM_EMAIL" - -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From MAIL_FROM_NAME" -msgstr "Aus MAIL_FROM_NAME" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "From OIDC_ADMIN_GROUP_NAME" @@ -1429,31 +1419,11 @@ msgstr "Aus OIDC_ONLY" msgid "From OIDC_REDIRECT_URI" msgstr "Aus OIDC_REDIRECT_URI" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_HOST" -msgstr "Von SMTP_HOST" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "From SMTP_PASSWORD" msgstr "Von SMTP_PASSWORD" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_PORT" -msgstr "Von SMTP_PORT" - -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_SSL" -msgstr "Von SMTP_SSL" - -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_USERNAME" -msgstr "Von SMTP_USERNAME" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "From VEREINFACHT_API_KEY" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 21e7b16..b995b1a 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -1385,16 +1385,6 @@ msgstr "" msgid "From %{first} to %{last} (relevant years with membership data)" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From MAIL_FROM_EMAIL" -msgstr "" - -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From MAIL_FROM_NAME" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "From OIDC_ADMIN_GROUP_NAME" @@ -1430,31 +1420,11 @@ msgstr "" msgid "From OIDC_REDIRECT_URI" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_HOST" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "From SMTP_PASSWORD" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_PORT" -msgstr "" - -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_SSL" -msgstr "" - -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_USERNAME" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "From VEREINFACHT_API_KEY" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 7d94b7c..f4526d1 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -1385,16 +1385,6 @@ msgstr "" msgid "From %{first} to %{last} (relevant years with membership data)" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From MAIL_FROM_EMAIL" -msgstr "" - -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From MAIL_FROM_NAME" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "From OIDC_ADMIN_GROUP_NAME" @@ -1430,31 +1420,11 @@ msgstr "" msgid "From OIDC_REDIRECT_URI" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_HOST" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "From SMTP_PASSWORD" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_PORT" -msgstr "" - -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_SSL" -msgstr "" - -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "From SMTP_USERNAME" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "From VEREINFACHT_API_KEY" @@ -3926,4 +3896,4 @@ 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 "" +msgstr "SMTP is fully managed via environment variables. All SMTP fields are read-only." diff --git a/test/mv/config_smtp_test.exs b/test/mv/config_smtp_test.exs index 7a1b895..ebf2b83 100644 --- a/test/mv/config_smtp_test.exs +++ b/test/mv/config_smtp_test.exs @@ -2,7 +2,7 @@ defmodule Mv.ConfigSmtpTest do @moduledoc """ Unit tests for Mv.Config SMTP-related helpers. - ENV overrides Settings (same pattern as OIDC/Vereinfacht). Uses real ENV and + SMTP uses ENV-only mode when SMTP_HOST is set. Uses real ENV and Settings; no mocking so we test the actual precedence. async: false because we mutate ENV. """ @@ -52,10 +52,10 @@ defmodule Mv.ConfigSmtpTest do end end - describe "smtp_env_configured?/0" do + describe "smtp_env_mode?/0" 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 + assert Mv.Config.smtp_env_mode?() == true after clear_smtp_env() end @@ -63,14 +63,14 @@ defmodule Mv.ConfigSmtpTest do 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?() + refute Mv.Config.smtp_env_mode?() 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?() + refute Mv.Config.smtp_env_mode?() end end diff --git a/test/mv_web/live/global_settings_live_test.exs b/test/mv_web/live/global_settings_live_test.exs index 4283ec0..9be12b9 100644 --- a/test/mv_web/live/global_settings_live_test.exs +++ b/test/mv_web/live/global_settings_live_test.exs @@ -1,5 +1,5 @@ defmodule MvWeb.GlobalSettingsLiveTest do - use MvWeb.ConnCase, async: true + use MvWeb.ConnCase, async: false import Phoenix.LiveViewTest alias Mv.Membership @@ -203,6 +203,20 @@ defmodule MvWeb.GlobalSettingsLiveTest do refute has_element?(view, "#setting_smtp_host[disabled]") refute has_element?(view, "#setting_smtp_username[disabled]") end + + @tag :ui + test "shows effective ENV SMTP host value in disabled field", %{conn: conn} do + clear_smtp_env() + System.put_env("SMTP_HOST", "smtp.env-active.example") + on_exit(fn -> clear_smtp_env() end) + + {:ok, settings} = Membership.get_settings() + {:ok, _} = Membership.update_settings(settings, %{smtp_host: "smtp.db-legacy.example"}) + + {:ok, _view, html} = live(conn, ~p"/settings") + assert html =~ ~s(value="smtp.env-active.example") + refute html =~ ~s(value="smtp.db-legacy.example") + end end describe "Authentication section when OIDC-only is enabled" do