From e47547e065ad2bf57c6975dce5f67419d8b5ec62 Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Wed, 4 Feb 2026 12:37:48 +0100
Subject: [PATCH 1/5] Add Role resource policies (defense-in-depth)

- PermissionSets: Role read :all for own_data, read_only, normal_user; admin keeps full CRUD
- Role resource: authorizers and policies with HasPermission
- Tests: role_policies_test.exs (read all, create/update/destroy admin only)
- Fix existing tests to pass actor or authorize?: false for Role operations
---
 lib/mv/authorization/permission_sets.ex      |  10 +-
 lib/mv/authorization/role.ex                 |  10 +-
 test/mv/authorization/role_policies_test.exs | 226 +++++++++++++++++++
 test/mv/authorization/role_test.exs          |  36 +--
 test/mv/helpers/system_actor_test.exs        |  45 ++--
 test/mv_web/authorization_test.exs           |   4 +-
 test/mv_web/live/role_live/show_test.exs     |   6 +-
 test/mv_web/live/role_live_test.exs          |  18 +-
 8 files changed, 304 insertions(+), 51 deletions(-)
 create mode 100644 test/mv/authorization/role_policies_test.exs

diff --git a/lib/mv/authorization/permission_sets.ex b/lib/mv/authorization/permission_sets.ex
index 9a5f7a7..b0e7015 100644
--- a/lib/mv/authorization/permission_sets.ex
+++ b/lib/mv/authorization/permission_sets.ex
@@ -78,6 +78,7 @@ defmodule Mv.Authorization.PermissionSets do
   defp custom_field_read_all, do: [perm("CustomField", :read, :all)]
   defp membership_fee_type_read_all, do: [perm("MembershipFeeType", :read, :all)]
   defp membership_fee_cycle_read_all, do: [perm("MembershipFeeCycle", :read, :all)]
+  defp role_read_all, do: [perm("Role", :read, :all)]
 
   @doc """
   Returns the list of all valid permission set names.
@@ -129,7 +130,8 @@ defmodule Mv.Authorization.PermissionSets do
           group_read_all() ++
           [perm("MemberGroup", :read, :linked)] ++
           membership_fee_type_read_all() ++
-          [perm("MembershipFeeCycle", :read, :linked)],
+          [perm("MembershipFeeCycle", :read, :linked)] ++
+          role_read_all(),
       pages: [
         # No "/" - Mitglied must not see member index at root (same content as /members).
         # Own profile (sidebar links to /users/:id) and own user edit
@@ -156,7 +158,8 @@ defmodule Mv.Authorization.PermissionSets do
           group_read_all() ++
           [perm("MemberGroup", :read, :all)] ++
           membership_fee_type_read_all() ++
-          membership_fee_cycle_read_all(),
+          membership_fee_cycle_read_all() ++
+          role_read_all(),
       pages: [
         "/",
         # Own profile (sidebar links to /users/:id; redirect target must be allowed)
@@ -211,7 +214,8 @@ defmodule Mv.Authorization.PermissionSets do
             perm("MembershipFeeCycle", :create, :all),
             perm("MembershipFeeCycle", :update, :all),
             perm("MembershipFeeCycle", :destroy, :all)
-          ],
+          ] ++
+          role_read_all(),
       pages: [
         "/",
         # Own profile (sidebar links to /users/:id; redirect target must be allowed)
diff --git a/lib/mv/authorization/role.ex b/lib/mv/authorization/role.ex
index 9c33e2d..59c0e51 100644
--- a/lib/mv/authorization/role.ex
+++ b/lib/mv/authorization/role.ex
@@ -37,7 +37,8 @@ defmodule Mv.Authorization.Role do
   """
   use Ash.Resource,
     domain: Mv.Authorization,
-    data_layer: AshPostgres.DataLayer
+    data_layer: AshPostgres.DataLayer,
+    authorizers: [Ash.Policy.Authorizer]
 
   postgres do
     table "roles"
@@ -86,6 +87,13 @@ defmodule Mv.Authorization.Role do
     end
   end
 
