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

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