From f342350537bc8ca2fc12dd049e89daf5bee9673d Mon Sep 17 00:00:00 2001 From: Moritz Date: Wed, 4 Feb 2026 13:46:49 +0100 Subject: [PATCH] Harden member user-link check: argument presence, nil actor, policy scope - Forbid on :user argument presence (not value) to block unlink via nil/empty - Defensive nil actor handling; policy restricted to create/update only - Test: Ash.load with actor; test non-admin cannot unlink via user: nil - Docs: unlink behaviour and policy split --- docs/roles-and-permissions-architecture.md | 20 ++++--- lib/membership/member.ex | 14 +++-- .../forbid_member_user_link_unless_admin.ex | 54 ++++++++++--------- test/mv/membership/member_policies_test.exs | 27 +++++++++- 4 files changed, 79 insertions(+), 36 deletions(-) diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md index 14a396d..0035a1e 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -1025,16 +1025,22 @@ defmodule Mv.Membership.Member do authorize_if expr(id == ^actor(:member_id)) end - # 2. GENERAL: Forbid user link unless admin; then check permissions from role - # ForbidMemberUserLinkUnlessAdmin: only admins may pass :user on create/update (no-op for read/destroy) + # 2. READ/DESTROY: Check permissions only (no :user argument on these actions) + policy action_type([:read, :destroy]) do + description "Check permissions from user's role" + authorize_if Mv.Authorization.Checks.HasPermission + end + + # 3. CREATE/UPDATE: Forbid user link unless admin; then check permissions + # ForbidMemberUserLinkUnlessAdmin: only admins may pass :user (link or unlink via nil/empty). # 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 and forbid user link unless admin" + policy action_type([:create, :update]) do + description "Forbid user link unless admin; then check permissions" forbid_if Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin authorize_if Mv.Authorization.Checks.HasPermission end - - # 3. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed) + + # 4. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed) end # Custom validation for email editing (see Special Cases section) @@ -1053,7 +1059,7 @@ end - **READ list queries**: No record at strict_check time → bypass with `expr(id == ^actor(:member_id))` needed for auto_filter ✅ - **UPDATE operations**: Changeset contains record → HasPermission evaluates `scope :linked` correctly ✅ -**User–member link:** Only admins may set or change the `:user` argument on create_member or update_member (see [User-Member Linking](#user-member-linking)). Non-admins can create/update members without passing `:user`. +**User–member link:** Only admins may pass the `:user` argument on create_member or update_member (link or unlink via `user: nil`/`user: %{}`). The check uses **argument presence** (key in arguments), not value, to avoid bypass (see [User-Member Linking](#user-member-linking)). **Permission Matrix:** diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 8517634..fc007ac 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -312,11 +312,17 @@ defmodule Mv.Membership.Member do authorize_if expr(id == ^actor(:member_id)) end - # 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). + # READ/DESTROY: Check permissions only (no :user argument on these actions) + policy action_type([:read, :destroy]) do + description "Check permissions from user's role" + authorize_if Mv.Authorization.Checks.HasPermission + end + + # CREATE/UPDATE: Forbid member–user link unless admin, then check permissions + # ForbidMemberUserLinkUnlessAdmin: only admins may pass :user (link or unlink via nil/empty). # 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 and forbid user link unless admin" + policy action_type([:create, :update]) do + description "Forbid user link unless admin; then check permissions" 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 index facfdb2..ab4af9d 100644 --- a/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex +++ b/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex @@ -3,13 +3,23 @@ defmodule Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin do 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. + **is present** (key in arguments, regardless of value), only admins may perform the action. + This covers: + - **Linking:** `user: %{id: user_id}` → only admin + - **Unlinking:** `user: nil` or `user: %{}` on update_member triggers `on_missing: :unrelate` → only admin + Non-admin users (e.g. normal_user / Kassenwart) can create and update members only when + they do **not** pass the `:user` argument at all. + + ## Unlink via Member actions + + Unlink is intended via Member update_member: when `:user` is not provided in params, + manage_relationship uses `on_missing: :unrelate` and removes the link. Passing `user: nil` + or `user: %{}` explicitly is still "changing the link" and is forbidden for non-admins + (argument presence is checked, not value). ## Usage - In Member resource policies, add **before** the general HasPermission policy: + In Member resource policies, restrict to create/update only: policy action_type([:create, :update]) do forbid_if Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin @@ -18,8 +28,9 @@ defmodule Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin do ## 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 the `:user` argument **key is not present** → does not forbid. + - If `:user` is present (any value, including nil or %{}) and actor is not admin → forbids. + - If actor is nil → treated as non-admin (forbid when :user present); no crash. - If actor is admin (or system actor) → does not forbid. """ use Ash.Policy.Check @@ -31,35 +42,30 @@ defmodule Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin do @impl true def strict_check(actor, authorizer, _opts) do - actor = Actor.ensure_loaded(actor) + # Defensive: nil actor → treat as non-admin (Actor.ensure_loaded(nil) and admin?(nil) are safe) + actor = if is_nil(actor), do: nil, else: Actor.ensure_loaded(actor) - if user_argument_set?(authorizer) and not Actor.admin?(actor) do + if user_argument_present?(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) + # Forbid when :user was passed at all (link, unlink via nil/empty, or invalid value). + # Check argument key presence, not value, to avoid bypass via user: nil or user: %{}. + defp user_argument_present?(authorizer) do + args = get_arguments(authorizer) + Map.has_key?(args || %{}, :user) end - defp get_user_argument(authorizer) do - changeset = authorizer.changeset || authorizer.subject + defp get_arguments(authorizer) do + subject = 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 + is_struct(subject, Ash.Changeset) -> subject.arguments + is_struct(subject, Ash.ActionInput) -> subject.arguments + true -> %{} 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 287d0bb..d9ab95c 100644 --- a/test/mv/membership/member_policies_test.exs +++ b/test/mv/membership/member_policies_test.exs @@ -432,7 +432,9 @@ defmodule Mv.Membership.MemberPoliciesTest do 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) + {:ok, member} = + Ash.load(member, :user, domain: Mv.Membership, actor: normal_user) + assert is_nil(member.user) end @@ -480,6 +482,29 @@ defmodule Mv.Membership.MemberPoliciesTest do Membership.update_member(unlinked_member, params, actor: normal_user) end + test "normal_user cannot update member with user: nil (unlink forbidden)", %{ + normal_user: normal_user, + unlinked_member: unlinked_member + } do + # Link member first (via admin), then normal_user tries to unlink via user: nil + admin = + Mv.Fixtures.user_with_role_fixture("admin") |> Mv.Authorization.Actor.ensure_loaded() + + link_target = + Mv.Fixtures.user_with_role_fixture("own_data") |> Mv.Authorization.Actor.ensure_loaded() + + {:ok, linked_member} = + Membership.update_member( + unlinked_member, + %{user: %{id: link_target.id}}, + actor: admin + ) + + # Passing user: nil explicitly tries to unlink; only admin may do that + assert {:error, %Ash.Error.Forbidden{}} = + Membership.update_member(linked_member, %{user: nil}, actor: normal_user) + end + test "admin can create member with :user argument", %{admin: admin} do link_target = Mv.Fixtures.user_with_role_fixture("own_data")