diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 76de1ff..034177a 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -391,38 +391,52 @@ defmodule Mv.Accounts.User do end end - # Last-admin: prevent the only admin from changing their role (at least one admin required). + # Last-admin: prevent the only admin from leaving the admin role (at least one admin required). + # Only block when the user is leaving admin (target role is not admin). Switching between + # two admin roles (e.g. "Admin" and "Superadmin" both with permission_set_name "admin") is allowed. validate fn changeset, _context -> if Ash.Changeset.changing_attribute?(changeset, :role_id) do - current_role_id = changeset.data.role_id + new_role_id = Ash.Changeset.get_attribute(changeset, :role_id) - current_role = - Mv.Authorization.Role - |> Ash.get!(current_role_id, authorize?: false) - - if current_role.permission_set_name != "admin" do + if is_nil(new_role_id) do :ok else - admin_role_ids = + current_role_id = changeset.data.role_id + + current_role = Mv.Authorization.Role - |> Ash.Query.for_read(:read) - |> Ash.Query.filter(expr(permission_set_name == "admin")) - |> Ash.read!(authorize?: false) - |> Enum.map(& &1.id) + |> Ash.get!(current_role_id, authorize?: false) - # Count only non-system users with admin role (system user is for internal ops) - system_email = Mv.Helpers.SystemActor.system_user_email() + new_role = + Mv.Authorization.Role + |> Ash.get!(new_role_id, authorize?: false) - 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) + # Only block when current user is admin and target role is not admin (leaving admin) + if current_role.permission_set_name == "admin" and + new_role.permission_set_name != "admin" do + 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) - if count <= 1 do - {:error, - field: :role_id, message: "At least one user must keep the Admin role."} + # 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 else :ok end diff --git a/test/mv/accounts/user_policies_test.exs b/test/mv/accounts/user_policies_test.exs index 9678a0e..66b550c 100644 --- a/test/mv/accounts/user_policies_test.exs +++ b/test/mv/accounts/user_policies_test.exs @@ -105,6 +105,19 @@ defmodule Mv.Accounts.UserPoliciesTest do Ash.destroy!(user, actor: user) end end + + @tag permission_set: permission_set + test "cannot change role via update_user - forbidden (#{permission_set})", %{ + user: user, + other_user: other_user + } do + other_role = Mv.Fixtures.role_fixture("read_only") + + assert {:error, %Ash.Error.Forbidden{}} = + other_user + |> Ash.Changeset.for_update(:update_user, %{role_id: other_role.id}) + |> Ash.update(actor: user, domain: Mv.Accounts) + end end end @@ -221,6 +234,31 @@ defmodule Mv.Accounts.UserPoliciesTest do end), "Expected last-admin validation message, got: #{inspect(error_messages)}" end + + test "admin can switch to another admin role (two roles with permission_set_name admin)", %{ + actor: _actor + } do + # Two distinct roles both with permission_set_name "admin" (e.g. "Admin" and "Superadmin") + admin_role_a = Mv.Fixtures.role_fixture("admin") + admin_role_b = Mv.Fixtures.role_fixture("admin") + + admin_user = Mv.Fixtures.user_with_role_fixture("admin") + # Ensure user has role_a so we can switch to role_b + {:ok, admin_user} = + admin_user + |> Ash.Changeset.for_update(:update_user, %{role_id: admin_role_a.id}) + |> Ash.update(actor: admin_user) + + assert admin_user.role_id == admin_role_a.id + + # Switching to another admin role must be allowed (no last-admin error) + {:ok, updated} = + admin_user + |> Ash.Changeset.for_update(:update_user, %{role_id: admin_role_b.id}) + |> Ash.update(actor: admin_user) + + assert updated.role_id == admin_role_b.id + end end describe "AshAuthentication bypass" do