From a25263b7219104dec2b01e434969ccf69a62df82 Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 17 Feb 2026 19:29:49 +0100 Subject: [PATCH 1/4] fix: adds user friendly flas message --- lib/mv_web/auth_overrides.ex | 7 ++ lib/mv_web/components/layouts/root.html.heex | 1 + lib/mv_web/controllers/auth_controller.ex | 93 ++++++++++++++++--- priv/gettext/de/LC_MESSAGES/default.po | 10 ++ priv/gettext/default.pot | 10 ++ priv/gettext/en/LC_MESSAGES/default.po | 10 ++ .../controllers/auth_controller_test.exs | 44 +++++++++ 7 files changed, 163 insertions(+), 12 deletions(-) diff --git a/lib/mv_web/auth_overrides.ex b/lib/mv_web/auth_overrides.ex index 1367150..b121c4e 100644 --- a/lib/mv_web/auth_overrides.ex +++ b/lib/mv_web/auth_overrides.ex @@ -45,4 +45,11 @@ defmodule MvWeb.AuthOverrides do Gettext.gettext(MvWeb.Gettext, "or") end) end + + # Hide AshAuthentication's Flash component since we use flash_group in root layout + # This prevents duplicate flash messages + override AshAuthentication.Phoenix.Components.Flash do + set :message_class_info, "hidden" + set :message_class_error, "hidden" + end end diff --git a/lib/mv_web/components/layouts/root.html.heex b/lib/mv_web/components/layouts/root.html.heex index 35e73ab..f6c3014 100644 --- a/lib/mv_web/components/layouts/root.html.heex +++ b/lib/mv_web/components/layouts/root.html.heex @@ -33,6 +33,7 @@ + <.flash_group flash={@flash} /> {@inner_content} diff --git a/lib/mv_web/controllers/auth_controller.ex b/lib/mv_web/controllers/auth_controller.ex index 20a8b20..8249a10 100644 --- a/lib/mv_web/controllers/auth_controller.ex +++ b/lib/mv_web/controllers/auth_controller.ex @@ -57,7 +57,9 @@ defmodule MvWeb.AuthController do handle_authentication_failed(conn, caused_by) _ -> - redirect_with_error(conn, gettext("Incorrect email or password")) + conn + |> put_flash(:error, gettext("Incorrect email or password")) + |> redirect(to: ~p"/sign-in") end end @@ -74,14 +76,39 @@ defmodule MvWeb.AuthController do handle_oidc_email_collision(conn, errors) _ -> - redirect_with_error(conn, gettext("Unable to authenticate with OIDC. Please try again.")) + conn + |> put_flash(:error, gettext("Unable to authenticate with OIDC. Please try again.")) + |> redirect(to: ~p"/sign-in") end end + # Handle Assent server unreachable errors (network/connectivity issues) + defp handle_rauthy_failure(conn, %Assent.ServerUnreachableError{} = err) do + # Use warning level: server unreachable is often transient, not a critical system error + Logger.warning("OIDC authentication server unreachable", safe_assent_meta(err)) + + conn + |> put_flash(:error, gettext("The authentication server is currently unavailable. Please try again later.")) + |> redirect(to: ~p"/sign-in") + end + + # Handle Assent invalid response errors (configuration or malformed responses) + defp handle_rauthy_failure(conn, %Assent.InvalidResponseError{} = err) do + # Use warning level: configuration errors are operational issues, not critical failures + Logger.warning("OIDC authentication invalid response", safe_assent_meta(err)) + + conn + |> put_flash(:error, gettext("Authentication configuration error. Please contact the administrator.")) + |> redirect(to: ~p"/sign-in") + end + # Catch-all clause for any other error types defp handle_rauthy_failure(conn, reason) do Logger.warning("Unhandled Rauthy failure reason: #{inspect(reason)}") - redirect_with_error(conn, gettext("Unable to authenticate with OIDC. Please try again.")) + + conn + |> put_flash(:error, gettext("Unable to authenticate with OIDC. Please try again.")) + |> redirect(to: ~p"/sign-in") end # Handle generic AuthenticationFailed errors @@ -93,14 +120,20 @@ defmodule MvWeb.AuthController do You can confirm your account using the link we sent to you, or by resetting your password. """) - redirect_with_error(conn, message) + conn + |> put_flash(:error, message) + |> redirect(to: ~p"/sign-in") else - redirect_with_error(conn, gettext("Authentication failed. Please try again.")) + conn + |> put_flash(:error, gettext("Authentication failed. Please try again.")) + |> redirect(to: ~p"/sign-in") end end defp handle_authentication_failed(conn, _other) do - redirect_with_error(conn, gettext("Authentication failed. Please try again.")) + conn + |> put_flash(:error, gettext("Authentication failed. Please try again.")) + |> redirect(to: ~p"/sign-in") end # Handle OIDC email collision - user needs to verify password to link accounts @@ -112,7 +145,10 @@ defmodule MvWeb.AuthController do nil -> # Check if it's a "different OIDC account" error or email uniqueness error error_message = extract_meaningful_error_message(errors) - redirect_with_error(conn, error_message) + + conn + |> put_flash(:error, error_message) + |> redirect(to: ~p"/sign-in") end end @@ -177,13 +213,46 @@ defmodule MvWeb.AuthController do |> redirect(to: ~p"/auth/link-oidc-account") end - # Generic error redirect helper - defp redirect_with_error(conn, message) do - conn - |> put_flash(:error, message) - |> redirect(to: ~p"/sign-in") + # Extract safe metadata from Assent errors for logging + # Never logs sensitive data: no tokens, secrets, or full request URLs + # Returns keyword list for Logger.warning/2 + defp safe_assent_meta(%{request_url: url} = err) when is_binary(url) do + [ + request_url: redact_url(url), + http_adapter: Map.get(err, :http_adapter) + ] + |> Enum.filter(fn {_key, value} -> not is_nil(value) end) end + defp safe_assent_meta(%{response: %{status_code: status_code}} = err) do + [ + status_code: status_code, + http_adapter: Map.get(err, :http_adapter) + ] + |> Enum.filter(fn {_key, value} -> not is_nil(value) end) + end + + defp safe_assent_meta(err) do + # Only extract safe, simple fields + [ + http_adapter: Map.get(err, :http_adapter) + ] + |> Enum.filter(fn {_key, value} -> not is_nil(value) end) + end + + # Redact URL to only show scheme and host, hiding path, query, and fragments + defp redact_url(url) when is_binary(url) do + case URI.parse(url) do + %URI{scheme: scheme, host: host} when not is_nil(scheme) and not is_nil(host) -> + "#{scheme}://#{host}" + + _ -> + "[redacted]" + end + end + + defp redact_url(_), do: "[redacted]" + def sign_out(conn, _params) do return_to = get_session(conn, :return_to) || ~p"/" diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 0187da6..527a849 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -582,6 +582,16 @@ msgstr "Ein Konto mit dieser E-Mail existiert bereits. Bitte verifiziere dein Pa msgid "Unable to authenticate with OIDC. Please try again." msgstr "OIDC-Authentifizierung fehlgeschlagen. Bitte versuche es erneut." +#: lib/mv_web/controllers/auth_controller.ex +#, elixir-autogen, elixir-format +msgid "The authentication server is currently unavailable. Please try again later." +msgstr "Der Authentifizierungsserver ist derzeit nicht erreichbar. Bitte versuche es später erneut." + +#: lib/mv_web/controllers/auth_controller.ex +#, elixir-autogen, elixir-format +msgid "Authentication configuration error. Please contact the administrator." +msgstr "Authentifizierungskonfigurationsfehler. Bitte kontaktiere den Administrator." + #: lib/mv_web/controllers/auth_controller.ex #, elixir-autogen, elixir-format msgid "Unable to sign in. Please try again." diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index a05597c..8247f31 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -583,6 +583,16 @@ msgstr "" msgid "Unable to authenticate with OIDC. Please try again." msgstr "" +#: lib/mv_web/controllers/auth_controller.ex +#, elixir-autogen, elixir-format +msgid "The authentication server is currently unavailable. Please try again later." +msgstr "" + +#: lib/mv_web/controllers/auth_controller.ex +#, elixir-autogen, elixir-format +msgid "Authentication configuration error. Please contact the administrator." +msgstr "" + #: lib/mv_web/controllers/auth_controller.ex #, elixir-autogen, elixir-format msgid "Unable to sign in. Please try again." diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 261cbe4..bff719d 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -583,6 +583,16 @@ msgstr "" msgid "Unable to authenticate with OIDC. Please try again." msgstr "" +#: lib/mv_web/controllers/auth_controller.ex +#, elixir-autogen, elixir-format +msgid "The authentication server is currently unavailable. Please try again later." +msgstr "" + +#: lib/mv_web/controllers/auth_controller.ex +#, elixir-autogen, elixir-format +msgid "Authentication configuration error. Please contact the administrator." +msgstr "" + #: lib/mv_web/controllers/auth_controller.ex #, elixir-autogen, elixir-format msgid "Unable to sign in. Please try again." diff --git a/test/mv_web/controllers/auth_controller_test.exs b/test/mv_web/controllers/auth_controller_test.exs index 444571b..c177c96 100644 --- a/test/mv_web/controllers/auth_controller_test.exs +++ b/test/mv_web/controllers/auth_controller_test.exs @@ -248,4 +248,48 @@ defmodule MvWeb.AuthControllerTest do assert to =~ "/auth/user/password/sign_in_with_token" end + + # OIDC/Rauthy error handling tests + describe "handle_rauthy_failure/2" do + test "Assent.ServerUnreachableError redirects to sign-in with error flash", %{ + conn: authenticated_conn + } do + conn = build_unauthenticated_conn(authenticated_conn) + # Create a mock Assent.ServerUnreachableError struct + error = %Assent.ServerUnreachableError{request_url: "https://auth.example.com/callback?token=secret123"} + + conn = MvWeb.AuthController.failure(conn, {:rauthy, :callback}, error) + + assert redirected_to(conn) == ~p"/sign-in" + assert get_flash(conn, :error) == "The authentication server is currently unavailable. Please try again later." + end + + test "Assent.InvalidResponseError redirects to sign-in with error flash", %{ + conn: authenticated_conn + } do + conn = build_unauthenticated_conn(authenticated_conn) + # Create a mock Assent.InvalidResponseError struct + error = %Assent.InvalidResponseError{ + response: %{status_code: 400, body: "invalid_request"}, + request_url: "https://auth.example.com/callback" + } + + conn = MvWeb.AuthController.failure(conn, {:rauthy, :callback}, error) + + assert redirected_to(conn) == ~p"/sign-in" + assert get_flash(conn, :error) == "Authentication configuration error. Please contact the administrator." + end + + test "unknown reason triggers catch-all and redirects to sign-in with error flash", %{ + conn: authenticated_conn + } do + conn = build_unauthenticated_conn(authenticated_conn) + unknown_reason = :oops + + conn = MvWeb.AuthController.failure(conn, {:rauthy, :callback}, unknown_reason) + + assert redirected_to(conn) == ~p"/sign-in" + assert get_flash(conn, :error) == "Unable to authenticate with OIDC. Please try again." + end + end end -- 2.47.2 From 002d723d0ea82831b2c84e1cd507f6bc18745b3d Mon Sep 17 00:00:00 2001 From: carla Date: Wed, 18 Feb 2026 12:53:25 +0100 Subject: [PATCH 2/4] fix: tests and flash layout --- lib/mv_web/components/layouts/root.html.heex | 39 ++++++++++++++++++- lib/mv_web/controllers/auth_controller.ex | 17 +++++--- priv/gettext/de/LC_MESSAGES/default.po | 3 ++ priv/gettext/default.pot | 3 ++ priv/gettext/en/LC_MESSAGES/default.po | 3 ++ .../controllers/auth_controller_test.exs | 35 ++++++++++++----- 6 files changed, 85 insertions(+), 15 deletions(-) diff --git a/lib/mv_web/components/layouts/root.html.heex b/lib/mv_web/components/layouts/root.html.heex index f6c3014..3ba099d 100644 --- a/lib/mv_web/components/layouts/root.html.heex +++ b/lib/mv_web/components/layouts/root.html.heex @@ -33,7 +33,44 @@ - <.flash_group flash={@flash} /> +
+ <.flash id="flash-success-root" kind={:success} flash={@flash} /> + <.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} /> + + <.flash + id="client-error-root" + kind={:error} + title={gettext("We can't find the internet")} + phx-disconnected={ + show(".phx-client-error #client-error-root") |> JS.remove_attribute("hidden") + } + phx-connected={hide("#client-error-root") |> JS.set_attribute({"hidden", ""})} + hidden + > + {gettext("Attempting to reconnect")} + <.icon name="hero-arrow-path" class="ml-1 size-3 motion-safe:animate-spin" /> + + + <.flash + id="server-error-root" + kind={:error} + title={gettext("Something went wrong!")} + phx-disconnected={ + show(".phx-server-error #server-error-root") |> JS.remove_attribute("hidden") + } + phx-connected={hide("#server-error-root") |> JS.set_attribute({"hidden", ""})} + hidden + > + {gettext("Attempting to reconnect")} + <.icon name="hero-arrow-path" class="ml-1 size-3 motion-safe:animate-spin" /> + +
{@inner_content} diff --git a/lib/mv_web/controllers/auth_controller.ex b/lib/mv_web/controllers/auth_controller.ex index 8249a10..24d1078 100644 --- a/lib/mv_web/controllers/auth_controller.ex +++ b/lib/mv_web/controllers/auth_controller.ex @@ -88,7 +88,10 @@ defmodule MvWeb.AuthController do Logger.warning("OIDC authentication server unreachable", safe_assent_meta(err)) conn - |> put_flash(:error, gettext("The authentication server is currently unavailable. Please try again later.")) + |> put_flash( + :error, + gettext("The authentication server is currently unavailable. Please try again later.") + ) |> redirect(to: ~p"/sign-in") end @@ -98,7 +101,10 @@ defmodule MvWeb.AuthController do Logger.warning("OIDC authentication invalid response", safe_assent_meta(err)) conn - |> put_flash(:error, gettext("Authentication configuration error. Please contact the administrator.")) + |> put_flash( + :error, + gettext("Authentication configuration error. Please contact the administrator.") + ) |> redirect(to: ~p"/sign-in") end @@ -224,10 +230,11 @@ defmodule MvWeb.AuthController do |> Enum.filter(fn {_key, value} -> not is_nil(value) end) end - defp safe_assent_meta(%{response: %{status_code: status_code}} = err) do + # Handle InvalidResponseError which has :response field (HTTPResponse struct) + defp safe_assent_meta(%{response: %{status: status} = response} = err) do [ - status_code: status_code, - http_adapter: Map.get(err, :http_adapter) + status: status, + http_adapter: Map.get(response, :http_adapter) || Map.get(err, :http_adapter) ] |> Enum.filter(fn {_key, value} -> not is_nil(value) end) end diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 527a849..9968f16 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -27,6 +27,7 @@ msgid "Are you sure?" msgstr "Bist du sicher?" #: lib/mv_web/components/layouts.ex +#: lib/mv_web/components/layouts/root.html.heex #, elixir-autogen, elixir-format msgid "Attempting to reconnect" msgstr "Verbindung wird wiederhergestellt" @@ -115,11 +116,13 @@ msgid "Show" msgstr "Anzeigen" #: lib/mv_web/components/layouts.ex +#: lib/mv_web/components/layouts/root.html.heex #, elixir-autogen, elixir-format msgid "Something went wrong!" msgstr "Etwas ist schiefgelaufen!" #: lib/mv_web/components/layouts.ex +#: lib/mv_web/components/layouts/root.html.heex #, elixir-autogen, elixir-format msgid "We can't find the internet" msgstr "Keine Internetverbindung gefunden" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 8247f31..a4bf53a 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -28,6 +28,7 @@ msgid "Are you sure?" msgstr "" #: lib/mv_web/components/layouts.ex +#: lib/mv_web/components/layouts/root.html.heex #, elixir-autogen, elixir-format msgid "Attempting to reconnect" msgstr "" @@ -116,11 +117,13 @@ msgid "Show" msgstr "" #: lib/mv_web/components/layouts.ex +#: lib/mv_web/components/layouts/root.html.heex #, elixir-autogen, elixir-format msgid "Something went wrong!" msgstr "" #: lib/mv_web/components/layouts.ex +#: lib/mv_web/components/layouts/root.html.heex #, elixir-autogen, elixir-format msgid "We can't find the internet" msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index bff719d..5ed6eb1 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -28,6 +28,7 @@ msgid "Are you sure?" msgstr "" #: lib/mv_web/components/layouts.ex +#: lib/mv_web/components/layouts/root.html.heex #, elixir-autogen, elixir-format msgid "Attempting to reconnect" msgstr "" @@ -116,11 +117,13 @@ msgid "Show" msgstr "" #: lib/mv_web/components/layouts.ex +#: lib/mv_web/components/layouts/root.html.heex #, elixir-autogen, elixir-format msgid "Something went wrong!" msgstr "" #: lib/mv_web/components/layouts.ex +#: lib/mv_web/components/layouts/root.html.heex #, elixir-autogen, elixir-format msgid "We can't find the internet" msgstr "" diff --git a/test/mv_web/controllers/auth_controller_test.exs b/test/mv_web/controllers/auth_controller_test.exs index c177c96..5f53e0f 100644 --- a/test/mv_web/controllers/auth_controller_test.exs +++ b/test/mv_web/controllers/auth_controller_test.exs @@ -6,7 +6,10 @@ defmodule MvWeb.AuthControllerTest do # Helper to create an unauthenticated conn (preserves sandbox metadata) defp build_unauthenticated_conn(authenticated_conn) do # Create new conn but preserve sandbox metadata for database access - new_conn = build_conn() + new_conn = + build_conn() + |> init_test_session(%{}) + |> fetch_flash() # Copy sandbox metadata from authenticated conn if authenticated_conn.private[:ecto_sandbox] do @@ -255,29 +258,41 @@ defmodule MvWeb.AuthControllerTest do conn: authenticated_conn } do conn = build_unauthenticated_conn(authenticated_conn) - # Create a mock Assent.ServerUnreachableError struct - error = %Assent.ServerUnreachableError{request_url: "https://auth.example.com/callback?token=secret123"} + # Create a mock Assent.ServerUnreachableError struct with required fields + error = %Assent.ServerUnreachableError{ + http_adapter: Assent.HTTPAdapter.Finch, + request_url: "https://auth.example.com/callback?token=secret123", + reason: %Mint.TransportError{reason: :econnrefused} + } conn = MvWeb.AuthController.failure(conn, {:rauthy, :callback}, error) assert redirected_to(conn) == ~p"/sign-in" - assert get_flash(conn, :error) == "The authentication server is currently unavailable. Please try again later." + + assert get_flash(conn, :error) == + "The authentication server is currently unavailable. Please try again later." end test "Assent.InvalidResponseError redirects to sign-in with error flash", %{ conn: authenticated_conn } do conn = build_unauthenticated_conn(authenticated_conn) - # Create a mock Assent.InvalidResponseError struct + # Create a mock Assent.InvalidResponseError struct with required field + # InvalidResponseError only has :response field (HTTPResponse struct) error = %Assent.InvalidResponseError{ - response: %{status_code: 400, body: "invalid_request"}, - request_url: "https://auth.example.com/callback" + response: %Assent.HTTPAdapter.HTTPResponse{ + status: 400, + headers: [], + body: "invalid_request" + } } conn = MvWeb.AuthController.failure(conn, {:rauthy, :callback}, error) assert redirected_to(conn) == ~p"/sign-in" - assert get_flash(conn, :error) == "Authentication configuration error. Please contact the administrator." + + assert get_flash(conn, :error) == + "Authentication configuration error. Please contact the administrator." end test "unknown reason triggers catch-all and redirects to sign-in with error flash", %{ @@ -289,7 +304,9 @@ defmodule MvWeb.AuthControllerTest do conn = MvWeb.AuthController.failure(conn, {:rauthy, :callback}, unknown_reason) assert redirected_to(conn) == ~p"/sign-in" - assert get_flash(conn, :error) == "Unable to authenticate with OIDC. Please try again." + + assert get_flash(conn, :error) == + "Unable to authenticate with OIDC. Please try again." end end end -- 2.47.2 From b5fc03e94f524c7e6c0759d87324f13bb34cde51 Mon Sep 17 00:00:00 2001 From: carla Date: Wed, 18 Feb 2026 16:10:46 +0100 Subject: [PATCH 3/4] refactor --- lib/mv_web/controllers/auth_controller.ex | 78 +++++++++++-- .../controllers/auth_controller_test.exs | 104 +++++++++++++++++- 2 files changed, 168 insertions(+), 14 deletions(-) diff --git a/lib/mv_web/controllers/auth_controller.ex b/lib/mv_web/controllers/auth_controller.ex index 24d1078..fb760e6 100644 --- a/lib/mv_web/controllers/auth_controller.ex +++ b/lib/mv_web/controllers/auth_controller.ex @@ -45,9 +45,7 @@ defmodule MvWeb.AuthController do - Generic authentication failures """ def failure(conn, activity, reason) do - Logger.warning( - "Authentication failure - Activity: #{inspect(activity)}, Reason: #{inspect(reason)}" - ) + log_failure_safely(activity, reason) case {activity, reason} do {{:rauthy, _action}, reason} -> @@ -63,6 +61,63 @@ defmodule MvWeb.AuthController do end end + # Log authentication failures safely, avoiding sensitive data for {:rauthy, _} activities + defp log_failure_safely({:rauthy, _action} = activity, reason) do + # For Assent errors, use safe_assent_meta to avoid logging tokens/URLs with query params + case reason do + %Assent.ServerUnreachableError{} = err -> + meta = safe_assent_meta(err) + message = format_safe_log_message("Authentication failure", activity, meta) + Logger.warning(message) + + %Assent.InvalidResponseError{} = err -> + meta = safe_assent_meta(err) + message = format_safe_log_message("Authentication failure", activity, meta) + Logger.warning(message) + + _ -> + # For other rauthy errors, log only error type, not full details + error_type = get_error_type(reason) + + Logger.warning( + "Authentication failure - Activity: #{inspect(activity)}, Error type: #{error_type}" + ) + end + end + + defp log_failure_safely(activity, reason) do + # For non-rauthy activities, safe to log full reason + Logger.warning( + "Authentication failure - Activity: #{inspect(activity)}, Reason: #{inspect(reason)}" + ) + end + + # Extract safe error type identifier without sensitive data + defp get_error_type(%struct{}), do: "#{struct}" + defp get_error_type(atom) when is_atom(atom), do: inspect(atom) + defp get_error_type(_other), do: "[redacted]" + + # Format safe log message with metadata included in the message string + defp format_safe_log_message(base_message, activity, meta) when is_list(meta) do + activity_str = "Activity: #{inspect(activity)}" + meta_str = format_meta_string(meta) + "#{base_message} - #{activity_str}#{meta_str}" + end + + defp format_meta_string([]), do: "" + defp format_meta_string(meta) when is_list(meta) do + parts = + Enum.map(meta, fn + {:request_url, url} -> "Request URL: #{url}" + {:status, status} -> "Status: #{status}" + {:http_adapter, adapter} -> "HTTP Adapter: #{inspect(adapter)}" + _ -> nil + end) + |> Enum.filter(&(&1 != nil)) + + if Enum.empty?(parts), do: "", else: " - " <> Enum.join(parts, ", ") + end + # Handle all Rauthy (OIDC) authentication failures defp handle_rauthy_failure(conn, %Ash.Error.Invalid{errors: errors}) do handle_oidc_email_collision(conn, errors) @@ -83,9 +138,9 @@ defmodule MvWeb.AuthController do end # Handle Assent server unreachable errors (network/connectivity issues) - defp handle_rauthy_failure(conn, %Assent.ServerUnreachableError{} = err) do - # Use warning level: server unreachable is often transient, not a critical system error - Logger.warning("OIDC authentication server unreachable", safe_assent_meta(err)) + defp handle_rauthy_failure(conn, %Assent.ServerUnreachableError{} = _err) do + # Logging already done safely in failure/3 via log_failure_safely/2 + # No need to log again here to avoid duplicate logs conn |> put_flash( @@ -96,9 +151,9 @@ defmodule MvWeb.AuthController do end # Handle Assent invalid response errors (configuration or malformed responses) - defp handle_rauthy_failure(conn, %Assent.InvalidResponseError{} = err) do - # Use warning level: configuration errors are operational issues, not critical failures - Logger.warning("OIDC authentication invalid response", safe_assent_meta(err)) + defp handle_rauthy_failure(conn, %Assent.InvalidResponseError{} = _err) do + # Logging already done safely in failure/3 via log_failure_safely/2 + # No need to log again here to avoid duplicate logs conn |> put_flash( @@ -109,8 +164,9 @@ defmodule MvWeb.AuthController do end # Catch-all clause for any other error types - defp handle_rauthy_failure(conn, reason) do - Logger.warning("Unhandled Rauthy failure reason: #{inspect(reason)}") + defp handle_rauthy_failure(conn, _reason) do + # Logging already done safely in failure/3 via log_failure_safely/2 + # No need to log again here to avoid duplicate logs conn |> put_flash(:error, gettext("Unable to authenticate with OIDC. Please try again.")) diff --git a/test/mv_web/controllers/auth_controller_test.exs b/test/mv_web/controllers/auth_controller_test.exs index 5f53e0f..bac46c8 100644 --- a/test/mv_web/controllers/auth_controller_test.exs +++ b/test/mv_web/controllers/auth_controller_test.exs @@ -2,6 +2,7 @@ defmodule MvWeb.AuthControllerTest do use MvWeb.ConnCase, async: true import Phoenix.LiveViewTest import Phoenix.ConnTest + import ExUnit.CaptureLog # Helper to create an unauthenticated conn (preserves sandbox metadata) defp build_unauthenticated_conn(authenticated_conn) do @@ -269,7 +270,7 @@ defmodule MvWeb.AuthControllerTest do assert redirected_to(conn) == ~p"/sign-in" - assert get_flash(conn, :error) == + assert Phoenix.Flash.get(conn.assigns.flash, :error) == "The authentication server is currently unavailable. Please try again later." end @@ -291,7 +292,7 @@ defmodule MvWeb.AuthControllerTest do assert redirected_to(conn) == ~p"/sign-in" - assert get_flash(conn, :error) == + assert Phoenix.Flash.get(conn.assigns.flash, :error) == "Authentication configuration error. Please contact the administrator." end @@ -305,8 +306,105 @@ defmodule MvWeb.AuthControllerTest do assert redirected_to(conn) == ~p"/sign-in" - assert get_flash(conn, :error) == + assert Phoenix.Flash.get(conn.assigns.flash, :error) == "Unable to authenticate with OIDC. Please try again." end end + + # Logging security tests - ensure no sensitive data is logged + describe "failure/3 logging security" do + test "does not log full URL with query params for Assent.ServerUnreachableError", %{ + conn: authenticated_conn + } do + conn = build_unauthenticated_conn(authenticated_conn) + + error = %Assent.ServerUnreachableError{ + http_adapter: Assent.HTTPAdapter.Finch, + request_url: "https://auth.example.com/callback?token=secret123&code=abc456", + reason: %Mint.TransportError{reason: :econnrefused} + } + + log = + capture_log(fn -> + MvWeb.AuthController.failure(conn, {:rauthy, :callback}, error) + end) + + # Should log redacted URL (only scheme and host) + assert log =~ "https://auth.example.com" + # Should NOT log query parameters or tokens + refute log =~ "token=secret123" + refute log =~ "code=abc456" + refute log =~ "callback?token" + end + + test "does not log sensitive data for Assent.InvalidResponseError", %{ + conn: authenticated_conn + } do + conn = build_unauthenticated_conn(authenticated_conn) + + error = %Assent.InvalidResponseError{ + response: %Assent.HTTPAdapter.HTTPResponse{ + status: 400, + headers: [], + body: "invalid_request" + } + } + + log = + capture_log(fn -> + MvWeb.AuthController.failure(conn, {:rauthy, :callback}, error) + end) + + # Should log error type but not full error details + assert log =~ "Authentication failure" + assert log =~ "rauthy" + # Should not log full error struct with inspect + refute log =~ "Assent.InvalidResponseError" + end + + test "does not log full reason for unknown rauthy errors", %{ + conn: authenticated_conn + } do + conn = build_unauthenticated_conn(authenticated_conn) + # Simulate an error that might contain sensitive data + error_with_sensitive_data = %{ + token: "secret_token_123", + url: "https://example.com/callback?access_token=abc123", + error: :something_went_wrong + } + + log = + capture_log(fn -> + MvWeb.AuthController.failure(conn, {:rauthy, :callback}, error_with_sensitive_data) + end) + + # Should log error type but not full error details + assert log =~ "Authentication failure" + assert log =~ "rauthy" + # Should NOT log sensitive data + refute log =~ "secret_token_123" + refute log =~ "access_token=abc123" + refute log =~ "callback?access_token" + end + + test "logs full reason for non-rauthy activities (password auth)", %{ + conn: authenticated_conn + } do + conn = build_unauthenticated_conn(authenticated_conn) + + reason = %AshAuthentication.Errors.AuthenticationFailed{ + caused_by: %Ash.Error.Forbidden{errors: []} + } + + log = + capture_log(fn -> + MvWeb.AuthController.failure(conn, {:password, :sign_in}, reason) + end) + + # For non-rauthy activities, full reason is safe to log + assert log =~ "Authentication failure" + assert log =~ "password" + assert log =~ "AuthenticationFailed" + end + end end -- 2.47.2 From d1fefcca7ddf10e4929c5c8b1b528eeed702555c Mon Sep 17 00:00:00 2001 From: carla Date: Wed, 18 Feb 2026 16:18:26 +0100 Subject: [PATCH 4/4] formatting --- lib/mv_web/controllers/auth_controller.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/mv_web/controllers/auth_controller.ex b/lib/mv_web/controllers/auth_controller.ex index fb760e6..d9690df 100644 --- a/lib/mv_web/controllers/auth_controller.ex +++ b/lib/mv_web/controllers/auth_controller.ex @@ -105,6 +105,7 @@ defmodule MvWeb.AuthController do end defp format_meta_string([]), do: "" + defp format_meta_string(meta) when is_list(meta) do parts = Enum.map(meta, fn -- 2.47.2