+  policies do
+    policy action_type([:read, :create, :update, :destroy]) do
+      description "Role access: read for all permission sets, create/update/destroy for admin only (PermissionSets)"
+      authorize_if Mv.Authorization.Checks.HasPermission
+    end
+  end
+
   validations do
     validate one_of(
                :permission_set_name,
diff --git a/test/mv/authorization/role_policies_test.exs b/test/mv/authorization/role_policies_test.exs
new file mode 100644
index 0000000..449f9d6
--- /dev/null
+++ b/test/mv/authorization/role_policies_test.exs
@@ -0,0 +1,226 @@
+defmodule Mv.Authorization.RolePoliciesTest do
+  @moduledoc """
+  Tests for Role resource authorization policies.
+
+  Rule: All permission sets (own_data, read_only, normal_user, admin) can **read** roles.
+  Only **admin** can create, update, or destroy roles.
+  """
+  use Mv.DataCase, async: false
+
+  alias Mv.Authorization
+  alias Mv.Authorization.Role
+
+  describe "read access - all permission sets can read roles" do
+    setup do
+      # Create a role to read (via system_actor; once policies exist, system_actor is admin)
+      role = Mv.Fixtures.role_fixture("read_only")
+      %{role: role}
+    end
+
+    @tag :permission_set_own_data
+    test "own_data can list roles", %{role: _role} do
+      user = Mv.Fixtures.user_with_role_fixture("own_data")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      {:ok, roles} = Authorization.list_roles(actor: user)
+
+      assert is_list(roles)
+      assert roles != []
+    end
+
+    @tag :permission_set_own_data
+    test "own_data can get role by id", %{role: role} do
+      user = Mv.Fixtures.user_with_role_fixture("own_data")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      {:ok, loaded} = Ash.get(Role, role.id, actor: user, domain: Mv.Authorization)
+
+      assert loaded.id == role.id
+    end
+
+    @tag :permission_set_read_only
+    test "read_only can list roles", %{role: _role} do
+      user = Mv.Fixtures.user_with_role_fixture("read_only")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      {:ok, roles} = Authorization.list_roles(actor: user)
+
+      assert is_list(roles)
+      assert roles != []
+    end
+
+    @tag :permission_set_read_only
+    test "read_only can get role by id", %{role: role} do
+      user = Mv.Fixtures.user_with_role_fixture("read_only")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      {:ok, loaded} = Ash.get(Role, role.id, actor: user, domain: Mv.Authorization)
+
+      assert loaded.id == role.id
+    end
+
+    @tag :permission_set_normal_user
+    test "normal_user can list roles", %{role: _role} do
+      user = Mv.Fixtures.user_with_role_fixture("normal_user")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      {:ok, roles} = Authorization.list_roles(actor: user)
+
+      assert is_list(roles)
+      assert roles != []
+    end
+
+    @tag :permission_set_normal_user
+    test "normal_user can get role by id", %{role: role} do
+      user = Mv.Fixtures.user_with_role_fixture("normal_user")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      {:ok, loaded} = Ash.get(Role, role.id, actor: user, domain: Mv.Authorization)
+
+      assert loaded.id == role.id
+    end
+
+    @tag :permission_set_admin
+    test "admin can list roles", %{role: _role} do
+      admin = Mv.Fixtures.user_with_role_fixture("admin")
+      admin = Mv.Authorization.Actor.ensure_loaded(admin)
+
+      {:ok, roles} = Authorization.list_roles(actor: admin)
+
+      assert is_list(roles)
+      assert roles != []
+    end
+
+    @tag :permission_set_admin
+    test "admin can get role by id", %{role: role} do
+      admin = Mv.Fixtures.user_with_role_fixture("admin")
+      admin = Mv.Authorization.Actor.ensure_loaded(admin)
+
+      {:ok, loaded} = Ash.get(Role, role.id, actor: admin, domain: Mv.Authorization)
+
+      assert loaded.id == role.id
+    end
+  end
+
+  describe "create/update/destroy - only admin allowed" do
+    setup do
+      # Non-system role for destroy test (role_fixture creates non-system roles)
+      role = Mv.Fixtures.role_fixture("normal_user")
+      %{role: role}
+    end
+
+    test "admin can create_role", %{role: _role} do
+      admin = Mv.Fixtures.user_with_role_fixture("admin")
+      admin = Mv.Authorization.Actor.ensure_loaded(admin)
+
+      attrs = %{
+        name: "New Role #{System.unique_integer([:positive])}",
+        description: "Test",
+        permission_set_name: "read_only"
+      }
+
+      assert {:ok, _created} = Authorization.create_role(attrs, actor: admin)
+    end
+
+    test "admin can update_role", %{role: role} do
+      admin = Mv.Fixtures.user_with_role_fixture("admin")
+      admin = Mv.Authorization.Actor.ensure_loaded(admin)
+
+      assert {:ok, updated} =
+               Authorization.update_role(role, %{description: "Updated by admin"}, actor: admin)
+
+      assert updated.description == "Updated by admin"
+    end
+
+    test "admin can destroy non-system role", %{role: role} do
+      admin = Mv.Fixtures.user_with_role_fixture("admin")
+      admin = Mv.Authorization.Actor.ensure_loaded(admin)
+
+      assert :ok = Authorization.destroy_role(role, actor: admin)
+    end
+
+    test "own_data cannot create_role (forbidden)", %{role: _role} do
+      user = Mv.Fixtures.user_with_role_fixture("own_data")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      attrs = %{
+        name: "New Role #{System.unique_integer([:positive])}",
+        description: "Test",
+        permission_set_name: "read_only"
+      }
+
+      assert {:error, %Ash.Error.Forbidden{}} = Authorization.create_role(attrs, actor: user)
+    end
+
+    test "own_data cannot update_role (forbidden)", %{role: role} do
+      user = Mv.Fixtures.user_with_role_fixture("own_data")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      assert {:error, %Ash.Error.Forbidden{}} =
+               Authorization.update_role(role, %{description: "Updated"}, actor: user)
+    end
+
+    test "own_data cannot destroy_role (forbidden)", %{role: role} do
+      user = Mv.Fixtures.user_with_role_fixture("own_data")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      assert {:error, %Ash.Error.Forbidden{}} = Authorization.destroy_role(role, actor: user)
+    end
+
+    test "read_only cannot create_role (forbidden)", %{role: _role} do
+      user = Mv.Fixtures.user_with_role_fixture("read_only")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      attrs = %{
+        name: "New Role #{System.unique_integer([:positive])}",
+        description: "Test",
+        permission_set_name: "read_only"
+      }
+
+      assert {:error, %Ash.Error.Forbidden{}} = Authorization.create_role(attrs, actor: user)
+    end
+
+    test "read_only cannot update_role (forbidden)", %{role: role} do
+      user = Mv.Fixtures.user_with_role_fixture("read_only")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      assert {:error, %Ash.Error.Forbidden{}} =
+               Authorization.update_role(role, %{description: "Updated"}, actor: user)
+    end
+
+    test "read_only cannot destroy_role (forbidden)", %{role: role} do
+      user = Mv.Fixtures.user_with_role_fixture("read_only")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      assert {:error, %Ash.Error.Forbidden{}} = Authorization.destroy_role(role, actor: user)
+    end
+
+    test "normal_user cannot create_role (forbidden)", %{role: _role} do
+      user = Mv.Fixtures.user_with_role_fixture("normal_user")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      attrs = %{
+        name: "New Role #{System.unique_integer([:positive])}",
+        description: "Test",
+        permission_set_name: "normal_user"
+      }
+
+      assert {:error, %Ash.Error.Forbidden{}} = Authorization.create_role(attrs, actor: user)
+    end
+
+    test "normal_user cannot update_role (forbidden)", %{role: role} do
+      user = Mv.Fixtures.user_with_role_fixture("normal_user")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      assert {:error, %Ash.Error.Forbidden{}} =
+               Authorization.update_role(role, %{description: "Updated"}, actor: user)
+    end
+
+    test "normal_user cannot destroy_role (forbidden)", %{role: role} do
+      user = Mv.Fixtures.user_with_role_fixture("normal_user")
+      user = Mv.Authorization.Actor.ensure_loaded(user)
+
+      assert {:error, %Ash.Error.Forbidden{}} = Authorization.destroy_role(role, actor: user)
+    end
+  end
+end
diff --git a/test/mv/authorization/role_test.exs b/test/mv/authorization/role_test.exs
index b7aa632..426719a 100644
--- a/test/mv/authorization/role_test.exs
+++ b/test/mv/authorization/role_test.exs
@@ -12,27 +12,29 @@ defmodule Mv.Authorization.RoleTest do
   end
 
   describe "permission_set_name validation" do
-    test "accepts valid permission set names" do
+    test "accepts valid permission set names", %{actor: actor} do
       attrs = %{
         name: "Test Role",
         permission_set_name: "own_data"
       }
 
-      assert {:ok, role} = Authorization.create_role(attrs)
+      assert {:ok, role} = Authorization.create_role(attrs, actor: actor)
       assert role.permission_set_name == "own_data"
     end
 
-    test "rejects invalid permission set names" do
+    test "rejects invalid permission set names", %{actor: actor} do
       attrs = %{
         name: "Test Role",
         permission_set_name: "invalid_set"
       }
 
-      assert {:error, %Ash.Error.Invalid{errors: errors}} = Authorization.create_role(attrs)
+      assert {:error, %Ash.Error.Invalid{errors: errors}} =
+               Authorization.create_role(attrs, actor: actor)
+
       assert error_message(errors, :permission_set_name) =~ "must be one of"
     end
 
-    test "accepts all four valid permission sets" do
+    test "accepts all four valid permission sets", %{actor: actor} do
       valid_sets = ["own_data", "read_only", "normal_user", "admin"]
 
       for permission_set <- valid_sets do
@@ -41,7 +43,7 @@ defmodule Mv.Authorization.RoleTest do
           permission_set_name: permission_set
         }
 
