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

@@ -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:**
 

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

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

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")