From 25f3b19f5090a675e7c66cbf8dd4f7492ba60e5d Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 16 Mar 2026 19:00:11 +0100 Subject: [PATCH] feat: add oidc cycle breaker --- docs/admin-bootstrap-and-oidc-role-sync.md | 1 + lib/mv_web/controllers/auth_controller.ex | 16 +++++--- lib/mv_web/live/global_settings_live.ex | 12 +++--- .../plugs/oidc_only_sign_in_redirect.ex | 30 +++++++++----- priv/gettext/de/LC_MESSAGES/default.po | 5 --- priv/gettext/en/LC_MESSAGES/default.po | 5 --- .../controllers/auth_controller_test.exs | 39 +++++++++++++++++-- 7 files changed, 74 insertions(+), 34 deletions(-) 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/lib/mv_web/controllers/auth_controller.ex b/lib/mv_web/controllers/auth_controller.ex index d279163..adde4e8 100644 --- a/lib/mv_web/controllers/auth_controller.ex +++ b/lib/mv_web/controllers/auth_controller.ex @@ -21,7 +21,7 @@ defmodule MvWeb.AuthController do if Config.oidc_only?() do conn |> put_flash(:error, gettext("Only sign-in via Single Sign-On (SSO) is allowed.")) - |> redirect(to: ~p"/sign-in") + |> redirect(to: sign_in_path_after_oidc_failure()) else success_continue(conn, {:password, :sign_in}, user, token) end @@ -149,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 @@ -163,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) @@ -176,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 @@ -186,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 @@ -226,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 25a8942..cb57631 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -900,17 +900,17 @@ 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?()) diff --git a/lib/mv_web/plugs/oidc_only_sign_in_redirect.ex b/lib/mv_web/plugs/oidc_only_sign_in_redirect.ex index 81a81b1..9cf2a16 100644 --- a/lib/mv_web/plugs/oidc_only_sign_in_redirect.ex +++ b/lib/mv_web/plugs/oidc_only_sign_in_redirect.ex @@ -2,6 +2,8 @@ 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. """ @@ -19,19 +21,29 @@ defmodule MvWeb.Plugs.OidcOnlySignInRedirect do end defp maybe_redirect_sign_in_to_oidc(conn) do - if conn.request_path == "/sign-in" and conn.method == "GET" do - if Config.oidc_only?() and Config.oidc_configured?() do - conn - |> redirect(to: "/auth/user/oidc") - |> halt() - else - conn - end - else + 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 diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 3d4ce2d..383fb1c 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -3905,8 +3905,3 @@ msgstr "Nur OIDC-Anmeldung ist aktiv. Diese Option ist deaktiviert." #, 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." - -#~ #: lib/accounts/user/validations/oidc_only_blocks_password_registration.ex -#~ #, elixir-autogen, elixir-format -#~ msgid "Registration with password is disabled when only OIDC sign-in is active." -#~ msgstr "Registrierung mit Passwort ist deaktiviert, wenn nur OIDC-Anmeldung aktiv ist." diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 8b6da9c..21314a5 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -3905,8 +3905,3 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Only sign-in via Single Sign-On (SSO) is allowed." msgstr "" - -#~ #: lib/accounts/user/validations/oidc_only_blocks_password_registration.ex -#~ #, elixir-autogen, elixir-format -#~ msgid "Registration with password is disabled when only OIDC sign-in is active." -#~ msgstr "Registration with password is disabled when only OIDC sign-in is active." diff --git a/test/mv_web/controllers/auth_controller_test.exs b/test/mv_web/controllers/auth_controller_test.exs index ffab450..f1fbd76 100644 --- a/test/mv_web/controllers/auth_controller_test.exs +++ b/test/mv_web/controllers/auth_controller_test.exs @@ -363,6 +363,39 @@ defmodule MvWeb.AuthControllerTest do 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) @@ -400,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." @@ -422,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." @@ -436,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."