-        assert {:ok, _role} = Authorization.create_role(attrs)
+        assert {:ok, _role} = Authorization.create_role(attrs, actor: actor)
       end
     end
   end
@@ -60,34 +62,36 @@ defmodule Mv.Authorization.RoleTest do
       {:ok, system_role} = Ash.create(changeset, actor: actor)
 
       assert {:error, %Ash.Error.Invalid{errors: errors}} =
-               Authorization.destroy_role(system_role)
+               Authorization.destroy_role(system_role, actor: actor)
 
       message = error_message(errors, :is_system_role)
       assert message =~ "Cannot delete system role"
     end
 
-    test "allows deletion of non-system roles" do
+    test "allows deletion of non-system roles", %{actor: actor} do
       # is_system_role defaults to false, so regular create works
       {:ok, regular_role} =
-        Authorization.create_role(%{
-          name: "Regular Role",
-          permission_set_name: "read_only"
-        })
+        Authorization.create_role(
+          %{name: "Regular Role", permission_set_name: "read_only"},
+          actor: actor
+        )
 
-      assert :ok = Authorization.destroy_role(regular_role)
+      assert :ok = Authorization.destroy_role(regular_role, actor: actor)
     end
   end
 
   describe "name uniqueness" do
-    test "enforces unique role names" do
+    test "enforces unique role names", %{actor: actor} do
       attrs = %{
         name: "Unique Role",
         permission_set_name: "own_data"
       }
 
