From 9ff9c4ddc8638c6a610dd59a1af1a21a76f151ab Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Sun, 1 Feb 2026 14:45:05 -0600 Subject: [PATCH] fix: address UserSudoController critical issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical fixes: - Renamed controller actions to follow Phoenix conventions (verify→new, create→verify) - Added sudo mode check in GET action using last_sudo_at timestamp - Added TOTP enrollment check in GET action with redirect - Changed default return path from /orgs to /users/settings - Updated route paths from /users/sudo/verify to /users/sudo-verify (dash separator) - Added success flash message "Identity verified." Implementation details: - new/2 (GET): Checks recently_verified_sudo?/1 helper that examines last_sudo_at instead of authenticated_at to distinguish sudo verification from regular login - verify/2 (POST): Adds info flash and defaults to /users/settings - recently_verified_sudo?/1: Private helper checking last_sudo_at within 10 minutes - Updated all routes, templates, and tests to use new paths and action names Tests: - Added tests for already-in-sudo-mode redirect behavior - Added test for TOTP enrollment requirement - Updated all test assertions for new route paths and success messages - All 15 controller tests passing - Full test suite passing (4076 tests, 0 failures) --- .../controllers/user_sudo_controller.ex | 44 +++++++-- .../user_sudo_html/verify.html.heex | 2 +- lib/towerops_web/router.ex | 4 +- .../controllers/user_sudo_controller_test.exs | 91 +++++++++++++++---- 4 files changed, 113 insertions(+), 28 deletions(-) diff --git a/lib/towerops_web/controllers/user_sudo_controller.ex b/lib/towerops_web/controllers/user_sudo_controller.ex index cf1a64a4..198b4168 100644 --- a/lib/towerops_web/controllers/user_sudo_controller.ex +++ b/lib/towerops_web/controllers/user_sudo_controller.ex @@ -14,13 +14,42 @@ defmodule ToweropsWeb.UserSudoController do plug :require_authenticated_user - def verify(conn, _params) do - # Render the sudo verification form - form = Phoenix.Component.to_form(%{}, as: "user") - render(conn, :verify, form: form) + def new(conn, _params) do + user = conn.assigns.current_scope.user + + cond do + # Already in sudo mode (verified within last 10 minutes), redirect to destination + recently_verified_sudo?(user) -> + return_to = get_session(conn, :user_return_to) || ~p"/users/settings" + + conn + |> delete_session(:user_return_to) + |> redirect(to: return_to) + + # No TOTP devices, must enroll first + !Accounts.totp_enabled?(user) -> + conn + |> put_flash(:error, "Two-factor authentication is required for this action.") + |> redirect(to: ~p"/account/totp-enrollment") + + # Show verification form + true -> + form = Phoenix.Component.to_form(%{}, as: "user") + render(conn, :verify, form: form) + end end - def create(conn, %{"user" => %{"totp_code" => totp_code}}) do + # Check if user verified sudo mode within the last 10 minutes + # Uses last_sudo_at timestamp instead of authenticated_at to distinguish + # between regular login and explicit sudo verification + defp recently_verified_sudo?(%{last_sudo_at: nil}), do: false + + defp recently_verified_sudo?(%{last_sudo_at: last_sudo_at}) do + ten_minutes_ago = DateTime.add(DateTime.utc_now(), -10, :minute) + DateTime.after?(last_sudo_at, ten_minutes_ago) + end + + def verify(conn, %{"user" => %{"totp_code" => totp_code}}) do user = conn.assigns.current_scope.user case Accounts.verify_totp_only(user, totp_code) do @@ -29,9 +58,10 @@ defmodule ToweropsWeb.UserSudoController do case Accounts.grant_sudo_mode(user) do {:ok, _updated_user} -> # Redirect to return_to path or default - return_to = get_session(conn, :user_return_to) || ~p"/orgs" + return_to = get_session(conn, :user_return_to) || ~p"/users/settings" conn + |> put_flash(:info, "Identity verified.") |> delete_session(:user_return_to) |> redirect(to: return_to) @@ -63,7 +93,7 @@ defmodule ToweropsWeb.UserSudoController do end end - def create(conn, _params) do + def verify(conn, _params) do # Handle missing totp_code parameter form = Phoenix.Component.to_form(%{}, as: "user") render(conn, :verify, form: form) diff --git a/lib/towerops_web/controllers/user_sudo_html/verify.html.heex b/lib/towerops_web/controllers/user_sudo_html/verify.html.heex index 2de6a728..c94127b6 100644 --- a/lib/towerops_web/controllers/user_sudo_html/verify.html.heex +++ b/lib/towerops_web/controllers/user_sudo_html/verify.html.heex @@ -10,7 +10,7 @@
- <.form :let={f} for={@form} as={:user} id="sudo_verify_form" action={~p"/users/sudo/verify"}> + <.form :let={f} for={@form} as={:user} id="sudo_verify_form" action={~p"/users/sudo-verify"}> <.input field={f[:totp_code]} type="text" diff --git a/lib/towerops_web/router.ex b/lib/towerops_web/router.ex index d381ff7d..08b5b0ed 100644 --- a/lib/towerops_web/router.ex +++ b/lib/towerops_web/router.ex @@ -189,8 +189,8 @@ defmodule ToweropsWeb.Router do scope "/", ToweropsWeb do pipe_through [:browser, :require_authenticated_user] - get "/users/sudo/verify", UserSudoController, :verify - post "/users/sudo/verify", UserSudoController, :create + get "/users/sudo-verify", UserSudoController, :new + post "/users/sudo-verify", UserSudoController, :verify end ## Admin routes (superuser only) diff --git a/test/towerops_web/controllers/user_sudo_controller_test.exs b/test/towerops_web/controllers/user_sudo_controller_test.exs index ff5e692d..c25190f5 100644 --- a/test/towerops_web/controllers/user_sudo_controller_test.exs +++ b/test/towerops_web/controllers/user_sudo_controller_test.exs @@ -11,12 +11,12 @@ defmodule ToweropsWeb.UserSudoControllerTest do %{user: user} end - describe "GET /users/sudo/verify" do + describe "GET /users/sudo-verify" do test "renders sudo verification page for authenticated user", %{conn: conn, user: user} do conn = conn |> log_in_user(user) - |> get(~p"/users/sudo/verify") + |> get(~p"/users/sudo-verify") response = html_response(conn, 200) assert response =~ "Re-authenticate" @@ -24,7 +24,7 @@ defmodule ToweropsWeb.UserSudoControllerTest do end test "redirects unauthenticated users to login", %{conn: conn} do - conn = get(conn, ~p"/users/sudo/verify") + conn = get(conn, ~p"/users/sudo-verify") assert redirected_to(conn) == ~p"/users/log-in" assert Phoenix.Flash.get(conn.assigns.flash, :error) == "You must log in to access this page." end @@ -34,14 +34,58 @@ defmodule ToweropsWeb.UserSudoControllerTest do conn |> log_in_user(user) |> init_test_session(user_return_to: "/users/settings") - |> get(~p"/users/sudo/verify") + |> get(~p"/users/sudo-verify") assert html_response(conn, 200) =~ "Re-authenticate" assert get_session(conn, :user_return_to) == "/users/settings" end + + test "redirects to destination if already in sudo mode", %{conn: conn, user: user} do + # Grant sudo mode first + {:ok, _user} = Accounts.grant_sudo_mode(user) + + conn = + conn + |> log_in_user(user) + |> init_test_session(user_return_to: "/users/settings") + |> get(~p"/users/sudo-verify") + + assert redirected_to(conn) == "/users/settings" + assert get_session(conn, :user_return_to) == nil + end + + test "redirects to /users/settings if already in sudo mode and no return_to", %{ + conn: conn, + user: user + } do + # Grant sudo mode first + {:ok, _user} = Accounts.grant_sudo_mode(user) + + conn = + conn + |> log_in_user(user) + |> get(~p"/users/sudo-verify") + + assert redirected_to(conn) == ~p"/users/settings" + end + + test "redirects to TOTP enrollment if user has no TOTP", %{conn: conn} do + # Create user without TOTP + user_no_totp = user_fixture(enable_totp: false) + + conn = + conn + |> log_in_user(user_no_totp) + |> get(~p"/users/sudo-verify") + + assert redirected_to(conn) == ~p"/account/totp-enrollment" + + assert Phoenix.Flash.get(conn.assigns.flash, :error) == + "Two-factor authentication is required for this action." + end end - describe "POST /users/sudo/verify" do + describe "POST /users/sudo-verify" do test "grants sudo mode and redirects on valid TOTP code", %{conn: conn, user: user} do # Get valid TOTP code for user code = NimbleTOTP.verification_code(user.totp_secret) @@ -50,11 +94,14 @@ defmodule ToweropsWeb.UserSudoControllerTest do conn |> log_in_user(user) |> init_test_session(user_return_to: "/users/settings") - |> post(~p"/users/sudo/verify", %{"user" => %{"totp_code" => code}}) + |> post(~p"/users/sudo-verify", %{"user" => %{"totp_code" => code}}) # Should redirect to return_to path (implies sudo mode was granted) assert redirected_to(conn) == "/users/settings" + # Should show success flash message + assert Phoenix.Flash.get(conn.assigns.flash, :info) == "Identity verified." + # Should clear return_to from session assert get_session(conn, :user_return_to) == nil @@ -64,16 +111,22 @@ defmodule ToweropsWeb.UserSudoControllerTest do assert DateTime.diff(updated_user.last_sudo_at, DateTime.utc_now(), :second) <= 1 end - test "grants sudo mode and redirects to /orgs if no return_to", %{conn: conn, user: user} do + test "grants sudo mode and redirects to /users/settings if no return_to", %{ + conn: conn, + user: user + } do code = NimbleTOTP.verification_code(user.totp_secret) conn = conn |> log_in_user(user) - |> post(~p"/users/sudo/verify", %{"user" => %{"totp_code" => code}}) + |> post(~p"/users/sudo-verify", %{"user" => %{"totp_code" => code}}) - # Should redirect to /orgs (implies sudo mode was granted) - assert redirected_to(conn) == ~p"/orgs" + # Should redirect to /users/settings (default) + assert redirected_to(conn) == ~p"/users/settings" + + # Should show success flash message + assert Phoenix.Flash.get(conn.assigns.flash, :info) == "Identity verified." # Should update last_sudo_at timestamp updated_user = Accounts.get_user!(user.id) @@ -85,7 +138,7 @@ defmodule ToweropsWeb.UserSudoControllerTest do conn = conn |> log_in_user(user) - |> post(~p"/users/sudo/verify", %{"user" => %{"totp_code" => "000000"}}) + |> post(~p"/users/sudo-verify", %{"user" => %{"totp_code" => "000000"}}) response = html_response(conn, 200) assert response =~ "Invalid authentication code" @@ -100,7 +153,7 @@ defmodule ToweropsWeb.UserSudoControllerTest do conn = conn |> log_in_user(user) - |> post(~p"/users/sudo/verify", %{"user" => %{"totp_code" => "ABCD-EFGH"}}) + |> post(~p"/users/sudo-verify", %{"user" => %{"totp_code" => "ABCD-EFGH"}}) response = html_response(conn, 200) assert response =~ "Recovery codes are not allowed for sudo mode verification" @@ -115,7 +168,7 @@ defmodule ToweropsWeb.UserSudoControllerTest do conn = conn |> log_in_user(user) - |> post(~p"/users/sudo/verify", %{"user" => %{"totp_code" => "1234567"}}) + |> post(~p"/users/sudo-verify", %{"user" => %{"totp_code" => "1234567"}}) response = html_response(conn, 200) assert response =~ "Recovery codes are not allowed for sudo mode verification" @@ -126,7 +179,7 @@ defmodule ToweropsWeb.UserSudoControllerTest do conn = conn |> log_in_user(user) - |> post(~p"/users/sudo/verify", %{"user" => %{"totp_code" => "12AB34"}}) + |> post(~p"/users/sudo-verify", %{"user" => %{"totp_code" => "12AB34"}}) response = html_response(conn, 200) assert response =~ "Recovery codes are not allowed for sudo mode verification" @@ -134,9 +187,11 @@ defmodule ToweropsWeb.UserSudoControllerTest do end test "redirects unauthenticated users to login", %{conn: conn} do - conn = post(conn, ~p"/users/sudo/verify", %{"user" => %{"totp_code" => "123456"}}) + conn = post(conn, ~p"/users/sudo-verify", %{"user" => %{"totp_code" => "123456"}}) assert redirected_to(conn) == ~p"/users/log-in" - assert Phoenix.Flash.get(conn.assigns.flash, :error) == "You must log in to access this page." + + assert Phoenix.Flash.get(conn.assigns.flash, :error) == + "You must log in to access this page." end test "preserves return_to path on verification failure", %{conn: conn, user: user} do @@ -144,7 +199,7 @@ defmodule ToweropsWeb.UserSudoControllerTest do conn |> log_in_user(user) |> init_test_session(user_return_to: "/users/settings") - |> post(~p"/users/sudo/verify", %{"user" => %{"totp_code" => "000000"}}) + |> post(~p"/users/sudo-verify", %{"user" => %{"totp_code" => "000000"}}) assert html_response(conn, 200) =~ "Invalid authentication code" # return_to should be preserved for next attempt @@ -155,7 +210,7 @@ defmodule ToweropsWeb.UserSudoControllerTest do conn = conn |> log_in_user(user) - |> post(~p"/users/sudo/verify", %{"user" => %{}}) + |> post(~p"/users/sudo-verify", %{"user" => %{}}) response = html_response(conn, 200) assert response =~ "Re-authenticate"