From e95c1d6254f4e43332e6e81b3a12cc883152926a Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 16 Mar 2026 14:00:23 +0100 Subject: [PATCH 1/5] fix: repaired smtp configuration for port 587 --- .env.example | 12 ++++ CHANGELOG.md | 3 + config/runtime.exs | 31 +++++----- docs/smtp-configuration-concept.md | 4 +- lib/mv/mailer.ex | 19 ++++-- test/mv/mailer_smtp_config_test.exs | 89 +++++++++++++++++++++++++++++ 6 files changed, 138 insertions(+), 20 deletions(-) create mode 100644 test/mv/mailer_smtp_config_test.exs diff --git a/.env.example b/.env.example index e24b118..661593b 100644 --- a/.env.example +++ b/.env.example @@ -41,3 +41,15 @@ ASSOCIATION_NAME="Sportsclub XYZ" # VEREINFACHT_API_KEY=your-api-key # VEREINFACHT_CLUB_ID=2 # VEREINFACHT_APP_URL=https://app.verein.visuel.dev + +# Optional: Mail / SMTP (transactional emails). If set, overrides Settings UI. +# Export current UI settings to .env: mix mv.export_smtp_to_env +# SMTP_HOST=smtp.example.com +# SMTP_PORT=587 +# SMTP_USERNAME=user +# SMTP_PASSWORD=secret +# SMTP_PASSWORD_FILE=/run/secrets/smtp_password +# SMTP_SSL=tls +# SMTP_VERIFY_PEER=false +# MAIL_FROM_EMAIL=noreply@example.com +# MAIL_FROM_NAME=Mila diff --git a/CHANGELOG.md b/CHANGELOG.md index 681169f..d39df9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **SMTP configuration** – Repaired so that both port 587 (TLS/STARTTLS) and 465 (SSL) work correctly. + ## [1.1.0] - 2026-03-13 ### Added diff --git a/config/runtime.exs b/config/runtime.exs index 1c55f64..6c2b109 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -263,20 +263,25 @@ if config_env() == :prod do verify_mode = if smtp_verify_peer, do: :verify_peer, else: :verify_none + base_smtp_opts = [ + adapter: Swoosh.Adapters.SMTP, + relay: String.trim(smtp_host_env), + port: smtp_port_env, + username: System.get_env("SMTP_USERNAME"), + password: smtp_password_env, + ssl: smtp_ssl_mode == "ssl", + tls: if(smtp_ssl_mode == "tls", do: :always, else: :never), + auth: :always, + tls_options: [verify: verify_mode] + ] + + # Port 465: pass verify in sockopts for initial ssl:connect. Port 587: do not (gen_tcp rejects it). smtp_opts = - [ - adapter: Swoosh.Adapters.SMTP, - relay: String.trim(smtp_host_env), - port: smtp_port_env, - username: System.get_env("SMTP_USERNAME"), - password: smtp_password_env, - ssl: smtp_ssl_mode == "ssl", - tls: if(smtp_ssl_mode == "tls", do: :always, else: :never), - auth: :always, - # tls_options: STARTTLS (587); sockopts: direct SSL (465). - tls_options: [verify: verify_mode], - sockopts: [verify: verify_mode] - ] + if smtp_ssl_mode == "ssl" do + Keyword.put(base_smtp_opts, :sockopts, verify: verify_mode) + else + base_smtp_opts + end |> Enum.reject(fn {_k, v} -> is_nil(v) end) config :mv, Mv.Mailer, smtp_opts diff --git a/docs/smtp-configuration-concept.md b/docs/smtp-configuration-concept.md index 8832b5e..9703c55 100644 --- a/docs/smtp-configuration-concept.md +++ b/docs/smtp-configuration-concept.md @@ -105,7 +105,7 @@ By default, TLS certificate verification is relaxed (`verify_none`) so self-sign - **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. +Verify mode is set in `tls_options` for port 587 (STARTTLS). For port 465 (implicit SSL), the initial connection is `ssl:connect`, so we also pass `sockopts: [verify: verify_mode]` so the SSL handshake uses the same mode. For 587 we must not pass `verify` in sockopts—gen_tcp is used first and rejects it (ArgumentError). The logic is in `config/runtime.exs` (boot) and `Mv.Mailer.smtp_config/0` (Settings-only); keep in sync. --- @@ -117,7 +117,7 @@ Both `tls_options` (STARTTLS, port 587) and `sockopts` (direct SSL, port 465) us - [x] Password from file: `SMTP_PASSWORD_FILE` supported in `runtime.exs`. - [x] Mailer: Swoosh SMTP adapter configured from merged ENV + Settings when SMTP is configured. - [x] Per-request SMTP config via `Mv.Mailer.smtp_config/0` for Settings-only scenarios. -- [x] TLS certificate validation relaxed for OTP 27 (tls_options + sockopts). +- [x] TLS certificate validation relaxed for OTP 27 (tls_options for 587; sockopts with verify only for 465). - [x] Prod warning: clear message in Settings when SMTP is not configured. - [x] Test email: form with recipient field, translatable content, classified success/error messages. - [x] Join confirmation email: uses `Mailer.smtp_config/0` (same as test mail); on failure returns `{:error, :email_delivery_failed}`, error shown in JoinLive, logged for admin. diff --git a/lib/mv/mailer.ex b/lib/mv/mailer.ex index e5ac4e9..05db87b 100644 --- a/lib/mv/mailer.ex +++ b/lib/mv/mailer.ex @@ -111,7 +111,7 @@ defmodule Mv.Mailer do do: :verify_peer, else: :verify_none - [ + base_opts = [ adapter: Swoosh.Adapters.SMTP, relay: host, port: port, @@ -120,11 +120,20 @@ defmodule Mv.Mailer do auth: :always, username: username, password: password, - # 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] + # tls_options: used for STARTTLS (587). For 465, gen_smtp uses sockopts for initial ssl:connect. + tls_options: [verify: verify_mode] ] - |> Enum.reject(fn {_k, v} -> is_nil(v) end) + + # Port 465: initial connection is ssl:connect; pass verify in sockopts so server cert is not required. + # Port 587: initial connection is gen_tcp; sockopts must NOT contain verify (gen_tcp rejects it). + opts = + if ssl_mode == "ssl" do + Keyword.put(base_opts, :sockopts, verify: verify_mode) + else + base_opts + end + + opts |> Enum.reject(fn {_k, v} -> is_nil(v) end) else [] end diff --git a/test/mv/mailer_smtp_config_test.exs b/test/mv/mailer_smtp_config_test.exs new file mode 100644 index 0000000..22325df --- /dev/null +++ b/test/mv/mailer_smtp_config_test.exs @@ -0,0 +1,89 @@ +defmodule Mv.MailerSmtpConfigTest do + @moduledoc """ + Unit tests for Mv.Mailer.smtp_config/0. + + Ensures both port 587 (STARTTLS) and 465 (implicit SSL) work: + - 587: sockopts must NOT contain :verify (gen_tcp:connect would raise ArgumentError). + - 465: sockopts MUST contain :verify so initial ssl:connect uses verify_none/verify_peer. + Uses ENV to drive config; async: false. + """ + use Mv.DataCase, async: false + + alias Mv.Mailer + + defp set_smtp_env(key, value), do: System.put_env(key, value) + + defp clear_smtp_env do + System.delete_env("SMTP_HOST") + System.delete_env("SMTP_PORT") + System.delete_env("SMTP_USERNAME") + System.delete_env("SMTP_PASSWORD") + System.delete_env("SMTP_PASSWORD_FILE") + System.delete_env("SMTP_SSL") + end + + describe "smtp_config/0" do + test "port 587 (TLS): does not include :verify in sockopts so gen_tcp:connect does not crash" do + set_smtp_env("SMTP_HOST", "smtp.example.com") + set_smtp_env("SMTP_PORT", "587") + set_smtp_env("SMTP_SSL", "tls") + + config = Mailer.smtp_config() + + assert config != [], "expected non-empty config when SMTP_HOST is set" + + sockopts = Keyword.get(config, :sockopts, []) + + refute Keyword.has_key?(sockopts, :verify), + "for 587 gen_tcp is used first; sockopts must not contain :verify" + after + clear_smtp_env() + end + + test "port 465 (SSL): includes :verify in sockopts so initial ssl:connect accepts verify mode" do + set_smtp_env("SMTP_HOST", "smtp.example.com") + set_smtp_env("SMTP_PORT", "465") + set_smtp_env("SMTP_SSL", "ssl") + + config = Mailer.smtp_config() + + assert config != [] + sockopts = Keyword.get(config, :sockopts, []) + + assert Keyword.has_key?(sockopts, :verify), + "for 465 initial connection is ssl:connect; sockopts must contain :verify" + + assert Keyword.get(sockopts, :verify) in [:verify_none, :verify_peer] + after + clear_smtp_env() + end + + test "builds TLS mode for port 587 (STARTTLS)" do + set_smtp_env("SMTP_HOST", "smtp.example.com") + set_smtp_env("SMTP_PORT", "587") + set_smtp_env("SMTP_SSL", "tls") + + config = Mailer.smtp_config() + + assert config != [] + assert Keyword.get(config, :tls) == :always + assert Keyword.get(config, :ssl) == false + after + clear_smtp_env() + end + + test "builds SSL mode for port 465 (implicit SSL)" do + set_smtp_env("SMTP_HOST", "smtp.example.com") + set_smtp_env("SMTP_PORT", "465") + set_smtp_env("SMTP_SSL", "ssl") + + config = Mailer.smtp_config() + + assert config != [] + assert Keyword.get(config, :ssl) == true + assert Keyword.get(config, :tls) == :never + after + clear_smtp_env() + end + end +end From e8f27690a161bf602e60a716c272ac5e1e3847a9 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 16 Mar 2026 14:23:46 +0100 Subject: [PATCH 2/5] refactor: unify smtp config logic --- CODE_GUIDELINES.md | 2 + config/runtime.exs | 33 +++-------- docs/smtp-configuration-concept.md | 2 +- lib/mv/mailer.ex | 38 +++---------- lib/mv/smtp/config_builder.ex | 58 +++++++++++++++++++ test/mv/smtp/config_builder_test.exs | 83 ++++++++++++++++++++++++++++ 6 files changed, 162 insertions(+), 54 deletions(-) create mode 100644 lib/mv/smtp/config_builder.ex create mode 100644 test/mv/smtp/config_builder_test.exs diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 8d53484..c0cb543 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -130,6 +130,8 @@ lib/ │ ├── constants.ex # Application constants (member_fields, custom_field_prefix, vereinfacht_required_member_fields) │ ├── application.ex # OTP application │ ├── mailer.ex # Email mailer +│ ├── smtp/ +│ │ └── config_builder.ex # SMTP adapter opts (TLS/sockopts); used by runtime.exs and Mailer │ ├── release.ex # Release tasks │ ├── repo.ex # Database repository │ ├── secrets.ex # Secret management diff --git a/config/runtime.exs b/config/runtime.exs index 6c2b109..6a434fa 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -226,11 +226,7 @@ if config_env() == :prod do # SMTP configuration from environment variables (overrides base adapter in prod). # 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). + # per-send at runtime using Mv.Mailer.smtp_config/0 (which uses the same Mv.Smtp.ConfigBuilder). smtp_host_env = System.get_env("SMTP_HOST") if smtp_host_env && String.trim(smtp_host_env) != "" do @@ -263,26 +259,15 @@ if config_env() == :prod do verify_mode = if smtp_verify_peer, do: :verify_peer, else: :verify_none - base_smtp_opts = [ - adapter: Swoosh.Adapters.SMTP, - relay: String.trim(smtp_host_env), - port: smtp_port_env, - username: System.get_env("SMTP_USERNAME"), - password: smtp_password_env, - ssl: smtp_ssl_mode == "ssl", - tls: if(smtp_ssl_mode == "tls", do: :always, else: :never), - auth: :always, - tls_options: [verify: verify_mode] - ] - - # Port 465: pass verify in sockopts for initial ssl:connect. Port 587: do not (gen_tcp rejects it). smtp_opts = - if smtp_ssl_mode == "ssl" do - Keyword.put(base_smtp_opts, :sockopts, verify: verify_mode) - else - base_smtp_opts - end - |> Enum.reject(fn {_k, v} -> is_nil(v) end) + Mv.Smtp.ConfigBuilder.build_opts( + host: String.trim(smtp_host_env), + port: smtp_port_env, + username: System.get_env("SMTP_USERNAME"), + password: smtp_password_env, + ssl_mode: smtp_ssl_mode, + verify_mode: verify_mode + ) config :mv, Mv.Mailer, smtp_opts end diff --git a/docs/smtp-configuration-concept.md b/docs/smtp-configuration-concept.md index 9703c55..13b0d17 100644 --- a/docs/smtp-configuration-concept.md +++ b/docs/smtp-configuration-concept.md @@ -105,7 +105,7 @@ By default, TLS certificate verification is relaxed (`verify_none`) so self-sign - **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. -Verify mode is set in `tls_options` for port 587 (STARTTLS). For port 465 (implicit SSL), the initial connection is `ssl:connect`, so we also pass `sockopts: [verify: verify_mode]` so the SSL handshake uses the same mode. For 587 we must not pass `verify` in sockopts—gen_tcp is used first and rejects it (ArgumentError). The logic is in `config/runtime.exs` (boot) and `Mv.Mailer.smtp_config/0` (Settings-only); keep in sync. +Verify mode is set in `tls_options` for port 587 (STARTTLS). For port 465 (implicit SSL), the initial connection is `ssl:connect`, so we also pass `sockopts: [verify: verify_mode]` so the SSL handshake uses the same mode. For 587 we must not pass `verify` in sockopts—gen_tcp is used first and rejects it (ArgumentError). The logic lives in `Mv.Smtp.ConfigBuilder.build_opts/1` (single source of truth), used by `config/runtime.exs` (boot) and `Mv.Mailer.smtp_config/0` (Settings-only). --- diff --git a/lib/mv/mailer.ex b/lib/mv/mailer.ex index 05db87b..41a77cd 100644 --- a/lib/mv/mailer.ex +++ b/lib/mv/mailer.ex @@ -31,6 +31,7 @@ defmodule Mv.Mailer do import Swoosh.Email use Gettext, backend: MvWeb.Gettext, otp_app: :mv + alias Mv.Smtp.ConfigBuilder require Logger # Simple format check for test-email recipient only (e.g. allows a@b.c). Not for strict RFC validation. @@ -100,40 +101,19 @@ defmodule Mv.Mailer do @spec smtp_config() :: keyword() def smtp_config do if Mv.Config.smtp_configured?() and not boot_smtp_configured?() do - host = Mv.Config.smtp_host() - port = Mv.Config.smtp_port() || 587 - username = Mv.Config.smtp_username() - 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 - base_opts = [ - adapter: Swoosh.Adapters.SMTP, - relay: host, - port: port, - ssl: ssl_mode == "ssl", - tls: if(ssl_mode == "tls", do: :always, else: :never), - auth: :always, - username: username, - password: password, - # tls_options: used for STARTTLS (587). For 465, gen_smtp uses sockopts for initial ssl:connect. - tls_options: [verify: verify_mode] - ] - - # Port 465: initial connection is ssl:connect; pass verify in sockopts so server cert is not required. - # Port 587: initial connection is gen_tcp; sockopts must NOT contain verify (gen_tcp rejects it). - opts = - if ssl_mode == "ssl" do - Keyword.put(base_opts, :sockopts, verify: verify_mode) - else - base_opts - end - - opts |> Enum.reject(fn {_k, v} -> is_nil(v) end) + ConfigBuilder.build_opts( + host: Mv.Config.smtp_host(), + port: Mv.Config.smtp_port() || 587, + username: Mv.Config.smtp_username(), + password: Mv.Config.smtp_password(), + ssl_mode: Mv.Config.smtp_ssl() || "tls", + verify_mode: verify_mode + ) else [] end diff --git a/lib/mv/smtp/config_builder.ex b/lib/mv/smtp/config_builder.ex new file mode 100644 index 0000000..5018dff --- /dev/null +++ b/lib/mv/smtp/config_builder.ex @@ -0,0 +1,58 @@ +defmodule Mv.Smtp.ConfigBuilder do + @moduledoc """ + Builds Swoosh/gen_smtp SMTP adapter options from connection parameters. + + Single source of truth for TLS/sockopts logic (port 587 vs 465): + - Port 587 (STARTTLS): `gen_tcp` is used first; `sockopts` must NOT contain `:verify`. + - Port 465 (implicit SSL): initial connection is `ssl:connect`; `sockopts` must contain `:verify`. + + Used by `config/runtime.exs` (boot-time ENV) and `Mv.Mailer.smtp_config/0` (Settings-only). + """ + + @doc """ + Builds the keyword list of Swoosh SMTP adapter options. + + Options (keyword list): + - `:host` (required) — relay hostname + - `:port` (required) — port number (e.g. 587 or 465) + - `:ssl_mode` (required) — `"tls"` or `"ssl"` + - `:verify_mode` (required) — `:verify_peer` or `:verify_none` + - `:username` (optional) + - `:password` (optional) + + Nil values are stripped from the result. + """ + @spec build_opts(keyword()) :: keyword() + def build_opts(opts) do + host = Keyword.fetch!(opts, :host) + port = Keyword.fetch!(opts, :port) + username = Keyword.get(opts, :username) + password = Keyword.get(opts, :password) + ssl_mode = Keyword.fetch!(opts, :ssl_mode) + verify_mode = Keyword.fetch!(opts, :verify_mode) + + base_opts = [ + adapter: Swoosh.Adapters.SMTP, + relay: host, + port: port, + username: username, + password: password, + ssl: ssl_mode == "ssl", + tls: if(ssl_mode == "tls", do: :always, else: :never), + auth: :always, + # tls_options: used for STARTTLS (587). For 465, gen_smtp uses sockopts for initial ssl:connect. + tls_options: [verify: verify_mode] + ] + + # Port 465: initial connection is ssl:connect; pass verify in sockopts. + # Port 587: initial connection is gen_tcp; sockopts must NOT contain verify (gen_tcp rejects it). + opts = + if ssl_mode == "ssl" do + Keyword.put(base_opts, :sockopts, verify: verify_mode) + else + base_opts + end + + Enum.reject(opts, fn {_k, v} -> is_nil(v) end) + end +end diff --git a/test/mv/smtp/config_builder_test.exs b/test/mv/smtp/config_builder_test.exs new file mode 100644 index 0000000..0b12a52 --- /dev/null +++ b/test/mv/smtp/config_builder_test.exs @@ -0,0 +1,83 @@ +defmodule Mv.Smtp.ConfigBuilderTest do + @moduledoc """ + Unit tests for Mv.Smtp.ConfigBuilder.build_opts/1. + + Ensures the single source of truth for SMTP opts correctly handles: + - Port 587 (TLS): sockopts must NOT contain :verify (gen_tcp rejects it). + - Port 465 (SSL): sockopts MUST contain :verify for initial ssl:connect. + """ + use ExUnit.Case, async: true + + alias Mv.Smtp.ConfigBuilder + + describe "build_opts/1" do + test "port 587 (TLS): does not include :verify in sockopts" do + opts = + ConfigBuilder.build_opts( + host: "smtp.example.com", + port: 587, + username: "user", + password: "secret", + ssl_mode: "tls", + verify_mode: :verify_none + ) + + sockopts = Keyword.get(opts, :sockopts, []) + refute Keyword.has_key?(sockopts, :verify), "for 587 sockopts must not contain :verify" + assert Keyword.get(opts, :tls) == :always + assert Keyword.get(opts, :ssl) == false + end + + test "port 465 (SSL): includes :verify in sockopts" do + opts = + ConfigBuilder.build_opts( + host: "smtp.example.com", + port: 465, + username: "user", + password: "secret", + ssl_mode: "ssl", + verify_mode: :verify_peer + ) + + sockopts = Keyword.get(opts, :sockopts, []) + assert Keyword.has_key?(sockopts, :verify), "for 465 sockopts must contain :verify" + assert Keyword.get(sockopts, :verify) == :verify_peer + assert Keyword.get(opts, :ssl) == true + assert Keyword.get(opts, :tls) == :never + end + + test "strips nil values" do + opts = + ConfigBuilder.build_opts( + host: "smtp.example.com", + port: 587, + username: nil, + password: nil, + ssl_mode: "tls", + verify_mode: :verify_none + ) + + refute Keyword.has_key?(opts, :username) + refute Keyword.has_key?(opts, :password) + assert Keyword.get(opts, :relay) == "smtp.example.com" + end + + test "includes adapter and required keys" do + opts = + ConfigBuilder.build_opts( + host: "mail.example.com", + port: 587, + username: "u", + password: "p", + ssl_mode: "tls", + verify_mode: :verify_none + ) + + assert Keyword.get(opts, :adapter) == Swoosh.Adapters.SMTP + assert Keyword.get(opts, :relay) == "mail.example.com" + assert Keyword.get(opts, :port) == 587 + assert Keyword.get(opts, :auth) == :always + assert Keyword.get(opts, :tls_options) == [verify: :verify_none] + end + end +end From f353f1cbc05527c407b201ee3edba438f23dac45 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 16 Mar 2026 14:58:21 +0100 Subject: [PATCH 3/5] fix: update smtp test --- priv/gettext/de/LC_MESSAGES/default.po | 2 +- .../live/join_live_email_failure_test.exs | 37 +++++++++++++++++-- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 078ef19..c4cacc8 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -3854,7 +3854,7 @@ msgstr "Wenn deaktiviert, können sich Nutzer*innen nicht über /register anmeld #: lib/mv_web/controllers/page_controller.ex #, elixir-autogen, elixir-format msgid "Home" -msgstr "" +msgstr "Startseite" #: lib/mv_web/controllers/join_confirm_controller.ex #, elixir-autogen, elixir-format, fuzzy diff --git a/test/mv_web/live/join_live_email_failure_test.exs b/test/mv_web/live/join_live_email_failure_test.exs index ef308c6..50a209c 100644 --- a/test/mv_web/live/join_live_email_failure_test.exs +++ b/test/mv_web/live/join_live_email_failure_test.exs @@ -2,6 +2,9 @@ defmodule MvWeb.JoinLiveEmailFailureTest do @moduledoc """ When join confirmation email delivery fails, the user sees an error message and no success copy. Uses FailingMailAdapter; async: false to avoid config races. + + Ensures SMTP is not configured (ENV cleared, Settings smtp_host nil) so that + Mailer.smtp_config/0 returns [] and the Application-configured FailingMailAdapter is used. """ use MvWeb.ConnCase, async: false import Phoenix.LiveViewTest @@ -12,18 +15,21 @@ defmodule MvWeb.JoinLiveEmailFailureTest do test "when confirmation email fails, user sees error flash and no success message", %{ conn: conn } do + # Clear SMTP config so Mailer.smtp_config() returns [] and FailingMailAdapter is used. + saved_env = clear_smtp_env_and_save() enable_join_form_for_test() - saved = Application.get_env(:mv, Mv.Mailer) + saved_mailer = Application.get_env(:mv, Mv.Mailer) Application.put_env( :mv, Mv.Mailer, - Keyword.put(saved || [], :adapter, Mv.TestSupport.FailingMailAdapter) + Keyword.put(saved_mailer || [], :adapter, Mv.TestSupport.FailingMailAdapter) ) on_exit(fn -> - Application.put_env(:mv, Mv.Mailer, saved) + Application.put_env(:mv, Mv.Mailer, saved_mailer) + restore_smtp_env(saved_env) end) {:ok, view, _html} = live(conn, "/join") @@ -46,13 +52,36 @@ defmodule MvWeb.JoinLiveEmailFailureTest do refute view |> element("[data-testid='join-success-message']") |> has_element?() end + defp clear_smtp_env_and_save do + keys = [ + "SMTP_HOST", + "SMTP_PORT", + "SMTP_USERNAME", + "SMTP_PASSWORD", + "SMTP_PASSWORD_FILE", + "SMTP_SSL" + ] + + saved = for k <- keys, into: %{}, do: {k, System.get_env(k)} + for k <- keys, do: System.delete_env(k) + saved + end + + defp restore_smtp_env(saved) do + for {k, v} <- saved do + if v != nil, do: System.put_env(k, v), else: System.delete_env(k) + end + end + defp enable_join_form_for_test do {:ok, settings} = Membership.get_settings() Membership.update_settings(settings, %{ join_form_enabled: true, join_form_field_ids: ["email", "first_name", "last_name"], - join_form_field_required: %{"email" => true, "first_name" => false, "last_name" => false} + join_form_field_required: %{"email" => true, "first_name" => false, "last_name" => false}, + # Clear SMTP host so Mv.Config.smtp_configured?() is false and Mailer.smtp_config() returns []. + smtp_host: nil }) end end From c381b86b5e94d670d66bb79e48f87f1389ed3239 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 16 Mar 2026 19:09:07 +0100 Subject: [PATCH 4/5] Improve oidc only mode (#474) ## Description of the implemented changes The changes were: - [x] Bugfixing - [x] New Feature - [ ] Breaking Change - [x] Refactoring **OIDC-only mode improvements and UX tweaks (success toasts, unauthenticated redirect).** ## What has been changed? ### OIDC-only mode (new feature) - **Admin settings:** "Only OIDC sign-in" is an immediate toggle at the top of the OIDC section (no save button). Enabling it also turns off "Allow direct registration". When OIDC-only is on, the registration checkbox is disabled and shows a tooltip (DaisyUI `<.tooltip>`). - **Backend:** Password sign-in is forbidden via Ash policy (`OidcOnlyActive` check). Password registration is blocked via validation `OidcOnlyBlocksPasswordRegistration`. New plug `OidcOnlySignInRedirect`: when OIDC-only and OIDC are configured, GET `/sign-in` redirects to the OIDC flow; GET `/auth/user/password/sign_in_with_token` is rejected with redirect + flash. `AuthController.success/4` also rejects password sign-in when OIDC-only. - **Tests:** GlobalSettingsLive (OIDC-only UI), AuthController (redirect and password sign-in rejection), User authentication (register_with_password blocked when OIDC-only). ### UX / behaviour (no new feature flag) - **Success toasts:** Success flash messages auto-dismiss after 5 seconds via JS hook `FlashAutoDismiss` and optional `auto_clear_ms` on `<.flash>` (used for success in root layout and `flash_group`). - **Unauthenticated users:** Redirect to sign-in without the "You don't have permission to access this page" flash; that message is only shown to logged-in users who lack access. Logic in `LiveHelpers` and `CheckPagePermission` plug; test updated accordingly. ### Other - Layouts: comment about unprocessed join-request count no longer uses "TODO" (Credo). - Gettext: German translation for "Home" (Startseite); POT/PO kept in sync. - CHANGELOG: Unreleased section updated with the above. ## Definition of Done ### Code Quality - [x] No new technical depths - [x] Linting passed - [x] Documentation is added where needed (module docs, comments where non-obvious) ### Accessibility - [x] New elements are properly defined with html-tags (labels, aria-label on checkboxes) - [x] Colour contrast follows WCAG criteria (unchanged) - [x] Aria labels are added when needed (e.g. oidc-only and registration checkboxes) - [x] Everything is accessible by keyboard (toggles and buttons unchanged) - [x] Tab-Order is comprehensible - [x] All interactive elements have a visible focus (existing patterns) ### Testing - [x] Tests for new code are written (OIDC-only UI, auth controller, user auth; SMTP config builder and mailer) - [x] All tests pass - [ ] axe-core dev tools show no critical or major issues (not re-run for this PR; suggest spot-check on settings and sign-in) ## Additional Notes - **OIDC-only:** When the `OIDC_ONLY` env var is set, the toggle is read-only and shows "(From OIDC_ONLY)". When OIDC is not configured, the toggle is disabled. - **Invalidation:** Enabling OIDC-only sets `registration_enabled: false` in one update; disabling OIDC-only only updates `oidc_only` (registration left as-is). - **Review focus:** Plug order in router (OidcOnlySignInRedirect), policy/validation order in User, and that all OIDC-only paths (form, plug, controller) stay consistent. Reviewed-on: https://git.local-it.org/local-it/mitgliederverwaltung/pulls/474 Co-authored-by: Simon Co-committed-by: Simon --- CHANGELOG.md | 7 + assets/js/app.js | 19 +++ docs/admin-bootstrap-and-oidc-role-sync.md | 1 + docs/feature-roadmap.md | 5 + lib/accounts/user.ex | 10 ++ .../oidc_only_blocks_password_registration.ex | 27 ++++ .../authorization/checks/oidc_only_active.ex | 16 ++ lib/mv_web/components/core_components.ex | 8 + lib/mv_web/components/layouts.ex | 2 +- lib/mv_web/components/layouts/root.html.heex | 2 +- lib/mv_web/controllers/auth_controller.ex | 31 +++- lib/mv_web/live/global_settings_live.ex | 145 +++++++++++++----- lib/mv_web/live_helpers.ex | 9 +- lib/mv_web/plugs/check_page_permission.ex | 9 +- .../plugs/oidc_only_sign_in_redirect.ex | 73 +++++++++ lib/mv_web/router.ex | 1 + priv/gettext/de/LC_MESSAGES/default.po | 10 ++ priv/gettext/default.pot | 10 ++ priv/gettext/en/LC_MESSAGES/default.po | 10 ++ test/accounts/user_authentication_test.exs | 27 ++++ .../controllers/auth_controller_test.exs | 141 ++++++++++++++++- .../mv_web/live/global_settings_live_test.exs | 65 ++++++++ .../plugs/check_page_permission_test.exs | 5 +- 23 files changed, 579 insertions(+), 54 deletions(-) create mode 100644 lib/accounts/user/validations/oidc_only_blocks_password_registration.ex create mode 100644 lib/mv/authorization/checks/oidc_only_active.ex create mode 100644 lib/mv_web/plugs/oidc_only_sign_in_redirect.ex diff --git a/CHANGELOG.md b/CHANGELOG.md index d39df9b..7cc8ea5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- **Improved OIDC-only mode** – Admin can enable “Only OIDC sign-in” in settings; when enabled, direct registration is disabled and sign-in page redirects to OIDC when configured. +- **Success toast auto-dismiss** – Success flash messages (e.g. “Settings saved”) hide automatically after 5 seconds instead of requiring the user to close them. + +### Changed +- **Unauthenticated access** – Users who are not logged in are redirected to sign-in without showing a “no permission” message; the message is only shown to logged-in users who lack access. + ### Fixed - **SMTP configuration** – Repaired so that both port 587 (TLS/STARTTLS) and 465 (SSL) work correctly. diff --git a/assets/js/app.js b/assets/js/app.js index 87f2c25..4c7e3c5 100644 --- a/assets/js/app.js +++ b/assets/js/app.js @@ -113,6 +113,25 @@ Hooks.FocusRestore = { } } +// FlashAutoDismiss: after a delay, clear the flash so the toast hides without user clicking X (e.g. success toasts) +Hooks.FlashAutoDismiss = { + mounted() { + const ms = this.el.dataset.autoClearMs + if (!ms) return + const delay = parseInt(ms, 10) + if (delay > 0) { + this.timer = setTimeout(() => { + const key = this.el.dataset.clearFlashKey || "success" + this.pushEvent("lv:clear-flash", {key}) + }, delay) + } + }, + + destroyed() { + if (this.timer) clearTimeout(this.timer) + } +} + // TabListKeydown hook: WCAG tab pattern — prevent default for ArrowLeft/ArrowRight so the server can handle tab switch (roving tabindex) Hooks.TabListKeydown = { mounted() { diff --git a/docs/admin-bootstrap-and-oidc-role-sync.md b/docs/admin-bootstrap-and-oidc-role-sync.md index 5e26c85..aa5c155 100644 --- a/docs/admin-bootstrap-and-oidc-role-sync.md +++ b/docs/admin-bootstrap-and-oidc-role-sync.md @@ -38,6 +38,7 @@ ### Sign-in page (OIDC-only mode) - `OIDC_ONLY` (or Settings → OIDC → "Only OIDC sign-in") – When set to true/1/yes and OIDC is configured, the sign-in page shows only the Single Sign-On button (password login is hidden). ENV takes precedence over Settings. +- **Redirect loop fix:** After an OIDC failure (e.g. provider down), the app redirects to `/sign-in?oidc_failed=1`. The plug `OidcOnlySignInRedirect` does not redirect that request back to OIDC, so the sign-in page is shown with the error (no endless redirect). ### Sync Logic diff --git a/docs/feature-roadmap.md b/docs/feature-roadmap.md index 6383660..2ec15a5 100644 --- a/docs/feature-roadmap.md +++ b/docs/feature-roadmap.md @@ -49,6 +49,11 @@ - ✅ **Page-level authorization** - LiveView page access control - ✅ **System role protection** - Critical roles cannot be deleted +**Planned: OIDC-only mode (TDD, tests first):** +- Admin Settings: When OIDC-only is enabled, disable "Allow direct registration" toggle and show hint (tests in `GlobalSettingsLiveTest`). +- Backend: Reject password sign-in and `register_with_password` when OIDC-only (tests in `AuthControllerTest`, `Accounts`). +- GET `/sign-in` redirect to OIDC when OIDC-only and OIDC configured (tests in `AuthControllerTest`). Implementation to follow after tests. + **Missing Features:** - ❌ Password reset flow - ❌ Email verification diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 29a2d4b..0127796 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -362,6 +362,12 @@ defmodule Mv.Accounts.User do # Authorization Policies # Order matters: Most specific policies first, then general permission check policies do + # When OIDC-only is active, password sign-in is forbidden (SSO only). + policy action(:sign_in_with_password) do + forbid_if Mv.Authorization.Checks.OidcOnlyActive + authorize_if always() + end + # AshAuthentication bypass (registration/login without actor) bypass AshAuthentication.Checks.AshAuthenticationInteraction do description "Allow AshAuthentication internal operations (registration, login)" @@ -409,6 +415,10 @@ defmodule Mv.Accounts.User do validate {Mv.Accounts.User.Validations.RegistrationEnabled, []}, where: [action_is(:register_with_password)] + # Block password registration when OIDC-only mode is active + validate {Mv.Accounts.User.Validations.OidcOnlyBlocksPasswordRegistration, []}, + where: [action_is(:register_with_password)] + # Email uniqueness check for all actions that change the email attribute # Validates that user email is not already used by another (unlinked) member validate Mv.Accounts.User.Validations.EmailNotUsedByOtherMember diff --git a/lib/accounts/user/validations/oidc_only_blocks_password_registration.ex b/lib/accounts/user/validations/oidc_only_blocks_password_registration.ex new file mode 100644 index 0000000..e4d9a35 --- /dev/null +++ b/lib/accounts/user/validations/oidc_only_blocks_password_registration.ex @@ -0,0 +1,27 @@ +defmodule Mv.Accounts.User.Validations.OidcOnlyBlocksPasswordRegistration do + @moduledoc """ + Validation that blocks direct registration (register_with_password) when + OIDC-only mode is active. In OIDC-only mode, sign-in and registration are + only allowed via OIDC (SSO). + """ + use Ash.Resource.Validation + + @impl true + def init(opts), do: {:ok, opts} + + @impl true + def validate(_changeset, _opts, _context) do + if Mv.Config.oidc_only?() do + {:error, + field: :base, + message: + Gettext.dgettext( + MvWeb.Gettext, + "default", + "Registration with password is disabled when only OIDC sign-in is active." + )} + else + :ok + end + end +end diff --git a/lib/mv/authorization/checks/oidc_only_active.ex b/lib/mv/authorization/checks/oidc_only_active.ex new file mode 100644 index 0000000..8d56ca1 --- /dev/null +++ b/lib/mv/authorization/checks/oidc_only_active.ex @@ -0,0 +1,16 @@ +defmodule Mv.Authorization.Checks.OidcOnlyActive do + @moduledoc """ + Policy check: true when OIDC-only mode is active (Config.oidc_only?()). + + Used to forbid password sign-in when only OIDC (SSO) sign-in is allowed. + """ + use Ash.Policy.SimpleCheck + + alias Mv.Config + + @impl true + def describe(_opts), do: "OIDC-only mode is active" + + @impl true + def match?(_actor, _context, _opts), do: Config.oidc_only?() +end diff --git a/lib/mv_web/components/core_components.ex b/lib/mv_web/components/core_components.ex index 8c58c32..b5bd763 100644 --- a/lib/mv_web/components/core_components.ex +++ b/lib/mv_web/components/core_components.ex @@ -63,6 +63,11 @@ defmodule MvWeb.CoreComponents do values: [:info, :error, :success, :warning], doc: "used for styling and flash lookup" + attr :auto_clear_ms, :integer, + default: nil, + doc: + "when set, flash is auto-dismissed after this many milliseconds (e.g. 5000 for success toasts)" + attr :rest, :global, doc: "the arbitrary HTML attributes to add to the flash container" slot :inner_block, doc: "the optional inner block that renders the flash message" @@ -74,6 +79,9 @@ defmodule MvWeb.CoreComponents do
hide("##{@id}")} role="alert" class="pointer-events-auto" diff --git a/lib/mv_web/components/layouts.ex b/lib/mv_web/components/layouts.ex index 5a96001..54f589d 100644 --- a/lib/mv_web/components/layouts.ex +++ b/lib/mv_web/components/layouts.ex @@ -265,7 +265,7 @@ defmodule MvWeb.Layouts do aria-live="polite" class="z-50 toast toast-bottom toast-end flex flex-col gap-2 pointer-events-none" > - <.flash kind={:success} flash={@flash} /> + <.flash kind={:success} flash={@flash} auto_clear_ms={5000} /> <.flash kind={:warning} flash={@flash} /> <.flash kind={:info} flash={@flash} /> <.flash kind={:error} flash={@flash} /> diff --git a/lib/mv_web/components/layouts/root.html.heex b/lib/mv_web/components/layouts/root.html.heex index 5419b73..bb900aa 100644 --- a/lib/mv_web/components/layouts/root.html.heex +++ b/lib/mv_web/components/layouts/root.html.heex @@ -74,7 +74,7 @@ aria-live="polite" class="z-50 flex flex-col gap-2 toast toast-bottom toast-end" > - <.flash id="flash-success-root" kind={:success} flash={@flash} /> + <.flash id="flash-success-root" kind={:success} flash={@flash} auto_clear_ms={5000} /> <.flash id="flash-warning-root" kind={:warning} flash={@flash} /> <.flash id="flash-info-root" kind={:info} flash={@flash} /> <.flash id="flash-error-root" kind={:error} flash={@flash} /> diff --git a/lib/mv_web/controllers/auth_controller.ex b/lib/mv_web/controllers/auth_controller.ex index 20a76f5..adde4e8 100644 --- a/lib/mv_web/controllers/auth_controller.ex +++ b/lib/mv_web/controllers/auth_controller.ex @@ -15,8 +15,23 @@ defmodule MvWeb.AuthController do use AshAuthentication.Phoenix.Controller alias Mv.Accounts.User.Errors.PasswordVerificationRequired + alias Mv.Config - def success(conn, activity, user, _token) do + def success(conn, {:password, :sign_in} = _activity, user, token) do + if Config.oidc_only?() do + conn + |> put_flash(:error, gettext("Only sign-in via Single Sign-On (SSO) is allowed.")) + |> redirect(to: sign_in_path_after_oidc_failure()) + else + success_continue(conn, {:password, :sign_in}, user, token) + end + end + + def success(conn, activity, user, token) do + success_continue(conn, activity, user, token) + end + + defp success_continue(conn, activity, user, _token) do return_to = get_session(conn, :return_to) || ~p"/" message = @@ -134,7 +149,7 @@ defmodule MvWeb.AuthController do _ -> conn |> put_flash(:error, gettext("Unable to authenticate with OIDC. Please try again.")) - |> redirect(to: ~p"/sign-in") + |> redirect(to: sign_in_path_after_oidc_failure()) end end @@ -148,7 +163,7 @@ defmodule MvWeb.AuthController do :error, gettext("The authentication server is currently unavailable. Please try again later.") ) - |> redirect(to: ~p"/sign-in") + |> redirect(to: sign_in_path_after_oidc_failure()) end # Handle Assent invalid response errors (configuration or malformed responses) @@ -161,7 +176,7 @@ defmodule MvWeb.AuthController do :error, gettext("Authentication configuration error. Please contact the administrator.") ) - |> redirect(to: ~p"/sign-in") + |> redirect(to: sign_in_path_after_oidc_failure()) end # Catch-all clause for any other error types @@ -171,7 +186,7 @@ defmodule MvWeb.AuthController do conn |> put_flash(:error, gettext("Unable to authenticate with OIDC. Please try again.")) - |> redirect(to: ~p"/sign-in") + |> redirect(to: sign_in_path_after_oidc_failure()) end # Handle generic AuthenticationFailed errors @@ -211,10 +226,14 @@ defmodule MvWeb.AuthController do conn |> put_flash(:error, error_message) - |> redirect(to: ~p"/sign-in") + |> redirect(to: sign_in_path_after_oidc_failure()) end end + # Path used when redirecting to sign-in after an OIDC failure. The query param tells + # OidcOnlySignInRedirect to show the sign-in page instead of redirecting back to OIDC (avoids loop). + defp sign_in_path_after_oidc_failure, do: "/sign-in?oidc_failed=1" + # Extract meaningful error message from Ash errors defp extract_meaningful_error_message(errors) do # Look for specific error messages in InvalidAttribute errors diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index f69b947..cb57631 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -19,6 +19,7 @@ defmodule MvWeb.GlobalSettingsLive do ## Events - `validate` / `save` - Club settings form - `toggle_registration_enabled` - Enable/disable direct registration (/register) + - `toggle_oidc_only` - Enable/disable OIDC-only sign-in (immediate, outside OIDC form) - `toggle_join_form_enabled` - Enable/disable the join form - `add_join_form_field` / `remove_join_form_field` - Manage join form fields - `toggle_join_form_field_required` - Toggle required flag per field @@ -80,6 +81,7 @@ defmodule MvWeb.GlobalSettingsLive do |> assign(:oidc_admin_group_name_env_set, Mv.Config.oidc_admin_group_name_env_set?()) |> 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_only, Mv.Config.oidc_only?()) |> 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) @@ -625,11 +627,30 @@ defmodule MvWeb.GlobalSettingsLive do class="checkbox checkbox-sm" checked={@registration_enabled} phx-click="toggle_registration_enabled" + disabled={@oidc_only} aria-label={gettext("Allow direct registration (/register)")} /> -