-      assert {:ok, _} = Authorization.create_role(attrs)
+      assert {:ok, _} = Authorization.create_role(attrs, actor: actor)
+
+      assert {:error, %Ash.Error.Invalid{errors: errors}} =
+               Authorization.create_role(attrs, actor: actor)
 
-      assert {:error, %Ash.Error.Invalid{errors: errors}} = Authorization.create_role(attrs)
       assert error_message(errors, :name) =~ "has already been taken"
     end
   end
diff --git a/test/mv/helpers/system_actor_test.exs b/test/mv/helpers/system_actor_test.exs
index c2715ae..add2ad5 100644
--- a/test/mv/helpers/system_actor_test.exs
+++ b/test/mv/helpers/system_actor_test.exs
@@ -18,18 +18,21 @@ defmodule Mv.Helpers.SystemActorTest do
     Ecto.Adapters.SQL.query!(Mv.Repo, "DELETE FROM users WHERE id = $1", [id])
   end
 
-  # Helper function to ensure admin role exists
+  # Helper function to ensure admin role exists (bootstrap: no actor yet, use authorize?: false)
   defp ensure_admin_role do
-    case Authorization.list_roles() do
+    case Authorization.list_roles(authorize?: false) do
       {:ok, roles} ->
         case Enum.find(roles, &(&1.permission_set_name == "admin")) do
           nil ->
             {:ok, role} =
