diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index f792973..76de1ff 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -8,6 +8,9 @@ defmodule Mv.Accounts.User do extensions: [AshAuthentication], authorizers: [Ash.Policy.Authorizer] + require Ash.Query + import Ash.Expr + postgres do table "users" repo Mv.Repo @@ -146,9 +149,10 @@ defmodule Mv.Accounts.User do update :update_user do description "Updates a user and manages the optional member relationship. To change an existing member link, first remove it (set member to nil), then add the new one." - # Only accept email directly - member_id is NOT in accept list - # This prevents direct foreign key manipulation, forcing use of manage_relationship - accept [:email] + + # Accept email and role_id (role_id only used by admins; policy restricts update_user to admins). + # member_id is NOT in accept list - use argument :member for relationship management. + accept [:email, :role_id] # Allow member to be passed as argument for relationship management argument :member, :map, allow_nil?: true @@ -387,6 +391,49 @@ defmodule Mv.Accounts.User do end end + # Last-admin: prevent the only admin from changing their role (at least one admin required). + validate fn changeset, _context -> + if Ash.Changeset.changing_attribute?(changeset, :role_id) do + current_role_id = changeset.data.role_id + + current_role = + Mv.Authorization.Role + |> Ash.get!(current_role_id, authorize?: false) + + if current_role.permission_set_name != "admin" do + :ok + else + admin_role_ids = + Mv.Authorization.Role + |> Ash.Query.for_read(:read) + |> Ash.Query.filter(expr(permission_set_name == "admin")) + |> Ash.read!(authorize?: false) + |> Enum.map(& &1.id) + + # Count only non-system users with admin role (system user is for internal ops) + system_email = Mv.Helpers.SystemActor.system_user_email() + + count = + Mv.Accounts.User + |> Ash.Query.for_read(:read) + |> Ash.Query.filter(expr(role_id in ^admin_role_ids)) + |> Ash.Query.filter(expr(email != ^system_email)) + |> Ash.count!(authorize?: false) + + if count <= 1 do + {:error, + field: :role_id, message: "At least one user must keep the Admin role."} + else + :ok + end + end + else + :ok + end + end, + on: [:update], + where: [action_is(:update_user)] + # Prevent modification of the system actor user (required for internal operations). # Block update/destroy on UI-exposed actions only; :update_internal is used by bootstrap/tests. validate fn changeset, _context -> diff --git a/lib/mv_web/live/user_live/form.ex b/lib/mv_web/live/user_live/form.ex index f3cec75..72d4741 100644 --- a/lib/mv_web/live/user_live/form.ex +++ b/lib/mv_web/live/user_live/form.ex @@ -35,6 +35,8 @@ defmodule MvWeb.UserLive.Form do require Jason + alias Mv.Authorization + import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] import MvWeb.Authorization, only: [can?: 3] @@ -49,6 +51,18 @@ defmodule MvWeb.UserLive.Form do <.form class="max-w-xl" for={@form} id="user-form" phx-change="validate" phx-submit="save"> <.input field={@form[:email]} label={gettext("Email")} required type="email" /> + + <%= if @user && @can_assign_role do %> +
+ <.input + field={@form[:role_id]} + type="select" + label={gettext("Role")} + options={Enum.map(@roles, &{&1.name, &1.id})} + prompt={gettext("Select role...")} + /> +
+ <% end %>
@@ -300,6 +314,9 @@ defmodule MvWeb.UserLive.Form do # Only admins can link/unlink users to members (permission docs; prevents privilege escalation). can_manage_member_linking = can?(actor, :destroy, Mv.Accounts.User) + # Only admins can assign user roles (Role update permission). + can_assign_role = can?(actor, :update, Mv.Authorization.Role) + roles = if can_assign_role, do: load_roles(actor), else: [] {:ok, socket @@ -307,6 +324,8 @@ defmodule MvWeb.UserLive.Form do |> assign(user: user) |> assign(:page_title, page_title) |> assign(:can_manage_member_linking, can_manage_member_linking) + |> assign(:can_assign_role, can_assign_role) + |> assign(:roles, roles) |> assign(:show_password_fields, false) |> assign(:member_search_query, "") |> assign(:available_members, []) @@ -357,7 +376,10 @@ defmodule MvWeb.UserLive.Form do def handle_event("save", %{"user" => user_params}, socket) do actor = current_actor(socket) - # First save the user without member changes + # Include current member in params when not linking/unlinking so update_user's + # manage_relationship(on_missing: :unrelate) does not accidentally unlink. + user_params = params_with_member_if_unchanged(socket, user_params) + case submit_form(socket.assigns.form, user_params, actor) do {:ok, user} -> handle_member_linking(socket, user, actor) @@ -529,6 +551,20 @@ defmodule MvWeb.UserLive.Form do defp get_action_name(:update), do: gettext("updated") defp get_action_name(other), do: to_string(other) + # When user has a linked member and we are not linking/unlinking, include current member in params + # so update_user's manage_relationship(on_missing: :unrelate) does not unlink the member. + defp params_with_member_if_unchanged(socket, params) do + user = socket.assigns.user + linking = socket.assigns.selected_member_id + unlinking = socket.assigns[:unlink_member] + + if user && user.member_id && !linking && !unlinking do + Map.put(params, "member", %{"id" => user.member_id}) + else + params + end + end + defp handle_member_link_error(socket, error) do error_message = extract_error_message(error) @@ -572,7 +608,8 @@ defmodule MvWeb.UserLive.Form do assigns: %{ user: user, show_password_fields: show_password_fields, - can_manage_member_linking: can_manage_member_linking + can_manage_member_linking: can_manage_member_linking, + can_assign_role: can_assign_role } } = socket ) do @@ -580,16 +617,25 @@ defmodule MvWeb.UserLive.Form do form = if user do - # For existing users: admin uses update_user (email + member); non-admin uses update (email only). + # For existing users: admin uses update_user (email + member + role_id); non-admin uses update (email only). # Password change uses admin_set_password for both. action = cond do show_password_fields -> :admin_set_password - can_manage_member_linking -> :update_user + can_manage_member_linking or can_assign_role -> :update_user true -> :update end - AshPhoenix.Form.for_update(user, action, domain: Mv.Accounts, as: "user", actor: actor) + form = + AshPhoenix.Form.for_update(user, action, domain: Mv.Accounts, as: "user", actor: actor) + + # Ensure role_id is always included on submit when role dropdown is shown (AshPhoenix.Form + # only submits keys in touched_forms; marking as touched avoids role change being dropped). + if can_assign_role and action == :update_user do + AshPhoenix.Form.touch(form, [:role_id]) + else + form + end else # For new users, use password registration if password fields are shown action = if show_password_fields, do: :register_with_password, else: :create_user @@ -668,6 +714,14 @@ defmodule MvWeb.UserLive.Form do Mv.Membership.Member.filter_by_email_match(members, user_email_str) end + @spec load_roles(any()) :: [Mv.Authorization.Role.t()] + defp load_roles(actor) do + case Authorization.list_roles(actor: actor) do + {:ok, roles} -> roles + {:error, _} -> [] + end + end + # Extract user-friendly error message from Ash.Error @spec extract_error_message(any()) :: String.t() defp extract_error_message(%Ash.Error.Invalid{errors: errors}) when is_list(errors) do diff --git a/test/mv/accounts/user_policies_test.exs b/test/mv/accounts/user_policies_test.exs index 736b336..97eea78 100644 --- a/test/mv/accounts/user_policies_test.exs +++ b/test/mv/accounts/user_policies_test.exs @@ -343,6 +343,64 @@ defmodule Mv.Accounts.UserPoliciesTest do # Verify user is deleted assert {:error, _} = Ash.get(Accounts.User, other_user.id, domain: Mv.Accounts) end + + test "admin can assign role to another user via update_user", %{ + actor: actor, + other_user: other_user + } do + admin = create_user_with_permission_set("admin", actor) + normal_user_role = create_role_with_permission_set("normal_user", actor) + + {:ok, updated} = + other_user + |> Ash.Changeset.for_update(:update_user, %{role_id: normal_user_role.id}) + |> Ash.update(actor: admin) + + assert updated.role_id == normal_user_role.id + end + end + + describe "admin role assignment and last-admin validation" do + test "two admins: one can change own role to normal_user (other remains admin)", %{ + actor: actor + } do + _admin_role = create_role_with_permission_set("admin", actor) + normal_user_role = create_role_with_permission_set("normal_user", actor) + + admin_a = create_user_with_permission_set("admin", actor) + _admin_b = create_user_with_permission_set("admin", actor) + + {:ok, updated} = + admin_a + |> Ash.Changeset.for_update(:update_user, %{role_id: normal_user_role.id}) + |> Ash.update(actor: admin_a) + + assert updated.role_id == normal_user_role.id + end + + test "single admin: changing own role to normal_user returns validation error", %{ + actor: actor + } do + normal_user_role = create_role_with_permission_set("normal_user", actor) + single_admin = create_user_with_permission_set("admin", actor) + + assert {:error, %Ash.Error.Invalid{errors: errors}} = + single_admin + |> Ash.Changeset.for_update(:update_user, %{role_id: normal_user_role.id}) + |> Ash.update(actor: single_admin) + + error_messages = + Enum.flat_map(errors, fn + %Ash.Error.Changes.InvalidAttribute{message: msg} when is_binary(msg) -> [msg] + %{message: msg} when is_binary(msg) -> [msg] + _ -> [] + end) + + assert Enum.any?(error_messages, fn msg -> + msg =~ "least one user must keep the Admin role" or msg =~ "Admin role" + end), + "Expected last-admin validation message, got: #{inspect(error_messages)}" + end end describe "AshAuthentication bypass" do diff --git a/test/mv_web/user_live/form_test.exs b/test/mv_web/user_live/form_test.exs index 48c0238..a22c230 100644 --- a/test/mv_web/user_live/form_test.exs +++ b/test/mv_web/user_live/form_test.exs @@ -213,6 +213,35 @@ defmodule MvWeb.UserLive.FormTest do assert not is_nil(updated_user.hashed_password) assert updated_user.hashed_password != "" end + + test "admin can change user role and change persists", %{conn: conn} do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + + role_a = Mv.Fixtures.role_fixture("normal_user") + role_b = Mv.Fixtures.role_fixture("read_only") + + user = create_test_user(%{email: "rolechange@example.com"}) + {:ok, user} = Mv.Accounts.update_user(user, %{role_id: role_a.id}, actor: system_actor) + assert user.role_id == role_a.id + + {:ok, view, _html} = setup_live_view(conn, "/users/#{user.id}/edit") + + view + |> form("#user-form", + user: %{ + email: "rolechange@example.com", + role_id: role_b.id + } + ) + |> render_submit() + + assert_redirected(view, "/users") + + updated_user = Ash.reload!(user, domain: Mv.Accounts, actor: system_actor) + + assert updated_user.role_id == role_b.id, + "Expected role_id to persist as #{role_b.id}, got #{inspect(updated_user.role_id)}" + end end describe "edit user form - validation" do