From 3a0fb4e84f626695f91d4cb9127b6750c7832a3a Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 6 Jan 2026 19:50:00 +0100 Subject: [PATCH 1/6] feat: implement PermissionSets module with all 4 permission sets - Add types for scope, action, resource_permission, permission_set - Implement get_permissions/1 for all 4 sets (own_data, read_only, normal_user, admin) - Implement valid_permission_set?/1 for string and atom validation - Implement permission_set_name_to_atom/1 with error handling --- lib/mv/authorization/permission_sets.ex | 250 +++++++- .../mv/authorization/permission_sets_test.exs | 568 ++++++++++++++++++ 2 files changed, 813 insertions(+), 5 deletions(-) create mode 100644 test/mv/authorization/permission_sets_test.exs diff --git a/lib/mv/authorization/permission_sets.ex b/lib/mv/authorization/permission_sets.ex index d01e285..22b1648 100644 --- a/lib/mv/authorization/permission_sets.ex +++ b/lib/mv/authorization/permission_sets.ex @@ -2,23 +2,60 @@ defmodule Mv.Authorization.PermissionSets do @moduledoc """ Defines the four hardcoded permission sets for the application. - This is a minimal stub implementation. The full implementation - with all permission details will be added in a subsequent issue. + Each permission set specifies: + - Resource permissions (what CRUD operations on which resources) + - Page permissions (which LiveView pages can be accessed) + - Scopes (own, linked, all) ## Permission Sets 1. **own_data** - Default for "Mitglied" role + - Can only access own user data and linked member/custom field values + - Cannot create new members or manage system + 2. **read_only** - For "Vorstand" and "Buchhaltung" roles + - Can read all member data + - Cannot create, update, or delete + 3. **normal_user** - For "Kassenwart" role + - Create/Read/Update members (no delete for safety), full CRUD on custom field values + - Cannot manage custom fields or users + 4. **admin** - For "Admin" role + - Unrestricted access to all resources + - Can manage users, roles, custom fields ## Usage - # Get list of all valid permission set names - PermissionSets.all_permission_sets() - # => [:own_data, :read_only, :normal_user, :admin] + # Get permissions for a role's permission set + permissions = PermissionSets.get_permissions(:admin) + + # Check if a permission set name is valid + PermissionSets.valid_permission_set?("read_only") # => true + + # Convert string to atom safely + {:ok, atom} = PermissionSets.permission_set_name_to_atom("own_data") + + ## Performance + + All functions are pure and compile-time. Permission lookups are < 1 microsecond. """ + @type scope :: :own | :linked | :all + @type action :: :read | :create | :update | :destroy + + @type resource_permission :: %{ + resource: String.t(), + action: action(), + scope: scope(), + granted: boolean() + } + + @type permission_set :: %{ + resources: [resource_permission()], + pages: [String.t()] + } + @doc """ Returns the list of all valid permission set names. @@ -31,4 +68,207 @@ defmodule Mv.Authorization.PermissionSets do def all_permission_sets do [:own_data, :read_only, :normal_user, :admin] end + + @doc """ + Returns permissions for the given permission set. + + ## Examples + + iex> permissions = PermissionSets.get_permissions(:admin) + iex> Enum.any?(permissions.resources, fn p -> + ...> p.resource == "User" and p.action == :destroy + ...> end) + true + + iex> PermissionSets.get_permissions(:invalid) + ** (FunctionClauseError) no function clause matching + """ + @spec get_permissions(atom()) :: permission_set() + + def get_permissions(:own_data) do + %{ + resources: [ + # User: Can always read/update own credentials + %{resource: "User", action: :read, scope: :own, granted: true}, + %{resource: "User", action: :update, scope: :own, granted: true}, + + # Member: Can read/update linked member + %{resource: "Member", action: :read, scope: :linked, granted: true}, + %{resource: "Member", action: :update, scope: :linked, granted: true}, + + # CustomFieldValue: Can read/update custom field values of linked member + %{resource: "CustomFieldValue", action: :read, scope: :linked, granted: true}, + %{resource: "CustomFieldValue", action: :update, scope: :linked, granted: true}, + + # CustomField: Can read all (needed for forms) + %{resource: "CustomField", action: :read, scope: :all, granted: true} + ], + pages: [ + # Home page + "/", + # Own profile + "/profile", + # Linked member detail (filtered by policy) + "/members/:id" + ] + } + end + + def get_permissions(:read_only) do + %{ + resources: [ + # User: Can read/update own credentials only + %{resource: "User", action: :read, scope: :own, granted: true}, + %{resource: "User", action: :update, scope: :own, granted: true}, + + # Member: Can read all members, no modifications + %{resource: "Member", action: :read, scope: :all, granted: true}, + + # CustomFieldValue: Can read all custom field values + %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, + + # CustomField: Can read all + %{resource: "CustomField", action: :read, scope: :all, granted: true} + ], + pages: [ + "/", + # Member list + "/members", + # Member detail + "/members/:id", + # Custom field values overview + "/custom_field_values" + ] + } + end + + def get_permissions(:normal_user) do + %{ + resources: [ + # User: Can read/update own credentials only + %{resource: "User", action: :read, scope: :own, granted: true}, + %{resource: "User", action: :update, scope: :own, granted: true}, + + # Member: Full CRUD except destroy (safety) + %{resource: "Member", action: :read, scope: :all, granted: true}, + %{resource: "Member", action: :create, scope: :all, granted: true}, + %{resource: "Member", action: :update, scope: :all, granted: true}, + # Note: destroy intentionally omitted for safety + + # CustomFieldValue: Full CRUD + %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :create, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :update, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :destroy, scope: :all, granted: true}, + + # CustomField: Read only (admin manages definitions) + %{resource: "CustomField", action: :read, scope: :all, granted: true} + ], + pages: [ + "/", + "/members", + # Create member + "/members/new", + "/members/:id", + # Edit member + "/members/:id/edit", + "/custom_field_values", + "/custom_field_values/new", + "/custom_field_values/:id/edit" + ] + } + end + + def get_permissions(:admin) do + %{ + resources: [ + # User: Full management including other users + %{resource: "User", action: :read, scope: :all, granted: true}, + %{resource: "User", action: :create, scope: :all, granted: true}, + %{resource: "User", action: :update, scope: :all, granted: true}, + %{resource: "User", action: :destroy, scope: :all, granted: true}, + + # Member: Full CRUD + %{resource: "Member", action: :read, scope: :all, granted: true}, + %{resource: "Member", action: :create, scope: :all, granted: true}, + %{resource: "Member", action: :update, scope: :all, granted: true}, + %{resource: "Member", action: :destroy, scope: :all, granted: true}, + + # CustomFieldValue: Full CRUD + %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :create, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :update, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :destroy, scope: :all, granted: true}, + + # CustomField: Full CRUD (admin manages custom field definitions) + %{resource: "CustomField", action: :read, scope: :all, granted: true}, + %{resource: "CustomField", action: :create, scope: :all, granted: true}, + %{resource: "CustomField", action: :update, scope: :all, granted: true}, + %{resource: "CustomField", action: :destroy, scope: :all, granted: true}, + + # Role: Full CRUD (admin manages roles) + %{resource: "Role", action: :read, scope: :all, granted: true}, + %{resource: "Role", action: :create, scope: :all, granted: true}, + %{resource: "Role", action: :update, scope: :all, granted: true}, + %{resource: "Role", action: :destroy, scope: :all, granted: true} + ], + pages: [ + # Wildcard: Admin can access all pages + "*" + ] + } + end + + @doc """ + Checks if a permission set name (string or atom) is valid. + + ## Examples + + iex> PermissionSets.valid_permission_set?("admin") + true + + iex> PermissionSets.valid_permission_set?(:read_only) + true + + iex> PermissionSets.valid_permission_set?("invalid") + false + """ + @spec valid_permission_set?(String.t() | atom()) :: boolean() + def valid_permission_set?(name) when is_binary(name) do + case permission_set_name_to_atom(name) do + {:ok, _atom} -> true + {:error, _} -> false + end + end + + def valid_permission_set?(name) when is_atom(name) do + name in all_permission_sets() + end + + def valid_permission_set?(_), do: false + + @doc """ + Converts a permission set name string to atom safely. + + ## Examples + + iex> PermissionSets.permission_set_name_to_atom("admin") + {:ok, :admin} + + iex> PermissionSets.permission_set_name_to_atom("invalid") + {:error, :invalid_permission_set} + """ + @spec permission_set_name_to_atom(String.t()) :: + {:ok, atom()} | {:error, :invalid_permission_set} + def permission_set_name_to_atom(name) when is_binary(name) do + atom = String.to_existing_atom(name) + + if valid_permission_set?(atom) do + {:ok, atom} + else + {:error, :invalid_permission_set} + end + rescue + ArgumentError -> {:error, :invalid_permission_set} + end end diff --git a/test/mv/authorization/permission_sets_test.exs b/test/mv/authorization/permission_sets_test.exs new file mode 100644 index 0000000..51dc797 --- /dev/null +++ b/test/mv/authorization/permission_sets_test.exs @@ -0,0 +1,568 @@ +defmodule Mv.Authorization.PermissionSetsTest do + @moduledoc """ + Tests for the PermissionSets module that defines hardcoded permission sets. + """ + use ExUnit.Case, async: true + + alias Mv.Authorization.PermissionSets + + describe "all_permission_sets/0" do + test "returns all four permission sets" do + sets = PermissionSets.all_permission_sets() + + assert length(sets) == 4 + assert :own_data in sets + assert :read_only in sets + assert :normal_user in sets + assert :admin in sets + end + end + + describe "get_permissions/1" do + test "returns map with :resources and :pages keys for :own_data" do + permissions = PermissionSets.get_permissions(:own_data) + + assert Map.has_key?(permissions, :resources) + assert Map.has_key?(permissions, :pages) + assert is_list(permissions.resources) + assert is_list(permissions.pages) + end + + test "returns map with :resources and :pages keys for :read_only" do + permissions = PermissionSets.get_permissions(:read_only) + + assert Map.has_key?(permissions, :resources) + assert Map.has_key?(permissions, :pages) + assert is_list(permissions.resources) + assert is_list(permissions.pages) + end + + test "returns map with :resources and :pages keys for :normal_user" do + permissions = PermissionSets.get_permissions(:normal_user) + + assert Map.has_key?(permissions, :resources) + assert Map.has_key?(permissions, :pages) + assert is_list(permissions.resources) + assert is_list(permissions.pages) + end + + test "returns map with :resources and :pages keys for :admin" do + permissions = PermissionSets.get_permissions(:admin) + + assert Map.has_key?(permissions, :resources) + assert Map.has_key?(permissions, :pages) + assert is_list(permissions.resources) + assert is_list(permissions.pages) + end + + test "each resource permission has required keys" do + permissions = PermissionSets.get_permissions(:own_data) + + Enum.each(permissions.resources, fn perm -> + assert Map.has_key?(perm, :resource) + assert Map.has_key?(perm, :action) + assert Map.has_key?(perm, :scope) + assert Map.has_key?(perm, :granted) + assert is_binary(perm.resource) + assert perm.action in [:read, :create, :update, :destroy] + assert perm.scope in [:own, :linked, :all] + assert is_boolean(perm.granted) + end) + end + + test "pages lists are non-empty for all permission sets" do + for set <- [:own_data, :read_only, :normal_user, :admin] do + permissions = PermissionSets.get_permissions(set) + + assert permissions.pages != [], + "Permission set #{set} should have at least one page" + end + end + end + + describe "get_permissions/1 - :own_data permission content" do + test "allows User read/update with scope :own" do + permissions = PermissionSets.get_permissions(:own_data) + + user_read = + Enum.find(permissions.resources, fn p -> p.resource == "User" && p.action == :read end) + + user_update = + Enum.find(permissions.resources, fn p -> p.resource == "User" && p.action == :update end) + + assert user_read.scope == :own + assert user_read.granted == true + assert user_update.scope == :own + assert user_update.granted == true + end + + test "allows Member read/update with scope :linked" do + permissions = PermissionSets.get_permissions(:own_data) + + member_read = + Enum.find(permissions.resources, fn p -> p.resource == "Member" && p.action == :read end) + + member_update = + Enum.find(permissions.resources, fn p -> p.resource == "Member" && p.action == :update end) + + assert member_read.scope == :linked + assert member_read.granted == true + assert member_update.scope == :linked + assert member_update.granted == true + end + + test "allows CustomFieldValue read/update with scope :linked" do + permissions = PermissionSets.get_permissions(:own_data) + + cfv_read = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :read + end) + + cfv_update = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :update + end) + + assert cfv_read.scope == :linked + assert cfv_read.granted == true + assert cfv_update.scope == :linked + assert cfv_update.granted == true + end + + test "allows CustomField read with scope :all" do + permissions = PermissionSets.get_permissions(:own_data) + + cf_read = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomField" && p.action == :read + end) + + assert cf_read.scope == :all + assert cf_read.granted == true + end + + test "includes correct pages" do + permissions = PermissionSets.get_permissions(:own_data) + + assert "/" in permissions.pages + assert "/profile" in permissions.pages + assert "/members/:id" in permissions.pages + end + end + + describe "get_permissions/1 - :read_only permission content" do + test "allows User read/update with scope :own" do + permissions = PermissionSets.get_permissions(:read_only) + + user_read = + Enum.find(permissions.resources, fn p -> p.resource == "User" && p.action == :read end) + + user_update = + Enum.find(permissions.resources, fn p -> p.resource == "User" && p.action == :update end) + + assert user_read.scope == :own + assert user_read.granted == true + assert user_update.scope == :own + assert user_update.granted == true + end + + test "allows Member read with scope :all" do + permissions = PermissionSets.get_permissions(:read_only) + + member_read = + Enum.find(permissions.resources, fn p -> p.resource == "Member" && p.action == :read end) + + assert member_read.scope == :all + assert member_read.granted == true + end + + test "does NOT allow Member create/update/destroy" do + permissions = PermissionSets.get_permissions(:read_only) + + member_create = + Enum.find(permissions.resources, fn p -> p.resource == "Member" && p.action == :create end) + + member_update = + Enum.find(permissions.resources, fn p -> p.resource == "Member" && p.action == :update end) + + member_destroy = + Enum.find(permissions.resources, fn p -> + p.resource == "Member" && p.action == :destroy + end) + + assert member_create == nil || member_create.granted == false + assert member_update == nil || member_update.granted == false + assert member_destroy == nil || member_destroy.granted == false + end + + test "allows CustomFieldValue read with scope :all" do + permissions = PermissionSets.get_permissions(:read_only) + + cfv_read = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :read + end) + + assert cfv_read.scope == :all + assert cfv_read.granted == true + end + + test "does NOT allow CustomFieldValue create/update/destroy" do + permissions = PermissionSets.get_permissions(:read_only) + + cfv_create = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :create + end) + + cfv_update = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :update + end) + + cfv_destroy = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :destroy + end) + + assert cfv_create == nil || cfv_create.granted == false + assert cfv_update == nil || cfv_update.granted == false + assert cfv_destroy == nil || cfv_destroy.granted == false + end + + test "allows CustomField read with scope :all" do + permissions = PermissionSets.get_permissions(:read_only) + + cf_read = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomField" && p.action == :read + end) + + assert cf_read.scope == :all + assert cf_read.granted == true + end + + test "includes correct pages" do + permissions = PermissionSets.get_permissions(:read_only) + + assert "/" in permissions.pages + assert "/members" in permissions.pages + assert "/members/:id" in permissions.pages + assert "/custom_field_values" in permissions.pages + end + end + + describe "get_permissions/1 - :normal_user permission content" do + test "allows User read/update with scope :own" do + permissions = PermissionSets.get_permissions(:normal_user) + + user_read = + Enum.find(permissions.resources, fn p -> p.resource == "User" && p.action == :read end) + + user_update = + Enum.find(permissions.resources, fn p -> p.resource == "User" && p.action == :update end) + + assert user_read.scope == :own + assert user_read.granted == true + assert user_update.scope == :own + assert user_update.granted == true + end + + test "allows Member read/create/update with scope :all" do + permissions = PermissionSets.get_permissions(:normal_user) + + member_read = + Enum.find(permissions.resources, fn p -> p.resource == "Member" && p.action == :read end) + + member_create = + Enum.find(permissions.resources, fn p -> p.resource == "Member" && p.action == :create end) + + member_update = + Enum.find(permissions.resources, fn p -> p.resource == "Member" && p.action == :update end) + + assert member_read.scope == :all + assert member_read.granted == true + assert member_create.scope == :all + assert member_create.granted == true + assert member_update.scope == :all + assert member_update.granted == true + end + + test "does NOT allow Member destroy (safety)" do + permissions = PermissionSets.get_permissions(:normal_user) + + member_destroy = + Enum.find(permissions.resources, fn p -> + p.resource == "Member" && p.action == :destroy + end) + + assert member_destroy == nil || member_destroy.granted == false + end + + test "allows CustomFieldValue full CRUD with scope :all" do + permissions = PermissionSets.get_permissions(:normal_user) + + cfv_read = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :read + end) + + cfv_create = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :create + end) + + cfv_update = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :update + end) + + cfv_destroy = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :destroy + end) + + assert cfv_read.scope == :all + assert cfv_read.granted == true + assert cfv_create.scope == :all + assert cfv_create.granted == true + assert cfv_update.scope == :all + assert cfv_update.granted == true + assert cfv_destroy.scope == :all + assert cfv_destroy.granted == true + end + + test "allows CustomField read with scope :all" do + permissions = PermissionSets.get_permissions(:normal_user) + + cf_read = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomField" && p.action == :read + end) + + assert cf_read.scope == :all + assert cf_read.granted == true + end + + test "includes correct pages" do + permissions = PermissionSets.get_permissions(:normal_user) + + assert "/" in permissions.pages + assert "/members" in permissions.pages + assert "/members/new" in permissions.pages + assert "/members/:id" in permissions.pages + assert "/members/:id/edit" in permissions.pages + assert "/custom_field_values" in permissions.pages + assert "/custom_field_values/new" in permissions.pages + assert "/custom_field_values/:id/edit" in permissions.pages + end + end + + describe "get_permissions/1 - :admin permission content" do + test "allows User full CRUD with scope :all" do + permissions = PermissionSets.get_permissions(:admin) + + user_read = + Enum.find(permissions.resources, fn p -> p.resource == "User" && p.action == :read end) + + user_create = + Enum.find(permissions.resources, fn p -> p.resource == "User" && p.action == :create end) + + user_update = + Enum.find(permissions.resources, fn p -> p.resource == "User" && p.action == :update end) + + user_destroy = + Enum.find(permissions.resources, fn p -> p.resource == "User" && p.action == :destroy end) + + assert user_read.scope == :all + assert user_read.granted == true + assert user_create.scope == :all + assert user_create.granted == true + assert user_update.scope == :all + assert user_update.granted == true + assert user_destroy.scope == :all + assert user_destroy.granted == true + end + + test "allows Member full CRUD with scope :all" do + permissions = PermissionSets.get_permissions(:admin) + + member_read = + Enum.find(permissions.resources, fn p -> p.resource == "Member" && p.action == :read end) + + member_create = + Enum.find(permissions.resources, fn p -> p.resource == "Member" && p.action == :create end) + + member_update = + Enum.find(permissions.resources, fn p -> p.resource == "Member" && p.action == :update end) + + member_destroy = + Enum.find(permissions.resources, fn p -> + p.resource == "Member" && p.action == :destroy + end) + + assert member_read.scope == :all + assert member_read.granted == true + assert member_create.scope == :all + assert member_create.granted == true + assert member_update.scope == :all + assert member_update.granted == true + assert member_destroy.scope == :all + assert member_destroy.granted == true + end + + test "allows CustomFieldValue full CRUD with scope :all" do + permissions = PermissionSets.get_permissions(:admin) + + cfv_read = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :read + end) + + cfv_create = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :create + end) + + cfv_update = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :update + end) + + cfv_destroy = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomFieldValue" && p.action == :destroy + end) + + assert cfv_read.scope == :all + assert cfv_read.granted == true + assert cfv_create.scope == :all + assert cfv_create.granted == true + assert cfv_update.scope == :all + assert cfv_update.granted == true + assert cfv_destroy.scope == :all + assert cfv_destroy.granted == true + end + + test "allows CustomField full CRUD with scope :all" do + permissions = PermissionSets.get_permissions(:admin) + + cf_read = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomField" && p.action == :read + end) + + cf_create = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomField" && p.action == :create + end) + + cf_update = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomField" && p.action == :update + end) + + cf_destroy = + Enum.find(permissions.resources, fn p -> + p.resource == "CustomField" && p.action == :destroy + end) + + assert cf_read.scope == :all + assert cf_read.granted == true + assert cf_create.scope == :all + assert cf_create.granted == true + assert cf_update.scope == :all + assert cf_update.granted == true + assert cf_destroy.scope == :all + assert cf_destroy.granted == true + end + + test "allows Role full CRUD with scope :all" do + permissions = PermissionSets.get_permissions(:admin) + + role_read = + Enum.find(permissions.resources, fn p -> p.resource == "Role" && p.action == :read end) + + role_create = + Enum.find(permissions.resources, fn p -> p.resource == "Role" && p.action == :create end) + + role_update = + Enum.find(permissions.resources, fn p -> p.resource == "Role" && p.action == :update end) + + role_destroy = + Enum.find(permissions.resources, fn p -> p.resource == "Role" && p.action == :destroy end) + + assert role_read.scope == :all + assert role_read.granted == true + assert role_create.scope == :all + assert role_create.granted == true + assert role_update.scope == :all + assert role_update.granted == true + assert role_destroy.scope == :all + assert role_destroy.granted == true + end + + test "has wildcard page permission" do + permissions = PermissionSets.get_permissions(:admin) + + assert "*" in permissions.pages + end + end + + describe "valid_permission_set?/1" do + test "returns true for valid permission set string" do + assert PermissionSets.valid_permission_set?("own_data") == true + assert PermissionSets.valid_permission_set?("read_only") == true + assert PermissionSets.valid_permission_set?("normal_user") == true + assert PermissionSets.valid_permission_set?("admin") == true + end + + test "returns true for valid permission set atom" do + assert PermissionSets.valid_permission_set?(:own_data) == true + assert PermissionSets.valid_permission_set?(:read_only) == true + assert PermissionSets.valid_permission_set?(:normal_user) == true + assert PermissionSets.valid_permission_set?(:admin) == true + end + + test "returns false for invalid permission set string" do + assert PermissionSets.valid_permission_set?("invalid") == false + assert PermissionSets.valid_permission_set?("") == false + assert PermissionSets.valid_permission_set?("admin_user") == false + end + + test "returns false for invalid permission set atom" do + assert PermissionSets.valid_permission_set?(:invalid) == false + assert PermissionSets.valid_permission_set?(:unknown) == false + end + + test "returns false for nil input" do + assert PermissionSets.valid_permission_set?(nil) == false + end + end + + describe "permission_set_name_to_atom/1" do + test "returns {:ok, atom} for valid permission set name" do + assert PermissionSets.permission_set_name_to_atom("own_data") == {:ok, :own_data} + assert PermissionSets.permission_set_name_to_atom("read_only") == {:ok, :read_only} + assert PermissionSets.permission_set_name_to_atom("normal_user") == {:ok, :normal_user} + assert PermissionSets.permission_set_name_to_atom("admin") == {:ok, :admin} + end + + test "returns {:error, :invalid_permission_set} for invalid permission set name" do + assert PermissionSets.permission_set_name_to_atom("invalid") == + {:error, :invalid_permission_set} + + assert PermissionSets.permission_set_name_to_atom("") == {:error, :invalid_permission_set} + + assert PermissionSets.permission_set_name_to_atom("admin_user") == + {:error, :invalid_permission_set} + end + + test "handles non-existent atom gracefully" do + # String.to_existing_atom will raise ArgumentError for non-existent atoms + assert PermissionSets.permission_set_name_to_atom("nonexistent_atom_12345") == + {:error, :invalid_permission_set} + end + end +end From 19a20635a77161ca65bcc110a9db6d6f320d3e44 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 6 Jan 2026 19:52:58 +0100 Subject: [PATCH 2/6] docs: update documentation to use CustomFieldValue/CustomField instead of Property/PropertyType --- docs/roles-and-permissions-architecture.md | 126 ++++++++--------- ...les-and-permissions-implementation-plan.md | 130 +++++++++--------- docs/roles-and-permissions-overview.md | 4 +- 3 files changed, 130 insertions(+), 130 deletions(-) diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md index fa45d86..b44604b 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -93,8 +93,8 @@ Five predefined roles stored in the `roles` table: Control CRUD operations on: - User (credentials, profile) - Member (member data) -- Property (custom field values) -- PropertyType (custom field definitions) +- CustomFieldValue (custom field values) +- CustomField (custom field definitions) - Role (role management) **4. Page-Level Permissions** @@ -111,7 +111,7 @@ Three scope levels for permissions: - **:own** - Only records where `record.id == user.id` (for User resource) - **:linked** - Only records linked to user via relationships - Member: `member.user_id == user.id` - - Property: `property.member.user_id == user.id` + - CustomFieldValue: `custom_field_value.member.user_id == user.id` - **:all** - All records, no filtering **6. Special Cases** @@ -414,7 +414,7 @@ defmodule Mv.Authorization.PermissionSets do ## Permission Sets 1. **own_data** - Default for "Mitglied" role - - Can only access own user data and linked member/properties + - Can only access own user data and linked member/custom field values - Cannot create new members or manage system 2. **read_only** - For "Vorstand" and "Buchhaltung" roles @@ -423,11 +423,11 @@ defmodule Mv.Authorization.PermissionSets do 3. **normal_user** - For "Kassenwart" role - Create/Read/Update members (no delete), full CRUD on properties - - Cannot manage property types or users + - Cannot manage custom fields or users 4. **admin** - For "Admin" role - Unrestricted access to all resources - - Can manage users, roles, property types + - Can manage users, roles, custom fields ## Usage @@ -500,12 +500,12 @@ defmodule Mv.Authorization.PermissionSets do %{resource: "Member", action: :read, scope: :linked, granted: true}, %{resource: "Member", action: :update, scope: :linked, granted: true}, - # Property: Can read/update properties of linked member - %{resource: "Property", action: :read, scope: :linked, granted: true}, - %{resource: "Property", action: :update, scope: :linked, granted: true}, + # CustomFieldValue: Can read/update custom field values of linked member + %{resource: "CustomFieldValue", action: :read, scope: :linked, granted: true}, + %{resource: "CustomFieldValue", action: :update, scope: :linked, granted: true}, - # PropertyType: Can read all (needed for forms) - %{resource: "PropertyType", action: :read, scope: :all, granted: true} + # CustomField: Can read all (needed for forms) + %{resource: "CustomField", action: :read, scope: :all, granted: true} ], pages: [ "/", # Home page @@ -525,17 +525,17 @@ defmodule Mv.Authorization.PermissionSets do # Member: Can read all members, no modifications %{resource: "Member", action: :read, scope: :all, granted: true}, - # Property: Can read all properties - %{resource: "Property", action: :read, scope: :all, granted: true}, + # CustomFieldValue: Can read all custom field values + %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, - # PropertyType: Can read all - %{resource: "PropertyType", action: :read, scope: :all, granted: true} + # CustomField: Can read all + %{resource: "CustomField", action: :read, scope: :all, granted: true} ], pages: [ "/", "/members", # Member list "/members/:id", # Member detail - "/properties", # Property overview + "/custom_field_values" # Custom field values overview "/profile" # Own profile ] } @@ -554,14 +554,14 @@ defmodule Mv.Authorization.PermissionSets do %{resource: "Member", action: :update, scope: :all, granted: true}, # Note: destroy intentionally omitted for safety - # Property: Full CRUD - %{resource: "Property", action: :read, scope: :all, granted: true}, - %{resource: "Property", action: :create, scope: :all, granted: true}, - %{resource: "Property", action: :update, scope: :all, granted: true}, - %{resource: "Property", action: :destroy, scope: :all, granted: true}, + # CustomFieldValue: Full CRUD + %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :create, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :update, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :destroy, scope: :all, granted: true}, - # PropertyType: Read only (admin manages definitions) - %{resource: "PropertyType", action: :read, scope: :all, granted: true} + # CustomField: Read only (admin manages definitions) + %{resource: "CustomField", action: :read, scope: :all, granted: true} ], pages: [ "/", @@ -569,9 +569,9 @@ defmodule Mv.Authorization.PermissionSets do "/members/new", # Create member "/members/:id", "/members/:id/edit", # Edit member - "/properties", - "/properties/new", - "/properties/:id/edit", + "/custom_field_values", + "/custom_field_values/new", + "/custom_field_values/:id/edit", "/profile" ] } @@ -592,17 +592,17 @@ defmodule Mv.Authorization.PermissionSets do %{resource: "Member", action: :update, scope: :all, granted: true}, %{resource: "Member", action: :destroy, scope: :all, granted: true}, - # Property: Full CRUD - %{resource: "Property", action: :read, scope: :all, granted: true}, - %{resource: "Property", action: :create, scope: :all, granted: true}, - %{resource: "Property", action: :update, scope: :all, granted: true}, - %{resource: "Property", action: :destroy, scope: :all, granted: true}, + # CustomFieldValue: Full CRUD + %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :create, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :update, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :destroy, scope: :all, granted: true}, - # PropertyType: Full CRUD (admin manages custom field definitions) - %{resource: "PropertyType", action: :read, scope: :all, granted: true}, - %{resource: "PropertyType", action: :create, scope: :all, granted: true}, - %{resource: "PropertyType", action: :update, scope: :all, granted: true}, - %{resource: "PropertyType", action: :destroy, scope: :all, granted: true}, + # CustomField: Full CRUD (admin manages custom field definitions) + %{resource: "CustomField", action: :read, scope: :all, granted: true}, + %{resource: "CustomField", action: :create, scope: :all, granted: true}, + %{resource: "CustomField", action: :update, scope: :all, granted: true}, + %{resource: "CustomField", action: :destroy, scope: :all, granted: true}, # Role: Full CRUD (admin manages roles) %{resource: "Role", action: :read, scope: :all, granted: true}, @@ -677,9 +677,9 @@ Quick reference table showing what each permission set allows: | **User** (all) | - | - | - | R, C, U, D | | **Member** (linked) | R, U | - | - | - | | **Member** (all) | - | R | R, C, U | R, C, U, D | -| **Property** (linked) | R, U | - | - | - | -| **Property** (all) | - | R | R, C, U, D | R, C, U, D | -| **PropertyType** (all) | R | R | R | R, C, U, D | +| **CustomFieldValue** (linked) | R, U | - | - | - | +| **CustomFieldValue** (all) | - | R | R, C, U, D | R, C, U, D | +| **CustomField** (all) | R | R | R | R, C, U, D | | **Role** (all) | - | - | - | R, C, U, D | **Legend:** R=Read, C=Create, U=Update, D=Destroy @@ -715,7 +715,7 @@ defmodule Mv.Authorization.Checks.HasPermission do - **:own** - Filters to records where record.id == actor.id - **:linked** - Filters based on resource type: - Member: member.user_id == actor.id - - Property: property.member.user_id == actor.id (traverses relationship!) + - CustomFieldValue: custom_field_value.member.user_id == actor.id (traverses relationship!) ## Error Handling @@ -802,8 +802,8 @@ defmodule Mv.Authorization.Checks.HasPermission do # Member.user_id == actor.id (direct relationship) {:filter, expr(user_id == ^actor.id)} - "Property" -> - # Property.member.user_id == actor.id (traverse through member!) + "CustomFieldValue" -> + # CustomFieldValue.member.user_id == actor.id (traverse through member!) {:filter, expr(member.user_id == ^actor.id)} _ -> @@ -832,7 +832,7 @@ end **Key Design Decisions:** -1. **Resource-Specific :linked Scope:** Property needs to traverse `member` relationship to check `user_id` +1. **Resource-Specific :linked Scope:** CustomFieldValue needs to traverse `member` relationship to check `user_id` 2. **Error Handling:** All errors log for debugging but return generic forbidden to user 3. **Module Name Extraction:** Uses `Module.split() |> List.last()` to match against PermissionSets strings 4. **Pure Function:** No side effects, deterministic, easily testable @@ -966,21 +966,21 @@ end *Email editing has additional validation (see Special Cases) -### Property Resource Policies +### CustomFieldValue Resource Policies -**Location:** `lib/mv/membership/property.ex` +**Location:** `lib/mv/membership/custom_field_value.ex` -**Special Case:** Users can access properties of their linked member. +**Special Case:** Users can access custom field values of their linked member. ```elixir -defmodule Mv.Membership.Property do +defmodule Mv.Membership.CustomFieldValue do use Ash.Resource, ... policies do - # SPECIAL CASE: Users can access properties of their linked member + # SPECIAL CASE: Users can access custom field values of their linked member # Note: This traverses the member relationship! policy action_type([:read, :update]) do - description "Users can access properties of their linked member" + description "Users can access custom field values of their linked member" authorize_if expr(member.user_id == ^actor(:id)) end @@ -1010,18 +1010,18 @@ end | Create | ❌ | ❌ | ✅ | ❌ | ✅ | | Destroy | ❌ | ❌ | ✅ | ❌ | ✅ | -### PropertyType Resource Policies +### CustomField Resource Policies -**Location:** `lib/mv/membership/property_type.ex` +**Location:** `lib/mv/membership/custom_field.ex` **No Special Cases:** All users can read, only admin can write. ```elixir -defmodule Mv.Membership.PropertyType do +defmodule Mv.Membership.CustomField do use Ash.Resource, ... policies do - # All authenticated users can read property types (needed for forms) + # All authenticated users can read custom fields (needed for forms) # Write operations are admin-only policy action_type([:read, :create, :update, :destroy]) do description "Check permissions from user's role" @@ -1308,12 +1308,12 @@ end - ❌ Cannot access: `/members`, `/members/new`, `/admin/roles` **Vorstand (read_only):** -- ✅ Can access: `/`, `/members`, `/members/123`, `/properties`, `/profile` +- ✅ Can access: `/`, `/members`, `/members/123`, `/custom_field_values`, `/profile` - ❌ Cannot access: `/members/new`, `/members/123/edit`, `/admin/roles` **Kassenwart (normal_user):** -- ✅ Can access: `/`, `/members`, `/members/new`, `/members/123/edit`, `/properties`, `/profile` -- ❌ Cannot access: `/admin/roles`, `/admin/property_types/new` +- ✅ Can access: `/`, `/members`, `/members/new`, `/members/123/edit`, `/custom_field_values`, `/profile` +- ❌ Cannot access: `/admin/roles`, `/admin/custom_fields/new` **Admin:** - ✅ Can access: `*` (all pages, including `/admin/roles`) @@ -1479,9 +1479,9 @@ defmodule MvWeb.Authorization do # Direct relationship: member.user_id Map.get(record, :user_id) == user.id - "Property" -> - # Need to traverse: property.member.user_id - # Note: In UI, property should have member preloaded + "CustomFieldValue" -> + # Need to traverse: custom_field_value.member.user_id + # Note: In UI, custom_field_value should have member preloaded case Map.get(record, :member) do %{user_id: member_user_id} -> member_user_id == user.id _ -> false @@ -1569,7 +1569,7 @@ end Admin <% end %> @@ -2409,8 +2409,8 @@ The `HasPermission` check extracts resource names via `Module.split() |> List.la |------------|------------------------| | `Mv.Accounts.User` | "User" | | `Mv.Membership.Member` | "Member" | -| `Mv.Membership.Property` | "Property" | -| `Mv.Membership.PropertyType` | "PropertyType" | +| `Mv.Membership.CustomFieldValue` | "CustomFieldValue" | +| `Mv.Membership.CustomField` | "CustomField" | | `Mv.Authorization.Role` | "Role" | These strings must match exactly in `PermissionSets` module. @@ -2450,7 +2450,7 @@ These strings must match exactly in `PermissionSets` module. **Integration:** - [ ] One complete user journey per role -- [ ] Cross-resource scenarios (e.g., Member -> Property) +- [ ] Cross-resource scenarios (e.g., Member -> CustomFieldValue) - [ ] Special cases in context (e.g., linked member email during full edit flow) ### Useful Commands diff --git a/docs/roles-and-permissions-implementation-plan.md b/docs/roles-and-permissions-implementation-plan.md index 0b173fa..2c29b8d 100644 --- a/docs/roles-and-permissions-implementation-plan.md +++ b/docs/roles-and-permissions-implementation-plan.md @@ -53,7 +53,7 @@ This document defines the implementation plan for the **MVP (Phase 1)** of the R Hardcoded in `Mv.Authorization.PermissionSets` module: 1. **own_data** - User can only access their own data (default for "Mitglied") -2. **read_only** - Read access to all members/properties (for "Vorstand", "Buchhaltung") +2. **read_only** - Read access to all members/custom field values (for "Vorstand", "Buchhaltung") 3. **normal_user** - Create/Read/Update members (no delete), full CRUD on properties (for "Kassenwart") 4. **admin** - Unrestricted access including user/role management (for "Admin") @@ -77,7 +77,7 @@ Stored in database `roles` table, each referencing a `permission_set_name`: - ✅ Hardcoded PermissionSets module with 4 permission sets - ✅ Role database table and CRUD interface - ✅ Custom Ash Policy Check (`HasPermission`) that reads from PermissionSets -- ✅ Policies on all resources (Member, User, Property, PropertyType, Role) +- ✅ Policies on all resources (Member, User, CustomFieldValue, CustomField, Role) - ✅ Page-level permissions via Phoenix Plug - ✅ UI authorization helpers for conditional rendering - ✅ Special case: Member email validation for linked users @@ -228,32 +228,32 @@ Create the core `PermissionSets` module that defines all four permission sets wi - Resources: - User: read/update :own - Member: read/update :linked - - Property: read/update :linked - - PropertyType: read :all + - CustomFieldValue: read/update :linked + - CustomField: read :all - Pages: `["/", "/profile", "/members/:id"]` **2. read_only (Vorstand, Buchhaltung):** - Resources: - User: read :own, update :own - Member: read :all - - Property: read :all - - PropertyType: read :all -- Pages: `["/", "/members", "/members/:id", "/properties"]` + - CustomFieldValue: read :all + - CustomField: read :all +- Pages: `["/", "/members", "/members/:id", "/custom_field_values"]` **3. normal_user (Kassenwart):** - Resources: - User: read/update :own - Member: read/create/update :all (no destroy for safety) - - Property: read/create/update/destroy :all - - PropertyType: read :all -- Pages: `["/", "/members", "/members/new", "/members/:id", "/members/:id/edit", "/properties", "/properties/new", "/properties/:id/edit"]` + - CustomFieldValue: read/create/update/destroy :all + - CustomField: read :all +- Pages: `["/", "/members", "/members/new", "/members/:id", "/members/:id/edit", "/custom_field_values", "/custom_field_values/new", "/custom_field_values/:id/edit"]` **4. admin:** - Resources: - User: read/update/destroy :all - Member: read/create/update/destroy :all - - Property: read/create/update/destroy :all - - PropertyType: read/create/update/destroy :all + - CustomFieldValue: read/create/update/destroy :all + - CustomField: read/create/update/destroy :all - Role: read/create/update/destroy :all - Pages: `["*"]` (wildcard = all pages) @@ -276,10 +276,10 @@ Create the core `PermissionSets` module that defines all four permission sets wi **Permission Content Tests:** - `:own_data` allows User read/update with scope :own -- `:own_data` allows Member/Property read/update with scope :linked -- `:read_only` allows Member/Property read with scope :all -- `:read_only` does NOT allow Member/Property create/update/destroy -- `:normal_user` allows Member/Property full CRUD with scope :all +- `:own_data` allows Member/CustomFieldValue read/update with scope :linked +- `:read_only` allows Member/CustomFieldValue read with scope :all +- `:read_only` does NOT allow Member/CustomFieldValue create/update/destroy +- `:normal_user` allows Member/CustomFieldValue full CRUD with scope :all - `:admin` allows everything with scope :all - `:admin` has wildcard page permission "*" @@ -387,7 +387,7 @@ Create the core custom Ash Policy Check that reads permissions from the `Permiss - `:own` → `{:filter, expr(id == ^actor.id)}` - `:linked` → resource-specific logic: - Member: `{:filter, expr(user_id == ^actor.id)}` - - Property: `{:filter, expr(member.user_id == ^actor.id)}` (traverse relationship!) + - CustomFieldValue: `{:filter, expr(member.user_id == ^actor.id)}` (traverse relationship!) 6. Handle errors gracefully: - No actor → `{:error, :no_actor}` - No role → `{:error, :no_role}` @@ -401,7 +401,7 @@ Create the core custom Ash Policy Check that reads permissions from the `Permiss - [ ] Check module implements `Ash.Policy.Check` behavior - [ ] `match?/3` correctly evaluates permissions from PermissionSets - [ ] Scope filters work correctly (:all, :own, :linked) -- [ ] `:linked` scope handles Member and Property differently +- [ ] `:linked` scope handles Member and CustomFieldValue differently - [ ] Errors are handled gracefully (no crashes) - [ ] Authorization failures are logged - [ ] Module is well-documented @@ -425,7 +425,7 @@ Create the core custom Ash Policy Check that reads permissions from the `Permiss **Scope Application Tests - :linked:** - Actor with scope :linked can access Member where member.user_id == actor.id -- Actor with scope :linked can access Property where property.member.user_id == actor.id (relationship traversal!) +- Actor with scope :linked can access CustomFieldValue where custom_field_value.member.user_id == actor.id (relationship traversal!) - Actor with scope :linked cannot access unlinked member - Query correctly filters based on user_id relationship @@ -581,7 +581,7 @@ Add authorization policies to the User resource. Special case: Users can always --- -#### Issue #9: Property Resource Policies +#### Issue #9: CustomFieldValue Resource Policies **Size:** M (2 days) **Dependencies:** #6 (HasPermission check) @@ -590,20 +590,20 @@ Add authorization policies to the User resource. Special case: Users can always **Description:** -Add authorization policies to the Property resource. Properties are linked to members, which are linked to users. +Add authorization policies to the CustomFieldValue resource. CustomFieldValues are linked to members, which are linked to users. **Tasks:** -1. Open `lib/mv/membership/property.ex` +1. Open `lib/mv/membership/custom_field_value.ex` 2. Add `policies` block -3. Add special policy: Allow user to read/update properties of their linked member +3. Add special policy: Allow user to read/update custom field values of their linked member ```elixir policy action_type([:read, :update]) do authorize_if expr(member.user_id == ^actor(:id)) end ``` 4. Add general policy: Check HasPermission -5. Ensure Property preloads :member relationship for scope checks +5. Ensure CustomFieldValue preloads :member relationship for scope checks 6. Preload :role relationship for actor **Policy Order:** @@ -620,27 +620,27 @@ Add authorization policies to the Property resource. Properties are linked to me **Test Strategy (TDD):** -**Linked Properties Tests (:own_data):** -- User can read properties of their linked member -- User can update properties of their linked member -- User cannot read properties of unlinked members -- Verify relationship traversal works (property.member.user_id) +**Linked CustomFieldValues Tests (:own_data):** +- User can read custom field values of their linked member +- User can update custom field values of their linked member +- User cannot read custom field values of unlinked members +- Verify relationship traversal works (custom_field_value.member.user_id) **Read-Only Tests:** -- User with :read_only can read all properties -- User with :read_only cannot create/update properties +- User with :read_only can read all custom field values +- User with :read_only cannot create/update custom field values **Normal User Tests:** -- User with :normal_user can CRUD properties +- User with :normal_user can CRUD custom field values **Admin Tests:** - Admin can perform all operations -**Test File:** `test/mv/membership/property_policies_test.exs` +**Test File:** `test/mv/membership/custom_field_value_policies_test.exs` --- -#### Issue #10: PropertyType Resource Policies +#### Issue #10: CustomField Resource Policies **Size:** S (1 day) **Dependencies:** #6 (HasPermission check) @@ -649,11 +649,11 @@ Add authorization policies to the Property resource. Properties are linked to me **Description:** -Add authorization policies to the PropertyType resource. PropertyTypes are admin-managed, but readable by all. +Add authorization policies to the CustomField resource. CustomFields are admin-managed, but readable by all. **Tasks:** -1. Open `lib/mv/membership/property_type.ex` +1. Open `lib/mv/membership/custom_field.ex` 2. Add `policies` block 3. Add read policy: All authenticated users can read (scope :all) 4. Add write policies: Only admin can create/update/destroy @@ -661,27 +661,27 @@ Add authorization policies to the PropertyType resource. PropertyTypes are admin **Acceptance Criteria:** -- [ ] All users can read property types -- [ ] Only admin can create/update/destroy property types +- [ ] All users can read custom fields +- [ ] Only admin can create/update/destroy custom fields - [ ] Policies tested **Test Strategy (TDD):** **Read Access (All Roles):** -- User with :own_data can read all property types -- User with :read_only can read all property types -- User with :normal_user can read all property types -- User with :admin can read all property types +- User with :own_data can read all custom fields +- User with :read_only can read all custom fields +- User with :normal_user can read all custom fields +- User with :admin can read all custom fields **Write Access (Admin Only):** -- Non-admin cannot create property type (Forbidden) -- Non-admin cannot update property type (Forbidden) -- Non-admin cannot destroy property type (Forbidden) -- Admin can create property type -- Admin can update property type -- Admin can destroy property type +- Non-admin cannot create custom field (Forbidden) +- Non-admin cannot update custom field (Forbidden) +- Non-admin cannot destroy custom field (Forbidden) +- Admin can create custom field +- Admin can update custom field +- Admin can destroy custom field -**Test File:** `test/mv/membership/property_type_policies_test.exs` +**Test File:** `test/mv/membership/custom_field_policies_test.exs` --- @@ -924,7 +924,7 @@ Create helper functions for UI-level authorization checks. These will be used in ``` 5. All functions use `PermissionSets.get_permissions/1` (same logic as HasPermission) 6. All functions handle nil user gracefully (return false) -7. Implement resource-specific scope checking (Member vs Property for :linked) +7. Implement resource-specific scope checking (Member vs CustomFieldValue for :linked) 8. Add comprehensive `@doc` with template examples 9. Import helper in `mv_web.ex` `html_helpers` section @@ -957,9 +957,9 @@ Create helper functions for UI-level authorization checks. These will be used in **can?/3 with Record Struct - Scope :linked:** - User can update linked Member (member.user_id == user.id) - User cannot update unlinked Member -- User can update Property of linked Member (property.member.user_id == user.id) -- User cannot update Property of unlinked Member -- Scope checking is resource-specific (Member vs Property) +- User can update CustomFieldValue of linked Member (custom_field_value.member.user_id == user.id) +- User cannot update CustomFieldValue of unlinked Member +- Scope checking is resource-specific (Member vs CustomFieldValue) **can_access_page?/2:** - User with page in list can access (returns true) @@ -1046,7 +1046,7 @@ Update Role management LiveViews to use authorization helpers for conditional re **Description:** -Update all existing LiveViews (Member, User, Property, PropertyType) to use authorization helpers for conditional rendering. +Update all existing LiveViews (Member, User, CustomFieldValue, CustomField) to use authorization helpers for conditional rendering. **Tasks:** @@ -1061,10 +1061,10 @@ Update all existing LiveViews (Member, User, Property, PropertyType) to use auth - Show: Only show other users if admin, always show own profile - Edit: Only allow editing own profile or admin editing anyone -3. **Property LiveViews:** +3. **CustomFieldValue LiveViews:** - Similar to Member (hide create/edit/delete based on permissions) -4. **PropertyType LiveViews:** +4. **CustomField LiveViews:** - All users can view - Only admin can create/edit/delete @@ -1110,13 +1110,13 @@ Update all existing LiveViews (Member, User, Property, PropertyType) to use auth - Vorstand: Sees "Home", "Members" (read-only), "Profile" - Kassenwart: Sees "Home", "Members", "Properties", "Profile" - Buchhaltung: Sees "Home", "Members" (read-only), "Profile" -- Admin: Sees "Home", "Members", "Properties", "Property Types", "Admin", "Profile" +- Admin: Sees "Home", "Members", "Custom Field Values", "Custom Fields", "Admin", "Profile" **Test Files:** - `test/mv_web/live/member_live_authorization_test.exs` - `test/mv_web/live/user_live_authorization_test.exs` -- `test/mv_web/live/property_live_authorization_test.exs` -- `test/mv_web/live/property_type_live_authorization_test.exs` +- `test/mv_web/live/custom_field_value_live_authorization_test.exs` +- `test/mv_web/live/custom_field_live_authorization_test.exs` - `test/mv_web/components/navbar_authorization_test.exs` --- @@ -1192,7 +1192,7 @@ Write comprehensive integration tests that follow complete user journeys for eac 4. Can edit any member (except email if linked - see special case) 5. Cannot delete member 6. Can manage properties -7. Cannot manage property types (read-only) +7. Cannot manage custom fields (read-only) 8. Cannot access /admin/roles **Buchhaltung Journey:** @@ -1266,7 +1266,7 @@ Write comprehensive integration tests that follow complete user journeys for eac │ │ │ ┌────▼─────┐ ┌──────▼──────┐ │ │ Issue #9 │ │ Issue #10 │ │ - │ Property │ │ PropType │ │ + │ CustomFieldValue │ │ CustomField │ │ │ Policies │ │ Policies │ │ └────┬─────┘ └──────┬──────┘ │ │ │ │ @@ -1384,8 +1384,8 @@ test/ ├── mv/membership/ │ ├── member_policies_test.exs # Issue #7 │ ├── member_email_validation_test.exs # Issue #12 -│ ├── property_policies_test.exs # Issue #9 -│ └── property_type_policies_test.exs # Issue #10 +│ ├── custom_field_value_policies_test.exs # Issue #9 +│ └── custom_field_policies_test.exs # Issue #10 ├── mv_web/ │ ├── authorization_test.exs # Issue #14 │ ├── plugs/ @@ -1395,8 +1395,8 @@ test/ │ ├── role_live_authorization_test.exs # Issue #15 │ ├── member_live_authorization_test.exs # Issue #16 │ ├── user_live_authorization_test.exs # Issue #16 -│ ├── property_live_authorization_test.exs # Issue #16 -│ └── property_type_live_authorization_test.exs # Issue #16 +│ ├── custom_field_value_live_authorization_test.exs # Issue #16 +│ └── custom_field_live_authorization_test.exs # Issue #16 ├── integration/ │ ├── mitglied_journey_test.exs # Issue #17 │ ├── vorstand_journey_test.exs # Issue #17 diff --git a/docs/roles-and-permissions-overview.md b/docs/roles-and-permissions-overview.md index 191e8b7..86e7273 100644 --- a/docs/roles-and-permissions-overview.md +++ b/docs/roles-and-permissions-overview.md @@ -201,7 +201,7 @@ When runtime permission editing becomes a business requirement, migrate to Appro **Resource Level (MVP):** - Controls create, read, update, destroy actions on resources -- Resources: Member, User, Property, PropertyType, Role +- Resources: Member, User, CustomFieldValue, CustomField, Role **Page Level (MVP):** - Controls access to LiveView pages @@ -280,7 +280,7 @@ Contains: Each Permission Set contains: **Resources:** List of resource permissions -- resource: "Member", "User", "Property", etc. +- resource: "Member", "User", "CustomFieldValue", etc. - action: :read, :create, :update, :destroy - scope: :own, :linked, :all - granted: true/false From 4bd08e85bb3bc6dc5600810fa727e590d5eb097b Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 6 Jan 2026 21:35:59 +0100 Subject: [PATCH 3/6] fix: use Enum.empty? instead of != [] to fix type warning Replace comparison with empty list using Enum.empty?/1 to satisfy type checker and avoid redundant comparison warning --- test/mv/authorization/permission_sets_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/mv/authorization/permission_sets_test.exs b/test/mv/authorization/permission_sets_test.exs index 51dc797..84bdc2f 100644 --- a/test/mv/authorization/permission_sets_test.exs +++ b/test/mv/authorization/permission_sets_test.exs @@ -74,7 +74,7 @@ defmodule Mv.Authorization.PermissionSetsTest do for set <- [:own_data, :read_only, :normal_user, :admin] do permissions = PermissionSets.get_permissions(set) - assert permissions.pages != [], + assert not Enum.empty?(permissions.pages), "Permission set #{set} should have at least one page" end end From 9b0d022767a258b2195764c7a1588a043cd6e686 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 6 Jan 2026 21:55:13 +0100 Subject: [PATCH 4/6] fix: add missing /profile page to read_only and normal_user permission sets Both permission sets allow User:update :own, so users should be able to access their profile page. This makes the implementation consistent with the documentation and the logical permission model. --- lib/mv/authorization/permission_sets.ex | 4 ++++ test/mv/authorization/permission_sets_test.exs | 2 ++ 2 files changed, 6 insertions(+) diff --git a/lib/mv/authorization/permission_sets.ex b/lib/mv/authorization/permission_sets.ex index 22b1648..6139f7f 100644 --- a/lib/mv/authorization/permission_sets.ex +++ b/lib/mv/authorization/permission_sets.ex @@ -132,6 +132,8 @@ defmodule Mv.Authorization.PermissionSets do ], pages: [ "/", + # Own profile + "/profile", # Member list "/members", # Member detail @@ -166,6 +168,8 @@ defmodule Mv.Authorization.PermissionSets do ], pages: [ "/", + # Own profile + "/profile", "/members", # Create member "/members/new", diff --git a/test/mv/authorization/permission_sets_test.exs b/test/mv/authorization/permission_sets_test.exs index 84bdc2f..06e2110 100644 --- a/test/mv/authorization/permission_sets_test.exs +++ b/test/mv/authorization/permission_sets_test.exs @@ -247,6 +247,7 @@ defmodule Mv.Authorization.PermissionSetsTest do permissions = PermissionSets.get_permissions(:read_only) assert "/" in permissions.pages + assert "/profile" in permissions.pages assert "/members" in permissions.pages assert "/members/:id" in permissions.pages assert "/custom_field_values" in permissions.pages @@ -349,6 +350,7 @@ defmodule Mv.Authorization.PermissionSetsTest do permissions = PermissionSets.get_permissions(:normal_user) assert "/" in permissions.pages + assert "/profile" in permissions.pages assert "/members" in permissions.pages assert "/members/new" in permissions.pages assert "/members/:id" in permissions.pages From 7845117fadcdaa4417d18c24d5bd213083eb13ec Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 6 Jan 2026 21:55:52 +0100 Subject: [PATCH 5/6] refactor: improve error handling and documentation in PermissionSets - Add explicit ArgumentError for invalid permission set names with helpful message - Soften performance claim in documentation (intended to be constant-time) - Add tests for error handling - Improve maintainability with guard clause for invalid inputs --- lib/mv/authorization/permission_sets.ex | 11 +++++++-- .../mv/authorization/permission_sets_test.exs | 23 +++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/lib/mv/authorization/permission_sets.ex b/lib/mv/authorization/permission_sets.ex index 6139f7f..f9197de 100644 --- a/lib/mv/authorization/permission_sets.ex +++ b/lib/mv/authorization/permission_sets.ex @@ -38,7 +38,9 @@ defmodule Mv.Authorization.PermissionSets do ## Performance - All functions are pure and compile-time. Permission lookups are < 1 microsecond. + All functions are pure and intended to be constant-time. Permission lookups + are very fast (typically < 1 microsecond in practice) as they are simple + pattern matches and map lookups with no database queries or external calls. """ @type scope :: :own | :linked | :all @@ -81,10 +83,15 @@ defmodule Mv.Authorization.PermissionSets do true iex> PermissionSets.get_permissions(:invalid) - ** (FunctionClauseError) no function clause matching + ** (ArgumentError) invalid permission set: :invalid. Must be one of: [:own_data, :read_only, :normal_user, :admin] """ @spec get_permissions(atom()) :: permission_set() + def get_permissions(set) when set not in [:own_data, :read_only, :normal_user, :admin] do + raise ArgumentError, + "invalid permission set: #{inspect(set)}. Must be one of: #{inspect(all_permission_sets())}" + end + def get_permissions(:own_data) do %{ resources: [ diff --git a/test/mv/authorization/permission_sets_test.exs b/test/mv/authorization/permission_sets_test.exs index 06e2110..de960a9 100644 --- a/test/mv/authorization/permission_sets_test.exs +++ b/test/mv/authorization/permission_sets_test.exs @@ -567,4 +567,27 @@ defmodule Mv.Authorization.PermissionSetsTest do {:error, :invalid_permission_set} end end + + describe "get_permissions/1 - error handling" do + test "raises ArgumentError for invalid permission set with helpful message" do + assert_raise ArgumentError, + ~r/invalid permission set: :invalid\. Must be one of:/, + fn -> + PermissionSets.get_permissions(:invalid) + end + end + + test "error message includes all valid permission sets" do + error = + assert_raise ArgumentError, fn -> + PermissionSets.get_permissions(:unknown) + end + + error_message = Exception.message(error) + assert error_message =~ "own_data" + assert error_message =~ "read_only" + assert error_message =~ "normal_user" + assert error_message =~ "admin" + end + end end From 18ec4bfd1682de0555d07fe84dc6c5966c334b45 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 6 Jan 2026 22:17:33 +0100 Subject: [PATCH 6/6] fix: add missing /custom_field_values/:id page to read_only and normal_user - Add /custom_field_values/:id to read_only pages (users can view list, should also view details) - Add /custom_field_values/:id to normal_user pages - Refactor tests to reduce duplication (use for-comprehension for structure tests) - Add tests for invalid input types in valid_permission_set?/1 - Update @spec for valid_permission_set?/1 to accept any() type --- lib/mv/authorization/permission_sets.ex | 13 ++++- .../mv/authorization/permission_sets_test.exs | 51 ++++++++----------- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/lib/mv/authorization/permission_sets.ex b/lib/mv/authorization/permission_sets.ex index f9197de..11ddb5a 100644 --- a/lib/mv/authorization/permission_sets.ex +++ b/lib/mv/authorization/permission_sets.ex @@ -146,7 +146,9 @@ defmodule Mv.Authorization.PermissionSets do # Member detail "/members/:id", # Custom field values overview - "/custom_field_values" + "/custom_field_values", + # Custom field value detail + "/custom_field_values/:id" ] } end @@ -184,6 +186,8 @@ defmodule Mv.Authorization.PermissionSets do # Edit member "/members/:id/edit", "/custom_field_values", + # Custom field value detail + "/custom_field_values/:id", "/custom_field_values/new", "/custom_field_values/:id/edit" ] @@ -230,6 +234,11 @@ defmodule Mv.Authorization.PermissionSets do } end + def get_permissions(invalid) do + raise ArgumentError, + "invalid permission set: #{inspect(invalid)}. Must be one of: #{inspect(all_permission_sets())}" + end + @doc """ Checks if a permission set name (string or atom) is valid. @@ -244,7 +253,7 @@ defmodule Mv.Authorization.PermissionSets do iex> PermissionSets.valid_permission_set?("invalid") false """ - @spec valid_permission_set?(String.t() | atom()) :: boolean() + @spec valid_permission_set?(any()) :: boolean() def valid_permission_set?(name) when is_binary(name) do case permission_set_name_to_atom(name) do {:ok, _atom} -> true diff --git a/test/mv/authorization/permission_sets_test.exs b/test/mv/authorization/permission_sets_test.exs index de960a9..dcd0680 100644 --- a/test/mv/authorization/permission_sets_test.exs +++ b/test/mv/authorization/permission_sets_test.exs @@ -19,40 +19,22 @@ defmodule Mv.Authorization.PermissionSetsTest do end describe "get_permissions/1" do - test "returns map with :resources and :pages keys for :own_data" do - permissions = PermissionSets.get_permissions(:own_data) + test "all permission sets return map with :resources and :pages keys" do + for set <- PermissionSets.all_permission_sets() do + permissions = PermissionSets.get_permissions(set) - assert Map.has_key?(permissions, :resources) - assert Map.has_key?(permissions, :pages) - assert is_list(permissions.resources) - assert is_list(permissions.pages) - end + assert Map.has_key?(permissions, :resources), + "#{set} missing :resources key" - test "returns map with :resources and :pages keys for :read_only" do - permissions = PermissionSets.get_permissions(:read_only) + assert Map.has_key?(permissions, :pages), + "#{set} missing :pages key" - assert Map.has_key?(permissions, :resources) - assert Map.has_key?(permissions, :pages) - assert is_list(permissions.resources) - assert is_list(permissions.pages) - end + assert is_list(permissions.resources), + "#{set} :resources must be a list" - test "returns map with :resources and :pages keys for :normal_user" do - permissions = PermissionSets.get_permissions(:normal_user) - - assert Map.has_key?(permissions, :resources) - assert Map.has_key?(permissions, :pages) - assert is_list(permissions.resources) - assert is_list(permissions.pages) - end - - test "returns map with :resources and :pages keys for :admin" do - permissions = PermissionSets.get_permissions(:admin) - - assert Map.has_key?(permissions, :resources) - assert Map.has_key?(permissions, :pages) - assert is_list(permissions.resources) - assert is_list(permissions.pages) + assert is_list(permissions.pages), + "#{set} :pages must be a list" + end end test "each resource permission has required keys" do @@ -251,6 +233,7 @@ defmodule Mv.Authorization.PermissionSetsTest do assert "/members" in permissions.pages assert "/members/:id" in permissions.pages assert "/custom_field_values" in permissions.pages + assert "/custom_field_values/:id" in permissions.pages end end @@ -356,6 +339,7 @@ defmodule Mv.Authorization.PermissionSetsTest do assert "/members/:id" in permissions.pages assert "/members/:id/edit" in permissions.pages assert "/custom_field_values" in permissions.pages + assert "/custom_field_values/:id" in permissions.pages assert "/custom_field_values/new" in permissions.pages assert "/custom_field_values/:id/edit" in permissions.pages end @@ -541,6 +525,13 @@ defmodule Mv.Authorization.PermissionSetsTest do test "returns false for nil input" do assert PermissionSets.valid_permission_set?(nil) == false end + + test "returns false for invalid types" do + assert PermissionSets.valid_permission_set?(123) == false + assert PermissionSets.valid_permission_set?([]) == false + assert PermissionSets.valid_permission_set?(%{}) == false + assert PermissionSets.valid_permission_set?("") == false + end end describe "permission_set_name_to_atom/1" do