From e47547e065ad2bf57c6975dce5f67419d8b5ec62 Mon Sep 17 00:00:00 2001 From: Moritz Date: Wed, 4 Feb 2026 12:37:48 +0100 Subject: [PATCH] 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