{gettext("OIDC (Single Sign-On)")}

@@ -638,6 +659,38 @@ defmodule MvWeb.GlobalSettingsLive do {gettext("Some values are set via environment variables. Those fields are read-only.")}

<% end %> +
+ + +
+

+ {gettext( + "When enabled and OIDC is configured, the sign-in page shows only the Single Sign-On button." + )} +

<.form for={@form} id="oidc-form" phx-change="validate" phx-submit="save">
<.input @@ -744,27 +797,6 @@ defmodule MvWeb.GlobalSettingsLive do ) } /> -
- <.input - field={@form[:oidc_only]} - type="checkbox" - class="checkbox checkbox-sm" - disabled={@oidc_only_env_set or not @oidc_configured} - label={ - if @oidc_only_env_set do - gettext("Only OIDC sign-in (hide password login)") <> - " (" <> gettext("From OIDC_ONLY") <> ")" - else - gettext("Only OIDC sign-in (hide password login)") - end - } - /> -

- {gettext( - "When enabled and OIDC is configured, the sign-in page shows only the Single Sign-On button." - )} -

-
<.button :if={ @@ -868,18 +900,19 @@ defmodule MvWeb.GlobalSettingsLive do saves_vereinfacht = vereinfacht_params?(setting_params_clean) case MvWeb.LiveHelpers.submit_form(socket.assigns.form, setting_params_clean, actor) do - {:ok, _updated_settings} -> - {:ok, fresh_settings} = Membership.get_settings() - + {:ok, updated_settings} -> + # Use the returned record for the form so saved values show immediately; + # get_settings() can return cached data without the new attribute until reload. test_result = if saves_vereinfacht, do: Mv.Vereinfacht.test_connection(), else: nil socket = socket - |> assign(:settings, fresh_settings) - |> assign(:registration_enabled, fresh_settings.registration_enabled != false) - |> assign(:vereinfacht_api_key_set, present?(fresh_settings.vereinfacht_api_key)) + |> assign(:settings, updated_settings) + |> assign(:registration_enabled, updated_settings.registration_enabled != false) + |> assign(:vereinfacht_api_key_set, present?(updated_settings.vereinfacht_api_key)) |> assign(:oidc_client_secret_set, Mv.Config.oidc_client_secret_set?()) + |> assign(:oidc_only, Mv.Config.oidc_only?()) |> assign(:oidc_configured, Mv.Config.oidc_configured?()) |> assign(:smtp_configured, Mv.Config.smtp_configured?()) |> assign(:smtp_password_set, present?(Mv.Config.smtp_password())) @@ -916,19 +949,53 @@ defmodule MvWeb.GlobalSettingsLive do @impl true def handle_event("toggle_registration_enabled", _params, socket) do - settings = socket.assigns.settings - new_value = not socket.assigns.registration_enabled + if Mv.Config.oidc_only?() do + {:noreply, socket} + else + settings = socket.assigns.settings + new_value = not socket.assigns.registration_enabled - case Membership.update_settings(settings, %{registration_enabled: new_value}) do - {:ok, updated_settings} -> - {:noreply, - socket - |> assign(:settings, updated_settings) - |> assign(:registration_enabled, updated_settings.registration_enabled != false) - |> assign_form()} + case Membership.update_settings(settings, %{registration_enabled: new_value}) do + {:ok, updated_settings} -> + {:noreply, + socket + |> assign(:settings, updated_settings) + |> assign(:registration_enabled, updated_settings.registration_enabled != false) + |> assign_form()} - {:error, _} -> - {:noreply, put_flash(socket, :error, gettext("Failed to update setting."))} + {:error, _} -> + {:noreply, put_flash(socket, :error, gettext("Failed to update setting."))} + end + end + end + + @impl true + def handle_event("toggle_oidc_only", _params, socket) do + if socket.assigns.oidc_only_env_set do + {:noreply, socket} + else + settings = socket.assigns.settings + new_value = not socket.assigns.oidc_only + + # When enabling OIDC-only, also disable direct registration; when disabling, only change oidc_only. + params = + if new_value, + do: %{oidc_only: true, registration_enabled: false}, + else: %{oidc_only: false} + + case Membership.update_settings(settings, params) do + {:ok, updated_settings} -> + {:noreply, + socket + |> assign(:settings, updated_settings) + |> assign(:oidc_only, updated_settings.oidc_only == true) + |> assign(:registration_enabled, updated_settings.registration_enabled != false) + |> assign_form() + |> put_flash(:success, gettext("Settings updated successfully"))} + + {:error, _} -> + {:noreply, put_flash(socket, :error, gettext("Failed to update setting."))} + end end end diff --git a/lib/mv_web/live_helpers.ex b/lib/mv_web/live_helpers.ex index 38a0157..4206aa6 100644 --- a/lib/mv_web/live_helpers.ex +++ b/lib/mv_web/live_helpers.ex @@ -74,7 +74,7 @@ defmodule MvWeb.LiveHelpers do socket = socket - |> Phoenix.LiveView.put_flash(:error, "You don't have permission to access this page.") + |> maybe_put_access_denied_flash(user) |> Phoenix.LiveView.push_navigate(to: redirect_to) {:halt, socket} @@ -82,6 +82,13 @@ defmodule MvWeb.LiveHelpers do end end + # Only show "no permission" when user is logged in; unauthenticated users are redirected to sign-in without flash. + defp maybe_put_access_denied_flash(socket, nil), do: socket + + defp maybe_put_access_denied_flash(socket, _user) do + Phoenix.LiveView.put_flash(socket, :error, "You don't have permission to access this page.") + end + defp ensure_user_role_loaded(socket) do user = socket.assigns[:current_user] diff --git a/lib/mv_web/plugs/check_page_permission.ex b/lib/mv_web/plugs/check_page_permission.ex index ff6d47d..b2f37ec 100644 --- a/lib/mv_web/plugs/check_page_permission.ex +++ b/lib/mv_web/plugs/check_page_permission.ex @@ -54,7 +54,7 @@ defmodule MvWeb.Plugs.CheckPagePermission do conn |> fetch_session() |> fetch_flash() - |> put_flash(:error, "You don't have permission to access this page.") + |> maybe_put_access_denied_flash(user) |> redirect(to: redirect_to) |> halt() end @@ -75,6 +75,13 @@ defmodule MvWeb.Plugs.CheckPagePermission do defp redirect_target(user), do: redirect_target_for_user(user) + # Only set "no permission" flash when user is logged in; unauthenticated users get redirect only, no flash. + defp maybe_put_access_denied_flash(conn, nil), do: conn + + defp maybe_put_access_denied_flash(conn, _user) do + put_flash(conn, :error, "You don't have permission to access this page.") + end + @doc """ Returns true if the path is public (no auth/permission check). Used by LiveView hook to skip redirect on sign-in etc. diff --git a/lib/mv_web/plugs/oidc_only_sign_in_redirect.ex b/lib/mv_web/plugs/oidc_only_sign_in_redirect.ex new file mode 100644 index 0000000..9cf2a16 --- /dev/null +++ b/lib/mv_web/plugs/oidc_only_sign_in_redirect.ex @@ -0,0 +1,73 @@ +defmodule MvWeb.Plugs.OidcOnlySignInRedirect do + @moduledoc """ + When OIDC-only mode is active: + - GET /sign-in redirects to the OIDC flow when OIDC is configured (sign-in page skipped). + - GET /sign-in?oidc_failed=1 is not redirected, so the sign-in page is shown after an OIDC + failure (avoids redirect loop when the provider is down or misconfigured). + - GET /auth/user/password/sign_in_with_token is rejected (redirect to /sign-in with error) + so password sign-in cannot complete. + """ + import Plug.Conn + import Phoenix.Controller + + alias Mv.Config + + def init(opts), do: opts + + def call(conn, _opts) do + conn + |> maybe_redirect_sign_in_to_oidc() + |> maybe_reject_password_token_sign_in() + end + + defp maybe_redirect_sign_in_to_oidc(conn) do + if conn.request_path != "/sign-in" or conn.method != "GET" do + conn + else + conn = fetch_query_params(conn) + maybe_redirect_sign_in_to_oidc_checked(conn) + end + end + + defp maybe_redirect_sign_in_to_oidc_checked(conn) do + cond do + # Show sign-in page when returning from OIDC failure to avoid redirect loop. + conn.query_params["oidc_failed"] -> conn + Config.oidc_only?() and Config.oidc_configured?() -> redirect_and_halt(conn) + true -> conn + end + end + + defp redirect_and_halt(conn) do + conn + |> redirect(to: "/auth/user/oidc") + |> halt() + end + + defp maybe_reject_password_token_sign_in(conn) do + if conn.halted, do: conn, else: reject_password_token_sign_in_if_applicable(conn) + end + + defp reject_password_token_sign_in_if_applicable(conn) do + path = conn.request_path + + password_token_path? = + path =~ ~r|/auth/user/password/sign_in_with_token| and conn.method == "GET" + + if password_token_path? and Config.oidc_only?() do + message = + Gettext.dgettext( + MvWeb.Gettext, + "default", + "Only sign-in via Single Sign-On (SSO) is allowed." + ) + + conn + |> put_flash(:error, message) + |> redirect(to: "/sign-in") + |> halt() + else + conn + end + end +end diff --git a/lib/mv_web/router.ex b/lib/mv_web/router.ex index f115535..591dead 100644 --- a/lib/mv_web/router.ex +++ b/lib/mv_web/router.ex @@ -18,6 +18,7 @@ defmodule MvWeb.Router do plug MvWeb.Plugs.CheckPagePermission plug MvWeb.Plugs.JoinFormEnabled plug MvWeb.Plugs.RegistrationEnabled + plug MvWeb.Plugs.OidcOnlySignInRedirect end pipeline :api do diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index c4cacc8..383fb1c 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -3895,3 +3895,13 @@ msgstr "Rolle %{name}" #, elixir-autogen, elixir-format msgid "User %{email}" msgstr "Benutzer*in %{email}" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "Only OIDC sign-in is active. This option is disabled." +msgstr "Nur OIDC-Anmeldung ist aktiv. Diese Option ist deaktiviert." + +#: lib/mv_web/controllers/auth_controller.ex +#, elixir-autogen, elixir-format +msgid "Only sign-in via Single Sign-On (SSO) is allowed." +msgstr "Nur Anmeldung per Single Sign-On (SSO) ist erlaubt." diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index a845e1e..3e2eb5d 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -3895,3 +3895,13 @@ msgstr "" #, elixir-autogen, elixir-format msgid "User %{email}" msgstr "" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "Only OIDC sign-in is active. This option is disabled." +msgstr "" + +#: lib/mv_web/controllers/auth_controller.ex +#, elixir-autogen, elixir-format +msgid "Only sign-in via Single Sign-On (SSO) is allowed." +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index d01da60..21314a5 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -3895,3 +3895,13 @@ msgstr "Role %{name}" #, elixir-autogen, elixir-format msgid "User %{email}" msgstr "User %{email}" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "Only OIDC sign-in is active. This option is disabled." +msgstr "" + +#: lib/mv_web/controllers/auth_controller.ex +#, elixir-autogen, elixir-format +msgid "Only sign-in via Single Sign-On (SSO) is allowed." +msgstr "" diff --git a/test/accounts/user_authentication_test.exs b/test/accounts/user_authentication_test.exs index 1b47165..b759e8a 100644 --- a/test/accounts/user_authentication_test.exs +++ b/test/accounts/user_authentication_test.exs @@ -288,4 +288,31 @@ defmodule Mv.Accounts.UserAuthenticationTest do end end end + + describe "register_with_password when OIDC-only is enabled" do + alias Mv.Membership + + test "returns error when OIDC-only is enabled" do + {:ok, settings} = Membership.get_settings() + original_oidc_only = Map.get(settings, :oidc_only, false) + {:ok, _} = Membership.update_settings(settings, %{oidc_only: true}) + + try do + attrs = %{ + email: "newuser#{System.unique_integer([:positive])}@example.com", + password: "SecurePassword123" + } + + result = + Mv.Accounts.User + |> Ash.Changeset.for_create(:register_with_password, attrs) + |> Ash.create() + + assert {:error, _} = result + after + {:ok, s} = Membership.get_settings() + Membership.update_settings(s, %{oidc_only: original_oidc_only}) + end + end + end end diff --git a/test/mv_web/controllers/auth_controller_test.exs b/test/mv_web/controllers/auth_controller_test.exs index e449284..f1fbd76 100644 --- a/test/mv_web/controllers/auth_controller_test.exs +++ b/test/mv_web/controllers/auth_controller_test.exs @@ -283,6 +283,141 @@ defmodule MvWeb.AuthControllerTest do assert to =~ "/auth/user/password/sign_in_with_token" end + describe "when OIDC-only is enabled" do + setup %{conn: authenticated_conn} do + {:ok, settings} = Membership.get_settings() + original_oidc_only = Map.get(settings, :oidc_only, false) + {:ok, _} = Membership.update_settings(settings, %{oidc_only: true}) + + conn = build_unauthenticated_conn(authenticated_conn) + {:ok, conn: conn, original_oidc_only: original_oidc_only} + end + + test "password sign-in is rejected and redirects to sign-in with error", %{ + conn: conn, + original_oidc_only: original + } do + try do + _user = + create_test_user(%{ + email: "password@example.com", + password: "secret123", + oidc_id: nil + }) + + {:ok, view, _html} = live(conn, "/sign-in") + + result = + view + |> form("#user-password-sign-in-with-password", + user: %{email: "password@example.com", password: "secret123"} + ) + |> render_submit() + + # When OIDC-only is enabled, password sign-in must not succeed (no redirect to sign_in_with_token). + case result do + {:error, {:redirect, %{to: to}}} -> + refute to =~ "sign_in_with_token", + "Expected password sign-in to be rejected when OIDC-only, got redirect to: #{to}" + + _ -> + # LiveView re-rendered (e.g. with flash error) instead of redirecting to success + :ok + end + after + {:ok, s} = Membership.get_settings() + Membership.update_settings(s, %{oidc_only: original}) + end + end + end + + describe "GET /sign-in when OIDC-only" do + test "redirects to OIDC flow when OIDC-only and OIDC are configured", %{ + conn: authenticated_conn + } do + {:ok, settings} = Membership.get_settings() + + prev = %{ + oidc_only: settings.oidc_only, + oidc_client_id: settings.oidc_client_id, + oidc_base_url: settings.oidc_base_url, + oidc_redirect_uri: settings.oidc_redirect_uri + } + + {:ok, _} = + Membership.update_settings(settings, %{ + oidc_only: true, + oidc_client_id: "test-client", + oidc_base_url: "https://idp.example.com", + oidc_redirect_uri: "http://localhost:4000/auth/user/oidc/callback", + oidc_client_secret: "test-secret" + }) + + try do + conn = build_unauthenticated_conn(authenticated_conn) + conn = get(conn, ~p"/sign-in") + assert redirected_to(conn) =~ "/auth/user/oidc" + after + {:ok, s} = Membership.get_settings() + Membership.update_settings(s, prev) + end + end + + test "returns 200 when OIDC-only but oidc_failed=1 (avoids redirect loop)", %{ + conn: authenticated_conn + } do + {:ok, settings} = Membership.get_settings() + + prev = %{ + oidc_only: settings.oidc_only, + oidc_client_id: settings.oidc_client_id, + oidc_base_url: settings.oidc_base_url, + oidc_redirect_uri: settings.oidc_redirect_uri + } + + {:ok, _} = + Membership.update_settings(settings, %{ + oidc_only: true, + oidc_client_id: "test-client", + oidc_base_url: "https://idp.example.com", + oidc_redirect_uri: "http://localhost:4000/auth/user/oidc/callback", + oidc_client_secret: "test-secret" + }) + + try do + conn = build_unauthenticated_conn(authenticated_conn) + conn = get(conn, "/sign-in?oidc_failed=1") + assert conn.status == 200 + # Sign-in page is shown, not redirect to OIDC + assert conn.resp_body =~ "Sign in" or conn.resp_body =~ "sign-in" + after + {:ok, s} = Membership.get_settings() + Membership.update_settings(s, prev) + end + end + + test "returns 200 when OIDC-only but OIDC not configured", %{conn: authenticated_conn} do + {:ok, settings} = Membership.get_settings() + original_oidc_only = Map.get(settings, :oidc_only, false) + {:ok, _} = Membership.update_settings(settings, %{oidc_only: true}) + + try do + conn = build_unauthenticated_conn(authenticated_conn) + conn = get(conn, ~p"/sign-in") + assert conn.status == 200 + after + {:ok, s} = Membership.get_settings() + Membership.update_settings(s, %{oidc_only: original_oidc_only}) + end + end + + test "returns 200 when OIDC-only is disabled", %{conn: authenticated_conn} do + conn = build_unauthenticated_conn(authenticated_conn) + conn = get(conn, ~p"/sign-in") + assert conn.status == 200 + end + end + # OIDC/Rauthy error handling tests describe "handle_oidc_failure/2" do test "Assent.ServerUnreachableError redirects to sign-in with error flash", %{ @@ -298,7 +433,7 @@ defmodule MvWeb.AuthControllerTest do conn = MvWeb.AuthController.failure(conn, {:oidc, :callback}, error) - assert redirected_to(conn) == ~p"/sign-in" + assert redirected_to(conn) == "/sign-in?oidc_failed=1" assert Phoenix.Flash.get(conn.assigns.flash, :error) == "The authentication server is currently unavailable. Please try again later." @@ -320,7 +455,7 @@ defmodule MvWeb.AuthControllerTest do conn = MvWeb.AuthController.failure(conn, {:oidc, :callback}, error) - assert redirected_to(conn) == ~p"/sign-in" + assert redirected_to(conn) == "/sign-in?oidc_failed=1" assert Phoenix.Flash.get(conn.assigns.flash, :error) == "Authentication configuration error. Please contact the administrator." @@ -334,7 +469,7 @@ defmodule MvWeb.AuthControllerTest do conn = MvWeb.AuthController.failure(conn, {:oidc, :callback}, unknown_reason) - assert redirected_to(conn) == ~p"/sign-in" + assert redirected_to(conn) == "/sign-in?oidc_failed=1" assert Phoenix.Flash.get(conn.assigns.flash, :error) == "Unable to authenticate with OIDC. Please try again." diff --git a/test/mv_web/live/global_settings_live_test.exs b/test/mv_web/live/global_settings_live_test.exs index e48c44b..92da11b 100644 --- a/test/mv_web/live/global_settings_live_test.exs +++ b/test/mv_web/live/global_settings_live_test.exs @@ -110,4 +110,69 @@ defmodule MvWeb.GlobalSettingsLiveTest do assert html =~ "SMTP" or html =~ "E-Mail" or html =~ "Settings" end end + + describe "Authentication section when OIDC-only is enabled" do + setup %{conn: conn} do + user = create_test_user(%{email: "admin@example.com"}) + conn = conn_with_oidc_user(conn, user) + {:ok, settings} = Membership.get_settings() + original_oidc_only = Map.get(settings, :oidc_only, false) + {:ok, _} = Membership.update_settings(settings, %{oidc_only: true}) + {:ok, conn: conn, original_oidc_only: original_oidc_only} + end + + @describetag :ui + test "registration checkbox is disabled when OIDC-only is enabled", %{ + conn: conn, + original_oidc_only: original + } do + try do + {:ok, view, _html} = live(conn, ~p"/settings") + assert has_element?(view, "#registration-enabled-checkbox[disabled]") + after + {:ok, s} = Membership.get_settings() + Membership.update_settings(s, %{oidc_only: original}) + end + end + + @describetag :ui + test "OIDC-only hint is visible when OIDC-only is enabled", %{ + conn: conn, + original_oidc_only: original + } do + try do + {:ok, view, _html} = live(conn, ~p"/settings") + assert has_element?(view, "[data-testid='oidc-only-registration-hint']") + after + {:ok, s} = Membership.get_settings() + Membership.update_settings(s, %{oidc_only: original}) + end + end + + test "when OIDC-only is disabled, registration checkbox is enabled and can be toggled", %{ + conn: conn, + original_oidc_only: original + } do + try do + {:ok, settings} = Membership.get_settings() + Membership.update_settings(settings, %{oidc_only: false}) + + {:ok, view, _html} = live(conn, ~p"/settings") + refute has_element?(view, "#registration-enabled-checkbox[disabled]") + + initial_checked = + view |> element("#registration-enabled-checkbox") |> render() =~ "checked" + + view + |> element("#registration-enabled-checkbox") + |> render_click() + + new_checked = view |> element("#registration-enabled-checkbox") |> render() =~ "checked" + assert new_checked != initial_checked + after + {:ok, s} = Membership.get_settings() + Membership.update_settings(s, %{oidc_only: original}) + end + end + end end diff --git a/test/mv_web/plugs/check_page_permission_test.exs b/test/mv_web/plugs/check_page_permission_test.exs index 2798161..76bed74 100644 --- a/test/mv_web/plugs/check_page_permission_test.exs +++ b/test/mv_web/plugs/check_page_permission_test.exs @@ -181,13 +181,14 @@ defmodule MvWeb.Plugs.CheckPagePermissionTest do end describe "unauthenticated user" do - test "nil current_user is denied and redirected to \"/sign-in\"" do + test "nil current_user is denied and redirected to \"/sign-in\" without access-denied flash" do conn = conn_without_user("/members") |> CheckPagePermission.call([]) assert conn.halted assert redirected_to(conn) == "/sign-in" - assert Phoenix.Flash.get(conn.assigns[:flash] || %{}, :error) == + # Unauthenticated users are redirected to sign-in only; no "no permission" message. + refute Phoenix.Flash.get(conn.assigns[:flash] || %{}, :error) == "You don't have permission to access this page." end end From f8a3cc4c47cdb077d8a9f9d2a641af0c52b18a93 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 16 Mar 2026 19:27:31 +0100 Subject: [PATCH 5/5] Run seeds only once (#475) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description of the implemented changes The changes were: - [ ] Bugfixing - [x] New Feature - [ ] Breaking Change - [x] Refactoring **Seeds run only on first startup.** On every application start (e.g. `just run`, Docker entrypoint), seed scripts are still invoked, but they exit immediately when the admin user already exists. This avoids duplicate seed data (e.g. join requests), keeps startup fast after the first run, and works the same in dev and production. ## What has been changed? - **`lib/mv/release.ex`** - Added `bootstrap_seeds_applied?/0`: returns whether the admin user (from `ADMIN_EMAIL` or default `admin@localhost`) exists. We check the admin *user*, not the Admin *role*, so we do not skip when only migrations have run (migrations can create the Admin role for the system actor). - `run_seeds/0`: if `bootstrap_seeds_applied?()` is true, prints “Seeds already applied (admin user exists). Skipping.” and returns without running bootstrap or dev seeds; otherwise unchanged behaviour. - Module docs updated for the new function and the skip behaviour. - **`priv/repo/seeds.exs`** - Ensures the app is started (`Application.ensure_all_started(:mv)`). - If `Mv.Release.bootstrap_seeds_applied?()` is true, prints the same skip message and does not run bootstrap or dev seeds; otherwise runs as before (bootstrap + dev seeds in dev/test). - Comment at the top updated to describe the skip behaviour. - **Documentation** - `CODE_GUIDELINES.md` §1.2.1: seeds run on every start but exit early when already applied; mentions `bootstrap_seeds_applied?/0`. - `docs/admin-bootstrap-and-oidc-role-sync.md`: run_seeds skips when admin user exists; description of `run_seeds/0` updated. - `CHANGELOG.md` [Unreleased]: new “Seeds run only when needed” entry under Changed. ## Definition of Done ### Code Quality - [x] No new technical depths - [x] Linting passed - [x] Documentation is added where needed ### Accessibility - [x] New elements are properly defined with html-tags *(no new UI)* - [x] Colour contrast follows WCAG criteria *(no new UI)* - [x] Aria labels are added when needed *(no new UI)* - [x] Everything is accessible by keyboard *(no new UI)* - [x] Tab-Order is comprehensible *(no new UI)* - [x] All interactive elements have a visible focus *(no new UI)* ### Testing - [x] Tests for new code are written *(existing seeds and release tests cover behaviour; idempotency test still passes when second run skips)* - [x] All tests pass - [x] axe-core dev tools show no critical or major issues *(no UI changes)* ## Additional Notes - **Review focus:** Logic in `Mv.Release` and `priv/repo/seeds.exs`; the “already applied” check is a single DB read for the admin user. On failure (e.g. DB down), `bootstrap_seeds_applied?/0` returns `false`, so seeds run (safe for first deploy). - **Suggested check:** Run `mix test test/seeds_test.exs test/mv/release_test.exs` to confirm seeds and release behaviour. Reviewed-on: https://git.local-it.org/local-it/mitgliederverwaltung/pulls/475 Co-authored-by: Simon Co-committed-by: Simon --- .env.example | 1 + CHANGELOG.md | 4 +- CODE_GUIDELINES.md | 6 +- docs/admin-bootstrap-and-oidc-role-sync.md | 5 +- lib/mv/release.ex | 65 ++++++++++++++++------ priv/repo/seeds.exs | 34 ++++++----- 6 files changed, 78 insertions(+), 37 deletions(-) diff --git a/.env.example b/.env.example index 661593b..d63e019 100644 --- a/.env.example +++ b/.env.example @@ -14,6 +14,7 @@ ASSOCIATION_NAME="Sportsclub XYZ" # Optional: Admin user (created/updated on container start via Release.seed_admin) # In production, set these so the first admin can log in. Change password without redeploy: # bin/mv eval "Mv.Release.seed_admin()" (with new ADMIN_PASSWORD or ADMIN_PASSWORD_FILE) +# FORCE_SEEDS=true re-runs bootstrap seeds even when admin user exists (e.g. after changing roles/custom fields). # ADMIN_EMAIL=admin@example.com # ADMIN_PASSWORD=secure-password # ADMIN_PASSWORD_FILE=/run/secrets/admin_password diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cc8ea5..b94ce50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,13 +5,15 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [Unreleased] +## [1.1.1] - 2026-03-16 ### Added +- **FORCE_SEEDS** – Environment variable. When set to `"true"`, bootstrap (and optionally dev) seeds are run even when the admin user already exists, so you can re-apply changed seed data (e.g. new roles or custom fields) without deleting the admin user. - **Improved OIDC-only mode** – Admin can enable “Only OIDC sign-in” in settings; when enabled, direct registration is disabled and sign-in page redirects to OIDC when configured. - **Success toast auto-dismiss** – Success flash messages (e.g. “Settings saved”) hide automatically after 5 seconds instead of requiring the user to close them. ### Changed +- **Seeds run only when needed** – Bootstrap and dev seeds are skipped on application start when the admin user already exists (`Mv.Release.bootstrap_seeds_applied?/0`). This avoids duplicate data and speeds up startup in dev and production after the first run. Set `FORCE_SEEDS=true` to override and re-run. - **Unauthenticated access** – Users who are not logged in are redirected to sign-in without showing a “no permission” message; the message is only shown to logged-in users who lack access. ### Fixed diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index c0cb543..f84c5ad 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -284,13 +284,13 @@ end ### 1.2.1 Database Seeds -Seeds are split into **bootstrap** and **dev**: +Seeds are split into **bootstrap** and **dev**. They run on every start (e.g. `just run`, Docker entrypoint) but **exit early** if already applied so startup stays fast and no duplicate data is created. -- **`priv/repo/seeds.exs`** – Entrypoint. Runs `seeds_bootstrap.exs` always; runs `seeds_dev.exs` only when `Mix.env()` is `:dev` or `:test`. +- **`priv/repo/seeds.exs`** – Entrypoint. If the admin user (ADMIN_EMAIL or default) already exists, skips entirely (unless `FORCE_SEEDS=true`); otherwise runs `seeds_bootstrap.exs` and, in dev/test, `seeds_dev.exs`. - **`priv/repo/seeds_bootstrap.exs`** – Creates only data required for system startup: membership fee types, custom fields, roles, admin user, system user, global settings (including default membership fee type). No members, no groups. Used in all environments (dev, test, prod). - **`priv/repo/seeds_dev.exs`** – Creates 20 sample members, groups, and optional custom field values. Run only in dev and test. -In production, running `mix run priv/repo/seeds.exs` executes only the bootstrap part (no dev seeds). +In production, running `mix run priv/repo/seeds.exs` (or `Mv.Release.run_seeds/0`) executes only the bootstrap part when not yet applied (no dev seeds unless `RUN_DEV_SEEDS=true`). The “already applied” check uses `Mv.Release.bootstrap_seeds_applied?/0` (admin user exists). Set `FORCE_SEEDS=true` to re-run seeds even when already applied. ### 1.3 Domain-Driven Design diff --git a/docs/admin-bootstrap-and-oidc-role-sync.md b/docs/admin-bootstrap-and-oidc-role-sync.md index aa5c155..5413f91 100644 --- a/docs/admin-bootstrap-and-oidc-role-sync.md +++ b/docs/admin-bootstrap-and-oidc-role-sync.md @@ -2,7 +2,7 @@ ## Overview -- **Admin bootstrap:** In production, the Docker entrypoint runs migrate, then `Mv.Release.run_seeds/0` (bootstrap seeds; set `RUN_DEV_SEEDS=true` to also run dev seeds), then `seed_admin/0` from ENV, then the server. Password can be changed without redeploy via `bin/mv eval "Mv.Release.seed_admin()"`. +- **Admin bootstrap:** In production, the Docker entrypoint runs migrate, then `Mv.Release.run_seeds/0` (skips if admin user already exists unless `FORCE_SEEDS=true`; set `RUN_DEV_SEEDS=true` to also run dev seeds), then `seed_admin/0` from ENV, then the server. Password can be changed without redeploy via `bin/mv eval "Mv.Release.seed_admin()"`. - **OIDC role sync:** Optional mapping from OIDC groups (e.g. from Authentik profile scope) to the Admin role. Users in the configured admin group get the Admin role on registration and on each sign-in. ## Admin Bootstrap (Part A) @@ -10,13 +10,14 @@ ### Environment Variables - `RUN_DEV_SEEDS` – If set to `"true"`, `run_seeds/0` also runs dev seeds (members, groups, sample data). Otherwise only bootstrap seeds run. +- `FORCE_SEEDS` – If set to `"true"`, seeds are run even when the admin user already exists (e.g. after changing bootstrap data such as roles or custom fields). Otherwise seeds are skipped when bootstrap was already applied. - `ADMIN_EMAIL` – Email of the admin user to create/update. If unset, seed_admin/0 does nothing. - `ADMIN_PASSWORD` – Password for the admin user. If unset (and no file), no new user is created; if a user with ADMIN_EMAIL already exists (e.g. OIDC-only), their role is set to Admin (no password change). - `ADMIN_PASSWORD_FILE` – Path to a file containing the password (e.g. Docker secret). ### Release Tasks -- `Mv.Release.run_seeds/0` – Runs bootstrap seeds (fee types, custom fields, roles, settings). If `RUN_DEV_SEEDS` env is `"true"`, also runs dev seeds (members, groups, sample data). Idempotent. +- `Mv.Release.run_seeds/0` – If the admin user already exists (bootstrap already applied), skips unless `FORCE_SEEDS=true`; otherwise runs bootstrap seeds (fee types, custom fields, roles, settings). If `RUN_DEV_SEEDS` env is `"true"`, also runs dev seeds (members, groups, sample data). Safe to call on every start. - `Mv.Release.seed_admin/0` – Reads ADMIN_EMAIL and password from ADMIN_PASSWORD or ADMIN_PASSWORD_FILE. If both email and password are set: creates or updates the user with the Admin role. If only ADMIN_EMAIL is set: sets the Admin role on an existing user with that email (for OIDC-only admins); does not create a user. Idempotent. ### Entrypoint diff --git a/lib/mv/release.ex b/lib/mv/release.ex index 00dcadf..116b276 100644 --- a/lib/mv/release.ex +++ b/lib/mv/release.ex @@ -6,8 +6,8 @@ defmodule Mv.Release do ## Tasks - `migrate/0` - Runs all pending Ecto migrations. - - `run_seeds/0` - Runs bootstrap seeds (fee types, custom fields, roles, settings). - In production, set `RUN_DEV_SEEDS=true` to also run dev seeds (members, groups, sample data). + - `bootstrap_seeds_applied?/0` - Returns whether bootstrap was already applied (admin user exists). Used to skip re-running seeds. + - `run_seeds/0` - If bootstrap already applied, skips; otherwise runs bootstrap seeds (fee types, custom fields, roles, settings). Set `FORCE_SEEDS=true` to re-run seeds even when already applied. In production, set `RUN_DEV_SEEDS=true` to also run dev seeds (members, groups, sample data). - `seed_admin/0` - Ensures an admin user exists from ENV (ADMIN_EMAIL, ADMIN_PASSWORD or ADMIN_PASSWORD_FILE). Idempotent; can be run on every deployment or via shell to update the admin password without redeploying. @@ -19,6 +19,7 @@ defmodule Mv.Release do alias Mv.Authorization.Role require Ash.Query + require Logger def migrate do load_app() @@ -28,13 +29,37 @@ defmodule Mv.Release do end end + @doc """ + Returns whether bootstrap seeds have already been applied (admin user exists). + + We check for the admin user (from ADMIN_EMAIL or default), not the Admin role, + because migrations may create the Admin role for the system actor. Only seeds + create the admin (login) user. Used to skip re-running seeds on subsequent starts. + Call only when the application is already started. + """ + def bootstrap_seeds_applied? do + admin_email = get_env("ADMIN_EMAIL", "admin@localhost") + + case User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(authorize?: false, domain: Mv.Accounts) do + {:ok, %User{}} -> true + _ -> false + end + rescue + e -> + Logger.warning("Could not check seed status (#{inspect(e)}), assuming not applied.") + false + end + @doc """ Runs seed scripts so the database has required bootstrap data (and optionally dev data). - - Always runs bootstrap seeds (fee types, custom fields, roles, system user, settings). - - If `RUN_DEV_SEEDS` env is set to `"true"`, also runs dev seeds (members, groups, sample data). + - Skips if bootstrap was already applied (admin user exists); set `FORCE_SEEDS=true` to override and re-run. + - If `RUN_DEV_SEEDS` env is set to `"true"`, also runs dev seeds (members, groups, sample data) + when bootstrap is run. - Uses paths from the application's priv dir so it works in releases (no Mix). Idempotent. + Uses paths from the application's priv dir so it works in releases (no Mix). """ def run_seeds do case Application.ensure_all_started(@app) do @@ -42,23 +67,27 @@ defmodule Mv.Release do {:error, {app, reason}} -> raise "Failed to start #{inspect(app)}: #{inspect(reason)}" end - priv = :code.priv_dir(@app) - bootstrap_path = Path.join(priv, "repo/seeds_bootstrap.exs") - dev_path = Path.join(priv, "repo/seeds_dev.exs") + if bootstrap_seeds_applied?() and System.get_env("FORCE_SEEDS") != "true" do + IO.puts("Seeds already applied. Skipping. (Set FORCE_SEEDS=true to override)") + else + priv = :code.priv_dir(@app) + bootstrap_path = Path.join(priv, "repo/seeds_bootstrap.exs") + dev_path = Path.join(priv, "repo/seeds_dev.exs") - prev = Code.compiler_options() - Code.compiler_options(ignore_module_conflict: true) + prev = Code.compiler_options() + Code.compiler_options(ignore_module_conflict: true) - try do - Code.eval_file(bootstrap_path) - IO.puts("✅ Bootstrap seeds completed.") + try do + Code.eval_file(bootstrap_path) + IO.puts("✅ Bootstrap seeds completed.") - if System.get_env("RUN_DEV_SEEDS") == "true" do - Code.eval_file(dev_path) - IO.puts("✅ Dev seeds completed.") + if System.get_env("RUN_DEV_SEEDS") == "true" do + Code.eval_file(dev_path) + IO.puts("✅ Dev seeds completed.") + end + after + Code.compiler_options(prev) end - after - Code.compiler_options(prev) end end diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index 7257f8b..c562a43 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -3,7 +3,9 @@ # mix run priv/repo/seeds.exs # # Bootstrap runs in all environments. Dev seeds (members, groups, sample data) -# run only in dev and test. +# run only in dev and test. Skips entirely if bootstrap was already applied +# (admin user exists), so safe to run on every start. Set FORCE_SEEDS=true to +# re-run seeds even when already applied. # # In production (release): seeds are run via Mv.Release.run_seeds/0 from the # container entrypoint. Set RUN_DEV_SEEDS=true to also run dev seeds there. @@ -12,19 +14,25 @@ # so that eval_file of bootstrap/dev does not emit "redefining module" warnings; # it is always restored in `after` to avoid hiding real conflicts elsewhere. -prev = Code.compiler_options() -Code.compiler_options(ignore_module_conflict: true) +_ = Application.ensure_all_started(:mv) -try do - # Always run bootstrap (fee types, custom fields, roles, admin, system user, settings) - Code.eval_file("priv/repo/seeds_bootstrap.exs") +if Mv.Release.bootstrap_seeds_applied?() and System.get_env("FORCE_SEEDS") != "true" do + IO.puts("Seeds already applied. Skipping. (Set FORCE_SEEDS=true to override)") +else + prev = Code.compiler_options() + Code.compiler_options(ignore_module_conflict: true) - # In dev and test only: run dev seeds (20 members, groups, custom field values) - if Mix.env() in [:dev, :test] do - Code.eval_file("priv/repo/seeds_dev.exs") + try do + # Always run bootstrap (fee types, custom fields, roles, admin, system user, settings) + Code.eval_file("priv/repo/seeds_bootstrap.exs") + + # In dev and test only: run dev seeds (20 members, groups, custom field values) + if Mix.env() in [:dev, :test] do + Code.eval_file("priv/repo/seeds_dev.exs") + end + + IO.puts("✅ All seeds completed.") + after + Code.compiler_options(prev) end - - IO.puts("✅ All seeds completed.") -after - Code.compiler_options(prev) end