From a25263b7219104dec2b01e434969ccf69a62df82 Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 17 Feb 2026 19:29:49 +0100 Subject: [PATCH 1/5] 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 From 002d723d0ea82831b2c84e1cd507f6bc18745b3d Mon Sep 17 00:00:00 2001 From: carla Date: Wed, 18 Feb 2026 12:53:25 +0100 Subject: [PATCH 2/5] 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 From b5fc03e94f524c7e6c0759d87324f13bb34cde51 Mon Sep 17 00:00:00 2001 From: carla Date: Wed, 18 Feb 2026 16:10:46 +0100 Subject: [PATCH 3/5] 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 From d1fefcca7ddf10e4929c5c8b1b528eeed702555c Mon Sep 17 00:00:00 2001 From: carla Date: Wed, 18 Feb 2026 16:18:26 +0100 Subject: [PATCH 4/5] 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 From f4554b8a4bd2b8587f0a0b5211220c128e8a1069 Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 20 Feb 2026 14:01:14 +0100 Subject: [PATCH 5/5] docs: update Code Guidelines with issues from meta review analysis --- CODE_GUIDELINES.md | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index cc58ca9..70e1596 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -385,6 +385,8 @@ def process_user(user), do: {:ok, perform_action(user)} ### 2.3 Error Handling +**No silent failures:** When an error path assigns a fallback (e.g. empty list, unchanged assigns), always log the error with enough context (e.g. `inspect(error)`, slug, action) and/or set a user-visible flash. Do not only assign the fallback without logging. + **Use Tagged Tuples:** ```elixir @@ -623,6 +625,10 @@ defmodule MvWeb.MemberLive.Index do end ``` +**LiveView load budget:** Keep UI-triggered events cheap. `phx-change`, `phx-focus`, and `phx-keydown` must **not** perform database reads by default; work from assigns (e.g. filter in memory) or defer reads to an explicit commit step (e.g. "Add", "Save", "Submit"). Perform DB reads or reloads only on commit events, not on every keystroke or focus. If a read in an event is unavoidable, do at most one deliberate read, document why, and prefer debounce/throttle. + +**LiveView size:** When a LiveView accumulates many features and event handlers (CRUD + add/remove + search + keyboard + modals), extract sub-flows into LiveComponents. The parent keeps auth, initial load, and a single reload after child actions; the component owns the sub-flow and notifies the parent when data changes. + **Component Design:** ```elixir @@ -1267,6 +1273,9 @@ gettext("Welcome to Mila") # With interpolation gettext("Hello, %{name}!", name: user.name) +# Plural: always pass count binding when message uses %{count} +ngettext("Found %{count} member", "Found %{count} members", @count, count: @count) + # Domain-specific translations dgettext("auth", "Sign in with email") ``` @@ -1507,6 +1516,8 @@ defmodule MvWeb.MemberLive.IndexTest do end ``` +**LiveView test standards:** Prefer selector-based assertions (`has_element?` on `data-testid`, stable IDs, or semantic markers) over free-text matching (`html =~ "..."`) or broad regex. For repeated flows (e.g. "open add member", "search", "select"), use helpers in `test/support/`. If delete is a LiveView event, test that event and assert both UI and data; if delete uses JS `data-confirm` + non-LV submit, cover the real delete path in a context/service test and add at most one smoke test in the UI. Do not assert or document "single request" or "DB-level sort" without measuring (e.g. query count or timing). + #### 4.3.5 Component Tests Test function components: @@ -1876,6 +1887,8 @@ policies do end ``` +**Record-based authorization:** When a concrete record is available, use `can?(actor, :action, record)` (e.g. `can?(@current_user, :update, @group)`). Use `can?(actor, :action, Resource)` only when no specific record exists (e.g. "can create any group"). In events: resolve the record from assigns, run `can?`, then mutate; avoid an extra DB read just for a "freshness" check if the assign is already the source of truth. + **Actor Handling in LiveViews:** Always use the `current_actor/1` helper for consistent actor access: @@ -2707,7 +2720,9 @@ Building accessible applications ensures that all users, including those with di ### 8.2 ARIA Labels and Roles -**Use ARIA Attributes When Necessary:** +**Terminology and semantics:** Use the same terms in UI, tests, and docs (e.g. "modal" vs "inline confirmation"). If the implementation is inline, do not call it a modal in tests or docs. + +**Use ARIA Attributes When Necessary:** Use `role="status"` only for live regions that present advisory status (e.g. search result count, progress). Do not use it on links, buttons, or purely static labels; use semantic HTML or the correct role for the widget. Attach `phx-debounce` / `phx-throttle` to the element that triggers the event (e.g. input), not to the whole form, unless the intent is form-wide. ```heex @@ -2931,11 +2946,11 @@ end **Announce Dynamic Content:** ```heex - +
<%= if @searched do %> - <%= ngettext("Found %{count} member", "Found %{count} members", @count) %> + <%= ngettext("Found %{count} member", "Found %{count} members", @count, count: @count) %> <% end %>