From b70ece21297edb847fee530bf134fd90fe715594 Mon Sep 17 00:00:00 2001 From: Moritz Date: Wed, 4 Feb 2026 12:50:10 +0100 Subject: [PATCH] Restrict member user link to admins (forbid policy) Add ForbidMemberUserLinkUnlessAdmin check; forbid_if on Member create/update. Fix member user-link tests: pass :user in params, assert via reload. --- lib/membership/member.ex | 12 +- .../forbid_member_user_link_unless_admin.ex | 65 ++++++++++ test/mv/membership/member_policies_test.exs | 116 ++++++++++++++++++ 3 files changed, 186 insertions(+), 7 deletions(-) create mode 100644 lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 8213ecb..8517634 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -312,14 +312,12 @@ defmodule Mv.Membership.Member do authorize_if expr(id == ^actor(:member_id)) end - # GENERAL: Check permissions from user's role - # HasPermission handles update permissions correctly: - # - :own_data → can update linked member (scope :linked) - # - :read_only → cannot update any member (no update permission) - # - :normal_user → can update all members (scope :all) - # - :admin → can update all members (scope :all) + # GENERAL: Check permissions from user's role; forbid member–user link unless admin + # ForbidMemberUserLinkUnlessAdmin: only admins may pass :user on create/update (no-op for read/destroy). + # HasPermission: :own_data → update linked; :read_only → no update; :normal_user/admin → update all. policy action_type([:read, :create, :update, :destroy]) do - description "Check permissions from user's role and permission set" + description "Check permissions and forbid user link unless admin" + forbid_if Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin authorize_if Mv.Authorization.Checks.HasPermission end diff --git a/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex b/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex new file mode 100644 index 0000000..facfdb2 --- /dev/null +++ b/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex @@ -0,0 +1,65 @@ +defmodule Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin do + @moduledoc """ + Policy check: forbids setting or changing the member–user link unless the actor is admin. + + Used on Member create_member and update_member actions. When the `:user` argument + is present (linking a member to a user account), only admins may perform the action. + Non-admin users (e.g. normal_user / Kassenwart) can still create and update members + as long as they do not pass the `:user` argument. + + ## Usage + + In Member resource policies, add **before** the general HasPermission policy: + + policy action_type([:create, :update]) do + forbid_if Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin + authorize_if Mv.Authorization.Checks.HasPermission + end + + ## Behaviour + + - If the action has no `:user` argument or it is nil/empty → does not forbid. + - If `:user` is set (e.g. `%{id: user_id}`) and actor is not admin → forbids (returns true). + - If actor is admin (or system actor) → does not forbid. + """ + use Ash.Policy.Check + + alias Mv.Authorization.Actor + + @impl true + def describe(_opts), do: "forbid setting member–user link unless actor is admin" + + @impl true + def strict_check(actor, authorizer, _opts) do + actor = Actor.ensure_loaded(actor) + + if user_argument_set?(authorizer) and not Actor.admin?(actor) do + {:ok, true} + else + {:ok, false} + end + end + + defp user_argument_set?(authorizer) do + user_arg = get_user_argument(authorizer) + not is_nil(user_arg) and not empty_user_arg?(user_arg) + end + + defp get_user_argument(authorizer) do + changeset = authorizer.changeset || authorizer.subject + + cond do + is_struct(changeset, Ash.Changeset) -> + Ash.Changeset.get_argument(changeset, :user) + + is_struct(changeset, Ash.ActionInput) -> + Map.get(changeset.arguments || %{}, :user) + + true -> + nil + end + end + + defp empty_user_arg?(%{} = m), do: map_size(m) == 0 + defp empty_user_arg?(_), do: false +end diff --git a/test/mv/membership/member_policies_test.exs b/test/mv/membership/member_policies_test.exs index a66941b..30936fe 100644 --- a/test/mv/membership/member_policies_test.exs +++ b/test/mv/membership/member_policies_test.exs @@ -403,4 +403,120 @@ defmodule Mv.Membership.MemberPoliciesTest do assert updated_member.first_name == "Updated" end end + + describe "member user link - only admin may set or change user link" do + test "normal_user can create member without :user argument", %{actor: _actor} do + normal_user = Mv.Fixtures.user_with_role_fixture("normal_user") + normal_user = Mv.Authorization.Actor.ensure_loaded(normal_user) + + {:ok, member} = + Membership.create_member( + %{ + first_name: "NoLink", + last_name: "Member", + email: "nolink#{System.unique_integer([:positive])}@example.com" + }, + actor: normal_user + ) + + assert member.first_name == "NoLink" + # Member has_one :user (FK on User side); ensure no user is linked + {:ok, member} = Ash.load(member, :user, domain: Mv.Membership) + assert is_nil(member.user) + end + + test "normal_user cannot create member with :user argument (forbidden)", %{actor: _actor} do + normal_user = Mv.Fixtures.user_with_role_fixture("normal_user") + normal_user = Mv.Authorization.Actor.ensure_loaded(normal_user) + # Another user to try to link to + other_user = Mv.Fixtures.user_with_role_fixture("read_only") + other_user = Mv.Authorization.Actor.ensure_loaded(other_user) + + attrs = %{ + first_name: "Linked", + last_name: "Member", + email: "linked#{System.unique_integer([:positive])}@example.com", + user: %{id: other_user.id} + } + + assert {:error, %Ash.Error.Forbidden{}} = + Membership.create_member(attrs, actor: normal_user) + end + + test "normal_user can update member without :user argument", %{actor: actor} do + normal_user = Mv.Fixtures.user_with_role_fixture("normal_user") + normal_user = Mv.Authorization.Actor.ensure_loaded(normal_user) + unlinked_member = create_unlinked_member(actor) + + {:ok, updated} = + Membership.update_member(unlinked_member, %{first_name: "UpdatedByNormal"}, + actor: normal_user + ) + + assert updated.first_name == "UpdatedByNormal" + end + + test "normal_user cannot update member with :user argument (forbidden)", %{actor: actor} do + normal_user = Mv.Fixtures.user_with_role_fixture("normal_user") + normal_user = Mv.Authorization.Actor.ensure_loaded(normal_user) + other_user = Mv.Fixtures.user_with_role_fixture("own_data") + other_user = Mv.Authorization.Actor.ensure_loaded(other_user) + unlinked_member = create_unlinked_member(actor) + + # Passing :user in params tries to link member to other_user - only admin may do that + params = %{first_name: unlinked_member.first_name, user: %{id: other_user.id}} + + assert {:error, %Ash.Error.Forbidden{}} = + Membership.update_member(unlinked_member, params, actor: normal_user) + end + + test "admin can create member with :user argument", %{actor: _actor} do + admin = Mv.Fixtures.user_with_role_fixture("admin") + admin = Mv.Authorization.Actor.ensure_loaded(admin) + link_target = Mv.Fixtures.user_with_role_fixture("own_data") + link_target = Mv.Authorization.Actor.ensure_loaded(link_target) + + attrs = %{ + first_name: "AdminLinked", + last_name: "Member", + email: "adminlinked#{System.unique_integer([:positive])}@example.com", + user: %{id: link_target.id} + } + + {:ok, member} = Membership.create_member(attrs, actor: admin) + + assert member.first_name == "AdminLinked" + # Reload link_target to see the new member_id set by manage_relationship + {:ok, link_target} = + Ash.get(Mv.Accounts.User, link_target.id, domain: Mv.Accounts, actor: admin) + + assert link_target.member_id == member.id + end + + test "admin can update member with :user argument (link)", %{actor: actor} do + admin = Mv.Fixtures.user_with_role_fixture("admin") + admin = Mv.Authorization.Actor.ensure_loaded(admin) + unlinked_member = create_unlinked_member(actor) + link_target = Mv.Fixtures.user_with_role_fixture("read_only") + link_target = Mv.Authorization.Actor.ensure_loaded(link_target) + + {:ok, updated} = + Membership.update_member( + unlinked_member, + %{user: %{id: link_target.id}}, + actor: admin + ) + + assert updated.id == unlinked_member.id + # Member should now be linked to link_target (user.member_id points to this member) + {:ok, reloaded_user} = + Ash.get(Mv.Accounts.User, link_target.id, + domain: Mv.Accounts, + load: [:member], + actor: admin + ) + + assert reloaded_user.member_id == updated.id + end + end end