From 93190d558fb246b8c474334c0bb9042a04dabdfa Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 21:03:17 +0100 Subject: [PATCH 01/34] test: add Member resource policy tests --- .../has_permission_integration_test.exs | 19 +- .../checks/has_permission_test.exs | 19 +- test/mv/membership/member_policies_test.exs | 418 ++++++++++++++++++ 3 files changed, 446 insertions(+), 10 deletions(-) create mode 100644 test/mv/membership/member_policies_test.exs diff --git a/test/mv/authorization/checks/has_permission_integration_test.exs b/test/mv/authorization/checks/has_permission_integration_test.exs index f1f32c3..5aa43a5 100644 --- a/test/mv/authorization/checks/has_permission_integration_test.exs +++ b/test/mv/authorization/checks/has_permission_integration_test.exs @@ -14,16 +14,22 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do alias Mv.Authorization.Checks.HasPermission # Helper to create mock actor with role - defp create_actor_with_role(permission_set_name) do - %{ + defp create_actor_with_role(permission_set_name, opts \\ []) do + actor = %{ id: "user-#{System.unique_integer([:positive])}", role: %{permission_set_name: permission_set_name} } + + # Add member_id if provided (needed for :linked scope tests) + case Keyword.get(opts, :member_id) do + nil -> actor + member_id -> Map.put(actor, :member_id, member_id) + end end describe "Filter Expression Structure - :linked scope" do test "Member filter uses user.id relationship path" do - actor = create_actor_with_role("own_data") + actor = create_actor_with_role("own_data", member_id: "member-123") authorizer = create_authorizer(Mv.Membership.Member, :read) filter = HasPermission.auto_filter(actor, authorizer, []) @@ -37,7 +43,7 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do end test "CustomFieldValue filter uses member.user.id relationship path" do - actor = create_actor_with_role("own_data") + actor = create_actor_with_role("own_data", member_id: "member-123") authorizer = create_authorizer(Mv.Membership.CustomFieldValue, :read) filter = HasPermission.auto_filter(actor, authorizer, []) @@ -81,7 +87,10 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do defp create_authorizer(resource, action) do %Ash.Policy.Authorizer{ resource: resource, - subject: %{action: %{name: action}} + subject: %{ + action: %{type: action}, + data: nil + } } end end diff --git a/test/mv/authorization/checks/has_permission_test.exs b/test/mv/authorization/checks/has_permission_test.exs index 5ab88c6..f577d03 100644 --- a/test/mv/authorization/checks/has_permission_test.exs +++ b/test/mv/authorization/checks/has_permission_test.exs @@ -13,16 +13,25 @@ defmodule Mv.Authorization.Checks.HasPermissionTest do defp create_authorizer(resource, action) do %Ash.Policy.Authorizer{ resource: resource, - subject: %{action: %{name: action}} + subject: %{ + action: %{type: action}, + data: nil + } } end # Helper to create actor with role - defp create_actor(id, permission_set_name) do - %{ + defp create_actor(id, permission_set_name, opts \\ []) do + actor = %{ id: id, role: %{permission_set_name: permission_set_name} } + + # Add member_id if provided (needed for :linked scope tests) + case Keyword.get(opts, :member_id) do + nil -> actor + member_id -> Map.put(actor, :member_id, member_id) + end end describe "describe/1" do @@ -120,7 +129,7 @@ defmodule Mv.Authorization.Checks.HasPermissionTest do describe "auto_filter/3 - Scope :linked" do test "scope :linked for Member returns user_id filter" do - user = create_actor("user-123", "own_data") + user = create_actor("user-123", "own_data", member_id: "member-456") authorizer = create_authorizer(Mv.Membership.Member, :read) filter = HasPermission.auto_filter(user, authorizer, []) @@ -130,7 +139,7 @@ defmodule Mv.Authorization.Checks.HasPermissionTest do end test "scope :linked for CustomFieldValue returns member.user_id filter" do - user = create_actor("user-123", "own_data") + user = create_actor("user-123", "own_data", member_id: "member-456") authorizer = create_authorizer(Mv.Membership.CustomFieldValue, :update) filter = HasPermission.auto_filter(user, authorizer, []) diff --git a/test/mv/membership/member_policies_test.exs b/test/mv/membership/member_policies_test.exs new file mode 100644 index 0000000..ce1e7ce --- /dev/null +++ b/test/mv/membership/member_policies_test.exs @@ -0,0 +1,418 @@ +defmodule Mv.Membership.MemberPoliciesTest do + @moduledoc """ + Tests for Member resource authorization policies. + + Tests all 4 permission sets (own_data, read_only, normal_user, admin) + and verifies that policies correctly enforce access control based on + user roles and permission sets. + """ + # async: false because we need database commits to be visible across queries + # in the same test (especially for unlinked members) + use Mv.DataCase, async: false + + alias Mv.Membership + alias Mv.Accounts + alias Mv.Authorization + + require Ash.Query + + # Helper to create a role with a specific permission set + defp create_role_with_permission_set(permission_set_name) do + role_name = "Test Role #{permission_set_name} #{System.unique_integer([:positive])}" + + case Authorization.create_role(%{ + name: role_name, + description: "Test role for #{permission_set_name}", + permission_set_name: permission_set_name + }) do + {:ok, role} -> role + {:error, error} -> raise "Failed to create role: #{inspect(error)}" + end + end + + # Helper to create a user with a specific permission set + # Returns user with role preloaded (required for authorization) + defp create_user_with_permission_set(permission_set_name) do + # Create role with permission set + role = create_role_with_permission_set(permission_set_name) + + # Create user + {:ok, user} = + Accounts.User + |> Ash.Changeset.for_create(:register_with_password, %{ + email: "user#{System.unique_integer([:positive])}@example.com", + password: "testpassword123" + }) + |> Ash.create() + + # Assign role to user + {:ok, user} = + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, role, type: :append_and_remove) + |> Ash.update() + + # Reload user with role preloaded (critical for authorization!) + {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts) + user_with_role + end + + # Helper to create an admin user (for creating test fixtures) + defp create_admin_user do + create_user_with_permission_set("admin") + end + + # Helper to create a member linked to a user + defp create_linked_member_for_user(user) do + admin = create_admin_user() + + # Create member + # NOTE: We need to ensure the member is actually persisted to the database + # before we try to link it. Ash may delay writes, so we explicitly return the struct. + {:ok, member} = + Membership.Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "Linked", + last_name: "Member", + email: "linked#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create(actor: admin, return_notifications?: false) + + # Link member to user (User.member_id = member.id) + # We use force_change_attribute because the member already exists and we just + # need to set the foreign key. This avoids the issue where manage_relationship + # tries to query the member without the actor context. + result = + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.force_change_attribute(:member_id, member.id) + |> Ash.update(actor: admin, domain: Mv.Accounts, return_notifications?: false) + + {:ok, _user} = result + + # Return the member struct directly - no need to reload since we just created it + # and we're in the same transaction/sandbox + member + end + + # Helper to create an unlinked member (no user relationship) + defp create_unlinked_member do + admin = create_admin_user() + + {:ok, member} = + Membership.Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "Unlinked", + last_name: "Member", + email: "unlinked#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create(actor: admin) + + member + end + + describe "own_data permission set (Mitglied)" do + setup do + user = create_user_with_permission_set("own_data") + linked_member = create_linked_member_for_user(user) + unlinked_member = create_unlinked_member() + + # Reload user to get updated member_id + {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) + {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) + + %{user: user, linked_member: linked_member, unlinked_member: unlinked_member} + end + + test "can read linked member", %{user: user, linked_member: linked_member} do + {:ok, member} = + Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership) + + assert member.id == linked_member.id + end + + test "can update linked member", %{user: user, linked_member: linked_member} do + {:ok, updated_member} = + linked_member + |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"}) + |> Ash.update(actor: user) + + assert updated_member.first_name == "Updated" + end + + test "cannot read unlinked member (returns forbidden)", %{ + user: user, + unlinked_member: unlinked_member + } do + # Note: With auto_filter policies, when a user tries to read a member that doesn't + # match the filter (id == actor.member_id), Ash returns NotFound, not Forbidden. + # This is the expected behavior - the filter makes the record "invisible" to the user. + assert_raise Ash.Error.Invalid, fn -> + Ash.get!(Membership.Member, unlinked_member.id, actor: user, domain: Mv.Membership) + end + end + + test "cannot update unlinked member (returns forbidden)", %{ + user: user, + unlinked_member: unlinked_member + } do + assert_raise Ash.Error.Forbidden, fn -> + unlinked_member + |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"}) + |> Ash.update!(actor: user) + end + end + + test "list members returns only linked member", %{user: user, linked_member: linked_member} do + {:ok, members} = Ash.read(Membership.Member, actor: user, domain: Mv.Membership) + + # Should only return the linked member (scope :linked filters) + assert length(members) == 1 + assert hd(members).id == linked_member.id + end + + test "cannot create member (returns forbidden)", %{user: user} do + assert_raise Ash.Error.Forbidden, fn -> + Membership.Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "New", + last_name: "Member", + email: "new#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create!(actor: user) + end + end + + test "cannot destroy member (returns forbidden)", %{user: user, linked_member: linked_member} do + assert_raise Ash.Error.Forbidden, fn -> + Ash.destroy!(linked_member, actor: user) + end + end + end + + describe "read_only permission set (Vorstand/Buchhaltung)" do + setup do + user = create_user_with_permission_set("read_only") + linked_member = create_linked_member_for_user(user) + unlinked_member = create_unlinked_member() + + # Reload user to get updated member_id + {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) + + %{user: user, linked_member: linked_member, unlinked_member: unlinked_member} + end + + test "can read all members", %{ + user: user, + linked_member: linked_member, + unlinked_member: unlinked_member + } do + {:ok, members} = Ash.read(Membership.Member, actor: user, domain: Mv.Membership) + + # Should return all members (scope :all) + member_ids = Enum.map(members, & &1.id) + assert linked_member.id in member_ids + assert unlinked_member.id in member_ids + end + + test "can read individual member", %{user: user, unlinked_member: unlinked_member} do + {:ok, member} = + Ash.get(Membership.Member, unlinked_member.id, actor: user, domain: Mv.Membership) + + assert member.id == unlinked_member.id + end + + test "cannot create member (returns forbidden)", %{user: user} do + assert_raise Ash.Error.Forbidden, fn -> + Membership.Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "New", + last_name: "Member", + email: "new#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create!(actor: user) + end + end + + test "cannot update any member (returns forbidden)", %{ + user: user, + linked_member: linked_member + } do + assert_raise Ash.Error.Forbidden, fn -> + linked_member + |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"}) + |> Ash.update!(actor: user) + end + end + + test "cannot destroy any member (returns forbidden)", %{ + user: user, + unlinked_member: unlinked_member + } do + assert_raise Ash.Error.Forbidden, fn -> + Ash.destroy!(unlinked_member, actor: user) + end + end + end + + describe "normal_user permission set (Kassenwart)" do + setup do + user = create_user_with_permission_set("normal_user") + linked_member = create_linked_member_for_user(user) + unlinked_member = create_unlinked_member() + + # Reload user to get updated member_id + {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) + + %{user: user, linked_member: linked_member, unlinked_member: unlinked_member} + end + + test "can read all members", %{ + user: user, + linked_member: linked_member, + unlinked_member: unlinked_member + } do + {:ok, members} = Ash.read(Membership.Member, actor: user, domain: Mv.Membership) + + # Should return all members (scope :all) + member_ids = Enum.map(members, & &1.id) + assert linked_member.id in member_ids + assert unlinked_member.id in member_ids + end + + test "can create member", %{user: user} do + {:ok, member} = + Membership.Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "New", + last_name: "Member", + email: "new#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create(actor: user) + + assert member.first_name == "New" + end + + test "can update any member", %{user: user, unlinked_member: unlinked_member} do + {:ok, updated_member} = + unlinked_member + |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"}) + |> Ash.update(actor: user) + + assert updated_member.first_name == "Updated" + end + + test "cannot destroy member (safety - not in permission set)", %{ + user: user, + unlinked_member: unlinked_member + } do + assert_raise Ash.Error.Forbidden, fn -> + Ash.destroy!(unlinked_member, actor: user) + end + end + end + + describe "admin permission set" do + setup do + user = create_user_with_permission_set("admin") + linked_member = create_linked_member_for_user(user) + unlinked_member = create_unlinked_member() + + # Reload user to get updated member_id + {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) + + %{user: user, linked_member: linked_member, unlinked_member: unlinked_member} + end + + test "can read all members", %{ + user: user, + linked_member: linked_member, + unlinked_member: unlinked_member + } do + {:ok, members} = Ash.read(Membership.Member, actor: user, domain: Mv.Membership) + + # Should return all members (scope :all) + member_ids = Enum.map(members, & &1.id) + assert linked_member.id in member_ids + assert unlinked_member.id in member_ids + end + + test "can create member", %{user: user} do + {:ok, member} = + Membership.Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "New", + last_name: "Member", + email: "new#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create(actor: user) + + assert member.first_name == "New" + end + + test "can update any member", %{user: user, unlinked_member: unlinked_member} do + {:ok, updated_member} = + unlinked_member + |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"}) + |> Ash.update(actor: user) + + assert updated_member.first_name == "Updated" + end + + test "can destroy any member", %{user: user, unlinked_member: unlinked_member} do + :ok = Ash.destroy(unlinked_member, actor: user) + + # Verify member is deleted + assert {:error, _} = Ash.get(Membership.Member, unlinked_member.id, domain: Mv.Membership) + end + end + + describe "special case: user can always access linked member" do + test "own_data user can read linked member even without explicit permission" do + user = create_user_with_permission_set("own_data") + linked_member = create_linked_member_for_user(user) + + # Reload user to get updated member_id + {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) + {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) + + # Should succeed (special case policy takes precedence) + {:ok, member} = + Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership) + + assert member.id == linked_member.id + end + + test "read_only user can read linked member (via special case)" do + user = create_user_with_permission_set("read_only") + linked_member = create_linked_member_for_user(user) + + # Reload user to get updated member_id + {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) + {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) + + # Should succeed (special case policy takes precedence) + {:ok, member} = + Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership) + + assert member.id == linked_member.id + end + + test "own_data user can update linked member even without explicit permission" do + user = create_user_with_permission_set("own_data") + linked_member = create_linked_member_for_user(user) + + # Reload user to get updated member_id + {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) + {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) + + # Should succeed (special case policy takes precedence) + {:ok, updated_member} = + linked_member + |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"}) + |> Ash.update(actor: user) + + assert updated_member.first_name == "Updated" + end + end +end From 4192922fd38007192f8e2aafbf00e249ba10d46a Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 21:03:27 +0100 Subject: [PATCH 02/34] feat: implement authorization policies for Member resource --- lib/membership/member.ex | 37 +++++++- lib/mv/authorization/checks/has_permission.ex | 87 +++++++++++++++---- lib/mv/authorization/checks/no_actor.ex | 60 +++++++++++++ mix.exs | 1 + mix.lock | 1 + 5 files changed, 169 insertions(+), 17 deletions(-) create mode 100644 lib/mv/authorization/checks/no_actor.ex diff --git a/lib/membership/member.ex b/lib/membership/member.ex index d2ea07d..0cd7174 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -34,7 +34,8 @@ defmodule Mv.Membership.Member do """ use Ash.Resource, domain: Mv.Membership, - data_layer: AshPostgres.DataLayer + data_layer: AshPostgres.DataLayer, + authorizers: [Ash.Policy.Authorizer] require Ash.Query import Ash.Expr @@ -296,6 +297,40 @@ defmodule Mv.Membership.Member do end end + # Authorization Policies + # Order matters: Most specific policies first, then general permission check + policies do + # SYSTEM OPERATIONS: Allow operations without actor (seeds, tests, system jobs) + # This must come first to allow database seeding and test fixtures + # IMPORTANT: Use bypass so this short-circuits and doesn't require other policies + bypass action_type([:create, :read, :update, :destroy]) do + description "Allow system operations without actor (seeds, tests)" + authorize_if Mv.Authorization.Checks.NoActor + end + + # SPECIAL CASE: Users can always READ their linked member + # This allows users with ANY permission set to read their own linked member + # Check using the inverse relationship: User.member_id → Member.id + bypass action_type(:read) do + description "Users can always read member linked to their account" + authorize_if expr(id == ^actor(:member_id)) + end + + # GENERAL: Check permissions from user's role + # HasPermission handles update permissions correctly: + # - :own_data → can update linked member (scope :linked) + # - :read_only → cannot update any member (no update permission) + # - :normal_user → can update all members (scope :all) + # - :admin → can update all members (scope :all) + policy action_type([:read, :create, :update, :destroy]) do + description "Check permissions from user's role and permission set" + authorize_if Mv.Authorization.Checks.HasPermission + end + + # DEFAULT: Forbid if no policy matched + # Ash implicitly forbids if no policy authorized + end + @doc """ Filters members list based on email match priority. diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 345d6e4..3858bd6 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -59,6 +59,7 @@ defmodule Mv.Authorization.Checks.HasPermission do def strict_check(actor, authorizer, _opts) do resource = authorizer.resource action = get_action_from_authorizer(authorizer) + record = get_record_from_authorizer(authorizer) cond do is_nil(actor) -> @@ -76,12 +77,12 @@ defmodule Mv.Authorization.Checks.HasPermission do {:ok, false} true -> - strict_check_with_permissions(actor, resource, action) + strict_check_with_permissions(actor, resource, action, record) end end # Helper function to reduce nesting depth - defp strict_check_with_permissions(actor, resource, action) do + defp strict_check_with_permissions(actor, resource, action, record) do with %{role: %{permission_set_name: ps_name}} when not is_nil(ps_name) <- actor, {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name), permissions <- PermissionSets.get_permissions(ps_atom), @@ -93,9 +94,15 @@ defmodule Mv.Authorization.Checks.HasPermission do actor, resource_name ) do - :authorized -> {:ok, true} - {:filter, _} -> {:ok, :unknown} - false -> {:ok, false} + :authorized -> + {:ok, true} + + {:filter, filter_expr} -> + # For strict_check on single records, evaluate the filter against the record + evaluate_filter_for_strict_check(filter_expr, actor, record, resource_name) + + false -> + {:ok, false} end else %{role: nil} -> @@ -150,15 +157,60 @@ defmodule Mv.Authorization.Checks.HasPermission do end end - # Helper to extract action from authorizer + # Helper to extract action type from authorizer + # CRITICAL: Must use action_type, not action.name! + # Action types: :create, :read, :update, :destroy + # Action names: :create_member, :update_member, etc. + # PermissionSets uses action types, not action names defp get_action_from_authorizer(authorizer) do case authorizer.subject do - %{action: %{name: action}} -> action - %{action: action} when is_atom(action) -> action + %{action_type: action_type} when action_type in [:create, :read, :update, :destroy] -> + action_type + + # Fallback for older Ash versions or different subject shapes + %{action: %{type: action_type}} when action_type in [:create, :read, :update, :destroy] -> + action_type + + _ -> + nil + end + end + + # Helper to extract record from authorizer for strict_check + defp get_record_from_authorizer(authorizer) do + case authorizer.subject do + %{data: data} when not is_nil(data) -> data _ -> nil end end + # Evaluate filter expression for strict_check on single records + # For :linked scope with Member resource: id == actor.member_id + defp evaluate_filter_for_strict_check(_filter_expr, actor, record, resource_name) do + case {resource_name, record} do + {"Member", %{id: member_id}} when not is_nil(member_id) -> + # Check if this member's ID matches the actor's member_id + if member_id == actor.member_id do + {:ok, true} + else + {:ok, false} + end + + {"CustomFieldValue", %{member_id: cfv_member_id}} when not is_nil(cfv_member_id) -> + # Check if this CFV's member_id matches the actor's member_id + if cfv_member_id == actor.member_id do + {:ok, true} + else + {:ok, false} + end + + _ -> + # For other cases or when record is not available, return :unknown + # This will cause Ash to use auto_filter instead + {:ok, :unknown} + end + end + # Extract resource name from module (e.g., Mv.Membership.Member -> "Member") defp get_resource_name(resource) when is_atom(resource) do resource |> Module.split() |> List.last() @@ -190,21 +242,24 @@ defmodule Mv.Authorization.Checks.HasPermission do end # Scope: linked - Filter based on user relationship (resource-specific!) - # Uses Ash relationships: Member has_one :user, CustomFieldValue belongs_to :member + # IMPORTANT: Understand the relationship direction! + # - User belongs_to :member (User.member_id → Member.id) + # - Member has_one :user (inverse, no FK on Member) defp apply_scope(:linked, actor, resource_name) do case resource_name do "Member" -> - # Member has_one :user → filter by user.id == actor.id - {:filter, expr(user.id == ^actor.id)} + # User.member_id → Member.id (inverse relationship) + # Filter: member.id == actor.member_id + {:filter, expr(id == ^actor.member_id)} "CustomFieldValue" -> - # CustomFieldValue belongs_to :member → member has_one :user - # Traverse: custom_field_value.member.user.id == actor.id - {:filter, expr(member.user.id == ^actor.id)} + # CustomFieldValue.member_id → Member.id → User.member_id + # Filter: custom_field_value.member_id == actor.member_id + {:filter, expr(member_id == ^actor.member_id)} _ -> - # Fallback for other resources: try user relationship first, then user_id - {:filter, expr(user.id == ^actor.id or user_id == ^actor.id)} + # Fallback for other resources + {:filter, expr(user_id == ^actor.id)} end end diff --git a/lib/mv/authorization/checks/no_actor.ex b/lib/mv/authorization/checks/no_actor.ex new file mode 100644 index 0000000..c80ea9b --- /dev/null +++ b/lib/mv/authorization/checks/no_actor.ex @@ -0,0 +1,60 @@ +defmodule Mv.Authorization.Checks.NoActor do + @moduledoc """ + Custom Ash Policy Check that allows actions when no actor is present. + + This is primarily used for: + - Database seeding (priv/repo/seeds.exs) + - Test fixtures that create data without authentication + - Background jobs that operate on behalf of the system + + ## Security Note + + This check should only be used for specific actions where system-level + access is appropriate. It should always be combined with other policy + checks that validate actor-based permissions when an actor IS present. + + ## Usage in Policies + + policies do + # Allow seeding and system operations + policy action_type(:create) do + authorize_if NoActor + end + + # Check permissions when actor is present + policy action_type([:read, :create, :update, :destroy]) do + authorize_if HasPermission + end + end + + ## Behavior + + - Returns `{:ok, true}` when actor is nil (allows action) + - Returns `{:ok, :unknown}` when actor is present (delegates to other policies) + - `auto_filter` returns nil (no filtering needed) + """ + + use Ash.Policy.Check + + @impl true + def describe(_opts) do + "allows actions when no actor is present (for seeds and system operations)" + end + + @impl true + def strict_check(actor, _authorizer, _opts) do + if is_nil(actor) do + # No actor present - allow (for seeds, tests, system operations) + {:ok, true} + else + # Actor present - let other policies decide + {:ok, :unknown} + end + end + + @impl true + def auto_filter(_actor, _authorizer, _opts) do + # No filtering needed - this check only validates presence/absence of actor + nil + end +end diff --git a/mix.exs b/mix.exs index 6aa5f9f..4092b7a 100644 --- a/mix.exs +++ b/mix.exs @@ -76,6 +76,7 @@ defmodule Mv.MixProject do {:mix_audit, "~> 2.1", only: [:dev, :test], runtime: false}, {:sobelow, "~> 0.14", only: [:dev, :test], runtime: false}, {:credo, "~> 1.7", only: [:dev, :test], runtime: false}, + {:picosat_elixir, "~> 0.1", only: [:dev, :test]}, {:ecto_commons, "~> 0.3"}, {:slugify, "~> 1.3"} ] diff --git a/mix.lock b/mix.lock index 1808eba..54b0e51 100644 --- a/mix.lock +++ b/mix.lock @@ -60,6 +60,7 @@ "phoenix_pubsub": {:hex, :phoenix_pubsub, "2.2.0", "ff3a5616e1bed6804de7773b92cbccfc0b0f473faf1f63d7daf1206c7aeaaa6f", [:mix], [], "hexpm", "adc313a5bf7136039f63cfd9668fde73bba0765e0614cba80c06ac9460ff3e96"}, "phoenix_template": {:hex, :phoenix_template, "1.0.4", "e2092c132f3b5e5b2d49c96695342eb36d0ed514c5b252a77048d5969330d639", [:mix], [{:phoenix_html, "~> 2.14.2 or ~> 3.0 or ~> 4.0", [hex: :phoenix_html, repo: "hexpm", optional: true]}], "hexpm", "2c0c81f0e5c6753faf5cca2f229c9709919aba34fab866d3bc05060c9c444206"}, "phoenix_view": {:hex, :phoenix_view, "2.0.4", "b45c9d9cf15b3a1af5fb555c674b525391b6a1fe975f040fb4d913397b31abf4", [:mix], [{:phoenix_html, "~> 2.14.2 or ~> 3.0 or ~> 4.0", [hex: :phoenix_html, repo: "hexpm", optional: true]}, {:phoenix_template, "~> 1.0", [hex: :phoenix_template, repo: "hexpm", optional: false]}], "hexpm", "4e992022ce14f31fe57335db27a28154afcc94e9983266835bb3040243eb620b"}, + "picosat_elixir": {:hex, :picosat_elixir, "0.2.3", "bf326d0f179fbb3b706bb2c15fbc367dacfa2517157d090fdfc32edae004c597", [:make, :mix], [{:elixir_make, "~> 0.6", [hex: :elixir_make, repo: "hexpm", optional: false]}], "hexpm", "f76c9db2dec9d2561ffaa9be35f65403d53e984e8cd99c832383b7ab78c16c66"}, "plug": {:hex, :plug, "1.19.1", "09bac17ae7a001a68ae393658aa23c7e38782be5c5c00c80be82901262c394c0", [:mix], [{:mime, "~> 1.0 or ~> 2.0", [hex: :mime, repo: "hexpm", optional: false]}, {:plug_crypto, "~> 1.1.1 or ~> 1.2 or ~> 2.0", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4.3 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "560a0017a8f6d5d30146916862aaf9300b7280063651dd7e532b8be168511e62"}, "plug_crypto": {:hex, :plug_crypto, "2.1.1", "19bda8184399cb24afa10be734f84a16ea0a2bc65054e23a62bb10f06bc89491", [:mix], [], "hexpm", "6470bce6ffe41c8bd497612ffde1a7e4af67f36a15eea5f921af71cf3e11247c"}, "postgrex": {:hex, :postgrex, "0.21.1", "2c5cc830ec11e7a0067dd4d623c049b3ef807e9507a424985b8dcf921224cd88", [:mix], [{:db_connection, "~> 2.1", [hex: :db_connection, repo: "hexpm", optional: false]}, {:decimal, "~> 1.5 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:table, "~> 0.1.0", [hex: :table, repo: "hexpm", optional: true]}], "hexpm", "27d8d21c103c3cc68851b533ff99eef353e6a0ff98dc444ea751de43eb48bdac"}, From 70729bdd73e8a798b552eba19383c3270ca50a2c Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 22:54:47 +0100 Subject: [PATCH 03/34] Fix: HasPermission auto_filter and strict_check implementation Fixes security issue where auto_filter returned nil instead of proper filter expressions, which could lead to incorrect authorization behavior. --- lib/mv/authorization/checks/has_permission.ex | 64 ++++++++++++++----- .../has_permission_integration_test.exs | 7 +- test/mv/membership/member_policies_test.exs | 50 +++++++++------ 3 files changed, 83 insertions(+), 38 deletions(-) diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 3858bd6..3512ef0 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -21,8 +21,8 @@ defmodule Mv.Authorization.Checks.HasPermission do - **:all** - Authorizes without filtering (returns all records) - **:own** - Filters to records where record.id == actor.id - **:linked** - Filters based on resource type: - - Member: member.user.id == actor.id (via has_one :user relationship) - - CustomFieldValue: custom_field_value.member.user.id == actor.id (traverses member → user relationship!) + - Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship) + - CustomFieldValue: `member_id == actor.member_id` (CustomFieldValue.member_id → Member.id → User.member_id) ## Error Handling @@ -129,9 +129,18 @@ defmodule Mv.Authorization.Checks.HasPermission do action = get_action_from_authorizer(authorizer) cond do - is_nil(actor) -> nil - is_nil(action) -> nil - true -> auto_filter_with_permissions(actor, resource, action) + is_nil(actor) -> + # No actor - deny access (fail-closed) + # Return filter that never matches (using impossible condition) + # This ensures no records are returned when actor is missing + [id: {:not, {:in, []}}] + + is_nil(action) -> + # Cannot determine action - deny access (fail-closed) + [id: {:not, {:in, []}}] + + true -> + auto_filter_with_permissions(actor, resource, action) end end @@ -148,12 +157,25 @@ defmodule Mv.Authorization.Checks.HasPermission do actor, resource_name ) do - :authorized -> nil - {:filter, filter_expr} -> filter_expr - false -> nil + :authorized -> + # :all scope - allow all records (no filter) + # Return empty keyword list (no filtering) + [] + + {:filter, filter_expr} -> + # :linked or :own scope - apply filter + # filter_expr is a keyword list from expr(...), return it directly + filter_expr + + false -> + # No permission - deny access (fail-closed) + # Return filter that never matches (using impossible condition) + [id: {:not, {:in, []}}] end else - _ -> nil + _ -> + # Error case (no role, invalid permission set, etc.) - deny access (fail-closed) + [id: {:not, {:in, []}}] end end @@ -162,17 +184,27 @@ defmodule Mv.Authorization.Checks.HasPermission do # Action types: :create, :read, :update, :destroy # Action names: :create_member, :update_member, etc. # PermissionSets uses action types, not action names + # + # Prefer authorizer.action.type (stable API) over authorizer.subject (varies by context) defp get_action_from_authorizer(authorizer) do - case authorizer.subject do - %{action_type: action_type} when action_type in [:create, :read, :update, :destroy] -> - action_type - - # Fallback for older Ash versions or different subject shapes - %{action: %{type: action_type}} when action_type in [:create, :read, :update, :destroy] -> + # Primary: Use authorizer.action.type (stable API) + case Map.get(authorizer, :action) do + %{type: action_type} when action_type in [:create, :read, :update, :destroy] -> action_type _ -> - nil + # Fallback: Try authorizer.subject (for compatibility with different Ash versions/contexts) + case Map.get(authorizer, :subject) do + %{action_type: action_type} when action_type in [:create, :read, :update, :destroy] -> + action_type + + %{action: %{type: action_type}} + when action_type in [:create, :read, :update, :destroy] -> + action_type + + _ -> + nil + end end end diff --git a/test/mv/authorization/checks/has_permission_integration_test.exs b/test/mv/authorization/checks/has_permission_integration_test.exs index 5aa43a5..dad68ea 100644 --- a/test/mv/authorization/checks/has_permission_integration_test.exs +++ b/test/mv/authorization/checks/has_permission_integration_test.exs @@ -72,14 +72,15 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do end describe "Filter Expression Structure - :all scope" do - test "Admin can read all members without filter" do + test "Admin can read all members without filter (returns expr(true))" do actor = create_actor_with_role("admin") authorizer = create_authorizer(Mv.Membership.Member, :read) filter = HasPermission.auto_filter(actor, authorizer, []) - # :all scope should return nil (no filter needed) - assert is_nil(filter) + # :all scope should return [] (empty keyword list = no filter = allow all records) + # After auto_filter fix: no longer returns nil, returns [] instead + assert filter == [] end end diff --git a/test/mv/membership/member_policies_test.exs b/test/mv/membership/member_policies_test.exs index ce1e7ce..69b0e22 100644 --- a/test/mv/membership/member_policies_test.exs +++ b/test/mv/membership/member_policies_test.exs @@ -132,6 +132,8 @@ defmodule Mv.Membership.MemberPoliciesTest do end test "can update linked member", %{user: user, linked_member: linked_member} do + # Update is allowed via HasPermission check with :linked scope (not via special case) + # The special case policy only applies to :read actions {:ok, updated_member} = linked_member |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"}) @@ -367,23 +369,14 @@ defmodule Mv.Membership.MemberPoliciesTest do end end - describe "special case: user can always access linked member" do - test "own_data user can read linked member even without explicit permission" do - user = create_user_with_permission_set("own_data") - linked_member = create_linked_member_for_user(user) + describe "special case: user can always READ linked member" do + # Note: The special case policy only applies to :read actions. + # Updates are handled by HasPermission with :linked scope (if permission exists). - # Reload user to get updated member_id - {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) - {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) - - # Should succeed (special case policy takes precedence) - {:ok, member} = - Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership) - - assert member.id == linked_member.id - end - - test "read_only user can read linked member (via special case)" do + test "read_only user can read linked member (via special case bypass)" do + # read_only has Member.read scope :all, but the special case ensures + # users can ALWAYS read their linked member, even if they had no read permission. + # This test verifies the special case works independently of permission sets. user = create_user_with_permission_set("read_only") linked_member = create_linked_member_for_user(user) @@ -391,14 +384,16 @@ defmodule Mv.Membership.MemberPoliciesTest do {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) - # Should succeed (special case policy takes precedence) + # Should succeed (special case bypass policy for :read takes precedence) {:ok, member} = Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership) assert member.id == linked_member.id end - test "own_data user can update linked member even without explicit permission" do + test "own_data user can read linked member (via special case bypass)" do + # own_data has Member.read scope :linked, but the special case ensures + # users can ALWAYS read their linked member regardless of permission set. user = create_user_with_permission_set("own_data") linked_member = create_linked_member_for_user(user) @@ -406,7 +401,24 @@ defmodule Mv.Membership.MemberPoliciesTest do {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) - # Should succeed (special case policy takes precedence) + # Should succeed (special case bypass policy for :read takes precedence) + {:ok, member} = + Ash.get(Membership.Member, linked_member.id, actor: user, domain: Mv.Membership) + + assert member.id == linked_member.id + end + + test "own_data user can update linked member (via HasPermission :linked scope)" do + # Update is NOT handled by special case - it's handled by HasPermission + # with :linked scope. own_data has Member.update scope :linked. + user = create_user_with_permission_set("own_data") + linked_member = create_linked_member_for_user(user) + + # Reload user to get updated member_id + {:ok, user} = Ash.get(Accounts.User, user.id, domain: Mv.Accounts, load: [:role]) + {:ok, user} = Ash.load(user, :member, domain: Mv.Accounts) + + # Should succeed via HasPermission check (not special case) {:ok, updated_member} = linked_member |> Ash.Changeset.for_update(:update_member, %{first_name: "Updated"}) From 6846363132f33d8b30a4f1df10b01dcdbb3999a6 Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 22:54:49 +0100 Subject: [PATCH 04/34] Refactor: NoActor to SimpleCheck with compile-time environment check This prevents security issues where :create/:read without actor would be allowed in production. Now all operations require an actor in production. --- lib/membership/member.ex | 9 ++-- lib/mv/authorization/checks/no_actor.ex | 66 +++++++++++++++---------- 2 files changed, 45 insertions(+), 30 deletions(-) diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 0cd7174..b26e3b4 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -300,11 +300,12 @@ defmodule Mv.Membership.Member do # Authorization Policies # Order matters: Most specific policies first, then general permission check policies do - # SYSTEM OPERATIONS: Allow operations without actor (seeds, tests, system jobs) - # This must come first to allow database seeding and test fixtures - # IMPORTANT: Use bypass so this short-circuits and doesn't require other policies + # SYSTEM OPERATIONS: Allow CRUD operations without actor + # In test: All operations allowed (for test fixtures) + # In production: Only :create and :read allowed (enforced by NoActor.check) + # :read is needed for internal Ash lookups (e.g., relationship validation during user creation). bypass action_type([:create, :read, :update, :destroy]) do - description "Allow system operations without actor (seeds, tests)" + description "Allow system operations without actor (seeds, tests, internal lookups)" authorize_if Mv.Authorization.Checks.NoActor end diff --git a/lib/mv/authorization/checks/no_actor.ex b/lib/mv/authorization/checks/no_actor.ex index c80ea9b..f5eebb7 100644 --- a/lib/mv/authorization/checks/no_actor.ex +++ b/lib/mv/authorization/checks/no_actor.ex @@ -2,22 +2,27 @@ defmodule Mv.Authorization.Checks.NoActor do @moduledoc """ Custom Ash Policy Check that allows actions when no actor is present. - This is primarily used for: - - Database seeding (priv/repo/seeds.exs) - - Test fixtures that create data without authentication - - Background jobs that operate on behalf of the system + **IMPORTANT:** This check ONLY works in test environment for security reasons. + In production/dev, ALL operations without an actor are denied. ## Security Note - This check should only be used for specific actions where system-level - access is appropriate. It should always be combined with other policy - checks that validate actor-based permissions when an actor IS present. + This check uses compile-time environment detection to prevent accidental + security issues in production. In production, ALL operations (including :create + and :read) will be denied if no actor is present. + + For seeds and system operations in production, use an admin actor instead: + + admin_user = get_admin_user() + Ash.create!(resource, attrs, actor: admin_user) ## Usage in Policies policies do - # Allow seeding and system operations - policy action_type(:create) do + # Allow system operations without actor (TEST ENVIRONMENT ONLY) + # In test: All operations allowed + # In production: ALL operations denied (fail-closed) + bypass action_type([:create, :read, :update, :destroy]) do authorize_if NoActor end @@ -29,32 +34,41 @@ defmodule Mv.Authorization.Checks.NoActor do ## Behavior - - Returns `{:ok, true}` when actor is nil (allows action) - - Returns `{:ok, :unknown}` when actor is present (delegates to other policies) - - `auto_filter` returns nil (no filtering needed) + - In test environment: Returns `true` when actor is nil (allows all operations) + - In production/dev: Returns `false` when actor is nil (denies all operations - fail-closed) + - Returns `false` when actor is present (delegates to other policies) """ - use Ash.Policy.Check + use Ash.Policy.SimpleCheck + + # Compile-time check: Only allow no-actor bypass in test environment + @allow_no_actor_bypass Mix.env() == :test + # Alternative (if you want to control via config): + # @allow_no_actor_bypass Application.compile_env(:mv, :allow_no_actor_bypass, false) @impl true def describe(_opts) do - "allows actions when no actor is present (for seeds and system operations)" - end - - @impl true - def strict_check(actor, _authorizer, _opts) do - if is_nil(actor) do - # No actor present - allow (for seeds, tests, system operations) - {:ok, true} + if @allow_no_actor_bypass do + "allows actions when no actor is present (test environment only)" else - # Actor present - let other policies decide - {:ok, :unknown} + "denies all actions when no actor is present (production/dev - fail-closed)" end end @impl true - def auto_filter(_actor, _authorizer, _opts) do - # No filtering needed - this check only validates presence/absence of actor - nil + def match?(nil, _context, _opts) do + # Actor is nil + if @allow_no_actor_bypass do + # Test environment: Allow all operations + true + else + # Production/dev: Deny all operations (fail-closed for security) + false + end + end + + def match?(_actor, _context, _opts) do + # Actor is present - don't match (let other policies decide) + false end end From 4fffeeaaa02d6c112c5414086d0d73d3974113a5 Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 22:54:51 +0100 Subject: [PATCH 05/34] Fix: Seeds use admin actor instead of NoActor bypass This ensures seeds work correctly with the new fail-closed NoActor policy in production, using proper authorization instead of bypass. --- priv/repo/seeds.exs | 46 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index 3fb2bad..7f5b322 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -162,6 +162,17 @@ if admin_role do |> Ash.update!() end +# Load admin user with role for use as actor in member operations +# This ensures all member operations have proper authorization +# If admin role creation failed, we cannot proceed with member operations +admin_user_with_role = + if admin_role do + admin_user + |> Ash.load!(:role) + else + raise "Failed to create or find admin role. Cannot proceed with member seeding." + end + # Load all membership fee types for assignment # Sort by name to ensure deterministic order all_fee_types = @@ -236,7 +247,8 @@ Enum.each(member_attrs_list, fn member_attrs -> member = Membership.create_member!(member_attrs_without_fee_type, upsert?: true, - upsert_identity: :unique_email + upsert_identity: :unique_email, + actor: admin_user_with_role ) # Only set membership_fee_type_id if member doesn't have one yet (idempotent) @@ -247,7 +259,7 @@ Enum.each(member_attrs_list, fn member_attrs -> |> Ash.Changeset.for_update(:update_member, %{ membership_fee_type_id: member_attrs_without_status.membership_fee_type_id }) - |> Ash.update!() + |> Ash.update!(actor: admin_user_with_role) else member end @@ -299,7 +311,7 @@ Enum.each(member_attrs_list, fn member_attrs -> if cycle.status != status do cycle |> Ash.Changeset.for_update(:update, %{status: status}) - |> Ash.update!() + |> Ash.update!(actor: admin_user_with_role) end end) end @@ -371,13 +383,15 @@ Enum.with_index(linked_members) Membership.create_member!( Map.put(member_attrs_without_fee_type, :user, %{id: user.id}), upsert?: true, - upsert_identity: :unique_email + upsert_identity: :unique_email, + actor: admin_user_with_role ) else # User already has a member, just create the member without linking - use upsert to prevent duplicates Membership.create_member!(member_attrs_without_fee_type, upsert?: true, - upsert_identity: :unique_email + upsert_identity: :unique_email, + actor: admin_user_with_role ) end @@ -391,7 +405,7 @@ Enum.with_index(linked_members) member |> Ash.Changeset.for_update(:update_member, %{membership_fee_type_id: fee_type.id}) - |> Ash.update!() + |> Ash.update!(actor: admin_user_with_role) else member end @@ -435,7 +449,7 @@ Enum.with_index(linked_members) end) # Create sample custom field values for some members -all_members = Ash.read!(Membership.Member) +all_members = Ash.read!(Membership.Member, actor: admin_user_with_role) all_custom_fields = Ash.read!(Membership.CustomField) # Helper function to find custom field by name @@ -463,7 +477,11 @@ if hans = find_member.("hans.mueller@example.de") do custom_field_id: field.id, value: value }) - |> Ash.create!(upsert?: true, upsert_identity: :unique_custom_field_per_member) + |> Ash.create!( + upsert?: true, + upsert_identity: :unique_custom_field_per_member, + actor: admin_user_with_role + ) end end) end @@ -488,7 +506,11 @@ if greta = find_member.("greta.schmidt@example.de") do custom_field_id: field.id, value: value }) - |> Ash.create!(upsert?: true, upsert_identity: :unique_custom_field_per_member) + |> Ash.create!( + upsert?: true, + upsert_identity: :unique_custom_field_per_member, + actor: admin_user_with_role + ) end end) end @@ -514,7 +536,11 @@ if friedrich = find_member.("friedrich.wagner@example.de") do custom_field_id: field.id, value: value }) - |> Ash.create!(upsert?: true, upsert_identity: :unique_custom_field_per_member) + |> Ash.create!( + upsert?: true, + upsert_identity: :unique_custom_field_per_member, + actor: admin_user_with_role + ) end end) end From b3eb6c922333c7dc4ad7a98054f899c39b6606d1 Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 22:54:53 +0100 Subject: [PATCH 06/34] Docs: Correct :linked scope documentation --- docs/roles-and-permissions-architecture.md | 24 ++++++++++++---------- docs/roles-and-permissions-overview.md | 4 +++- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md index b44604b..8c89af7 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -110,8 +110,8 @@ Control access to LiveView pages: 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` - - CustomFieldValue: `custom_field_value.member.user_id == user.id` + - Member: `id == user.member_id` (User.member_id → Member.id, inverse relationship) + - CustomFieldValue: `member_id == user.member_id` (traverses Member → User relationship) - **:all** - All records, no filtering **6. Special Cases** @@ -714,8 +714,8 @@ defmodule Mv.Authorization.Checks.HasPermission do - **:all** - Authorizes without filtering (returns all records) - **:own** - Filters to records where record.id == actor.id - **:linked** - Filters based on resource type: - - Member: member.user_id == actor.id - - CustomFieldValue: custom_field_value.member.user_id == actor.id (traverses relationship!) + - Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship) + - CustomFieldValue: `member_id == actor.member_id` (CustomFieldValue.member_id → Member.id → User.member_id) ## Error Handling @@ -799,12 +799,14 @@ defmodule Mv.Authorization.Checks.HasPermission do defp apply_scope(:linked, actor, resource_name) do case resource_name do "Member" -> - # Member.user_id == actor.id (direct relationship) - {:filter, expr(user_id == ^actor.id)} + # User.member_id → Member.id (inverse relationship) + # Filter: member.id == actor.member_id + {:filter, expr(id == ^actor.member_id)} "CustomFieldValue" -> - # CustomFieldValue.member.user_id == actor.id (traverse through member!) - {:filter, expr(member.user_id == ^actor.id)} + # CustomFieldValue.member_id → Member.id → User.member_id + # Filter: custom_field_value.member_id == actor.member_id + {:filter, expr(member_id == ^actor.member_id)} _ -> # Fallback for other resources: try direct user_id @@ -918,7 +920,7 @@ end **Location:** `lib/mv/membership/member.ex` -**Special Case:** Users can always access their linked member (where `member.user_id == user.id`). +**Special Case:** Users can always READ their linked member (where `id == user.member_id`). ```elixir defmodule Mv.Membership.Member do @@ -978,10 +980,10 @@ defmodule Mv.Membership.CustomFieldValue do policies do # SPECIAL CASE: Users can access custom field values of their linked member - # Note: This traverses the member relationship! + # Note: This uses member_id relationship (CustomFieldValue.member_id → Member.id → User.member_id) policy action_type([:read, :update]) do description "Users can access custom field values of their linked member" - authorize_if expr(member.user_id == ^actor(:id)) + authorize_if expr(member_id == ^actor(:member_id)) end # GENERAL: Check permissions from role diff --git a/docs/roles-and-permissions-overview.md b/docs/roles-and-permissions-overview.md index 86e7273..61aef9c 100644 --- a/docs/roles-and-permissions-overview.md +++ b/docs/roles-and-permissions-overview.md @@ -294,7 +294,9 @@ Each Permission Set contains: **:own** - Only records where id == actor.id - Example: User can read their own User record -**:linked** - Only records where user_id == actor.id +**:linked** - Only records linked to actor via relationships +- Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship) +- CustomFieldValue: `member_id == actor.member_id` (traverses Member → User relationship) - Example: User can read Member linked to their account **:all** - All records without restriction From 42a463f42221b9d903353549ba178d249dbdbf18 Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 23:12:07 +0100 Subject: [PATCH 07/34] Security: Fix critical deny-filter bug and improve authorization CRITICAL FIX: Deny-filter was allowing all records instead of denying Fix: User validation in Member now uses actor from changeset.context --- ...-_membership_fee_features_e886dc4b.plan.md | 318 ------------------ lib/membership/member.ex | 15 +- lib/mv/authorization/checks/has_permission.ex | 20 +- .../has_permission_integration_test.exs | 4 +- 4 files changed, 25 insertions(+), 332 deletions(-) delete mode 100644 code_review_fixes_-_membership_fee_features_e886dc4b.plan.md diff --git a/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md b/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md deleted file mode 100644 index eebf419..0000000 --- a/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md +++ /dev/null @@ -1,318 +0,0 @@ ---- -name: Code Review Fixes - Membership Fee Features -overview: Umsetzung der validen Code Review Punkte aus beiden Reviews mit Priorisierung nach Kritikalität. Fokus auf Transaktionssicherheit, Code-Qualität, Performance und UX-Verbesserungen. -todos: - - id: fix-after-action-tasks - content: "after_action mit Task.start → after_transaction + Task.Supervisor: Task.Supervisor zu application.ex hinzufügen, after_action Hooks in after_transaction umwandeln, Task.Supervisor.async_nolink verwenden" - status: pending - - id: reduce-code-duplication - content: "Code-Duplikation reduzieren: handle_cycle_generation/2 private Funktion extrahieren, alle drei Stellen (Create, Type Change, Date Change) verwenden" - status: pending - dependencies: - - fix-after-action-tasks - - id: fix-join-date-validation - content: "join_date Validierung: Entweder Validierung wieder hinzufügen (validate compare(:join_date, less_than_or_equal_to: &Date.utc_today/0)) oder Dokumentation anpassen" - status: pending - - id: fix-load-cycles-docs - content: "load_cycles_for_members: Entweder Dokumentation korrigieren (ehrlich machen) oder echte Filterung implementieren (z.B. nur letzte 2 Intervalle)" - status: pending - - id: fix-get-current-cycle-sort - content: "get_current_cycle nondeterministisch: Vor List.first() nach cycle_start sortieren (desc) in MembershipFeeHelpers.get_current_cycle" - status: pending - - id: fix-n1-query-member-count - content: "N+1 Query beheben: Aggregate auf MembershipFeeType definieren oder member_count einmalig vorab laden und in assigns cachen" - status: pending - - id: fix-assign-new-stale - content: "assign_new → assign: In MembershipFeesComponent.update/2 immer assign(:cycles, cycles) und assign(:available_fee_types, available_fee_types) setzen" - status: pending - - id: fix-regenerating-flag - content: "@regenerating auf true setzen: Direkt beim Event-Start in handle_event(\"regenerate_cycles\", ...) socket |> assign(:regenerating, true) setzen" - status: pending - - id: fix-create-cycle-parsing - content: "Create-cycle parsing Fix: Decimal.parse explizit behandeln und {:error, :invalid_amount} zurückgeben statt :error" - status: pending - - id: fix-delete-all-atomic - content: "Delete all cycles atomar: Bulk Delete Query verwenden (Ash bulk destroy oder Query-basiert) statt Enum.map" - status: pending - - id: improve-async-error-handling - content: "Fehlerbehandlung bei async Tasks: Strukturierte Error-Logs mit Context, optional Retry-Mechanismus oder Event-System für Benachrichtigung" - status: pending - - id: improve-format-currency - content: "format_currency Robustheit: Number.Currency verwenden oder robusteres Pattern Matching + Tests für Edge Cases (negative Zahlen, sehr große Zahlen)" - status: pending - - id: add-missing-typespecs - content: "Fehlende Typespecs: @spec für SetDefaultMembershipFeeType.change/3 hinzufügen" - status: pending - - id: fix-race-condition - content: "Potenzielle Race Condition: Prüfen ob Ash doppelte Auslösung verhindert, ggf. Logik anpassen (beide Änderungen in einem Hook zusammenfassen)" - status: pending - - id: extract-magic-values - content: "Magic Numbers/Strings: Application.get_env(:mv, :sql_sandbox, false) in Konstante/Helper extrahieren (z.B. Mv.Config.sql_sandbox?/0)" - status: pending - - id: fix-domain-consistency - content: "Domain-Konsistenz: Überall in MembershipFeesComponent domain: MembershipFees explizit angeben" - status: pending - - id: fix-test-helper - content: "Test-Helper Fix: create_cycle/3 Helper - Cleanup nur einmal im Setup oder gezielt nur auto-generierte löschen" - status: pending - - id: fix-date-utc-today-param - content: "Date.utc_today() Parameter: today Parameter durchgeben in get_cycle_status_for_member und Helper-Funktionen" - status: pending - - id: fix-ui-locale-input - content: "UI/Locale Input Fix: type=\"number\" → type=\"text\" + inputmode=\"decimal\" + serverseitig \",\" → \".\" normalisieren" - status: pending - - id: fix-delete-confirmation - content: "Delete-all-Confirmation robuster: String.trim() + case-insensitive Vergleich oder \"type DELETE\" Pattern" - status: pending - - id: fix-warning-state - content: "Warning-State Fix: Bei Decimal.parse(:error) explizit hide_amount_warning(socket) aufrufen" - status: pending - - id: fix-double-toggle - content: "Toggle entfernen: Toggle-Button im Spalten-Header entfernen (nur in Toolbar behalten)" - status: pending - - id: fix-format-consistency - content: "Format-Konsistenz: Inputs ebenfalls auf Komma ausrichten oder serverseitig normalisieren" - status: pending - dependencies: - - fix-ui-locale-input ---- - -# Code Review Fixes - Membership Fee Features - -## Kritische Probleme (Müssen vor Merge behoben werden) - -### 1. after_action mit Task.start - Transaktionsprobleme - -**Dateien:** `lib/membership/member.ex` (Zeilen 142, 279) - -**Problem:** `Task.start/1` wird innerhalb von `after_action` Hooks verwendet. `after_action` läuft innerhalb der DB-Transaktion, daher: - -- Tasks sehen möglicherweise noch nicht committed state -- Tasks werden auch bei Rollback gestartet -- Keine Supervision → Memory Leaks möglich - -**Lösung:** - -- `after_transaction` Hook verwenden (Ash Best Practice) -- `Task.Supervisor` zum Supervision Tree hinzufügen (`lib/mv/application.ex`) -- `Task.Supervisor.async_nolink/3` statt `Task.start/1` verwenden - -**Betroffene Stellen:** - -- Member Creation (Zeile 116-164) -- Join/Exit Date Change (Zeile 250-301) - -### 2. Code-Duplikation in Cycle-Generation-Logik - -**Datei:** `lib/membership/member.ex` - -**Problem:** Cycle-Generation-Logik ist dreimal dupliziert (Create, Type Change, Date Change) - -**Lösung:** Extrahiere in private Funktion `handle_cycle_generation/2` - -## Wichtige Probleme (Sollten behoben werden) - -### 3. join_date Validierung entfernt, aber Dokumentation behauptet Gegenteil - -**Datei:** `lib/membership/member.ex` (Zeile 27, 516-518) - -**Problem:** Dokumentation sagt "join_date not in future", aber Validierung fehlt - -**Lösung:** Dokumentation anpassen - -### 4. load_cycles_for_members overpromises - -**Datei:** `lib/mv_web/member_live/index/membership_fee_status.ex` (Zeile 36-40) - -**Problem:** Dokumentation sagt "Only loads the relevant cycle per member" und "Filters cycles at database level", aber lädt alle Cycles - -**Lösung:** echte Filterung implementieren (z.B. nur letzte 2 Intervalle) - -### 5. get_current_cycle nondeterministisch - -**Datei:** `lib/mv_web/helpers/membership_fee_helpers.ex` (Zeile 178-182) - -**Problem:** `List.first()` ohne explizite Sortierung → Ergebnis hängt von Reihenfolge ab - -**Lösung:** Vor `List.first()` nach `cycle_start` sortieren (desc) - -### 6. N+1 Query durch get_member_count - -**Datei:** `lib/mv_web/live/membership_fee_type_live/index.ex` (Zeile 134-140) - -**Problem:** `get_member_count/1` wird pro Row aufgerufen → N+1 Query - -**Lösung:** Aggregate auf MembershipFeeType definieren oder einmalig vorab laden - -### 7. assign_new kann stale werden - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 402-403) - -**Problem:** `assign_new(:cycles, ...)` und `assign_new(:available_fee_types, ...)` werden nur gesetzt, wenn Assign noch nicht existiert - -**Lösung:** In `update/2` immer `assign(:cycles, cycles)` / `assign(:available_fee_types, available_fee_types)` setzen - -### 8. @regenerating wird nie auf true gesetzt - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 526-561) - -**Problem:** `regenerating` wird nur auf `false` gesetzt, nie auf `true` → Button/Spinner werden nie disabled - -**Lösung:** Direkt beim Event-Start `socket |> assign(:regenerating, true)` setzen - -### 9. Create-cycle parsing: invalid amount zeigt falsche Fehlermeldung - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 748-812) - -**Problem:** `Decimal.parse/1` gibt `:error` zurück, aber `with` behandelt es als `:error` → landet in "Invalid date format" Branch - -**Lösung:** Explizit `{:error, :invalid_amount}` zurückgeben: - -```elixir -amount = case Decimal.parse(amount_str) do - {d, _} -> {:ok, d} - :error -> {:error, :invalid_amount} -end -``` - -### 10. Delete all cycles: nicht atomar - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 666-714) - -**Problem:** `Enum.map(cycles, &Ash.destroy/1)` → nicht atomar, teilweise gelöscht möglich - -**Lösung:** Bulk Delete Query verwenden (Ash bulk destroy oder Query-basiert) - -### 11. Fehlerbehandlung bei async Tasks - -**Datei:** `lib/membership/member.ex` - -**Problem:** Bei Fehlern in async Tasks wird nur geloggt, aber der Benutzer erhält keine Rückmeldung. Die Member-Aktion wird als erfolgreich zurückgegeben, auch wenn die Cycle-Generierung fehlschlägt. Keine Retry-Logik oder Monitoring. - -**Lösung:** - -- Für kritische Fälle: synchron ausführen oder Retry-Mechanismus implementieren -- Für nicht-kritische Fälle: Event-System für spätere Benachrichtigung -- Strukturierte Error-Logs mit Context -- Optional: Error-Tracking (Sentry, etc.) - -### 12. format_currency Robustheit - -**Datei:** `lib/mv_web/helpers/membership_fee_helpers.ex` (Zeilen 27-51) - -**Problem:** Die Funktion verwendet String-Manipulation für Formatierung. Edge Cases könnten problematisch sein (z.B. sehr große Zahlen, negative Werte). - -**Lösung:** - -- `Number.Currency` oder ähnliche Bibliothek verwenden -- Oder: Robusteres Pattern Matching für Edge Cases -- Tests für Edge Cases hinzufügen (negative Zahlen, sehr große Zahlen) - -### 13. Fehlende Typespecs - -**Datei:** `lib/membership/member/changes/set_default_membership_fee_type.ex` - -**Problem:** Keine `@spec` für die `change/3` Funktion. - -**Lösung:** Typespecs hinzufügen für bessere Dokumentation und Dialyzer-Support. - -### 14. Potenzielle Race Condition - -**Datei:** `lib/membership/member.ex` (Zeile 250-301) - -**Problem:** Wenn `join_date` und `exit_date` gleichzeitig geändert werden, könnte die Cycle-Generierung zweimal ausgelöst werden (einmal pro Änderung). - -**Lösung:** Prüfen, ob Ash dies bereits verhindert, oder Logik anpassen (z.B. beide Änderungen in einem Hook zusammenfassen). - -### 15. Magic Numbers/Strings - -**Problem:** `Application.get_env(:mv, :sql_sandbox, false)` wird mehrfach verwendet. - -**Lösung:** Extrahiere in Konstante oder Helper-Funktion (z.B. `Mv.Config.sql_sandbox?/0`). - -## Mittlere Probleme (Nice-to-have) - -### 16. Inconsistent use of domain - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 819-821) - -**Problem:** Einige Actions verwenden `domain: MembershipFees`, andere nicht - -**Lösung:** Konsistent `domain` überall verwenden - -### 17. Tests: create_cycle/3 löscht jedes Mal alle Cycles - -**Datei:** `test/mv_web/member_live/index/membership_fee_status_test.exs` (Zeile 45-52) - -**Problem:** Helper löscht vor jedem Create alle Cycles → Tests prüfen nicht, was sie denken - -**Lösung:** Cleanup nur einmal im Setup oder gezielt nur auto-generierte löschen - -### 18. Tests/Design: Date.utc_today() macht Tests flaky - -**Problem:** Tests hängen von `Date.utc_today()` ab → nicht deterministisch - -**Lösung:** `today` Parameter durchgeben (z.B. `get_cycle_status_for_member(member, show_current, today \\ Date.utc_today())`) - -### 19. UI/Locale: input type="number" + Decimal/Komma - -**Problem:** `type="number"` funktioniert nicht zuverlässig mit Komma als Dezimaltrenner - -**Lösung:** `type="text"` + `inputmode="decimal"` + serverseitig "," → "." normalisieren - -### 20. Delete-all-Confirmation: String-Vergleich ist fragil - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 296-298) - -**Problem:** String-Vergleich gegen `gettext("Yes")` und `"Yes"` → fragil bei Whitespace/Locale - -**Lösung:** `String.trim()` + case-insensitive Vergleich oder "type DELETE" Pattern - -### 21. MembershipFeeType Form: Warning-State kann hängen bleiben - -**Datei:** `lib/mv_web/live/membership_fee_type_live/form.ex` (Zeile 367-378) - -**Problem:** Bei `Decimal.parse(:error)` wird nur `socket` zurückgegeben → Warning kann stehen bleiben - -**Lösung:** Bei `:error` explizit `hide_amount_warning(socket)` aufrufen - -### 22. UI/UX: Toggle ist doppelt vorhanden - -**Datei:** `lib/mv_web/live/member_live/index.html.heex` (Zeile 45-72, 284-296) - -**Problem:** Toggle-Button sowohl in Toolbar als auch im Spalten-Header - -**Lösung:** Toggle im Spalten-Header entfernen (nur in Toolbar behalten) - -### 23. Konsistenz: format_currency vs Inputs - -**Problem:** `format_currency` formatiert deutsch (Komma), aber Inputs erwarten Punkt - -**Lösung:** Inputs ebenfalls auf Komma ausrichten oder serverseitig normalisieren - -## Implementierungsreihenfolge - -1. **Kritisch:** after_action → after_transaction + Task.Supervisor -2. **Kritisch:** Code-Duplikation reduzieren -3. **Wichtig:** join_date Validierung/Dokumentation -4. **Wichtig:** load_cycles_for_members Dokumentation/Implementierung -5. **Wichtig:** get_current_cycle Sortierung -6. **Wichtig:** N+1 Query beheben -7. **Wichtig:** assign_new → assign -8. **Wichtig:** @regenerating auf true setzen -9. **Wichtig:** Create-cycle parsing Fix -10. **Wichtig:** Delete all cycles atomar -11. **Wichtig:** Fehlerbehandlung bei async Tasks -12. **Wichtig:** format_currency Robustheit -13. **Wichtig:** Fehlende Typespecs -14. **Wichtig:** Potenzielle Race Condition prüfen/beheben -15. **Wichtig:** Magic Numbers/Strings extrahieren -16. **Mittel:** Domain-Konsistenz -17. **Mittel:** Test-Helper Fix -18. **Mittel:** Date.utc_today() Parameter -19. **Mittel:** UI/Locale Fixes -20. **Mittel:** String-Vergleich robuster -21. **Mittel:** Warning-State Fix -22. **Mittel:** Toggle entfernen -23. **Mittel:** Format-Konsistenz - diff --git a/lib/membership/member.ex b/lib/membership/member.ex index b26e3b4..873aac0 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -300,12 +300,12 @@ defmodule Mv.Membership.Member do # Authorization Policies # Order matters: Most specific policies first, then general permission check policies do - # SYSTEM OPERATIONS: Allow CRUD operations without actor + # SYSTEM OPERATIONS: Allow CRUD operations without actor (TEST ENVIRONMENT ONLY) # In test: All operations allowed (for test fixtures) - # In production: Only :create and :read allowed (enforced by NoActor.check) - # :read is needed for internal Ash lookups (e.g., relationship validation during user creation). + # In production/dev: ALL operations denied without actor (fail-closed for security) + # NoActor.check uses compile-time environment detection to prevent security issues bypass action_type([:create, :read, :update, :destroy]) do - description "Allow system operations without actor (seeds, tests, internal lookups)" + description "Allow system operations without actor (test environment only)" authorize_if Mv.Authorization.Checks.NoActor end @@ -399,8 +399,13 @@ defmodule Mv.Membership.Member do user_id = user_arg[:id] current_member_id = changeset.data.id + # Get actor from changeset context for authorization + # If no actor is present, this will fail in production (fail-closed) + actor = Map.get(changeset.context || %{}, :actor) + # Check the current state of the user in the database - case Ash.get(Mv.Accounts.User, user_id) do + # Pass actor to ensure proper authorization (User might have policies in future) + case Ash.get(Mv.Accounts.User, user_id, actor: actor) do # User is free to be linked {:ok, %{member_id: nil}} -> :ok diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 3512ef0..68d83d1 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -131,13 +131,12 @@ defmodule Mv.Authorization.Checks.HasPermission do cond do is_nil(actor) -> # No actor - deny access (fail-closed) - # Return filter that never matches (using impossible condition) - # This ensures no records are returned when actor is missing - [id: {:not, {:in, []}}] + # Return filter that never matches (id IN [] = never matches) + deny_filter() is_nil(action) -> # Cannot determine action - deny access (fail-closed) - [id: {:not, {:in, []}}] + deny_filter() true -> auto_filter_with_permissions(actor, resource, action) @@ -169,16 +168,23 @@ defmodule Mv.Authorization.Checks.HasPermission do false -> # No permission - deny access (fail-closed) - # Return filter that never matches (using impossible condition) - [id: {:not, {:in, []}}] + deny_filter() end else _ -> # Error case (no role, invalid permission set, etc.) - deny access (fail-closed) - [id: {:not, {:in, []}}] + deny_filter() end end + # Helper function to return a filter that never matches (deny all records) + # Used when authorization should be denied (fail-closed) + # Returns [id: {:in, []}] which means "id IN []" - never matches (correct deny-all) + # NOTE: [id: {:not, {:in, []}}] would be "NOT (id IN [])" = true for all IDs (allow-all) - WRONG! + defp deny_filter do + [id: {:in, []}] + end + # Helper to extract action type from authorizer # CRITICAL: Must use action_type, not action.name! # Action types: :create, :read, :update, :destroy diff --git a/test/mv/authorization/checks/has_permission_integration_test.exs b/test/mv/authorization/checks/has_permission_integration_test.exs index dad68ea..9ef5fd2 100644 --- a/test/mv/authorization/checks/has_permission_integration_test.exs +++ b/test/mv/authorization/checks/has_permission_integration_test.exs @@ -28,7 +28,7 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do end describe "Filter Expression Structure - :linked scope" do - test "Member filter uses user.id relationship path" do + test "Member filter uses actor.member_id (inverse relationship)" do actor = create_actor_with_role("own_data", member_id: "member-123") authorizer = create_authorizer(Mv.Membership.Member, :read) @@ -42,7 +42,7 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do assert is_list(filter) or is_map(filter) end - test "CustomFieldValue filter uses member.user.id relationship path" do + test "CustomFieldValue filter uses actor.member_id (via member relationship)" do actor = create_actor_with_role("own_data", member_id: "member-123") authorizer = create_authorizer(Mv.Membership.CustomFieldValue, :read) From c95a6fac69eb3f2f876dad9da068ae3a2cbabc8b Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 23:29:37 +0100 Subject: [PATCH 08/34] Improve: Make deny_filter robust and add regression test - Change deny_filter from [id: {:in, []}] to expr(false) - Add regression test to ensure deny-filter matches 0 records --- lib/mv/authorization/checks/has_permission.ex | 7 +-- .../has_permission_fail_closed_test.exs | 44 +++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) create mode 100644 test/mv/authorization/checks/has_permission_fail_closed_test.exs diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 68d83d1..31c18cb 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -179,10 +179,11 @@ defmodule Mv.Authorization.Checks.HasPermission do # Helper function to return a filter that never matches (deny all records) # Used when authorization should be denied (fail-closed) - # Returns [id: {:in, []}] which means "id IN []" - never matches (correct deny-all) - # NOTE: [id: {:not, {:in, []}}] would be "NOT (id IN [])" = true for all IDs (allow-all) - WRONG! + # + # Using `expr(false)` avoids depending on the primary key being named `:id`. + # This is more robust than [id: {:in, []}] which assumes the primary key is `:id`. defp deny_filter do - [id: {:in, []}] + expr(false) end # Helper to extract action type from authorizer diff --git a/test/mv/authorization/checks/has_permission_fail_closed_test.exs b/test/mv/authorization/checks/has_permission_fail_closed_test.exs new file mode 100644 index 0000000..822e5aa --- /dev/null +++ b/test/mv/authorization/checks/has_permission_fail_closed_test.exs @@ -0,0 +1,44 @@ +defmodule Mv.Authorization.Checks.HasPermissionFailClosedTest do + @moduledoc """ + Regression tests to ensure deny-filter behavior is fail-closed (matches no records). + + These tests verify that when HasPermission.auto_filter returns a deny-filter + (e.g., when actor is nil or no permission is found), the filter actually + matches zero records in the database. + + This prevents regressions like the previous bug where [id: {:not, {:in, []}}] + was used, which logically evaluates to "NOT (id IN [])" = true for all IDs, + effectively allowing all records instead of denying them. + """ + use Mv.DataCase, async: true + + alias Mv.Authorization.Checks.HasPermission + + import Mv.Fixtures + + test "auto_filter deny-filter matches no records (regression for NOT IN [] allow-all bug)" do + # Arrange: create some members in DB + _m1 = member_fixture() + _m2 = member_fixture() + + # Build a minimal authorizer with a stable action type (:read) + authorizer = %Ash.Policy.Authorizer{ + resource: Mv.Membership.Member, + action: %{type: :read} + } + + # Act: missing actor must yield a deny-all filter (fail-closed) + deny_filter = HasPermission.auto_filter(nil, authorizer, []) + + # Apply the returned filter to a real DB query (no authorization involved) + query = + Mv.Membership.Member + |> Ash.Query.new() + |> Ash.Query.filter_input(deny_filter) + + {:ok, results} = Ash.read(query, domain: Mv.Membership, authorize?: false) + + # Assert: deny-filter must match nothing + assert results == [] + end +end From dc3268cbf473972686cb9bf963dfdcdbf3f1179c Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 23:34:04 +0100 Subject: [PATCH 09/34] Fix: Update comment in auto_filter to reflect expr(false) usage Update comment from 'id IN [] = never matches' to 'expr(false) = match none' to match the actual implementation of deny_filter(). --- lib/mv/authorization/checks/has_permission.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 31c18cb..18ffe5b 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -131,7 +131,7 @@ defmodule Mv.Authorization.Checks.HasPermission do cond do is_nil(actor) -> # No actor - deny access (fail-closed) - # Return filter that never matches (id IN [] = never matches) + # Return filter that never matches (expr(false) = match none) deny_filter() is_nil(action) -> From bc87893134ec436c41f9ef1a55f1a3e56896dbda Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 9 Jan 2026 00:02:19 +0100 Subject: [PATCH 10/34] Integrate Member policies in LiveViews - Add on_mount hook to ensure user role is loaded in all Member LiveViews - Pass actor parameter to all Ash operations (read, get, create, update, destroy, load) --- lib/mv_web/live/member_live/form.ex | 82 +++++++++----- lib/mv_web/live/member_live/index.ex | 16 ++- lib/mv_web/live/member_live/show.ex | 25 +++-- .../show/membership_fees_component.ex | 103 ++++++++++++------ priv/gettext/de/LC_MESSAGES/default.po | 5 + priv/gettext/default.pot | 5 + priv/gettext/en/LC_MESSAGES/default.po | 5 + 7 files changed, 167 insertions(+), 74 deletions(-) diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index 0a05e1f..c7c718e 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -21,6 +21,8 @@ defmodule MvWeb.MemberLive.Form do """ use MvWeb, :live_view + on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} + alias Mv.MembershipFees alias Mv.MembershipFees.MembershipFeeType alias MvWeb.Helpers.MembershipFeeHelpers @@ -222,6 +224,8 @@ defmodule MvWeb.MemberLive.Form do @impl true def mount(params, _session, socket) do + # current_user should be set by on_mount hooks (LiveUserAuth + LiveHelpers) + actor = socket.assigns[:current_user] || socket.assigns.current_user {:ok, custom_fields} = Mv.Membership.list_custom_fields() initial_custom_field_values = @@ -239,14 +243,14 @@ defmodule MvWeb.MemberLive.Form do member = case params["id"] do nil -> nil - id -> Ash.get!(Mv.Membership.Member, id, load: [:membership_fee_type]) + id -> Ash.get!(Mv.Membership.Member, id, load: [:membership_fee_type], actor: actor) end page_title = if is_nil(member), do: gettext("Create Member"), else: gettext("Edit Member") # Load available membership fee types - available_fee_types = load_available_fee_types(member) + available_fee_types = load_available_fee_types(member, actor) {:ok, socket @@ -283,35 +287,59 @@ defmodule MvWeb.MemberLive.Form do end def handle_event("save", %{"member" => member_params}, socket) do - case AshPhoenix.Form.submit(socket.assigns.form, params: member_params) do - {:ok, member} -> - notify_parent({:saved, member}) + try do + actor = socket.assigns[:current_user] || socket.assigns.current_user - action = - case socket.assigns.form.source.type do - :create -> gettext("create") - :update -> gettext("update") - other -> to_string(other) - end + case AshPhoenix.Form.submit(socket.assigns.form, params: member_params, actor: actor) do + {:ok, member} -> + handle_save_success(socket, member) - socket = - socket - |> put_flash(:info, gettext("Member %{action} successfully", action: action)) - |> push_navigate(to: return_path(socket.assigns.return_to, member)) - - {:noreply, socket} - - {:error, form} -> - {:noreply, assign(socket, form: form)} + {:error, form} -> + {:noreply, assign(socket, form: form)} + end + rescue + _e in [Ash.Error.Forbidden, Ash.Error.Forbidden.Policy] -> + handle_save_forbidden(socket) end end + defp handle_save_success(socket, member) do + notify_parent({:saved, member}) + + action = get_action_name(socket.assigns.form.source.type) + + socket = + socket + |> put_flash(:info, gettext("Member %{action} successfully", action: action)) + |> push_navigate(to: return_path(socket.assigns.return_to, member)) + + {:noreply, socket} + end + + defp handle_save_forbidden(socket) do + # Handle policy violations that aren't properly displayed in forms + # AshPhoenix.Form doesn't implement FormData.Error protocol for Forbidden errors + action = get_action_name(socket.assigns.form.source.type) + + error_message = + gettext("You do not have permission to %{action} members.", action: action) + + {:noreply, put_flash(socket, :error, error_message)} + end + + defp get_action_name(:create), do: gettext("create") + defp get_action_name(:update), do: gettext("update") + defp get_action_name(other), do: to_string(other) + defp notify_parent(msg), do: send(self(), {__MODULE__, msg}) - defp assign_form(%{assigns: %{member: member}} = socket) do + defp assign_form(%{assigns: assigns} = socket) do + member = assigns.member + actor = assigns[:current_user] || assigns.current_user + form = if member do - {:ok, member} = Ash.load(member, custom_field_values: [:custom_field]) + {:ok, member} = Ash.load(member, custom_field_values: [:custom_field], actor: actor) existing_custom_field_values = member.custom_field_values @@ -342,7 +370,8 @@ defmodule MvWeb.MemberLive.Form do api: Mv.Membership, as: "member", params: params, - forms: [auto?: true] + forms: [auto?: true], + actor: actor ) missing_custom_field_values = @@ -360,7 +389,8 @@ defmodule MvWeb.MemberLive.Form do api: Mv.Membership, as: "member", params: %{"custom_field_values" => socket.assigns[:initial_custom_field_values]}, - forms: [auto?: true] + forms: [auto?: true], + actor: actor ) end @@ -375,11 +405,11 @@ defmodule MvWeb.MemberLive.Form do # Helper Functions # ----------------------------------------------------------------- - defp load_available_fee_types(member) do + defp load_available_fee_types(member, actor) do all_types = MembershipFeeType |> Ash.Query.sort(name: :asc) - |> Ash.read!(domain: MembershipFees) + |> Ash.read!(domain: MembershipFees, actor: actor) # If member has a fee type, filter to same interval if member && member.membership_fee_type do diff --git a/lib/mv_web/live/member_live/index.ex b/lib/mv_web/live/member_live/index.ex index 34928cd..d676369 100644 --- a/lib/mv_web/live/member_live/index.ex +++ b/lib/mv_web/live/member_live/index.ex @@ -27,6 +27,8 @@ defmodule MvWeb.MemberLive.Index do """ use MvWeb, :live_view + on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} + require Ash.Query import Ash.Expr @@ -58,17 +60,19 @@ defmodule MvWeb.MemberLive.Index do # Note: Using Ash.read! (bang version) - errors will be handled by Phoenix LiveView # and result in a 500 error page. This is appropriate for LiveViews where errors # should be visible to the user rather than silently failing. + actor = socket.assigns[:current_user] + custom_fields_visible = Mv.Membership.CustomField |> Ash.Query.filter(expr(show_in_overview == true)) |> Ash.Query.sort(name: :asc) - |> Ash.read!() + |> Ash.read!(actor: actor) # Load ALL custom fields for the dropdown (to show all available fields) all_custom_fields = Mv.Membership.CustomField |> Ash.Query.sort(name: :asc) - |> Ash.read!() + |> Ash.read!(actor: actor) # Load settings once to avoid N+1 queries settings = @@ -132,8 +136,9 @@ defmodule MvWeb.MemberLive.Index do def handle_event("delete", %{"id" => id}, socket) do # Note: Using bang versions (!) - errors will be handled by Phoenix LiveView # This ensures users see error messages if deletion fails (e.g., permission denied) - member = Ash.get!(Mv.Membership.Member, id) - Ash.destroy!(member) + actor = socket.assigns[:current_user] + member = Ash.get!(Mv.Membership.Member, id, actor: actor) + Ash.destroy!(member, actor: actor) updated_members = Enum.reject(socket.assigns.members, &(&1.id == id)) {:noreply, assign(socket, :members, updated_members)} @@ -678,7 +683,8 @@ defmodule MvWeb.MemberLive.Index do # Note: Using Ash.read! - errors will be handled by Phoenix LiveView # This is appropriate for data loading in LiveViews - members = Ash.read!(query) + actor = socket.assigns[:current_user] + members = Ash.read!(query, actor: actor) # Custom field values are already filtered at the database level in load_custom_field_values/2 # No need for in-memory filtering anymore diff --git a/lib/mv_web/live/member_live/show.ex b/lib/mv_web/live/member_live/show.ex index 5aa4d93..7917e43 100644 --- a/lib/mv_web/live/member_live/show.ex +++ b/lib/mv_web/live/member_live/show.ex @@ -21,6 +21,8 @@ defmodule MvWeb.MemberLive.Show do use MvWeb, :live_view import Ash.Query + on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} + alias MvWeb.Helpers.MembershipFeeHelpers @impl true @@ -148,9 +150,9 @@ defmodule MvWeb.MemberLive.Show do <%!-- Custom Fields Section --%> - <%= if is_list(@custom_fields) && Enum.any?(@custom_fields) do %> + <%= if Enum.any?(@custom_fields) do %>
- <.section_box title={gettext("Additional Data Fields")}> + <.section_box title={gettext("Custom Fields")}>
<%= for custom_field <- @custom_fields do %> <% cfv = find_custom_field_value(@member.custom_field_values, custom_field.id) %> @@ -220,6 +222,7 @@ defmodule MvWeb.MemberLive.Show do module={MvWeb.MemberLive.Show.MembershipFeesComponent} id={"membership-fees-#{@member.id}"} member={@member} + current_user={@current_user} /> <% end %> @@ -233,15 +236,15 @@ defmodule MvWeb.MemberLive.Show do @impl true def handle_params(%{"id" => id}, _, socket) do - # Load custom fields for display - # Note: Each page load starts a new LiveView process, so caching with - # assign_new is not necessary here (mount creates a fresh socket each time) - custom_fields = - Mv.Membership.CustomField - |> Ash.Query.sort(name: :asc) - |> Ash.read!() + actor = socket.assigns[:current_user] - socket = assign(socket, :custom_fields, custom_fields) + # Load custom fields once using assign_new to avoid repeated queries + socket = + assign_new(socket, :custom_fields, fn -> + Mv.Membership.CustomField + |> Ash.Query.sort(name: :asc) + |> Ash.read!(actor: actor) + end) query = Mv.Membership.Member @@ -253,7 +256,7 @@ defmodule MvWeb.MemberLive.Show do membership_fee_cycles: [:membership_fee_type] ]) - member = Ash.read_one!(query) + member = Ash.read_one!(query, actor: actor) # Calculate last and current cycle status from loaded cycles last_cycle_status = get_last_cycle_status(member) diff --git a/lib/mv_web/live/member_live/show/membership_fees_component.ex b/lib/mv_web/live/member_live/show/membership_fees_component.ex index d8c49eb..0b32efe 100644 --- a/lib/mv_web/live/member_live/show/membership_fees_component.ex +++ b/lib/mv_web/live/member_live/show/membership_fees_component.ex @@ -388,6 +388,7 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do @impl true def update(assigns, socket) do member = assigns.member + actor = assigns.current_user # Load cycles if not already loaded cycles = @@ -401,7 +402,7 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do cycles = Enum.sort_by(cycles, & &1.cycle_start, {:desc, Date}) # Get available fee types (filtered to same interval if member has a type) - available_fee_types = get_available_fee_types(member) + available_fee_types = get_available_fee_types(member, actor) {:ok, socket @@ -422,7 +423,9 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do @impl true def handle_event("change_membership_fee_type", %{"value" => ""}, socket) do # Remove membership fee type - case update_member_fee_type(socket.assigns.member, nil) do + actor = socket.assigns.current_user + + case update_member_fee_type(socket.assigns.member, nil, actor) do {:ok, updated_member} -> send(self(), {:member_updated, updated_member}) @@ -430,7 +433,10 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do socket |> assign(:member, updated_member) |> assign(:cycles, []) - |> assign(:available_fee_types, get_available_fee_types(updated_member)) + |> assign( + :available_fee_types, + get_available_fee_types(updated_member, socket.assigns.current_user) + ) |> assign(:interval_warning, nil) |> put_flash(:info, gettext("Membership fee type removed"))} @@ -441,7 +447,8 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do def handle_event("change_membership_fee_type", %{"value" => fee_type_id}, socket) do member = socket.assigns.member - new_fee_type = Ash.get!(MembershipFeeType, fee_type_id, domain: MembershipFees) + actor = socket.assigns.current_user + new_fee_type = Ash.get!(MembershipFeeType, fee_type_id, domain: MembershipFees, actor: actor) # Check if interval matches interval_warning = @@ -459,15 +466,22 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do if interval_warning do {:noreply, assign(socket, :interval_warning, interval_warning)} else - case update_member_fee_type(member, fee_type_id) do + actor = socket.assigns.current_user + + case update_member_fee_type(member, fee_type_id, actor) do {:ok, updated_member} -> # Reload member with cycles + actor = socket.assigns.current_user + updated_member = updated_member - |> Ash.load!([ - :membership_fee_type, - membership_fee_cycles: [:membership_fee_type] - ]) + |> Ash.load!( + [ + :membership_fee_type, + membership_fee_cycles: [:membership_fee_type] + ], + actor: actor + ) cycles = Enum.sort_by( @@ -482,7 +496,10 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do socket |> assign(:member, updated_member) |> assign(:cycles, cycles) - |> assign(:available_fee_types, get_available_fee_types(updated_member)) + |> assign( + :available_fee_types, + get_available_fee_types(updated_member, socket.assigns.current_user) + ) |> assign(:interval_warning, nil) |> put_flash(:info, gettext("Membership fee type updated. Cycles regenerated."))} @@ -503,7 +520,9 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do :suspended -> :mark_as_suspended end - case Ash.update(cycle, action: action, domain: MembershipFees) do + actor = socket.assigns.current_user + + case Ash.update(cycle, action: action, domain: MembershipFees, actor: actor) do {:ok, updated_cycle} -> updated_cycles = replace_cycle(socket.assigns.cycles, updated_cycle) @@ -537,12 +556,17 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do case CycleGenerator.generate_cycles_for_member(member.id) do {:ok, _new_cycles, _notifications} -> # Reload member with cycles + actor = socket.assigns.current_user + updated_member = member - |> Ash.load!([ - :membership_fee_type, - membership_fee_cycles: [:membership_fee_type] - ]) + |> Ash.load!( + [ + :membership_fee_type, + membership_fee_cycles: [:membership_fee_type] + ], + actor: actor + ) cycles = Enum.sort_by( @@ -572,7 +596,8 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do cycle = find_cycle(socket.assigns.cycles, cycle_id) # Load cycle with membership_fee_type for display - cycle = Ash.load!(cycle, :membership_fee_type) + actor = socket.assigns.current_user + cycle = Ash.load!(cycle, :membership_fee_type, actor: actor) {:noreply, assign(socket, :editing_cycle, cycle)} end @@ -589,9 +614,11 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do case Decimal.parse(normalized_amount_str) do {amount, _} when is_struct(amount, Decimal) -> + actor = socket.assigns.current_user + case cycle |> Ash.Changeset.for_update(:update, %{amount: amount}) - |> Ash.update(domain: MembershipFees) do + |> Ash.update(domain: MembershipFees, actor: actor) do {:ok, updated_cycle} -> updated_cycles = replace_cycle(socket.assigns.cycles, updated_cycle) @@ -616,7 +643,8 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do cycle = find_cycle(socket.assigns.cycles, cycle_id) # Load cycle with membership_fee_type for display - cycle = Ash.load!(cycle, :membership_fee_type) + actor = socket.assigns.current_user + cycle = Ash.load!(cycle, :membership_fee_type, actor: actor) {:noreply, assign(socket, :deleting_cycle, cycle)} end @@ -627,8 +655,9 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do def handle_event("confirm_delete_cycle", %{"cycle_id" => cycle_id}, socket) do cycle = find_cycle(socket.assigns.cycles, cycle_id) + actor = socket.assigns.current_user - case Ash.destroy(cycle, domain: MembershipFees) do + case Ash.destroy(cycle, domain: MembershipFees, actor: actor) do :ok -> updated_cycles = Enum.reject(socket.assigns.cycles, &(&1.id == cycle_id)) @@ -699,12 +728,17 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do if deleted_count > 0 do # Reload member to get updated cycles + actor = socket.assigns.current_user + updated_member = member - |> Ash.load!([ - :membership_fee_type, - membership_fee_cycles: [:membership_fee_type] - ]) + |> Ash.load!( + [ + :membership_fee_type, + membership_fee_cycles: [:membership_fee_type] + ], + actor: actor + ) updated_cycles = Enum.sort_by( @@ -786,15 +820,20 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do membership_fee_type_id: member.membership_fee_type_id } - case Ash.create(MembershipFeeCycle, attrs, domain: MembershipFees) do + actor = socket.assigns.current_user + + case Ash.create(MembershipFeeCycle, attrs, domain: MembershipFees, actor: actor) do {:ok, _new_cycle} -> # Reload member with cycles updated_member = member - |> Ash.load!([ - :membership_fee_type, - membership_fee_cycles: [:membership_fee_type] - ]) + |> Ash.load!( + [ + :membership_fee_type, + membership_fee_cycles: [:membership_fee_type] + ], + actor: actor + ) cycles = Enum.sort_by( @@ -842,11 +881,11 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do # Helper functions - defp get_available_fee_types(member) do + defp get_available_fee_types(member, actor) do all_types = MembershipFeeType |> Ash.Query.sort(name: :asc) - |> Ash.read!() + |> Ash.read!(domain: MembershipFees, actor: actor) # If member has a fee type, filter to same interval if member.membership_fee_type do @@ -858,12 +897,12 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do end end - defp update_member_fee_type(member, fee_type_id) do + defp update_member_fee_type(member, fee_type_id, actor) do attrs = %{membership_fee_type_id: fee_type_id} member |> Ash.Changeset.for_update(:update_member, attrs, domain: Membership) - |> Ash.update(domain: Membership) + |> Ash.update(domain: Membership, actor: actor) end defp find_cycle(cycles, cycle_id) do diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 182a428..369d014 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -2125,6 +2125,11 @@ msgstr "Zusätzliche Datenfelder" #~ msgid "Pending" #~ msgstr "Ausstehend" +#: lib/mv_web/live/member_live/form.ex +#, elixir-autogen, elixir-format +msgid "You do not have permission to %{action} members." +msgstr "" + #~ #: lib/mv_web/live/member_live/form.ex #~ #: lib/mv_web/live/member_live/show.ex #~ #: lib/mv_web/translations/member_fields.ex diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 1be771a..681a498 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -2063,3 +2063,8 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Additional Data Fields" msgstr "" + +#: lib/mv_web/live/member_live/form.ex +#, elixir-autogen, elixir-format +msgid "You do not have permission to %{action} members." +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 8b01f61..7ca3c1e 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2064,6 +2064,11 @@ msgstr "" msgid "Additional Data Fields" msgstr "" +#: lib/mv_web/live/member_live/form.ex +#, elixir-autogen, elixir-format +msgid "You do not have permission to %{action} members." +msgstr "" + #~ #: lib/mv_web/live/components/payment_filter_component.ex #~ #, elixir-autogen, elixir-format #~ msgid "All payment statuses" From 075a06ba6f2105cac8ee5d5267dc76e6b7d11807 Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 9 Jan 2026 02:23:29 +0100 Subject: [PATCH 11/34] Refactor test setup: use global setup and fix MembershipFees domain alias - Remove redundant setup blocks from member_live tests - Add build_unauthenticated_conn helper for AuthController tests - Add global setup in conn_case.ex --- lib/mv_web/live/member_live/form.ex | 2 +- .../show/membership_fees_component.ex | 17 +-- .../controllers/auth_controller_test.exs | 89 +++++++++++++--- .../form_membership_fee_type_test.exs | 14 --- .../index_membership_fee_status_test.exs | 14 --- .../membership_fee_integration_test.exs | 14 --- .../member_live/show_membership_fees_test.exs | 14 --- test/support/conn_case.ex | 33 +++++- test/support/fixtures.ex | 100 ++++++++++++++++++ 9 files changed, 216 insertions(+), 81 deletions(-) diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index c7c718e..42d0da2 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -339,7 +339,7 @@ defmodule MvWeb.MemberLive.Form do form = if member do - {:ok, member} = Ash.load(member, custom_field_values: [:custom_field], actor: actor) + {:ok, member} = Ash.load(member, [custom_field_values: [:custom_field]], actor: actor) existing_custom_field_values = member.custom_field_values diff --git a/lib/mv_web/live/member_live/show/membership_fees_component.ex b/lib/mv_web/live/member_live/show/membership_fees_component.ex index 0b32efe..864889a 100644 --- a/lib/mv_web/live/member_live/show/membership_fees_component.ex +++ b/lib/mv_web/live/member_live/show/membership_fees_component.ex @@ -15,10 +15,11 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do require Ash.Query alias Mv.Membership - alias Mv.MembershipFees.CalendarCycles - alias Mv.MembershipFees.CycleGenerator - alias Mv.MembershipFees.MembershipFeeCycle + alias Mv.MembershipFees alias Mv.MembershipFees.MembershipFeeType + alias Mv.MembershipFees.MembershipFeeCycle + alias Mv.MembershipFees.CycleGenerator + alias Mv.MembershipFees.CalendarCycles alias MvWeb.Helpers.MembershipFeeHelpers @impl true @@ -63,7 +64,7 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do phx-click="delete_all_cycles" phx-target={@myself} class="btn btn-sm btn-error btn-outline" - title={gettext("Delete All Cycles")} + title={gettext("Delete all cycles")} > <.icon name="hero-trash" class="size-4" /> {gettext("Delete All Cycles")} @@ -168,7 +169,7 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do phx-value-cycle_id={cycle.id} phx-target={@myself} class="btn btn-sm btn-error btn-outline" - title={gettext("Delete Cycle")} + title={gettext("Delete cycle")} > <.icon name="hero-trash" class="size-4" /> {gettext("Delete")} @@ -329,14 +330,16 @@ defmodule MvWeb.MemberLive.Show.MembershipFeesComponent do />
<%= if @create_cycle_date do %>
{format_create_cycle_period( diff --git a/test/mv_web/controllers/auth_controller_test.exs b/test/mv_web/controllers/auth_controller_test.exs index 3c3956d..444571b 100644 --- a/test/mv_web/controllers/auth_controller_test.exs +++ b/test/mv_web/controllers/auth_controller_test.exs @@ -1,21 +1,42 @@ defmodule MvWeb.AuthControllerTest do use MvWeb.ConnCase, async: true import Phoenix.LiveViewTest + import Phoenix.ConnTest + + # Helper to create an unauthenticated conn (preserves sandbox metadata) + defp build_unauthenticated_conn(authenticated_conn) do + # Create new conn but preserve sandbox metadata for database access + new_conn = build_conn() + + # Copy sandbox metadata from authenticated conn + if authenticated_conn.private[:ecto_sandbox] do + Plug.Conn.put_private(new_conn, :ecto_sandbox, authenticated_conn.private[:ecto_sandbox]) + else + new_conn + end + end # Basic UI tests - test "GET /sign-in shows sign in form", %{conn: conn} do + test "GET /sign-in shows sign in form", %{conn: authenticated_conn} do + # Create unauthenticated conn for this test + conn = build_unauthenticated_conn(authenticated_conn) conn = get(conn, ~p"/sign-in") assert html_response(conn, 200) =~ "Sign in" end - test "GET /sign-out redirects to home", %{conn: conn} do - conn = conn_with_oidc_user(conn) + test "GET /sign-out redirects to home", %{conn: authenticated_conn} do + conn = conn_with_oidc_user(authenticated_conn) conn = get(conn, ~p"/sign-out") assert redirected_to(conn) == ~p"/" end # Password authentication (LiveView) - test "password user can sign in with valid credentials via LiveView", %{conn: conn} do + test "password user can sign in with valid credentials via LiveView", %{ + conn: authenticated_conn + } do + # Create unauthenticated conn for this test + conn = build_unauthenticated_conn(authenticated_conn) + _user = create_test_user(%{ email: "password@example.com", @@ -35,7 +56,12 @@ defmodule MvWeb.AuthControllerTest do assert to =~ "/auth/user/password/sign_in_with_token" end - test "password user with invalid credentials shows error via LiveView", %{conn: conn} do + test "password user with invalid credentials shows error via LiveView", %{ + conn: authenticated_conn + } do + # Create unauthenticated conn for this test + conn = build_unauthenticated_conn(authenticated_conn) + _user = create_test_user(%{ email: "test@example.com", @@ -55,7 +81,12 @@ defmodule MvWeb.AuthControllerTest do assert html =~ "Email or password was incorrect" end - test "password user with non-existent email shows error via LiveView", %{conn: conn} do + test "password user with non-existent email shows error via LiveView", %{ + conn: authenticated_conn + } do + # Create unauthenticated conn for this test + conn = build_unauthenticated_conn(authenticated_conn) + {:ok, view, _html} = live(conn, "/sign-in") html = @@ -69,7 +100,10 @@ defmodule MvWeb.AuthControllerTest do end # Registration (LiveView) - test "user can register with valid credentials via LiveView", %{conn: conn} do + test "user can register with valid credentials via LiveView", %{conn: authenticated_conn} do + # Create unauthenticated conn for this test + conn = build_unauthenticated_conn(authenticated_conn) + {:ok, view, _html} = live(conn, "/register") {:error, {:redirect, %{to: to}}} = @@ -82,7 +116,10 @@ defmodule MvWeb.AuthControllerTest do assert to =~ "/auth/user/password/sign_in_with_token" end - test "registration with existing email shows error via LiveView", %{conn: conn} do + test "registration with existing email shows error via LiveView", %{conn: authenticated_conn} do + # Create unauthenticated conn for this test + conn = build_unauthenticated_conn(authenticated_conn) + _user = create_test_user(%{ email: "existing@example.com", @@ -102,7 +139,10 @@ defmodule MvWeb.AuthControllerTest do assert html =~ "has already been taken" end - test "registration with weak password shows error via LiveView", %{conn: conn} do + test "registration with weak password shows error via LiveView", %{conn: authenticated_conn} do + # Create unauthenticated conn for this test + conn = build_unauthenticated_conn(authenticated_conn) + {:ok, view, _html} = live(conn, "/register") html = @@ -116,18 +156,27 @@ defmodule MvWeb.AuthControllerTest do end # Access control - test "unauthenticated user accessing protected route gets redirected to sign-in", %{conn: conn} do + test "unauthenticated user accessing protected route gets redirected to sign-in", %{ + conn: authenticated_conn + } do + # Create unauthenticated conn for this test + conn = build_unauthenticated_conn(authenticated_conn) conn = get(conn, ~p"/members") assert redirected_to(conn) == ~p"/sign-in" end - test "authenticated user can access protected route", %{conn: conn} do - conn = conn_with_oidc_user(conn) + test "authenticated user can access protected route", %{conn: authenticated_conn} do + conn = conn_with_oidc_user(authenticated_conn) conn = get(conn, ~p"/members") assert conn.status == 200 end - test "password authenticated user can access protected route via LiveView", %{conn: conn} do + test "password authenticated user can access protected route via LiveView", %{ + conn: authenticated_conn + } do + # Create unauthenticated conn for this test + conn = build_unauthenticated_conn(authenticated_conn) + _user = create_test_user(%{ email: "auth@example.com", @@ -150,7 +199,12 @@ defmodule MvWeb.AuthControllerTest do end # Edge cases - test "user with nil oidc_id can still sign in with password via LiveView", %{conn: conn} do + test "user with nil oidc_id can still sign in with password via LiveView", %{ + conn: authenticated_conn + } do + # Create unauthenticated conn for this test + conn = build_unauthenticated_conn(authenticated_conn) + _user = create_test_user(%{ email: "nil_oidc@example.com", @@ -170,7 +224,12 @@ defmodule MvWeb.AuthControllerTest do assert to =~ "/auth/user/password/sign_in_with_token" end - test "user with empty string oidc_id is handled correctly via LiveView", %{conn: conn} do + test "user with empty string oidc_id is handled correctly via LiveView", %{ + conn: authenticated_conn + } do + # Create unauthenticated conn for this test + conn = build_unauthenticated_conn(authenticated_conn) + _user = create_test_user(%{ email: "empty_oidc@example.com", diff --git a/test/mv_web/member_live/form_membership_fee_type_test.exs b/test/mv_web/member_live/form_membership_fee_type_test.exs index cc4388f..63c5a0d 100644 --- a/test/mv_web/member_live/form_membership_fee_type_test.exs +++ b/test/mv_web/member_live/form_membership_fee_type_test.exs @@ -11,20 +11,6 @@ defmodule MvWeb.MemberLive.FormMembershipFeeTypeTest do require Ash.Query - setup %{conn: conn} do - # Create admin user - {:ok, user} = - Mv.Accounts.User - |> Ash.Changeset.for_create(:register_with_password, %{ - email: "admin#{System.unique_integer([:positive])}@mv.local", - password: "testpassword123" - }) - |> Ash.create() - - authenticated_conn = conn_with_password_user(conn, user) - %{conn: authenticated_conn, user: user} - end - # Helper to create a membership fee type defp create_fee_type(attrs) do default_attrs = %{ diff --git a/test/mv_web/member_live/index_membership_fee_status_test.exs b/test/mv_web/member_live/index_membership_fee_status_test.exs index baa5f67..a189873 100644 --- a/test/mv_web/member_live/index_membership_fee_status_test.exs +++ b/test/mv_web/member_live/index_membership_fee_status_test.exs @@ -12,20 +12,6 @@ defmodule MvWeb.MemberLive.IndexMembershipFeeStatusTest do require Ash.Query - setup %{conn: conn} do - # Create admin user - {:ok, user} = - Mv.Accounts.User - |> Ash.Changeset.for_create(:register_with_password, %{ - email: "admin#{System.unique_integer([:positive])}@mv.local", - password: "testpassword123" - }) - |> Ash.create() - - conn = conn_with_password_user(conn, user) - %{conn: conn, user: user} - end - # Helper to create a membership fee type defp create_fee_type(attrs) do default_attrs = %{ diff --git a/test/mv_web/member_live/membership_fee_integration_test.exs b/test/mv_web/member_live/membership_fee_integration_test.exs index e76e422..9358c70 100644 --- a/test/mv_web/member_live/membership_fee_integration_test.exs +++ b/test/mv_web/member_live/membership_fee_integration_test.exs @@ -12,20 +12,6 @@ defmodule MvWeb.MemberLive.MembershipFeeIntegrationTest do require Ash.Query - setup do - # Create admin user - {:ok, user} = - Mv.Accounts.User - |> Ash.Changeset.for_create(:register_with_password, %{ - email: "admin#{System.unique_integer([:positive])}@mv.local", - password: "testpassword123" - }) - |> Ash.create() - - conn = conn_with_password_user(build_conn(), user) - %{conn: conn, user: user} - end - # Helper to create a membership fee type defp create_fee_type(attrs) do default_attrs = %{ diff --git a/test/mv_web/member_live/show_membership_fees_test.exs b/test/mv_web/member_live/show_membership_fees_test.exs index 1f68244..d0402c3 100644 --- a/test/mv_web/member_live/show_membership_fees_test.exs +++ b/test/mv_web/member_live/show_membership_fees_test.exs @@ -12,20 +12,6 @@ defmodule MvWeb.MemberLive.ShowMembershipFeesTest do require Ash.Query - setup %{conn: conn} do - # Create admin user - {:ok, user} = - Mv.Accounts.User - |> Ash.Changeset.for_create(:register_with_password, %{ - email: "admin#{System.unique_integer([:positive])}@mv.local", - password: "testpassword123" - }) - |> Ash.create() - - conn = conn_with_password_user(conn, user) - %{conn: conn, user: user} - end - # Helper to create a membership fee type defp create_fee_type(attrs) do default_attrs = %{ diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index 853a326..8f943d9 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -99,6 +99,7 @@ defmodule MvWeb.ConnCase do @doc """ Signs in a user via OIDC and returns a connection with the user authenticated. By default creates a user with "user@example.com" for consistency. + The user will have an admin role for authorization. """ def conn_with_oidc_user(conn, user_attrs \\ %{}) do # Ensure unique email for OIDC users @@ -109,8 +110,22 @@ defmodule MvWeb.ConnCase do oidc_id: "oidc_#{unique_id}" } + # Create user using Ash.Seed (supports oidc_id) user = create_test_user(Map.merge(default_attrs, user_attrs)) - sign_in_user_via_oidc(conn, user) + + # Create admin role and assign it + admin_role = Mv.Fixtures.role_fixture("admin") + + {:ok, user} = + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) + |> Ash.update() + + # Load role for authorization + user_with_role = Ash.load!(user, :role, domain: Mv.Accounts) + + sign_in_user_via_oidc(conn, user_with_role) end @doc """ @@ -122,6 +137,15 @@ defmodule MvWeb.ConnCase do |> AshAuthentication.Plug.Helpers.store_in_session(user) end + @doc """ + Creates a connection with an authenticated user that has an admin role. + This is useful for tests that need full access to resources. + """ + def conn_with_admin_user(conn) do + admin_user = Mv.Fixtures.user_with_role_fixture("admin") + conn_with_password_user(conn, admin_user) + end + setup tags do pid = Mv.DataCase.setup_sandbox(tags) @@ -130,6 +154,11 @@ defmodule MvWeb.ConnCase do # to share the test's database connection in async tests conn = Plug.Conn.put_private(conn, :ecto_sandbox, pid) - {:ok, conn: conn} + # Create admin user with role for all tests (unless test overrides with its own user) + # This ensures all tests have an authenticated user with proper authorization + admin_user = Mv.Fixtures.user_with_role_fixture("admin") + authenticated_conn = conn_with_password_user(conn, admin_user) + + {:ok, conn: authenticated_conn, current_user: admin_user} end end diff --git a/test/support/fixtures.ex b/test/support/fixtures.ex index 5dd14a9..d474764 100644 --- a/test/support/fixtures.ex +++ b/test/support/fixtures.ex @@ -93,4 +93,104 @@ defmodule Mv.Fixtures do {user, member} end + + @doc """ + Creates a role with a specific permission set. + + ## Parameters + - `permission_set_name` - The permission set name (e.g., "admin", "read_only", "normal_user", "own_data") + + ## Returns + - Role struct + + ## Examples + + iex> role_fixture("admin") + %Mv.Authorization.Role{permission_set_name: "admin", ...} + + """ + def role_fixture(permission_set_name) do + role_name = "Test Role #{permission_set_name} #{System.unique_integer([:positive])}" + + case Mv.Authorization.create_role(%{ + name: role_name, + description: "Test role for #{permission_set_name}", + permission_set_name: permission_set_name + }) do + {:ok, role} -> role + {:error, error} -> raise "Failed to create role: #{inspect(error)}" + end + end + + @doc """ + Creates a user with a specific permission set (role). + + ## Parameters + - `permission_set_name` - The permission set name (e.g., "admin", "read_only", "normal_user", "own_data") + - `user_attrs` - Optional user attributes + + ## Returns + - User struct with role preloaded + + ## Examples + + iex> admin_user = user_with_role_fixture("admin") + iex> admin_user.role.permission_set_name + "admin" + + """ + def user_with_role_fixture(permission_set_name \\ "admin", user_attrs \\ %{}) do + # Create role with permission set + role = role_fixture(permission_set_name) + + # Create user + {:ok, user} = + user_attrs + |> Enum.into(%{ + email: "user#{System.unique_integer([:positive])}@example.com" + }) + |> Mv.Accounts.create_user() + + # Assign role to user + {:ok, user} = + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, role, type: :append_and_remove) + |> Ash.update() + + # Reload user with role preloaded (critical for authorization!) + {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts) + user_with_role + end + + @doc """ + Creates a member with an actor (for use in tests with policies). + + ## Parameters + - `attrs` - Map or keyword list of attributes to override defaults + - `actor` - The actor (user) to use for authorization + + ## Returns + - Member struct + + ## Examples + + iex> admin = user_with_role_fixture("admin") + iex> member_fixture_with_actor(%{first_name: "Alice"}, admin) + %Mv.Membership.Member{first_name: "Alice", ...} + + """ + def member_fixture_with_actor(attrs \\ %{}, actor) do + attrs + |> Enum.into(%{ + first_name: "Test", + last_name: "Member", + email: "test#{System.unique_integer([:positive])}@example.com" + }) + |> Mv.Membership.create_member(actor: actor) + |> case do + {:ok, member} -> member + {:error, error} -> raise "Failed to create member: #{inspect(error)}" + end + end end From 01cc5aa3a145b24f07c3ab38acbbb3a406556fd8 Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 9 Jan 2026 05:25:59 +0100 Subject: [PATCH 12/34] Add current_actor/1 helper for consistent actor access Provides a single function to access current_user from socket assigns across all LiveViews, ensuring consistent access pattern. --- lib/mv_web/live_helpers.ex | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/lib/mv_web/live_helpers.ex b/lib/mv_web/live_helpers.ex index f217ee2..6af71ec 100644 --- a/lib/mv_web/live_helpers.ex +++ b/lib/mv_web/live_helpers.ex @@ -59,4 +59,19 @@ defmodule MvWeb.LiveHelpers do user end end + + @doc """ + Helper function to get the current actor (user) from socket assigns. + + Provides consistent access pattern across all LiveViews. + Returns nil if no current_user is present. + + ## Examples + + actor = current_actor(socket) + members = Membership.list_members!(actor: actor) + """ + def current_actor(socket) do + socket.assigns[:current_user] || socket.assigns.current_user + end end From dbd79075f592a515fbe852a715380296e80038fc Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 9 Jan 2026 05:26:01 +0100 Subject: [PATCH 13/34] Pass actor parameter through cycle generation Extract actor from changeset context in Member hooks and pass it through all cycle generation functions to ensure proper authorization. --- lib/membership/member.ex | 84 ++++++++++++++++------- lib/mv/membership_fees/cycle_generator.ex | 56 ++++++++++----- 2 files changed, 95 insertions(+), 45 deletions(-) diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 873aac0..43322f6 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -119,11 +119,12 @@ defmodule Mv.Membership.Member do # Only runs if membership_fee_type_id is set # Note: Cycle generation runs asynchronously to not block the action, # but in test environment it runs synchronously for DB sandbox compatibility - change after_transaction(fn _changeset, result, _context -> + change after_transaction(fn changeset, result, _context -> case result do {:ok, member} -> if member.membership_fee_type_id && member.join_date do - handle_cycle_generation(member) + actor = Map.get(changeset.context, :actor) + handle_cycle_generation(member, actor: actor) end {:error, _} -> @@ -194,7 +195,9 @@ defmodule Mv.Membership.Member do Ash.Changeset.changing_attribute?(changeset, :membership_fee_type_id) if fee_type_changed && member.membership_fee_type_id && member.join_date do - case regenerate_cycles_on_type_change(member) do + actor = Map.get(changeset.context, :actor) + + case regenerate_cycles_on_type_change(member, actor: actor) do {:ok, notifications} -> # Return notifications to Ash - they will be sent automatically after commit {:ok, member, notifications} @@ -226,7 +229,8 @@ defmodule Mv.Membership.Member do exit_date_changed = Ash.Changeset.changing_attribute?(changeset, :exit_date) if (join_date_changed || exit_date_changed) && member.membership_fee_type_id do - handle_cycle_generation(member) + actor = Map.get(changeset.context, :actor) + handle_cycle_generation(member, actor: actor) end {:error, _} -> @@ -783,33 +787,37 @@ defmodule Mv.Membership.Member do # Returns {:ok, notifications} or {:error, reason} where notifications are collected # to be sent after transaction commits @doc false - def regenerate_cycles_on_type_change(member) do + def regenerate_cycles_on_type_change(member, opts \\ []) do today = Date.utc_today() lock_key = :erlang.phash2(member.id) + actor = Keyword.get(opts, :actor) # Use advisory lock to prevent concurrent deletion and regeneration # This ensures atomicity when multiple updates happen simultaneously if Mv.Repo.in_transaction?() do - regenerate_cycles_in_transaction(member, today, lock_key) + regenerate_cycles_in_transaction(member, today, lock_key, actor: actor) else - regenerate_cycles_new_transaction(member, today, lock_key) + regenerate_cycles_new_transaction(member, today, lock_key, actor: actor) end end # Already in transaction: use advisory lock directly # Returns {:ok, notifications} - notifications should be returned to after_action hook - defp regenerate_cycles_in_transaction(member, today, lock_key) do + defp regenerate_cycles_in_transaction(member, today, lock_key, opts) do + actor = Keyword.get(opts, :actor) Ecto.Adapters.SQL.query!(Mv.Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - do_regenerate_cycles_on_type_change(member, today, skip_lock?: true) + do_regenerate_cycles_on_type_change(member, today, skip_lock?: true, actor: actor) end # Not in transaction: start new transaction with advisory lock # Returns {:ok, notifications} - notifications should be sent by caller (e.g., via after_action) - defp regenerate_cycles_new_transaction(member, today, lock_key) do + defp regenerate_cycles_new_transaction(member, today, lock_key, opts) do + actor = Keyword.get(opts, :actor) + Mv.Repo.transaction(fn -> Ecto.Adapters.SQL.query!(Mv.Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - case do_regenerate_cycles_on_type_change(member, today, skip_lock?: true) do + case do_regenerate_cycles_on_type_change(member, today, skip_lock?: true, actor: actor) do {:ok, notifications} -> # Return notifications - they will be sent by the caller notifications @@ -831,6 +839,7 @@ defmodule Mv.Membership.Member do require Ash.Query skip_lock? = Keyword.get(opts, :skip_lock?, false) + actor = Keyword.get(opts, :actor) # Find all unpaid cycles for this member # We need to check cycle_end for each cycle using its own interval @@ -840,10 +849,21 @@ defmodule Mv.Membership.Member do |> Ash.Query.filter(status == :unpaid) |> Ash.Query.load([:membership_fee_type]) - case Ash.read(all_unpaid_cycles_query) do + result = + if actor do + Ash.read(all_unpaid_cycles_query, actor: actor) + else + Ash.read(all_unpaid_cycles_query) + end + + case result do {:ok, all_unpaid_cycles} -> cycles_to_delete = filter_future_cycles(all_unpaid_cycles, today) - delete_and_regenerate_cycles(cycles_to_delete, member.id, today, skip_lock?: skip_lock?) + + delete_and_regenerate_cycles(cycles_to_delete, member.id, today, + skip_lock?: skip_lock?, + actor: actor + ) {:error, reason} -> {:error, reason} @@ -872,13 +892,14 @@ defmodule Mv.Membership.Member do # Returns {:ok, notifications} or {:error, reason} defp delete_and_regenerate_cycles(cycles_to_delete, member_id, today, opts) do skip_lock? = Keyword.get(opts, :skip_lock?, false) + actor = Keyword.get(opts, :actor) if Enum.empty?(cycles_to_delete) do # No cycles to delete, just regenerate - regenerate_cycles(member_id, today, skip_lock?: skip_lock?) + regenerate_cycles(member_id, today, skip_lock?: skip_lock?, actor: actor) else case delete_cycles(cycles_to_delete) do - :ok -> regenerate_cycles(member_id, today, skip_lock?: skip_lock?) + :ok -> regenerate_cycles(member_id, today, skip_lock?: skip_lock?, actor: actor) {:error, reason} -> {:error, reason} end end @@ -904,11 +925,13 @@ defmodule Mv.Membership.Member do # Returns {:ok, notifications} - notifications should be returned to after_action hook defp regenerate_cycles(member_id, today, opts) do skip_lock? = Keyword.get(opts, :skip_lock?, false) + actor = Keyword.get(opts, :actor) case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member( member_id, today: today, - skip_lock?: skip_lock? + skip_lock?: skip_lock?, + actor: actor ) do {:ok, _cycles, notifications} when is_list(notifications) -> {:ok, notifications} @@ -922,21 +945,25 @@ defmodule Mv.Membership.Member do # based on environment (test vs production) # This function encapsulates the common logic for cycle generation # to avoid code duplication across different hooks - defp handle_cycle_generation(member) do + defp handle_cycle_generation(member, opts) do + actor = Keyword.get(opts, :actor) + if Mv.Config.sql_sandbox?() do - handle_cycle_generation_sync(member) + handle_cycle_generation_sync(member, actor: actor) else - handle_cycle_generation_async(member) + handle_cycle_generation_async(member, actor: actor) end end # Runs cycle generation synchronously (for test environment) - defp handle_cycle_generation_sync(member) do + defp handle_cycle_generation_sync(member, opts) do require Logger + actor = Keyword.get(opts, :actor) case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member( member.id, - today: Date.utc_today() + today: Date.utc_today(), + actor: actor ) do {:ok, cycles, notifications} -> send_notifications_if_any(notifications) @@ -948,9 +975,11 @@ defmodule Mv.Membership.Member do end # Runs cycle generation asynchronously (for production environment) - defp handle_cycle_generation_async(member) do + defp handle_cycle_generation_async(member, opts) do + actor = Keyword.get(opts, :actor) + Task.Supervisor.async_nolink(Mv.TaskSupervisor, fn -> - case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member.id) do + case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member.id, actor: actor) do {:ok, cycles, notifications} -> send_notifications_if_any(notifications) log_cycle_generation_success(member, cycles, notifications, sync: false) @@ -1179,15 +1208,18 @@ defmodule Mv.Membership.Member do custom_field_values_arg = Ash.Changeset.get_argument(changeset, :custom_field_values) if is_nil(custom_field_values_arg) do - extract_existing_values(changeset.data) + extract_existing_values(changeset.data, changeset) else extract_argument_values(custom_field_values_arg) end end # Extracts custom field values from existing member data (update scenario) - defp extract_existing_values(member_data) do - case Ash.load(member_data, :custom_field_values) do + defp extract_existing_values(member_data, changeset) do + actor = Map.get(changeset.context, :actor) + opts = if actor, do: [actor: actor], else: [] + + case Ash.load(member_data, :custom_field_values, opts) do {:ok, %{custom_field_values: existing_values}} -> Enum.reduce(existing_values, %{}, &extract_value_from_cfv/2) diff --git a/lib/mv/membership_fees/cycle_generator.ex b/lib/mv/membership_fees/cycle_generator.ex index 7d6c798..32b4e79 100644 --- a/lib/mv/membership_fees/cycle_generator.ex +++ b/lib/mv/membership_fees/cycle_generator.ex @@ -77,7 +77,9 @@ defmodule Mv.MembershipFees.CycleGenerator do def generate_cycles_for_member(member_or_id, opts \\ []) def generate_cycles_for_member(member_id, opts) when is_binary(member_id) do - case load_member(member_id) do + actor = Keyword.get(opts, :actor) + + case load_member(member_id, actor: actor) do {:ok, member} -> generate_cycles_for_member(member, opts) {:error, reason} -> {:error, reason} end @@ -87,25 +89,27 @@ defmodule Mv.MembershipFees.CycleGenerator do today = Keyword.get(opts, :today, Date.utc_today()) skip_lock? = Keyword.get(opts, :skip_lock?, false) - do_generate_cycles_with_lock(member, today, skip_lock?) + do_generate_cycles_with_lock(member, today, skip_lock?, opts) end # Generate cycles with lock handling # Returns {:ok, cycles, notifications} - notifications are never sent here, # they should be returned to the caller (e.g., via after_action hook) - defp do_generate_cycles_with_lock(member, today, true = _skip_lock?) do + defp do_generate_cycles_with_lock(member, today, true = _skip_lock?, opts) do # Lock already set by caller (e.g., regenerate_cycles_on_type_change) # Just generate cycles without additional locking - do_generate_cycles(member, today) + actor = Keyword.get(opts, :actor) + do_generate_cycles(member, today, actor: actor) end - defp do_generate_cycles_with_lock(member, today, false) do + defp do_generate_cycles_with_lock(member, today, false, opts) do lock_key = :erlang.phash2(member.id) + actor = Keyword.get(opts, :actor) Repo.transaction(fn -> Ecto.Adapters.SQL.query!(Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - case do_generate_cycles(member, today) do + case do_generate_cycles(member, today, actor: actor) do {:ok, cycles, notifications} -> # Return cycles and notifications - do NOT send notifications here # They will be sent by the caller (e.g., via after_action hook) @@ -222,21 +226,33 @@ defmodule Mv.MembershipFees.CycleGenerator do # Private functions - defp load_member(member_id) do - Member - |> Ash.Query.filter(id == ^member_id) - |> Ash.Query.load([:membership_fee_type, :membership_fee_cycles]) - |> Ash.read_one() - |> case do + defp load_member(member_id, opts) do + actor = Keyword.get(opts, :actor) + + query = + Member + |> Ash.Query.filter(id == ^member_id) + |> Ash.Query.load([:membership_fee_type, :membership_fee_cycles]) + + result = + if actor do + Ash.read_one(query, actor: actor) + else + Ash.read_one(query) + end + + case result do {:ok, nil} -> {:error, :member_not_found} {:ok, member} -> {:ok, member} {:error, reason} -> {:error, reason} end end - defp do_generate_cycles(member, today) do + defp do_generate_cycles(member, today, opts) do + actor = Keyword.get(opts, :actor) + # Reload member with relationships to ensure fresh data - case load_member(member.id) do + case load_member(member.id, actor: actor) do {:ok, member} -> cond do is_nil(member.membership_fee_type_id) -> @@ -246,7 +262,7 @@ defmodule Mv.MembershipFees.CycleGenerator do {:error, :no_join_date} true -> - generate_missing_cycles(member, today) + generate_missing_cycles(member, today, actor: actor) end {:error, reason} -> @@ -254,7 +270,8 @@ defmodule Mv.MembershipFees.CycleGenerator do end end - defp generate_missing_cycles(member, today) do + defp generate_missing_cycles(member, today, opts) do + actor = Keyword.get(opts, :actor) fee_type = member.membership_fee_type interval = fee_type.interval amount = fee_type.amount @@ -270,7 +287,7 @@ defmodule Mv.MembershipFees.CycleGenerator do # Only generate if start_date <= end_date if start_date && Date.compare(start_date, end_date) != :gt do cycle_starts = generate_cycle_starts(start_date, end_date, interval) - create_cycles(cycle_starts, member.id, fee_type.id, amount) + create_cycles(cycle_starts, member.id, fee_type.id, amount, actor: actor) else {:ok, [], []} end @@ -365,7 +382,8 @@ defmodule Mv.MembershipFees.CycleGenerator do end end - defp create_cycles(cycle_starts, member_id, fee_type_id, amount) do + defp create_cycles(cycle_starts, member_id, fee_type_id, amount, opts) do + actor = Keyword.get(opts, :actor) # Always use return_notifications?: true to collect notifications # Notifications will be returned to the caller, who is responsible for # sending them (e.g., via after_action hook returning {:ok, result, notifications}) @@ -380,7 +398,7 @@ defmodule Mv.MembershipFees.CycleGenerator do } handle_cycle_creation_result( - Ash.create(MembershipFeeCycle, attrs, return_notifications?: true), + Ash.create(MembershipFeeCycle, attrs, return_notifications?: true, actor: actor), cycle_start ) end) From 5ffd2b334ee0a1466734abd6c7c537e7f2d41bc3 Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 9 Jan 2026 05:26:02 +0100 Subject: [PATCH 14/34] Pass actor parameter through email sync operations Extract actor from changeset context and pass it to all email sync loader functions to ensure proper authorization when loading linked users and members. --- .../changes/sync_member_email_to_user.ex | 4 ++- .../changes/sync_user_email_to_member.ex | 26 +++++++++++++---- lib/mv/email_sync/loader.ex | 29 ++++++++++++++----- 3 files changed, 44 insertions(+), 15 deletions(-) diff --git a/lib/mv/email_sync/changes/sync_member_email_to_user.ex b/lib/mv/email_sync/changes/sync_member_email_to_user.ex index 48c7955..0c0d8f7 100644 --- a/lib/mv/email_sync/changes/sync_member_email_to_user.ex +++ b/lib/mv/email_sync/changes/sync_member_email_to_user.ex @@ -41,8 +41,10 @@ defmodule Mv.EmailSync.Changes.SyncMemberEmailToUser do Ash.Changeset.around_transaction(changeset, fn cs, callback -> result = callback.(cs) + actor = Map.get(changeset.context, :actor) + with {:ok, member} <- Helpers.extract_record(result), - linked_user <- Loader.get_linked_user(member) do + linked_user <- Loader.get_linked_user(member, actor) do Helpers.sync_email_to_linked_record(result, linked_user, new_email) else _ -> result diff --git a/lib/mv/email_sync/changes/sync_user_email_to_member.ex b/lib/mv/email_sync/changes/sync_user_email_to_member.ex index 7148067..54829a4 100644 --- a/lib/mv/email_sync/changes/sync_user_email_to_member.ex +++ b/lib/mv/email_sync/changes/sync_user_email_to_member.ex @@ -33,7 +33,17 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do if Map.get(context, :syncing_email, false) do changeset else - sync_email(changeset) + # Ensure actor is in changeset context - get it from context if available + actor = Map.get(changeset.context, :actor) || Map.get(context, :actor) + + changeset_with_actor = + if actor && !Map.has_key?(changeset.context, :actor) do + Ash.Changeset.put_context(changeset, :actor, actor) + else + changeset + end + + sync_email(changeset_with_actor) end end @@ -42,7 +52,7 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do result = callback.(cs) with {:ok, record} <- Helpers.extract_record(result), - {:ok, user, member} <- get_user_and_member(record) do + {:ok, user, member} <- get_user_and_member(record, cs) do # When called from Member-side, we need to update the member in the result # When called from User-side, we update the linked member in DB only case record do @@ -61,15 +71,19 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do end # Retrieves user and member - works for both resource types - defp get_user_and_member(%Mv.Accounts.User{} = user) do - case Loader.get_linked_member(user) do + defp get_user_and_member(%Mv.Accounts.User{} = user, changeset) do + actor = Map.get(changeset.context, :actor) + + case Loader.get_linked_member(user, actor) do nil -> {:error, :no_member} member -> {:ok, user, member} end end - defp get_user_and_member(%Mv.Membership.Member{} = member) do - case Loader.load_linked_user!(member) do + defp get_user_and_member(%Mv.Membership.Member{} = member, changeset) do + actor = Map.get(changeset.context, :actor) + + case Loader.load_linked_user!(member, actor) do {:ok, user} -> {:ok, user, member} error -> error end diff --git a/lib/mv/email_sync/loader.ex b/lib/mv/email_sync/loader.ex index ecb1038..f6f6ecc 100644 --- a/lib/mv/email_sync/loader.ex +++ b/lib/mv/email_sync/loader.ex @@ -6,11 +6,16 @@ defmodule Mv.EmailSync.Loader do @doc """ Loads the member linked to a user, returns nil if not linked or on error. - """ - def get_linked_member(%{member_id: nil}), do: nil - def get_linked_member(%{member_id: id}) do - case Ash.get(Mv.Membership.Member, id) do + Accepts optional actor for authorization. + """ + def get_linked_member(user, actor \\ nil) + def get_linked_member(%{member_id: nil}, _actor), do: nil + + def get_linked_member(%{member_id: id}, actor) do + opts = if actor, do: [actor: actor], else: [] + + case Ash.get(Mv.Membership.Member, id, opts) do {:ok, member} -> member {:error, _} -> nil end @@ -18,9 +23,13 @@ defmodule Mv.EmailSync.Loader do @doc """ Loads the user linked to a member, returns nil if not linked or on error. + + Accepts optional actor for authorization. """ - def get_linked_user(member) do - case Ash.load(member, :user) do + def get_linked_user(member, actor \\ nil) do + opts = if actor, do: [actor: actor], else: [] + + case Ash.load(member, :user, opts) do {:ok, %{user: user}} -> user {:error, _} -> nil end @@ -29,9 +38,13 @@ defmodule Mv.EmailSync.Loader do @doc """ Loads the user linked to a member, returning an error tuple if not linked. Useful when a link is required for the operation. + + Accepts optional actor for authorization. """ - def load_linked_user!(member) do - case Ash.load(member, :user) do + def load_linked_user!(member, actor \\ nil) do + opts = if actor, do: [actor: actor], else: [] + + case Ash.load(member, :user, opts) do {:ok, %{user: user}} when not is_nil(user) -> {:ok, user} {:ok, _} -> {:error, :no_linked_user} {:error, _} = error -> error From 74fe60f76817e0462ed35411bc9893a8c035b343 Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 9 Jan 2026 05:26:04 +0100 Subject: [PATCH 15/34] Pass actor parameter to member email validation Extract actor from changeset context and pass it to Ash.read and Ash.load calls in email uniqueness validation. --- .../validations/email_not_used_by_other_user.ex | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex index a14bc0b..649493b 100644 --- a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex +++ b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex @@ -29,7 +29,8 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do def validate(changeset, _opts, _context) do email_changing? = Ash.Changeset.changing_attribute?(changeset, :email) - linked_user_id = get_linked_user_id(changeset.data) + actor = Map.get(changeset.context || %{}, :actor) + linked_user_id = get_linked_user_id(changeset.data, actor) is_linked? = not is_nil(linked_user_id) # Only validate if member is already linked AND email is changing @@ -38,19 +39,21 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do if should_validate? do new_email = Ash.Changeset.get_attribute(changeset, :email) - check_email_uniqueness(new_email, linked_user_id) + check_email_uniqueness(new_email, linked_user_id, actor) else :ok end end - defp check_email_uniqueness(email, exclude_user_id) do + defp check_email_uniqueness(email, exclude_user_id, actor) do query = Mv.Accounts.User |> Ash.Query.filter(email == ^email) |> maybe_exclude_id(exclude_user_id) - case Ash.read(query) do + opts = if actor, do: [actor: actor], else: [] + + case Ash.read(query, opts) do {:ok, []} -> :ok @@ -65,8 +68,10 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do defp maybe_exclude_id(query, nil), do: query defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) - defp get_linked_user_id(member_data) do - case Ash.load(member_data, :user) do + defp get_linked_user_id(member_data, actor) do + opts = if actor, do: [actor: actor], else: [] + + case Ash.load(member_data, :user, opts) do {:ok, %{user: %{id: id}}} -> id _ -> nil end From cd7e6b0843acf7db77fa4380313aafb3b6a22bd7 Mon Sep 17 00:00:00 2001 From: Moritz Date: Fri, 9 Jan 2026 05:26:06 +0100 Subject: [PATCH 16/34] Use current_actor/1 helper in all LiveViews Replace inconsistent actor access patterns with current_actor/1 helper and ensure actor is passed to all Ash operations for proper authorization. --- .../live/custom_field_value_live/form.ex | 8 +- lib/mv_web/live/member_live/form.ex | 196 ++++++++++++++++-- lib/mv_web/live/member_live/show.ex | 3 +- .../show/membership_fees_component.ex | 32 +-- .../live/membership_fee_type_live/form.ex | 13 +- .../live/membership_fee_type_live/index.ex | 16 +- lib/mv_web/live/user_live/form.ex | 45 ++-- lib/mv_web/live/user_live/index.ex | 6 +- lib/mv_web/live/user_live/show.ex | 6 +- 9 files changed, 268 insertions(+), 57 deletions(-) diff --git a/lib/mv_web/live/custom_field_value_live/form.ex b/lib/mv_web/live/custom_field_value_live/form.ex index 4db2bed..fde54c3 100644 --- a/lib/mv_web/live/custom_field_value_live/form.ex +++ b/lib/mv_web/live/custom_field_value_live/form.ex @@ -32,6 +32,9 @@ defmodule MvWeb.CustomFieldValueLive.Form do """ use MvWeb, :live_view + on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} + import MvWeb.LiveHelpers, only: [current_actor: 1] + @impl true def render(assigns) do ~H""" @@ -172,8 +175,9 @@ defmodule MvWeb.CustomFieldValueLive.Form do page_title = action <> " " <> "Custom field value" # Load all CustomFields and Members for the selection fields - custom_fields = Ash.read!(Mv.Membership.CustomField) - members = Ash.read!(Mv.Membership.Member) + actor = current_actor(socket) + custom_fields = Ash.read!(Mv.Membership.CustomField, actor: actor) + members = Ash.read!(Mv.Membership.Member, actor: actor) {:ok, socket diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index 42d0da2..1f5ab2d 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -23,6 +23,8 @@ defmodule MvWeb.MemberLive.Form do on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} + import MvWeb.LiveHelpers, only: [current_actor: 1] + alias Mv.MembershipFees alias Mv.MembershipFees.MembershipFeeType alias MvWeb.Helpers.MembershipFeeHelpers @@ -174,7 +176,7 @@ defmodule MvWeb.MemberLive.Form do