From 942f2afd9ec765c02e752ac09b71a2272d82694c Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 12 Mar 2026 15:29:54 +0100 Subject: [PATCH] refactor: adress review --- CODE_GUIDELINES.md | 5 ++- config/config.exs | 8 ++++ config/runtime.exs | 18 +++++++-- docs/smtp-configuration-concept.md | 14 ++++++- lib/membership/setting.ex | 54 +++++++++---------------- lib/mv/config.ex | 38 ++++++++++++++--- lib/mv/mailer.ex | 13 ++++-- lib/mv_web/live/global_settings_live.ex | 20 ++++----- 8 files changed, 108 insertions(+), 62 deletions(-) diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 7dfa3ef..0cb8d65 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -1274,15 +1274,16 @@ 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). +- **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()`. - Sender identity is also configurable via ENV (`MAIL_FROM_NAME`, `MAIL_FROM_EMAIL`) or Settings (`smtp_from_name`, `smtp_from_email`). - `SMTP_PASSWORD_FILE`: path to a file containing the password (Docker Secrets / Kubernetes secrets pattern); overridden by `SMTP_PASSWORD` when both are set. - `SMTP_SSL` values: `tls` (default, port 587), `ssl` (port 465), `none` (port 25). - When `SMTP_HOST` ENV is present at boot, `runtime.exs` configures `Swoosh.Adapters.SMTP` automatically. - When SMTP is configured only via Settings, `Mv.Mailer.smtp_config/0` builds the adapter config per-send. - In test environment, `Swoosh.Adapters.Test` is used regardless of SMTP config. -- **TLS in OTP 27:** `tls_options: [verify: :verify_none]` (STARTTLS/587) and `sockopts: [verify: :verify_none]` (SSL/465) are set to allow self-signed / internal certs. +- **TLS in OTP 27:** Verify mode defaults to `verify_none` for self-signed/internal certs. Set `SMTP_VERIFY_PEER=true` (or `1`/`yes`) in prod when using public SMTP (Gmail, Mailgun). Config key `:smtp_verify_peer` is set in `runtime.exs` and read by `Mv.Mailer.smtp_config/0`. - **Test email:** `Mv.Mailer.send_test_email(to_email)` sends a transactional test email; returns `{:ok, email}` or `{:error, classified_reason}`. Classified errors: `:sender_rejected`, `:auth_failed`, `:recipient_rejected`, `:tls_failed`, `:connection_failed`, `{:smtp_error, message}`. Each shows a specific message in the UI. -- **Production warning:** When SMTP is not configured in production, a warning is shown in the Settings UI. +- **Production warning:** When SMTP is not configured in production, a warning is shown in the Settings UI. Use `Application.get_env(:mv, :environment, :dev)` (or assign in mount) for environment checks in LiveView/templates; do not use `Mix.env()` at runtime (it is not available in releases). - Access config values via `Mv.Config.smtp_host/0`, `smtp_port/0`, `smtp_username/0`, `smtp_password/0`, `smtp_ssl/0`, `smtp_configured?/0`. **AshAuthentication senders:** diff --git a/config/config.exs b/config/config.exs index ab55f2a..35e4160 100644 --- a/config/config.exs +++ b/config/config.exs @@ -51,6 +51,10 @@ config :mv, generators: [timestamp_type: :utc_datetime], ash_domains: [Mv.Membership, Mv.Accounts, Mv.MembershipFees, Mv.Authorization] +# Environment (dev/test/prod). Use this instead of Mix.env() at runtime; Mix.env() is +# not available in releases. Set once at compile time via config_env(). +config :mv, :environment, config_env() + # CSV Import configuration config :mv, csv_import: [ @@ -89,6 +93,10 @@ config :mv, MvWeb.Endpoint, # at the `config/runtime.exs`. config :mv, Mv.Mailer, adapter: Swoosh.Adapters.Local +# SMTP TLS verification: false = allow self-signed/internal certs; true = verify_peer (use for public SMTP). +# Overridden in runtime.exs from SMTP_VERIFY_PEER when SMTP is configured via ENV in prod. +config :mv, :smtp_verify_peer, false + # Default mail "from" address for transactional emails (join confirmation, # user confirmation, password reset). Override in config/runtime.exs from ENV. config :mv, :mail_from, {"Mila", "noreply@example.com"} diff --git a/config/runtime.exs b/config/runtime.exs index b522426..1c55f64 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -227,6 +227,10 @@ if config_env() == :prod do # When SMTP_HOST is set, configure Swoosh to use the SMTP adapter at boot time. # If SMTP is configured only via Settings (Admin UI), the mailer builds the config # per-send at runtime using Mv.Config.smtp_*() helpers. + # + # TLS/SSL options (tls_options, sockopts) are duplicated here and in Mv.Mailer.smtp_config/0 + # because boot config must be set in this file; the Mailer uses the same logic for + # Settings-only config. Keep verify behaviour in sync (see SMTP_VERIFY_PEER below). smtp_host_env = System.get_env("SMTP_HOST") if smtp_host_env && String.trim(smtp_host_env) != "" do @@ -250,6 +254,15 @@ if config_env() == :prod do smtp_ssl_mode = System.get_env("SMTP_SSL", "tls") + # SMTP_VERIFY_PEER: set to true/1/yes to enable TLS certificate verification (recommended + # for public SMTP like Gmail/Mailgun). Default false for self-signed/internal certs. + smtp_verify_peer = + (System.get_env("SMTP_VERIFY_PEER", "false") |> String.downcase()) in ~w(true 1 yes) + + config :mv, :smtp_verify_peer, smtp_verify_peer + + verify_mode = if smtp_verify_peer, do: :verify_peer, else: :verify_none + smtp_opts = [ adapter: Swoosh.Adapters.SMTP, @@ -260,10 +273,9 @@ if config_env() == :prod do ssl: smtp_ssl_mode == "ssl", tls: if(smtp_ssl_mode == "tls", do: :always, else: :never), auth: :always, - # Allow self-signed or internal SMTP server certs (OTP 26+ enforces verify_peer with cacerts). # tls_options: STARTTLS (587); sockopts: direct SSL (465). - tls_options: [verify: :verify_none], - sockopts: [verify: :verify_none] + tls_options: [verify: verify_mode], + sockopts: [verify: verify_mode] ] |> Enum.reject(fn {_k, v} -> is_nil(v) end) diff --git a/docs/smtp-configuration-concept.md b/docs/smtp-configuration-concept.md index 75e3e85..30fd7de 100644 --- a/docs/smtp-configuration-concept.md +++ b/docs/smtp-configuration-concept.md @@ -92,9 +92,12 @@ Both `SendPasswordResetEmail` and `SendNewUserConfirmationEmail` use `Mv.Mailer. OTP 26+ enforces `verify_peer` by default, which fails for self-signed or internal SMTP server certificates. -Both `tls_options: [verify: :verify_none]` (for STARTTLS, port 587) and `sockopts: [verify: :verify_none]` (for direct SSL, port 465) are set in `Mv.Mailer.smtp_config/0` to allow such certificates. +By default, TLS certificate verification is relaxed (`verify_none`) so self-signed or internal SMTP servers work. For public SMTP providers (Gmail, Mailgun, etc.) you can enable verification: -For ENV-based boot config, the same options are set in `config/runtime.exs`. +- **ENV (prod):** Set `SMTP_VERIFY_PEER=true` (or `1`/`yes`) when configuring SMTP via environment variables in `config/runtime.exs`. This sets `config :mv, :smtp_verify_peer` and is used for both boot-time and per-send config. +- **Default:** `false` (verify_none) for backward compatibility and internal/self-signed certs. + +Both `tls_options` (STARTTLS, port 587) and `sockopts` (direct SSL, port 465) use the same verify mode. The logic is duplicated in `config/runtime.exs` (boot) and `Mv.Mailer.smtp_config/0` (Settings-only); keep in sync. --- @@ -112,3 +115,10 @@ For ENV-based boot config, the same options are set in `config/runtime.exs`. - [x] AshAuthentication senders: graceful error handling (no crash on delivery failure). - [x] Gettext for all new UI strings, translated to German. - [x] Docs and code guidelines updated. + +--- + +## 12. Follow-up / Future Work + +- **SMTP password at-rest encryption:** The `smtp_password` attribute is currently stored in plaintext in the `settings` table. It is excluded from default reads (same pattern as `oidc_client_secret`); both are read only via explicit select when needed. For production systems at-rest encryption (e.g. with [Cloak](https://hexdocs.pm/cloak)) should be considered and tracked as a follow-up issue. +- **Error classification:** SMTP error categorization currently uses substring matching on server messages (e.g. "535", "authentication"). A more robust approach would be to pattern-match on `gen_smtp` error tuples first where possible, and fall back to string analysis only when needed. Server wording varies; consider extending patterns as new providers are used. diff --git a/lib/membership/setting.ex b/lib/membership/setting.ex index 827e194..ce63589 100644 --- a/lib/membership/setting.ex +++ b/lib/membership/setting.ex @@ -56,6 +56,9 @@ defmodule Mv.Membership.Setting do # Update membership fee settings {:ok, updated} = Mv.Membership.update_settings(settings, %{include_joining_cycle: false}) """ + # primary_read_warning?: false — We use a custom read prepare that selects only public + # attributes and explicitly excludes smtp_password. Ash warns when the primary read does + # not load all attributes; we intentionally omit the password for security. use Ash.Resource, domain: Mv.Membership, data_layer: AshPostgres.DataLayer, @@ -65,6 +68,8 @@ defmodule Mv.Membership.Setting do @uuid_pattern ~r/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i @valid_join_form_member_fields Mv.Constants.member_fields() |> Enum.map(&Atom.to_string/1) + alias Ash.Resource.Info, as: ResourceInfo + postgres do table "settings" repo Mv.Repo @@ -74,48 +79,25 @@ defmodule Mv.Membership.Setting do description "Global application settings (singleton resource)" end - # All public attributes except smtp_password, used to exclude it from default reads. - # This list is used in the read prepare to prevent the sensitive password from being - # returned in standard reads (it can still be read via explicit select in Config). - @public_attributes [ - :id, - :club_name, - :member_field_visibility, - :member_field_required, - :include_joining_cycle, - :default_membership_fee_type_id, - :vereinfacht_api_url, - :vereinfacht_api_key, - :vereinfacht_club_id, - :vereinfacht_app_url, - :oidc_client_id, - :oidc_base_url, - :oidc_redirect_uri, - :oidc_client_secret, - :oidc_admin_group_name, - :oidc_groups_claim, - :oidc_only, - :smtp_host, - :smtp_port, - :smtp_username, - :smtp_ssl, - :smtp_from_name, - :smtp_from_email, - :join_form_enabled, - :join_form_field_ids, - :join_form_field_required, - :inserted_at, - :updated_at - ] + # Attributes excluded from the default read (sensitive data). Same pattern as smtp_password: + # read only via explicit select when needed; never loaded into default get_settings(). + @excluded_from_read [:smtp_password, :oidc_client_secret] actions do read :read do primary? true - # smtp_password is excluded from the default select to prevent it from being returned - # in plaintext via standard reads. Config reads it via an explicit select internally. + # Exclude sensitive attributes (e.g. smtp_password) from default reads. Config reads + # them via explicit select when needed. Uses all attribute names minus excluded so + # the list stays correct when new attributes are added to the resource. prepare fn query, _context -> - Ash.Query.select(query, @public_attributes) + select_attrs = + __MODULE__ + |> ResourceInfo.attribute_names() + |> MapSet.to_list() + |> Kernel.--(@excluded_from_read) + + Ash.Query.select(query, select_attrs) end end diff --git a/lib/mv/config.ex b/lib/mv/config.ex index b824c1d..3494937 100644 --- a/lib/mv/config.ex +++ b/lib/mv/config.ex @@ -362,26 +362,41 @@ defmodule Mv.Config do @doc """ Returns the OIDC client secret. In production, uses the value from config :mv, :oidc (set by runtime.exs from OIDC_CLIENT_SECRET or OIDC_CLIENT_SECRET_FILE). - Otherwise ENV OIDC_CLIENT_SECRET, then Settings. + Otherwise ENV OIDC_CLIENT_SECRET, then Settings (read via explicit select; not in default get_settings). """ @spec oidc_client_secret() :: String.t() | nil def oidc_client_secret do case Application.get_env(:mv, :oidc) do oidc when is_list(oidc) -> oidc_client_secret_from_config(Keyword.get(oidc, :client_secret)) - _ -> env_or_setting("OIDC_CLIENT_SECRET", :oidc_client_secret) + _ -> oidc_client_secret_from_env_or_settings() end end + @doc """ + Returns whether the OIDC client secret is set in Settings (for UI badge). Does not expose the value. + """ + @spec oidc_client_secret_set?() :: boolean() + def oidc_client_secret_set? do + present?(get_oidc_client_secret_from_settings()) + end + defp oidc_client_secret_from_config(nil), - do: env_or_setting("OIDC_CLIENT_SECRET", :oidc_client_secret) + do: oidc_client_secret_from_env_or_settings() defp oidc_client_secret_from_config(secret) when is_binary(secret) do s = String.trim(secret) - if s != "", do: s, else: env_or_setting("OIDC_CLIENT_SECRET", :oidc_client_secret) + if s != "", do: s, else: oidc_client_secret_from_env_or_settings() end defp oidc_client_secret_from_config(_), - do: env_or_setting("OIDC_CLIENT_SECRET", :oidc_client_secret) + do: oidc_client_secret_from_env_or_settings() + + defp oidc_client_secret_from_env_or_settings do + case System.get_env("OIDC_CLIENT_SECRET") do + nil -> get_oidc_client_secret_from_settings() + value -> trim_nil(value) + end + end @doc """ Returns the OIDC admin group name (for role sync). ENV first, then Settings. @@ -638,4 +653,17 @@ defmodule Mv.Config do nil end end + + # Reads the OIDC client secret via explicit select (excluded from default read, same as smtp_password). + defp get_oidc_client_secret_from_settings do + query = Ash.Query.select(Mv.Membership.Setting, [:id, :oidc_client_secret]) + + case Ash.read_one(query, authorize?: false, domain: Mv.Membership) do + {:ok, settings} when not is_nil(settings) -> + settings |> Map.get(:oidc_client_secret) |> trim_nil() + + _ -> + nil + end + end end diff --git a/lib/mv/mailer.ex b/lib/mv/mailer.ex index 8fca77b..e5ac4e9 100644 --- a/lib/mv/mailer.ex +++ b/lib/mv/mailer.ex @@ -33,6 +33,7 @@ defmodule Mv.Mailer do require Logger + # Simple format check for test-email recipient only (e.g. allows a@b.c). Not for strict RFC validation. @email_regex ~r/^[^\s@]+@[^\s@]+\.[^\s@]+$/ @doc """ @@ -105,6 +106,11 @@ defmodule Mv.Mailer do password = Mv.Config.smtp_password() ssl_mode = Mv.Config.smtp_ssl() || "tls" + verify_mode = + if Application.get_env(:mv, :smtp_verify_peer, false), + do: :verify_peer, + else: :verify_none + [ adapter: Swoosh.Adapters.SMTP, relay: host, @@ -114,10 +120,9 @@ defmodule Mv.Mailer do auth: :always, username: username, password: password, - # OTP 26+ enforces verify_peer; allow self-signed / internal certs. - # tls_options: STARTTLS upgrade (port 587); sockopts: direct SSL connect (port 465). - tls_options: [verify: :verify_none], - sockopts: [verify: :verify_none] + # tls_options: STARTTLS (587); sockopts: direct SSL (465). Verify from :smtp_verify_peer (ENV SMTP_VERIFY_PEER). + tls_options: [verify: verify_mode], + sockopts: [verify: verify_mode] ] |> Enum.reject(fn {_k, v} -> is_nil(v) end) else diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index 60c486d..ce3351a 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -54,11 +54,14 @@ defmodule MvWeb.GlobalSettingsLive do actor = MvWeb.LiveHelpers.current_actor(socket) custom_fields = load_custom_fields(actor) + environment = Application.get_env(:mv, :environment, :dev) + socket = socket |> assign(:page_title, gettext("Settings")) |> assign(:settings, settings) |> assign(:locale, locale) + |> assign(:environment, environment) |> assign(:vereinfacht_env_configured, Mv.Config.vereinfacht_env_configured?()) |> assign(:vereinfacht_api_url_env_set, Mv.Config.vereinfacht_api_url_env_set?()) |> assign(:vereinfacht_api_key_env_set, Mv.Config.vereinfacht_api_key_env_set?()) @@ -76,7 +79,7 @@ defmodule MvWeb.GlobalSettingsLive do |> assign(:oidc_groups_claim_env_set, Mv.Config.oidc_groups_claim_env_set?()) |> assign(:oidc_only_env_set, Mv.Config.oidc_only_env_set?()) |> assign(:oidc_configured, Mv.Config.oidc_configured?()) - |> assign(:oidc_client_secret_set, present?(settings.oidc_client_secret)) + |> assign(:oidc_client_secret_set, Mv.Config.oidc_client_secret_set?()) |> assign(:smtp_env_configured, Mv.Config.smtp_env_configured?()) |> assign(:smtp_host_env_set, Mv.Config.smtp_host_env_set?()) |> assign(:smtp_port_env_set, Mv.Config.smtp_port_env_set?()) @@ -274,7 +277,7 @@ defmodule MvWeb.GlobalSettingsLive do