-              Authorization.create_role(%{
-                name: "Admin",
-                description: "Administrator with full access",
-                permission_set_name: "admin"
-              })
+              Authorization.create_role(
+                %{
+                  name: "Admin",
+                  description: "Administrator with full access",
+                  permission_set_name: "admin"
+                },
+                authorize?: false
+              )
 
             role
 
@@ -39,11 +42,14 @@ defmodule Mv.Helpers.SystemActorTest do
 
       _ ->
         {:ok, role} =
-          Authorization.create_role(%{
-            name: "Admin",
-            description: "Administrator with full access",
-            permission_set_name: "admin"
-          })
+          Authorization.create_role(
+            %{
+              name: "Admin",
+              description: "Administrator with full access",
+              permission_set_name: "admin"
+            },
+            authorize?: false
+          )
 
         role
     end
@@ -364,12 +370,17 @@ defmodule Mv.Helpers.SystemActorTest do
 
     test "raises error if system user has wrong role", %{system_user: system_user} do
       # Create a non-admin role (using read_only as it's a valid permission set)
+      system_actor = SystemActor.get_system_actor()
+
       {:ok, read_only_role} =
-        Authorization.create_role(%{
-          name: "Read Only Role",
-          description: "Read-only access",
-          permission_set_name: "read_only"
-        })
+        Authorization.create_role(
+          %{
+            name: "Read Only Role",
+            description: "Read-only access",
+            permission_set_name: "read_only"
+          },
+          actor: system_actor
+        )
 
       system_actor = SystemActor.get_system_actor()
 
diff --git a/test/mv_web/authorization_test.exs b/test/mv_web/authorization_test.exs
index d07e482..7bb0b2a 100644
--- a/test/mv_web/authorization_test.exs
+++ b/test/mv_web/authorization_test.exs
@@ -50,14 +50,14 @@ defmodule MvWeb.AuthorizationTest do
       assert Authorization.can?(admin, :destroy, Mv.Authorization.Role) == true
     end
 
-    test "non-admin cannot manage roles" do
+    test "non-admin can read roles but cannot create/update/destroy" do
       normal_user = %{
         id: "normal-123",
         role: %{permission_set_name: "normal_user"}
       }
 
+      assert Authorization.can?(normal_user, :read, Mv.Authorization.Role) == true
       assert Authorization.can?(normal_user, :create, Mv.Authorization.Role) == false
-      assert Authorization.can?(normal_user, :read, Mv.Authorization.Role) == false
       assert Authorization.can?(normal_user, :update, Mv.Authorization.Role) == false
       assert Authorization.can?(normal_user, :destroy, Mv.Authorization.Role) == false
     end
diff --git a/test/mv_web/live/role_live/show_test.exs b/test/mv_web/live/role_live/show_test.exs
index ed099ec..fe5c48d 100644
--- a/test/mv_web/live/role_live/show_test.exs
+++ b/test/mv_web/live/role_live/show_test.exs
@@ -18,7 +18,7 @@ defmodule MvWeb.RoleLive.ShowTest do
   alias Mv.Authorization
   alias Mv.Authorization.Role
 
