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

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

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,

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

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(%{
+        Authorization.create_role(
-          name: "Regular Role",
+          %{name: "Regular Role", permission_set_name: "read_only"},
-          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

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(%{
+              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(%{
+          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(%{
+        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()
 

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

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

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