<% end %> - <%= if Mix.env() == :prod and not @smtp_configured do %> + <%= if @environment == :prod and not @smtp_configured do %>
<.icon name="hero-exclamation-triangle" class="size-5 shrink-0 mt-0.5" /> @@ -688,13 +691,10 @@ defmodule MvWeb.GlobalSettingsLive do assign(socket, form: AshPhoenix.Form.validate(socket.assigns.form, setting_params))} end - # phx-change can fire with only _target (e.g. when focusing a field); avoid FunctionClauseError - def handle_event("validate", params, socket) when is_map(params) do - setting_params = - params["setting"] || Map.get(socket.assigns.form.params || %{}, "setting") || %{} - - {:noreply, - assign(socket, form: AshPhoenix.Form.validate(socket.assigns.form, setting_params))} + # phx-change can fire without "setting" (e.g. only _target when focusing). Do not validate + # with previous form params to avoid surprising behaviour; wait for the next event with setting data. + def handle_event("validate", _params, socket) do + {:noreply, socket} end @impl true @@ -777,7 +777,7 @@ defmodule MvWeb.GlobalSettingsLive do socket |> assign(:settings, fresh_settings) |> assign(:vereinfacht_api_key_set, present?(fresh_settings.vereinfacht_api_key)) - |> assign(:oidc_client_secret_set, present?(fresh_settings.oidc_client_secret)) + |> assign(:oidc_client_secret_set, Mv.Config.oidc_client_secret_set?()) |> assign(:oidc_configured, Mv.Config.oidc_configured?()) |> assign(:smtp_configured, Mv.Config.smtp_configured?()) |> assign(:smtp_password_set, present?(Mv.Config.smtp_password()))