-  # Helper to create a role
+  # Helper to create a role (authorize?: false for test data setup)
   defp create_role(attrs \\ %{}) do
     default_attrs = %{
       name: "Test Role #{System.unique_integer([:positive])}",
@@ -28,7 +28,7 @@ defmodule MvWeb.RoleLive.ShowTest do
 
     attrs = Map.merge(default_attrs, attrs)
 
-    case Authorization.create_role(attrs) do
+    case Authorization.create_role(attrs, authorize?: false) do
       {:ok, role} -> role
       {:error, error} -> raise "Failed to create role: #{inspect(error)}"
     end
@@ -38,7 +38,7 @@ defmodule MvWeb.RoleLive.ShowTest do
   defp create_admin_user(conn, actor) do
     # Create admin role
     admin_role =
-      case Authorization.list_roles() do
+      case Authorization.list_roles(authorize?: false) do
         {:ok, roles} ->
           case Enum.find(roles, &(&1.name == "Admin")) do
             nil ->
diff --git a/test/mv_web/live/role_live_test.exs b/test/mv_web/live/role_live_test.exs
index 0edd2a4..cb112f2 100644
--- a/test/mv_web/live/role_live_test.exs
+++ b/test/mv_web/live/role_live_test.exs
@@ -9,7 +9,7 @@ defmodule MvWeb.RoleLiveTest do
   alias Mv.Authorization
   alias Mv.Authorization.Role
 
-  # Helper to create a role
+  # Helper to create a role (authorize?: false for test data setup)
   defp create_role(attrs \\ %{}) do
     default_attrs = %{
       name: "Test Role #{System.unique_integer([:positive])}",
@@ -19,7 +19,7 @@ defmodule MvWeb.RoleLiveTest do
 
     attrs = Map.merge(default_attrs, attrs)
 
-    case Authorization.create_role(attrs) do
+    case Authorization.create_role(attrs, authorize?: false) do
       {:ok, role} -> role
       {:error, error} -> raise "Failed to create role: #{inspect(error)}"
     end
@@ -29,7 +29,7 @@ defmodule MvWeb.RoleLiveTest do
   defp create_admin_user(conn, actor) do
     # Create admin role
     admin_role =
-      case Authorization.list_roles() do
+      case Authorization.list_roles(authorize?: false) do
         {:ok, roles} ->
           case Enum.find(roles, &(&1.name == "Admin")) do
             nil ->
@@ -332,7 +332,7 @@ defmodule MvWeb.RoleLiveTest do
       assert match?({:error, {:redirect, %{to: "/admin/roles"}}}, result)
     end
 
-    test "updates role name", %{conn: conn, role: role} do
+    test "updates role name", %{conn: conn, role: role, actor: actor} do
       {:ok, view, _html} = live(conn, "/admin/roles/#{role.id}/edit?return_to=show")
 
       attrs = %{
@@ -348,7 +348,7 @@ defmodule MvWeb.RoleLiveTest do
       assert_redirect(view, "/admin/roles/#{role.id}")
 
       # Verify update
-      {:ok, updated_role} = Authorization.get_role(role.id)
+      {:ok, updated_role} = Authorization.get_role(role.id, actor: actor)
       assert updated_role.name == "Updated Role Name"
     end
 
@@ -377,7 +377,7 @@ defmodule MvWeb.RoleLiveTest do
       assert_redirect(view, "/admin/roles/#{system_role.id}")
 
       # Verify update
-      {:ok, updated_role} = Authorization.get_role(system_role.id)
+      {:ok, updated_role} = Authorization.get_role(system_role.id, actor: actor)
       assert updated_role.permission_set_name == "read_only"
     end
   end
@@ -390,7 +390,7 @@ defmodule MvWeb.RoleLiveTest do
     end
 
     @tag :slow
-    test "deletes non-system role", %{conn: conn} do
+    test "deletes non-system role", %{conn: conn, actor: actor} do
       role = create_role()
 
       {:ok, view, html} = live(conn, "/admin/roles")
@@ -404,7 +404,7 @@ defmodule MvWeb.RoleLiveTest do
 
       # Verify deletion by checking database
       assert {:error, %Ash.Error.Invalid{errors: [%Ash.Error.Query.NotFound{}]}} =
-               Authorization.get_role(role.id)
+               Authorization.get_role(role.id, actor: actor)
     end
 
     test "fails to delete system role with error message", %{conn: conn, actor: actor} do
@@ -430,7 +430,7 @@ defmodule MvWeb.RoleLiveTest do
       assert render(view) =~ "System roles cannot be deleted"
 
       # Role should still exist
-      {:ok, _role} = Authorization.get_role(system_role.id)
+      {:ok, _role} = Authorization.get_role(system_role.id, actor: actor)
     end
   end
 

From b70ece21297edb847fee530bf134fd90fe715594 Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Wed, 4 Feb 2026 12:50:10 +0100
Subject: [PATCH 2/5] 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

From 46dcb932d8a24f8808d3fe6f6ebc4346a362fffa Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Wed, 4 Feb 2026 12:54:15 +0100
Subject: [PATCH 3/5] Docs: permission hardening Role and member user link
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Role: Ash policies (HasPermission); read for all, create/update/destroy admin only.
User–member link: only admins may set :user on Member create/update (ForbidMemberUserLinkUnlessAdmin).
---
 docs/roles-and-permissions-architecture.md | 37 +++++++++++-----------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md
index 92ad3c5..14a396d 100644
--- a/docs/roles-and-permissions-architecture.md
+++ b/docs/roles-and-permissions-architecture.md
@@ -1025,17 +1025,16 @@ defmodule Mv.Membership.Member do
       authorize_if expr(id == ^actor(:member_id))
     end
     
-    # 2. GENERAL: Check permissions from role
-    # - :own_data → can UPDATE linked member (scope :linked via HasPermission)
-    # - :read_only → can READ all members (scope :all), no update permission
-    # - :normal_user → can CRUD all members (scope :all)
-    # - :admin → can CRUD all members (scope :all)
+    # 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)
+    # 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"
+      description "Check permissions and forbid user link unless admin"
+      forbid_if Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin
       authorize_if Mv.Authorization.Checks.HasPermission
     end
     
-    # 4. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed)
+    # 3. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed)
   end
   
   # Custom validation for email editing (see Special Cases section)
@@ -1054,6 +1053,8 @@ 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`.
+
 **Permission Matrix:**
 
 | Action | Mitglied | Vorstand | Kassenwart | Buchhaltung | Admin |
@@ -1148,23 +1149,20 @@ end
 
 **Location:** `lib/mv/authorization/role.ex`
 
-**Special Protection:** System roles cannot be deleted.
+**Defense-in-depth:** The Role resource uses `authorizers: [Ash.Policy.Authorizer]` and policies with `Mv.Authorization.Checks.HasPermission`. **Read** is allowed for all permission sets (own_data, read_only, normal_user, admin) via `perm("Role", :read, :all)` in PermissionSets; reading roles is not a security concern. **Create, update, and destroy** are allowed only for admin (admin has full Role CRUD in PermissionSets). Seeds and bootstrap use `authorize?: false` where necessary.
+
+**Special Protection:** System roles cannot be deleted (validation on destroy).
 
 ```elixir
 defmodule Mv.Authorization.Role do
-  use Ash.Resource, ...
+  use Ash.Resource,
+    authorizers: [Ash.Policy.Authorizer]
   
   policies do
-    # Only admin can manage roles
     policy action_type([:read, :create, :update, :destroy]) do
-      description "Check permissions from user's role"
+      description "Check permissions from user's role (read all, create/update/destroy admin only)"
       authorize_if Mv.Authorization.Checks.HasPermission
     end
-    
-    # DEFAULT: Forbid
-    policy action_type([:read, :create, :update, :destroy]) do
-      forbid_if always()
-    end
   end
   
   # Prevent deletion of system roles
@@ -1201,7 +1199,7 @@ end
 
 | Action | Mitglied | Vorstand | Kassenwart | Buchhaltung | Admin |
 |--------|----------|----------|------------|-------------|-------|
-| Read | ❌ | ❌ | ❌ | ❌ | ✅ |
+| Read | ✅ | ✅ | ✅ | ✅ | ✅ |
 | Create | ❌ | ❌ | ❌ | ❌ | ✅ |
 | Update | ❌ | ❌ | ❌ | ❌ | ✅ |
 | Destroy* | ❌ | ❌ | ❌ | ❌ | ✅ |
@@ -2045,7 +2043,10 @@ Users and Members are separate entities that can be linked. Special rules:
 - A user cannot link themselves to an existing member
 - A user CAN create a new member and be directly linked to it (self-service)
 
-**Enforcement:** The User resource restricts the `update_user` action (which accepts the `member` argument for link/unlink) to admins only via `Mv.Authorization.Checks.ActorIsAdmin`. The UserLive.Form shows the Member-Linking UI and runs member link/unlink on save only when the current user is admin; non-admins use the `:update` action (email only) for profile edit.
+**Enforcement:**
+
+- **User side:** The User resource restricts the `update_user` action (which accepts the `member` argument for link/unlink) to admins only via `Mv.Authorization.Checks.ActorIsAdmin`. The UserLive.Form shows the Member-Linking UI and runs member link/unlink on save only when the current user is admin; non-admins use the `:update` action (email only) for profile edit.
+- **Member side:** Only admins may set or change the user–member link on **Member** create or update. When creating or updating a member, the `:user` argument (which links the member to a user account) is forbidden for non-admins. This is enforced by `Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin` in the Member resource policies (`forbid_if` before `authorize_if HasPermission`). Non-admins (e.g. normal_user / Kassenwart) can still create and update members as long as they do not pass the `:user` argument.
 
 ### Approach: Separate Ash Actions
 

From 65ac6ca1d082355c184443adad061a36215f005f Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Wed, 4 Feb 2026 13:24:14 +0100
Subject: [PATCH 4/5] Refactor member user-link tests: shared setup

Use describe-level setup for normal_user, admin, unlinked_member.
---
 test/mv/membership/member_policies_test.exs | 78 ++++++++++++---------
 1 file changed, 44 insertions(+), 34 deletions(-)

diff --git a/test/mv/membership/member_policies_test.exs b/test/mv/membership/member_policies_test.exs
index 30936fe..287d0bb 100644
--- a/test/mv/membership/member_policies_test.exs
+++ b/test/mv/membership/member_policies_test.exs
@@ -405,10 +405,21 @@ defmodule Mv.Membership.MemberPoliciesTest do
   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)
+    setup %{actor: actor} do
+      normal_user =
+        Mv.Fixtures.user_with_role_fixture("normal_user")
+        |> Mv.Authorization.Actor.ensure_loaded()
 
+      admin =
+        Mv.Fixtures.user_with_role_fixture("admin")
+        |> Mv.Authorization.Actor.ensure_loaded()
+
+      unlinked_member = create_unlinked_member(actor)
+
+      %{normal_user: normal_user, admin: admin, unlinked_member: unlinked_member}
+    end
+
+    test "normal_user can create member without :user argument", %{normal_user: normal_user} do
       {:ok, member} =
         Membership.create_member(
           %{
@@ -425,12 +436,12 @@ defmodule Mv.Membership.MemberPoliciesTest do
       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)
+    test "normal_user cannot create member with :user argument (forbidden)", %{
+      normal_user: normal_user
+    } do
+      other_user =
+        Mv.Fixtures.user_with_role_fixture("read_only")
+        |> Mv.Authorization.Actor.ensure_loaded()
 
       attrs = %{
         first_name: "Linked",
@@ -443,11 +454,10 @@ defmodule Mv.Membership.MemberPoliciesTest do
                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)
-
+    test "normal_user can update member without :user argument", %{
+      normal_user: normal_user,
+      unlinked_member: unlinked_member
+    } do
       {:ok, updated} =
         Membership.update_member(unlinked_member, %{first_name: "UpdatedByNormal"},
           actor: normal_user
@@ -456,25 +466,24 @@ defmodule Mv.Membership.MemberPoliciesTest do
       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)
+    test "normal_user cannot update member with :user argument (forbidden)", %{
+      normal_user: normal_user,
+      unlinked_member: unlinked_member
+    } do
+      other_user =
+        Mv.Fixtures.user_with_role_fixture("own_data")
+        |> Mv.Authorization.Actor.ensure_loaded()
 
-      # 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)
+    test "admin can create member with :user argument", %{admin: admin} do
+      link_target =
+        Mv.Fixtures.user_with_role_fixture("own_data")
+        |> Mv.Authorization.Actor.ensure_loaded()
 
       attrs = %{
         first_name: "AdminLinked",
@@ -486,19 +495,20 @@ defmodule Mv.Membership.MemberPoliciesTest do
       {: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)
+    test "admin can update member with :user argument (link)", %{
+      admin: admin,
+      unlinked_member: unlinked_member
+    } do
+      link_target =
+        Mv.Fixtures.user_with_role_fixture("read_only")
+        |> Mv.Authorization.Actor.ensure_loaded()
 
       {:ok, updated} =
         Membership.update_member(
@@ -508,7 +518,7 @@ defmodule Mv.Membership.MemberPoliciesTest do
         )
 
       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,

From f342350537bc8ca2fc12dd049e89daf5bee9673d Mon Sep 17 00:00:00 2001
From: Moritz <moritz.m@local-it.org>
Date: Wed, 4 Feb 2026 13:46:49 +0100
Subject: [PATCH 5/5] 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")