fix: address UserSudoController critical issues
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)
This commit is contained in:
parent
264154a3d8
commit
9ff9c4ddc8
4 changed files with 113 additions and 28 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -10,7 +10,7 @@
|
|||
</div>
|
||||
|
||||
<div class="mt-8">
|
||||
<.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"
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue