refactor: adress review

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:**

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"}

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)
 

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.

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
 

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

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

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
           </p>
         <% end %>
 
-        <%= if Mix.env() == :prod and not @smtp_configured do %>
+        <%= if @environment == :prod and not @smtp_configured do %>
           <div class="mb-4 flex items-start gap-2 p-3 rounded-lg border border-warning bg-warning/10 text-warning-aa text-sm">
             <.icon name="hero-exclamation-triangle" class="size-5 shrink-0 mt-0.5" />
             <span>
@@ -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()))