From 2f6d5ff81864ee4a9eec87d66126b2f5ffd5555f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Eppl=C3=A9e?= Date: Wed, 3 Dec 2025 21:25:39 +0100 Subject: [PATCH 01/98] Add simple sidebar --- lib/mv_web/components/layouts.ex | 36 ++++++++--- lib/mv_web/components/layouts/navbar.ex | 64 ++++++++----------- lib/mv_web/components/layouts/sidebar.ex | 80 ++++++++++++++++++++++++ priv/gettext/de/LC_MESSAGES/default.po | 19 +++--- priv/gettext/default.pot | 15 +++-- priv/gettext/en/LC_MESSAGES/default.po | 15 +++-- 6 files changed, 162 insertions(+), 67 deletions(-) create mode 100644 lib/mv_web/components/layouts/sidebar.ex diff --git a/lib/mv_web/components/layouts.ex b/lib/mv_web/components/layouts.ex index 487a01f..d45b8d5 100644 --- a/lib/mv_web/components/layouts.ex +++ b/lib/mv_web/components/layouts.ex @@ -10,6 +10,7 @@ defmodule MvWeb.Layouts do use MvWeb, :html use Gettext, backend: MvWeb.Gettext import MvWeb.Layouts.Navbar + import MvWeb.Layouts.Sidebar embed_templates "layouts/*" @@ -39,20 +40,39 @@ defmodule MvWeb.Layouts do slot :inner_block, required: true def app(assigns) do + club_name = get_club_name() + assigns = assign(assigns, :club_name, club_name) + ~H""" - <%= if @current_user do %> - <.navbar current_user={@current_user} /> - <% end %> -
-
- {render_slot(@inner_block)} +
+ +
+ <%= if @current_user do %> + <.navbar current_user={@current_user} /> + <% end %> +
+
+ {render_slot(@inner_block)} +
+
-
+ + <.sidebar current_user={@current_user} club_name={@club_name} /> + <.flash_group flash={@flash} /> """ end + # Helper function to get club name from settings + # Falls back to "Mitgliederverwaltung" if settings can't be loaded + defp get_club_name do + case Mv.Membership.get_settings() do + {:ok, settings} -> settings.club_name + _ -> "Mitgliederverwaltung" + end + end + @doc """ Shows the flash group with standard titles and content. @@ -65,7 +85,7 @@ defmodule MvWeb.Layouts do def flash_group(assigns) do ~H""" -
+
<.flash kind={:success} flash={@flash} /> <.flash kind={:warning} flash={@flash} /> <.flash kind={:info} flash={@flash} /> diff --git a/lib/mv_web/components/layouts/navbar.ex b/lib/mv_web/components/layouts/navbar.ex index 4246c99..8258d43 100644 --- a/lib/mv_web/components/layouts/navbar.ex +++ b/lib/mv_web/components/layouts/navbar.ex @@ -6,37 +6,35 @@ defmodule MvWeb.Layouts.Navbar do use Gettext, backend: MvWeb.Gettext use MvWeb, :verified_routes - alias Mv.Membership - attr :current_user, :map, required: true, doc: "The current user - navbar is only shown when user is present" def navbar(assigns) do - club_name = get_club_name() - - assigns = assign(assigns, :club_name, club_name) - ~H""" - - """ - end - - # Helper function to get club name from settings - # Falls back to "Mitgliederverwaltung" if settings can't be loaded - defp get_club_name do - case Mv.Membership.get_settings() do - {:ok, settings} -> settings.club_name - _ -> "Mitgliederverwaltung" - end - end -end diff --git a/lib/mv_web/components/layouts/sidebar.ex b/lib/mv_web/components/layouts/sidebar.ex index 4db4e5a..eb5f77d 100644 --- a/lib/mv_web/components/layouts/sidebar.ex +++ b/lib/mv_web/components/layouts/sidebar.ex @@ -34,12 +34,12 @@ defmodule MvWeb.Layouts.Sidebar do
Mila Logo - + {@club_name} - + <%= unless @mobile do %>
<%!-- Custom Fields Section --%> - <%= if Enum.any?(@custom_fields) do %> + <%= if is_list(@custom_fields) && Enum.any?(@custom_fields) do %>
- <.section_box title={gettext("Custom Fields")}> + <.section_box title={gettext("Additional Data Fields")}>
<%= for custom_field <- @custom_fields do %> <% cfv = find_custom_field_value(@member.custom_field_values, custom_field.id) %> @@ -233,13 +233,15 @@ defmodule MvWeb.MemberLive.Show do @impl true def handle_params(%{"id" => id}, _, socket) do - # 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!() - end) + # 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!() + + socket = assign(socket, :custom_fields, custom_fields) query = Mv.Membership.Member diff --git a/test/mv_web/member_live/index_display_name_test.exs b/test/mv_web/member_live/index_display_name_test.exs deleted file mode 100644 index 7a11235..0000000 --- a/test/mv_web/member_live/index_display_name_test.exs +++ /dev/null @@ -1,141 +0,0 @@ -defmodule MvWeb.Helpers.MemberHelpersTest do - @moduledoc """ - Tests for the display_name/1 helper function in MemberHelpers. - """ - use Mv.DataCase, async: true - - alias Mv.Membership.Member - alias MvWeb.Helpers.MemberHelpers - - describe "display_name/1" do - test "returns full name when both first_name and last_name are present" do - member = %Member{ - first_name: "John", - last_name: "Doe", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "John Doe" - end - - test "returns email when both first_name and last_name are nil" do - member = %Member{ - first_name: nil, - last_name: nil, - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "returns first_name only when last_name is nil" do - member = %Member{ - first_name: "John", - last_name: nil, - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "John" - end - - test "returns last_name only when first_name is nil" do - member = %Member{ - first_name: nil, - last_name: "Doe", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "Doe" - end - - test "returns email when first_name and last_name are empty strings" do - member = %Member{ - first_name: "", - last_name: "", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "returns email when first_name and last_name are whitespace only" do - member = %Member{ - first_name: " ", - last_name: " \t ", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "trims whitespace from name parts" do - member = %Member{ - first_name: " John ", - last_name: " Doe ", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "John Doe" - end - - test "handles one empty string and one nil" do - member = %Member{ - first_name: "", - last_name: nil, - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "handles one nil and one empty string" do - member = %Member{ - first_name: nil, - last_name: "", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "handles one whitespace and one nil" do - member = %Member{ - first_name: " ", - last_name: nil, - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "handles one valid name and one whitespace" do - member = %Member{ - first_name: "John", - last_name: " ", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "John" - end - - test "handles member with only first_name containing whitespace" do - member = %Member{ - first_name: " John ", - last_name: nil, - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "John" - end - - test "handles member with only last_name containing whitespace" do - member = %Member{ - first_name: nil, - last_name: " Doe ", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "Doe" - end - end -end diff --git a/test/mv_web/member_live/show_test.exs b/test/mv_web/member_live/show_test.exs index 1e04559..33505ec 100644 --- a/test/mv_web/member_live/show_test.exs +++ b/test/mv_web/member_live/show_test.exs @@ -7,13 +7,13 @@ defmodule MvWeb.MemberLive.ShowTest do - Custom Fields section visibility (Issue #282 regression test) - Custom field values formatting - ## Note on async: false - Tests use `async: false` (not `async: true`) to prevent PostgreSQL deadlocks - when creating members and custom fields concurrently. This is intentional and - documented here to avoid confusion in commit messages. + ## Note on async + Tests can run with `async: true` because: + - Each test explicitly manages its own custom fields (creates/deletes as needed) + - The SQL Sandbox provides proper isolation between parallel tests + - Custom field cleanup in tests ensures no interference between tests """ - # async: false to prevent PostgreSQL deadlocks when creating members and custom fields - use MvWeb.ConnCase, async: false + use MvWeb.ConnCase, async: true import Phoenix.LiveViewTest require Ash.Query use Gettext, backend: MvWeb.Gettext @@ -113,11 +113,21 @@ defmodule MvWeb.MemberLive.ShowTest do conn: conn, member: member } do + # Ensure no custom fields exist for this test + # This ensures test isolation even if previous tests created custom fields + existing_custom_fields = Ash.read!(CustomField) + for cf <- existing_custom_fields do + Ash.destroy!(cf) + end + + # Verify no custom fields exist + assert Ash.read!(CustomField) == [] + conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, ~p"/members/#{member}") # Custom Fields section should NOT be visible - refute html =~ gettext("Custom Fields") + refute html =~ gettext("Additional Data Fields") end end From 6fe75db56da6c9564227e12f2675242daa51ba11 Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 13 Jan 2026 10:50:33 +0100 Subject: [PATCH 53/98] formatting --- test/mv_web/member_live/show_test.exs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/mv_web/member_live/show_test.exs b/test/mv_web/member_live/show_test.exs index 33505ec..fdcfebb 100644 --- a/test/mv_web/member_live/show_test.exs +++ b/test/mv_web/member_live/show_test.exs @@ -116,6 +116,7 @@ defmodule MvWeb.MemberLive.ShowTest do # Ensure no custom fields exist for this test # This ensures test isolation even if previous tests created custom fields existing_custom_fields = Ash.read!(CustomField) + for cf <- existing_custom_fields do Ash.destroy!(cf) end From 469c4c0c1d8fd47e64f16df95a0d88805d52b36e Mon Sep 17 00:00:00 2001 From: carla Date: Tue, 13 Jan 2026 10:55:09 +0100 Subject: [PATCH 54/98] i18n: update translations --- priv/gettext/de/LC_MESSAGES/default.po | 6 +++++- priv/gettext/default.pot | 6 +++++- priv/gettext/en/LC_MESSAGES/default.po | 6 +++++- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index abf6f8d..182a428 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -633,7 +633,6 @@ msgstr "Bitte wähle zuerst ein Benutzerdefiniertes Feld" #: lib/mv_web/components/layouts/sidebar.ex #: lib/mv_web/live/member_live/form.ex -#: lib/mv_web/live/member_live/show.ex #, elixir-autogen, elixir-format msgid "Custom Fields" msgstr "Benutzerdefinierte Felder" @@ -2059,6 +2058,11 @@ msgstr "" msgid "read_only - Read access to all data" msgstr "" +#: lib/mv_web/live/member_live/show.ex +#, elixir-autogen, elixir-format +msgid "Additional Data Fields" +msgstr "Zusätzliche Datenfelder" + #~ #: lib/mv_web/live/custom_field_live/show.ex #~ #, elixir-autogen, elixir-format #~ msgid "Auto-generated identifier (immutable)" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index f7f0f0e..1be771a 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -634,7 +634,6 @@ msgstr "" #: lib/mv_web/components/layouts/sidebar.ex #: lib/mv_web/live/member_live/form.ex -#: lib/mv_web/live/member_live/show.ex #, elixir-autogen, elixir-format msgid "Custom Fields" msgstr "" @@ -2059,3 +2058,8 @@ msgstr "" #, elixir-autogen, elixir-format msgid "read_only - Read access to all data" msgstr "" + +#: lib/mv_web/live/member_live/show.ex +#, elixir-autogen, elixir-format +msgid "Additional Data Fields" +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 034392e..8b01f61 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -634,7 +634,6 @@ msgstr "" #: lib/mv_web/components/layouts/sidebar.ex #: lib/mv_web/live/member_live/form.ex -#: lib/mv_web/live/member_live/show.ex #, elixir-autogen, elixir-format, fuzzy msgid "Custom Fields" msgstr "" @@ -2060,6 +2059,11 @@ msgstr "" msgid "read_only - Read access to all data" msgstr "" +#: lib/mv_web/live/member_live/show.ex +#, elixir-autogen, elixir-format +msgid "Additional Data Fields" +msgstr "" + #~ #: lib/mv_web/live/components/payment_filter_component.ex #~ #, elixir-autogen, elixir-format #~ msgid "All payment statuses" From 295d71176840f3621ca9ce12f1819ca1eee077d3 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:05:36 +0100 Subject: [PATCH 55/98] test: Add role tag support to ConnCase and fix test issues - Add role tag support (@tag role: :admin/:member/:unauthenticated) to ConnCase - Fix Keyword.get -> Map.get for tags Map - Remove duplicate test file index_display_name_test.exs - Fix CustomField creation in tests (remove slug, use :string instead of :text) - Fix CustomFieldValue value format to use _union_type/_union_value --- .../form_membership_fee_type_test.exs | 149 ++++++++++++++++++ .../member_live/index_display_name_test.exs | 141 ----------------- test/support/conn_case.ex | 35 +++- 3 files changed, 179 insertions(+), 146 deletions(-) delete mode 100644 test/mv_web/member_live/index_display_name_test.exs 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 63c5a0d..4293e67 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 @@ -150,4 +150,153 @@ defmodule MvWeb.MemberLive.FormMembershipFeeTypeTest do assert html =~ fee_type.name || html =~ "selected" end end + + describe "custom field value preservation" do + test "custom field values preserved when membership fee type changes", %{ + conn: conn, + current_user: admin_user + } do + # Create custom field + custom_field = + Mv.Membership.CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "Test Field", + value_type: :string, + required: false + }) + |> Ash.create!() + + # Create two fee types with same interval + fee_type1 = create_fee_type(%{name: "Type 1", interval: :yearly}) + fee_type2 = create_fee_type(%{name: "Type 2", interval: :yearly}) + + # Create member with fee type 1 and custom field value + member = + Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "Test", + last_name: "Member", + email: "test#{System.unique_integer([:positive])}@example.com", + membership_fee_type_id: fee_type1.id + }) + |> Ash.create!(actor: admin_user) + + # Add custom field value + Mv.Membership.CustomFieldValue + |> Ash.Changeset.for_create(:create, %{ + member_id: member.id, + custom_field_id: custom_field.id, + value: %{"_union_type" => "string", "_union_value" => "Test Value"} + }) + |> Ash.create!(actor: admin_user) + + {:ok, view, _html} = live(conn, "/members/#{member.id}/edit") + + # Change membership fee type dropdown + html = + view + |> form("#member-form", %{"member[membership_fee_type_id]" => fee_type2.id}) + |> render_change() + + # Verify custom field value is still present (check for field name or value) + assert html =~ custom_field.name || html =~ "Test Value" + end + + test "union/typed values roundtrip correctly", %{conn: conn, current_user: admin_user} do + # Create date custom field + custom_field = + Mv.Membership.CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "Date Field", + value_type: :date, + required: false + }) + |> Ash.create!() + + fee_type = create_fee_type(%{interval: :yearly}) + + # Create member with date custom field value + member = + Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "Test", + last_name: "Member", + email: "test#{System.unique_integer([:positive])}@example.com", + membership_fee_type_id: fee_type.id + }) + |> Ash.create!(actor: admin_user) + + test_date = ~D[2024-01-15] + + # Add date custom field value + Mv.Membership.CustomFieldValue + |> Ash.Changeset.for_create(:create, %{ + member_id: member.id, + custom_field_id: custom_field.id, + value: %{"_union_type" => "date", "_union_value" => test_date} + }) + |> Ash.create!(actor: admin_user) + + {:ok, view, _html} = live(conn, "/members/#{member.id}/edit") + + # Trigger validation (simulates dropdown change) + html = + view + |> form("#member-form", %{"member[membership_fee_type_id]" => fee_type.id}) + |> render_change() + + # Verify date value is still present (check for date input or formatted date) + assert html =~ "2024" || html =~ "date" + end + + test "removing custom field values works correctly", %{conn: conn, current_user: admin_user} do + # Create custom field + custom_field = + Mv.Membership.CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "Test Field", + value_type: :string, + required: false + }) + |> Ash.create!() + + fee_type = create_fee_type(%{interval: :yearly}) + + # Create member with custom field value + member = + Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "Test", + last_name: "Member", + email: "test#{System.unique_integer([:positive])}@example.com", + membership_fee_type_id: fee_type.id + }) + |> Ash.create!(actor: admin_user) + + # Add custom field value + _cfv = + Mv.Membership.CustomFieldValue + |> Ash.Changeset.for_create(:create, %{ + member_id: member.id, + custom_field_id: custom_field.id, + value: %{"_union_type" => "string", "_union_value" => "Test Value"} + }) + |> Ash.create!(actor: admin_user) + + {:ok, view, _html} = live(conn, "/members/#{member.id}/edit") + + # Change membership fee type to trigger validation + # This should preserve the custom field value + html = + view + |> form("#member-form", %{ + "member[membership_fee_type_id]" => fee_type.id + }) + |> render_change() + + # Form should still be valid and custom field value should be preserved + # The custom field value should still be visible in the form + assert html =~ "Test Value" || html =~ custom_field.name + end + end end diff --git a/test/mv_web/member_live/index_display_name_test.exs b/test/mv_web/member_live/index_display_name_test.exs deleted file mode 100644 index 7a11235..0000000 --- a/test/mv_web/member_live/index_display_name_test.exs +++ /dev/null @@ -1,141 +0,0 @@ -defmodule MvWeb.Helpers.MemberHelpersTest do - @moduledoc """ - Tests for the display_name/1 helper function in MemberHelpers. - """ - use Mv.DataCase, async: true - - alias Mv.Membership.Member - alias MvWeb.Helpers.MemberHelpers - - describe "display_name/1" do - test "returns full name when both first_name and last_name are present" do - member = %Member{ - first_name: "John", - last_name: "Doe", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "John Doe" - end - - test "returns email when both first_name and last_name are nil" do - member = %Member{ - first_name: nil, - last_name: nil, - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "returns first_name only when last_name is nil" do - member = %Member{ - first_name: "John", - last_name: nil, - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "John" - end - - test "returns last_name only when first_name is nil" do - member = %Member{ - first_name: nil, - last_name: "Doe", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "Doe" - end - - test "returns email when first_name and last_name are empty strings" do - member = %Member{ - first_name: "", - last_name: "", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "returns email when first_name and last_name are whitespace only" do - member = %Member{ - first_name: " ", - last_name: " \t ", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "trims whitespace from name parts" do - member = %Member{ - first_name: " John ", - last_name: " Doe ", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "John Doe" - end - - test "handles one empty string and one nil" do - member = %Member{ - first_name: "", - last_name: nil, - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "handles one nil and one empty string" do - member = %Member{ - first_name: nil, - last_name: "", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "handles one whitespace and one nil" do - member = %Member{ - first_name: " ", - last_name: nil, - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "john@example.com" - end - - test "handles one valid name and one whitespace" do - member = %Member{ - first_name: "John", - last_name: " ", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "John" - end - - test "handles member with only first_name containing whitespace" do - member = %Member{ - first_name: " John ", - last_name: nil, - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "John" - end - - test "handles member with only last_name containing whitespace" do - member = %Member{ - first_name: nil, - last_name: " Doe ", - email: "john@example.com" - } - - assert MemberHelpers.display_name(member) == "Doe" - end - end -end diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index 8f943d9..3b2a5ed 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -154,11 +154,36 @@ defmodule MvWeb.ConnCase do # to share the test's database connection in async tests conn = Plug.Conn.put_private(conn, :ecto_sandbox, pid) - # 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) + # Handle role tags for future test extensions + # Default to admin to maintain backward compatibility with existing tests + role = Map.get(tags, :role, :admin) - {:ok, conn: authenticated_conn, current_user: admin_user} + {conn, user} = + case role do + :admin -> + # 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) + {authenticated_conn, admin_user} + + :member -> + # Create member user for role-based testing + member_user = Mv.Fixtures.user_with_role_fixture("member") + authenticated_conn = conn_with_password_user(conn, member_user) + {authenticated_conn, member_user} + + :unauthenticated -> + # No authentication for unauthenticated tests + {conn, nil} + + _other -> + # Fallback: treat unknown role as admin for safety + admin_user = Mv.Fixtures.user_with_role_fixture("admin") + authenticated_conn = conn_with_password_user(conn, admin_user) + {authenticated_conn, admin_user} + end + + {:ok, conn: conn, current_user: user} end end From fdbe673a65345082fc610b0e8f52cd8ed604d166 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:05:39 +0100 Subject: [PATCH 56/98] feat: Add shared helper functions for actor handling - Add Mv.Helpers module with ash_actor_opts/1 helper - Add current_actor/1 with @spec to LiveHelpers - Add ash_actor_opts/1 delegate and submit_form/3 wrapper to LiveHelpers - Standardize actor access pattern across LiveViews --- lib/mv/helpers.ex | 27 +++++++++++++++++++++++++++ lib/mv_web/live_helpers.ex | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 lib/mv/helpers.ex diff --git a/lib/mv/helpers.ex b/lib/mv/helpers.ex new file mode 100644 index 0000000..e20db67 --- /dev/null +++ b/lib/mv/helpers.ex @@ -0,0 +1,27 @@ +defmodule Mv.Helpers do + @moduledoc """ + Shared helper functions used across the Mv application. + + Provides utilities that are not specific to a single domain or layer. + """ + + @doc """ + Converts an actor to Ash options list for authorization. + Returns empty list if actor is nil. + + This helper ensures consistent actor handling across all Ash operations + in the application, whether called from LiveViews, changes, validations, + or other contexts. + + ## Examples + + opts = ash_actor_opts(actor) + Ash.read(query, opts) + + opts = ash_actor_opts(nil) + # => [] + """ + @spec ash_actor_opts(Mv.Accounts.User.t() | nil) :: keyword() + def ash_actor_opts(nil), do: [] + def ash_actor_opts(actor) when not is_nil(actor), do: [actor: actor] +end diff --git a/lib/mv_web/live_helpers.ex b/lib/mv_web/live_helpers.ex index 6af71ec..95d8235 100644 --- a/lib/mv_web/live_helpers.ex +++ b/lib/mv_web/live_helpers.ex @@ -71,7 +71,41 @@ defmodule MvWeb.LiveHelpers do actor = current_actor(socket) members = Membership.list_members!(actor: actor) """ + @spec current_actor(Phoenix.LiveView.Socket.t()) :: Mv.Accounts.User.t() | nil def current_actor(socket) do socket.assigns[:current_user] || socket.assigns.current_user end + + @doc """ + Converts an actor to Ash options list for authorization. + Returns empty list if actor is nil. + + Delegates to `Mv.Helpers.ash_actor_opts/1` for consistency across the application. + + ## Examples + + opts = ash_actor_opts(actor) + Ash.read(query, opts) + """ + @spec ash_actor_opts(Mv.Accounts.User.t() | nil) :: keyword() + defdelegate ash_actor_opts(actor), to: Mv.Helpers + + @doc """ + Submits an AshPhoenix form with consistent actor handling. + + This wrapper ensures that actor is always passed via `action_opts` + in a consistent manner across all LiveViews. + + ## Examples + + case submit_form(form, params, actor) do + {:ok, resource} -> # success + {:error, form} -> # validation errors + end + """ + @spec submit_form(AshPhoenix.Form.t(), map(), Mv.Accounts.User.t() | nil) :: + {:ok, Ash.Resource.t()} | {:error, AshPhoenix.Form.t()} + def submit_form(form, params, actor) do + AshPhoenix.Form.submit(form, params: params, action_opts: ash_actor_opts(actor)) + end end From 7b28b03cd48a8cdb3bf08f1a612e64bad2496964 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:05:41 +0100 Subject: [PATCH 57/98] refactor: Replace actor option patterns with ash_actor_opts helper - Replace if actor, do: [actor: actor], else: [] with Mv.Helpers.ash_actor_opts/1 - Update email_sync/loader.ex, member validations, member.ex, cycle_generator.ex - Consistent actor handling across non-LiveView modules --- lib/membership/member.ex | 3 ++- lib/mv/email_sync/loader.ex | 16 +++++++++++++--- .../validations/email_not_used_by_other_user.ex | 5 +++-- lib/mv/membership_fees/cycle_generator.ex | 9 +++++++++ 4 files changed, 27 insertions(+), 6 deletions(-) diff --git a/lib/membership/member.ex b/lib/membership/member.ex index c5994f6..31d51cc 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -39,6 +39,7 @@ defmodule Mv.Membership.Member do require Ash.Query import Ash.Expr + alias Mv.Helpers require Logger # Module constants @@ -1235,7 +1236,7 @@ defmodule Mv.Membership.Member do # Extracts custom field values from existing member data (update scenario) defp extract_existing_values(member_data, changeset) do actor = Map.get(changeset.context, :actor) - opts = if actor, do: [actor: actor], else: [] + opts = Helpers.ash_actor_opts(actor) case Ash.load(member_data, :custom_field_values, opts) do {:ok, %{custom_field_values: existing_values}} -> diff --git a/lib/mv/email_sync/loader.ex b/lib/mv/email_sync/loader.ex index f6f6ecc..91927fb 100644 --- a/lib/mv/email_sync/loader.ex +++ b/lib/mv/email_sync/loader.ex @@ -2,7 +2,17 @@ defmodule Mv.EmailSync.Loader do @moduledoc """ Helper functions for loading linked records in email synchronization. Centralizes the logic for retrieving related User/Member entities. + + ## Authorization + + This module runs systemically and accepts optional actor parameters. + When called from hooks/changes, actor is extracted from changeset context. + When called directly, actor should be provided for proper authorization. + + All functions accept an optional `actor` parameter that is passed to Ash operations + to ensure proper authorization checks are performed. """ + alias Mv.Helpers @doc """ Loads the member linked to a user, returns nil if not linked or on error. @@ -13,7 +23,7 @@ defmodule Mv.EmailSync.Loader do 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: [] + opts = Helpers.ash_actor_opts(actor) case Ash.get(Mv.Membership.Member, id, opts) do {:ok, member} -> member @@ -27,7 +37,7 @@ defmodule Mv.EmailSync.Loader do Accepts optional actor for authorization. """ def get_linked_user(member, actor \\ nil) do - opts = if actor, do: [actor: actor], else: [] + opts = Helpers.ash_actor_opts(actor) case Ash.load(member, :user, opts) do {:ok, %{user: user}} -> user @@ -42,7 +52,7 @@ defmodule Mv.EmailSync.Loader do Accepts optional actor for authorization. """ def load_linked_user!(member, actor \\ nil) do - opts = if actor, do: [actor: actor], else: [] + opts = Helpers.ash_actor_opts(actor) case Ash.load(member, :user, opts) do {:ok, %{user: user}} when not is_nil(user) -> {:ok, user} 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 649493b..e1bbc4e 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 @@ -8,6 +8,7 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do This allows creating members with the same email as unlinked users. """ use Ash.Resource.Validation + alias Mv.Helpers @doc """ Validates email uniqueness across linked Member-User pairs. @@ -51,7 +52,7 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do |> Ash.Query.filter(email == ^email) |> maybe_exclude_id(exclude_user_id) - opts = if actor, do: [actor: actor], else: [] + opts = Helpers.ash_actor_opts(actor) case Ash.read(query, opts) do {:ok, []} -> @@ -69,7 +70,7 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) defp get_linked_user_id(member_data, actor) do - opts = if actor, do: [actor: actor], else: [] + opts = Helpers.ash_actor_opts(actor) case Ash.load(member_data, :user, opts) do {:ok, %{user: %{id: id}}} -> id diff --git a/lib/mv/membership_fees/cycle_generator.ex b/lib/mv/membership_fees/cycle_generator.ex index 32b4e79..0d17dd3 100644 --- a/lib/mv/membership_fees/cycle_generator.ex +++ b/lib/mv/membership_fees/cycle_generator.ex @@ -28,6 +28,15 @@ defmodule Mv.MembershipFees.CycleGenerator do Uses PostgreSQL advisory locks to prevent race conditions when generating cycles for the same member concurrently. + ## Authorization + + This module runs systemically and accepts optional actor parameters. + When called from hooks/changes, actor is extracted from changeset context. + When called directly, actor should be provided for proper authorization. + + All functions accept an optional `actor` parameter in the `opts` keyword list + that is passed to Ash operations to ensure proper authorization checks are performed. + ## Examples # Generate cycles for a single member From 6c94146aca7a7e53fbb94e7d6249267bfae25cfa Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:05:44 +0100 Subject: [PATCH 58/98] refactor: Use submit_form wrapper in all LiveView forms - Replace AshPhoenix.Form.submit with submit_form/3 wrapper - Import current_actor and submit_form from LiveHelpers - Consistent actor handling in all form submissions --- lib/mv_web/live/custom_field_live/form_component.ex | 4 +++- lib/mv_web/live/custom_field_value_live/form.ex | 6 ++++-- lib/mv_web/live/global_settings_live.ex | 4 +++- lib/mv_web/live/member_live/form.ex | 7 ++----- lib/mv_web/live/membership_fee_settings_live.ex | 4 +++- lib/mv_web/live/membership_fee_type_live/form.ex | 6 ++++-- lib/mv_web/live/role_live/form.ex | 4 +++- 7 files changed, 22 insertions(+), 13 deletions(-) diff --git a/lib/mv_web/live/custom_field_live/form_component.ex b/lib/mv_web/live/custom_field_live/form_component.ex index 172cfd3..bf2e882 100644 --- a/lib/mv_web/live/custom_field_live/form_component.ex +++ b/lib/mv_web/live/custom_field_live/form_component.ex @@ -91,7 +91,9 @@ defmodule MvWeb.CustomFieldLive.FormComponent do @impl true def handle_event("save", %{"custom_field" => custom_field_params}, socket) do - case AshPhoenix.Form.submit(socket.assigns.form, params: custom_field_params) do + actor = MvWeb.LiveHelpers.current_actor(socket) + + case MvWeb.LiveHelpers.submit_form(socket.assigns.form, custom_field_params, actor) do {:ok, custom_field} -> action = case socket.assigns.form.source.type do 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 fde54c3..599ce1d 100644 --- a/lib/mv_web/live/custom_field_value_live/form.ex +++ b/lib/mv_web/live/custom_field_value_live/form.ex @@ -33,7 +33,7 @@ defmodule MvWeb.CustomFieldValueLive.Form do use MvWeb, :live_view on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - import MvWeb.LiveHelpers, only: [current_actor: 1] + import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] @impl true def render(assigns) do @@ -228,7 +228,9 @@ defmodule MvWeb.CustomFieldValueLive.Form do custom_field_value_params end - case AshPhoenix.Form.submit(socket.assigns.form, params: updated_params) do + actor = current_actor(socket) + + case submit_form(socket.assigns.form, updated_params, actor) do {:ok, custom_field_value} -> notify_parent({:saved, custom_field_value}) diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index 9bce04b..0a968d6 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -79,7 +79,9 @@ defmodule MvWeb.GlobalSettingsLive do @impl true def handle_event("save", %{"setting" => setting_params}, socket) do - case AshPhoenix.Form.submit(socket.assigns.form, params: setting_params) do + actor = MvWeb.LiveHelpers.current_actor(socket) + + case MvWeb.LiveHelpers.submit_form(socket.assigns.form, setting_params, actor) do {:ok, _updated_settings} -> # Reload settings from database to ensure all dependent data is updated {:ok, fresh_settings} = Membership.get_settings() diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index 1f5ab2d..07d4e2d 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -23,7 +23,7 @@ defmodule MvWeb.MemberLive.Form do on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - import MvWeb.LiveHelpers, only: [current_actor: 1] + import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] alias Mv.MembershipFees alias Mv.MembershipFees.MembershipFeeType @@ -290,10 +290,7 @@ defmodule MvWeb.MemberLive.Form do try do actor = current_actor(socket) - case AshPhoenix.Form.submit(socket.assigns.form, - params: member_params, - action_opts: [actor: actor] - ) do + case submit_form(socket.assigns.form, member_params, actor) do {:ok, member} -> handle_save_success(socket, member) diff --git a/lib/mv_web/live/membership_fee_settings_live.ex b/lib/mv_web/live/membership_fee_settings_live.ex index 61774e8..a98ccdb 100644 --- a/lib/mv_web/live/membership_fee_settings_live.ex +++ b/lib/mv_web/live/membership_fee_settings_live.ex @@ -63,7 +63,9 @@ defmodule MvWeb.MembershipFeeSettingsLive do Map.put(params, "include_joining_cycle", false) end - case AshPhoenix.Form.submit(socket.assigns.form, params: normalized_params) do + actor = MvWeb.LiveHelpers.current_actor(socket) + + case MvWeb.LiveHelpers.submit_form(socket.assigns.form, normalized_params, actor) do {:ok, updated_settings} -> {:noreply, socket diff --git a/lib/mv_web/live/membership_fee_type_live/form.ex b/lib/mv_web/live/membership_fee_type_live/form.ex index b6339fd..077e89d 100644 --- a/lib/mv_web/live/membership_fee_type_live/form.ex +++ b/lib/mv_web/live/membership_fee_type_live/form.ex @@ -14,7 +14,7 @@ defmodule MvWeb.MembershipFeeTypeLive.Form do use MvWeb, :live_view on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - import MvWeb.LiveHelpers, only: [current_actor: 1] + import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] require Ash.Query @@ -308,7 +308,9 @@ defmodule MvWeb.MembershipFeeTypeLive.Form do if socket.assigns.show_amount_warning do {:noreply, put_flash(socket, :error, gettext("Please confirm the amount change first"))} else - case AshPhoenix.Form.submit(socket.assigns.form, params: params) do + actor = current_actor(socket) + + case submit_form(socket.assigns.form, params, actor) do {:ok, membership_fee_type} -> notify_parent({:saved, membership_fee_type}) diff --git a/lib/mv_web/live/role_live/form.ex b/lib/mv_web/live/role_live/form.ex index 7b74c7e..0ad0fdd 100644 --- a/lib/mv_web/live/role_live/form.ex +++ b/lib/mv_web/live/role_live/form.ex @@ -162,7 +162,9 @@ defmodule MvWeb.RoleLive.Form do end def handle_event("save", %{"role" => role_params}, socket) do - case AshPhoenix.Form.submit(socket.assigns.form, params: role_params) do + actor = MvWeb.LiveHelpers.current_actor(socket) + + case MvWeb.LiveHelpers.submit_form(socket.assigns.form, role_params, actor) do {:ok, role} -> notify_parent({:saved, role}) From 6d10b9799d76342f37b1d9cfc037adb15a069890 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:05:46 +0100 Subject: [PATCH 59/98] refactor: Replace bang calls with error handling in Index LiveViews - Replace Ash.get!/Ash.destroy! with Ash.get/Ash.destroy - Add case statements for Forbidden, NotFound, and generic errors - Display user-friendly flash messages for all error cases - Use Enum.map_join/3 for efficient error formatting --- .../live/custom_field_value_live/index.ex | 54 +++++++++++++++++-- lib/mv_web/live/member_live/index.ex | 8 ++- .../live/membership_fee_type_live/index.ex | 43 +++++++++++---- lib/mv_web/live/role_live/index.ex | 2 +- lib/mv_web/live/user_live/index.ex | 46 ++++++++++++++-- 5 files changed, 130 insertions(+), 23 deletions(-) diff --git a/lib/mv_web/live/custom_field_value_live/index.ex b/lib/mv_web/live/custom_field_value_live/index.ex index b52fd96..0847cd6 100644 --- a/lib/mv_web/live/custom_field_value_live/index.ex +++ b/lib/mv_web/live/custom_field_value_live/index.ex @@ -23,6 +23,9 @@ defmodule MvWeb.CustomFieldValueLive.Index 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""" @@ -70,17 +73,60 @@ defmodule MvWeb.CustomFieldValueLive.Index do @impl true def mount(_params, _session, socket) do + actor = current_actor(socket) + {:ok, socket |> assign(:page_title, "Listing Custom field values") - |> stream(:custom_field_values, Ash.read!(Mv.Membership.CustomFieldValue))} + |> stream(:custom_field_values, Ash.read!(Mv.Membership.CustomFieldValue, actor: actor))} end @impl true def handle_event("delete", %{"id" => id}, socket) do - custom_field_value = Ash.get!(Mv.Membership.CustomFieldValue, id) - Ash.destroy!(custom_field_value) + actor = MvWeb.LiveHelpers.current_actor(socket) - {:noreply, stream_delete(socket, :custom_field_values, custom_field_value)} + case Ash.get(Mv.Membership.CustomFieldValue, id, actor: actor) do + {:ok, custom_field_value} -> + case Ash.destroy(custom_field_value, actor: actor) do + :ok -> + {:noreply, + socket + |> stream_delete(:custom_field_values, custom_field_value) + |> put_flash(:info, gettext("Custom field value deleted successfully"))} + + {:error, %Ash.Error.Forbidden{}} -> + {:noreply, + put_flash( + socket, + :error, + gettext("You do not have permission to delete this custom field value") + )} + + {:error, error} -> + {:noreply, put_flash(socket, :error, format_error(error))} + end + + {:error, %Ash.Error.Query.NotFound{}} -> + {:noreply, put_flash(socket, :error, gettext("Custom field value not found"))} + + {:error, %Ash.Error.Forbidden{} = _error} -> + {:noreply, + put_flash( + socket, + :error, + gettext("You do not have permission to access this custom field value") + )} + + {:error, error} -> + {:noreply, put_flash(socket, :error, format_error(error))} + end + end + + defp format_error(%Ash.Error.Invalid{errors: errors}) do + Enum.map_join(errors, ", ", fn %{message: message} -> message end) + end + + defp format_error(error) do + inspect(error) end end diff --git a/lib/mv_web/live/member_live/index.ex b/lib/mv_web/live/member_live/index.ex index f7eddfb..a972d1e 100644 --- a/lib/mv_web/live/member_live/index.ex +++ b/lib/mv_web/live/member_live/index.ex @@ -58,9 +58,8 @@ defmodule MvWeb.MemberLive.Index do @impl true def mount(_params, session, socket) do # Load custom fields that should be shown in overview (for display) - # 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. + # Errors in mount are handled by Phoenix LiveView and result in a 500 error page. + # This is appropriate for initialization errors that should be visible to the user. actor = current_actor(socket) custom_fields_visible = @@ -727,8 +726,7 @@ defmodule MvWeb.MemberLive.Index do socket.assigns.custom_fields_visible ) - # Note: Using Ash.read! - errors will be handled by Phoenix LiveView - # This is appropriate for data loading in LiveViews + # Errors in handle_params are handled by Phoenix LiveView actor = current_actor(socket) members = Ash.read!(query, actor: actor) diff --git a/lib/mv_web/live/membership_fee_type_live/index.ex b/lib/mv_web/live/membership_fee_type_live/index.ex index 976e9c1..fb2b9e7 100644 --- a/lib/mv_web/live/membership_fee_type_live/index.ex +++ b/lib/mv_web/live/membership_fee_type_live/index.ex @@ -133,18 +133,43 @@ defmodule MvWeb.MembershipFeeTypeLive.Index do @impl true def handle_event("delete", %{"id" => id}, socket) do - fee_type = Ash.get!(MembershipFeeType, id, domain: MembershipFees) + actor = current_actor(socket) - case Ash.destroy(fee_type, domain: MembershipFees) do - :ok -> - updated_types = Enum.reject(socket.assigns.membership_fee_types, &(&1.id == id)) - updated_counts = Map.delete(socket.assigns.member_counts, id) + case Ash.get(MembershipFeeType, id, domain: MembershipFees, actor: actor) do + {:ok, fee_type} -> + case Ash.destroy(fee_type, domain: MembershipFees, actor: actor) do + :ok -> + updated_types = Enum.reject(socket.assigns.membership_fee_types, &(&1.id == id)) + updated_counts = Map.delete(socket.assigns.member_counts, id) + {:noreply, + socket + |> assign(:membership_fee_types, updated_types) + |> assign(:member_counts, updated_counts) + |> put_flash(:info, gettext("Membership fee type deleted"))} + + {:error, %Ash.Error.Forbidden{}} -> + {:noreply, + put_flash( + socket, + :error, + gettext("You do not have permission to delete this membership fee type") + )} + + {:error, error} -> + {:noreply, put_flash(socket, :error, format_error(error))} + end + + {:error, %Ash.Error.Query.NotFound{}} -> + {:noreply, put_flash(socket, :error, gettext("Membership fee type not found"))} + + {:error, %Ash.Error.Forbidden{} = _error} -> {:noreply, - socket - |> assign(:membership_fee_types, updated_types) - |> assign(:member_counts, updated_counts) - |> put_flash(:info, gettext("Membership fee type deleted"))} + put_flash( + socket, + :error, + gettext("You do not have permission to access this membership fee type") + )} {:error, error} -> {:noreply, put_flash(socket, :error, format_error(error))} diff --git a/lib/mv_web/live/role_live/index.ex b/lib/mv_web/live/role_live/index.ex index 9d75da6..4f8e45d 100644 --- a/lib/mv_web/live/role_live/index.ex +++ b/lib/mv_web/live/role_live/index.ex @@ -118,7 +118,7 @@ defmodule MvWeb.RoleLive.Index do @spec load_roles(map() | nil) :: [Mv.Authorization.Role.t()] defp load_roles(actor) do - opts = if actor, do: [actor: actor], else: [] + opts = MvWeb.LiveHelpers.ash_actor_opts(actor) case Authorization.list_roles(opts) do {:ok, roles} -> Enum.sort_by(roles, & &1.name) diff --git a/lib/mv_web/live/user_live/index.ex b/lib/mv_web/live/user_live/index.ex index bd9c881..5e5949e 100644 --- a/lib/mv_web/live/user_live/index.ex +++ b/lib/mv_web/live/user_live/index.ex @@ -43,11 +43,41 @@ defmodule MvWeb.UserLive.Index do @impl true def handle_event("delete", %{"id" => id}, socket) do - user = Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts) - Ash.destroy!(user, domain: Mv.Accounts) + actor = current_actor(socket) - updated_users = Enum.reject(socket.assigns.users, &(&1.id == id)) - {:noreply, assign(socket, :users, updated_users)} + case Ash.get(Mv.Accounts.User, id, domain: Mv.Accounts, actor: actor) do + {:ok, user} -> + case Ash.destroy(user, domain: Mv.Accounts, actor: actor) do + :ok -> + updated_users = Enum.reject(socket.assigns.users, &(&1.id == id)) + + {:noreply, + socket + |> assign(:users, updated_users) + |> put_flash(:info, gettext("User deleted successfully"))} + + {:error, %Ash.Error.Forbidden{}} -> + {:noreply, + put_flash( + socket, + :error, + gettext("You do not have permission to delete this user") + )} + + {:error, error} -> + {:noreply, put_flash(socket, :error, format_error(error))} + end + + {:error, %Ash.Error.Query.NotFound{}} -> + {:noreply, put_flash(socket, :error, gettext("User not found"))} + + {:error, %Ash.Error.Forbidden{} = _error} -> + {:noreply, + put_flash(socket, :error, gettext("You do not have permission to access this user"))} + + {:error, error} -> + {:noreply, put_flash(socket, :error, format_error(error))} + end end # Selects one user in the list of users @@ -108,4 +138,12 @@ defmodule MvWeb.UserLive.Index do defp toggle_order(:desc), do: :asc defp sort_fun(:asc), do: &<=/2 defp sort_fun(:desc), do: &>=/2 + + defp format_error(%Ash.Error.Invalid{errors: errors}) do + Enum.map_join(errors, ", ", fn %{message: message} -> message end) + end + + defp format_error(error) do + inspect(error) + end end From 51e7c457ae7435810f7d694b52af9126ac3d7eb2 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:05:49 +0100 Subject: [PATCH 60/98] refactor: Simplify UserLive.Form handle_event and improve error handling - Extract handle_member_linking, perform_member_link_action helpers - Extract handle_save_success, get_action_name, handle_member_link_error - Replace hardcoded strings with gettext translations - Use submit_form wrapper for consistent actor handling - Group all handle_event/3 clauses together - Add early return in load_members_for_linking if actor is nil --- lib/mv_web/live/user_live/form.ex | 134 +++++++++++++++++------------- 1 file changed, 78 insertions(+), 56 deletions(-) diff --git a/lib/mv_web/live/user_live/form.ex b/lib/mv_web/live/user_live/form.ex index a7e56e4..1cc6f6a 100644 --- a/lib/mv_web/live/user_live/form.ex +++ b/lib/mv_web/live/user_live/form.ex @@ -34,7 +34,7 @@ defmodule MvWeb.UserLive.Form do use MvWeb, :live_view on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - import MvWeb.LiveHelpers, only: [current_actor: 1] + import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] @impl true def render(assigns) do @@ -305,6 +305,7 @@ defmodule MvWeb.UserLive.Form do {:noreply, socket} end + @impl true def handle_event("validate", %{"user" => user_params}, socket) do validated_form = AshPhoenix.Form.validate(socket.assigns.form, user_params) @@ -322,70 +323,30 @@ defmodule MvWeb.UserLive.Form do {:noreply, socket} end + @impl true def handle_event("save", %{"user" => user_params}, socket) do actor = current_actor(socket) # First save the user without member changes - case AshPhoenix.Form.submit(socket.assigns.form, - params: user_params, - action_opts: [actor: actor] - ) do + case submit_form(socket.assigns.form, user_params, actor) do {:ok, user} -> - # Then handle member linking/unlinking as a separate step - actor = current_actor(socket) - - result = - cond do - # Selected member ID takes precedence (new link) - socket.assigns.selected_member_id -> - Mv.Accounts.update_user(user, %{member: %{id: socket.assigns.selected_member_id}}, - actor: actor - ) - - # Unlink flag is set - socket.assigns[:unlink_member] -> - Mv.Accounts.update_user(user, %{member: nil}, actor: actor) - - # No changes to member relationship - true -> - {:ok, user} - end - - case result do - {:ok, updated_user} -> - notify_parent({:saved, updated_user}) - - socket = - socket - |> put_flash(:info, "User #{socket.assigns.form.source.type}d successfully") - |> push_navigate(to: return_path(socket.assigns.return_to, updated_user)) - - {:noreply, socket} - - {:error, error} -> - # Show user-friendly error from member linking/unlinking - error_message = extract_error_message(error) - - {:noreply, - put_flash( - socket, - :error, - gettext("Failed to link member: %{error}", error: error_message) - )} - end + handle_member_linking(socket, user, actor) {:error, form} -> {:noreply, assign(socket, form: form)} end end + @impl true def handle_event("show_member_dropdown", _params, socket) do {:noreply, assign(socket, show_member_dropdown: true)} end + @impl true def handle_event("hide_member_dropdown", _params, socket) do {:noreply, assign(socket, show_member_dropdown: false, focused_member_index: nil)} end + @impl true def handle_event("member_dropdown_keydown", %{"key" => "ArrowDown"}, socket) do return_if_dropdown_closed(socket, fn -> max_index = length(socket.assigns.available_members) - 1 @@ -402,6 +363,7 @@ defmodule MvWeb.UserLive.Form do end) end + @impl true def handle_event("member_dropdown_keydown", %{"key" => "ArrowUp"}, socket) do return_if_dropdown_closed(socket, fn -> current = socket.assigns.focused_member_index @@ -417,23 +379,27 @@ defmodule MvWeb.UserLive.Form do end) end + @impl true def handle_event("member_dropdown_keydown", %{"key" => "Enter"}, socket) do return_if_dropdown_closed(socket, fn -> select_focused_member(socket) end) end + @impl true def handle_event("member_dropdown_keydown", %{"key" => "Escape"}, socket) do return_if_dropdown_closed(socket, fn -> {:noreply, assign(socket, show_member_dropdown: false, focused_member_index: nil)} end) end + @impl true def handle_event("member_dropdown_keydown", _params, socket) do # Ignore other keys {:noreply, socket} end + @impl true def handle_event("search_members", %{"member_search" => query}, socket) do socket = socket @@ -445,6 +411,7 @@ defmodule MvWeb.UserLive.Form do {:noreply, socket} end + @impl true def handle_event("select_member", %{"id" => member_id}, socket) do # Find the selected member to get their name selected_member = Enum.find(socket.assigns.available_members, &(&1.id == member_id)) @@ -461,27 +428,82 @@ defmodule MvWeb.UserLive.Form do |> assign(:selected_member_name, member_name) |> assign(:unlink_member, false) |> assign(:show_member_dropdown, false) - |> assign(:member_search_query, member_name) - |> push_event("set-input-value", %{id: "member-search-input", value: member_name}) + |> assign(:focused_member_index, nil) {:noreply, socket} end + @impl true def handle_event("unlink_member", _params, socket) do - # Set flag to unlink member on save - # Clear all member selection state and keep dropdown hidden socket = socket - |> assign(:unlink_member, true) |> assign(:selected_member_id, nil) |> assign(:selected_member_name, nil) - |> assign(:member_search_query, "") + |> assign(:unlink_member, true) |> assign(:show_member_dropdown, false) - |> load_initial_members() + |> assign(:focused_member_index, nil) {:noreply, socket} end + defp handle_member_linking(socket, user, actor) do + result = perform_member_link_action(socket, user, actor) + + case result do + {:ok, updated_user} -> + handle_save_success(socket, updated_user) + + {:error, error} -> + handle_member_link_error(socket, error) + end + end + + defp perform_member_link_action(socket, user, actor) do + cond do + # Selected member ID takes precedence (new link) + socket.assigns.selected_member_id -> + Mv.Accounts.update_user(user, %{member: %{id: socket.assigns.selected_member_id}}, + actor: actor + ) + + # Unlink flag is set + socket.assigns[:unlink_member] -> + Mv.Accounts.update_user(user, %{member: nil}, actor: actor) + + # No changes to member relationship + true -> + {:ok, user} + end + end + + defp handle_save_success(socket, updated_user) do + notify_parent({:saved, updated_user}) + + action = get_action_name(socket.assigns.form.source.type) + + socket = + socket + |> put_flash(:info, gettext("User %{action} successfully", action: action)) + |> push_navigate(to: return_path(socket.assigns.return_to, updated_user)) + + {:noreply, socket} + end + + defp get_action_name(:create), do: gettext("created") + defp get_action_name(:update), do: gettext("updated") + defp get_action_name(other), do: to_string(other) + + defp handle_member_link_error(socket, error) do + error_message = extract_error_message(error) + + {:noreply, + put_flash( + socket, + :error, + gettext("Failed to link member: %{error}", error: error_message) + )} + end + @spec notify_parent(any()) :: any() defp notify_parent(msg), do: send(self(), {__MODULE__, msg}) @@ -597,10 +619,10 @@ defmodule MvWeb.UserLive.Form do case List.first(errors) do %{message: message} when is_binary(message) -> message %{field: field, message: message} -> "#{field}: #{message}" - _ -> "Unknown error" + _ -> gettext("Unknown error") end end defp extract_error_message(error) when is_binary(error), do: error - defp extract_error_message(_), do: "Unknown error" + defp extract_error_message(_), do: gettext("Unknown error") end From 4c55ced5364d53e2485ec89a33faa1020a0ff641 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:05:51 +0100 Subject: [PATCH 61/98] i18n: Add German and English translations for UI strings - Fill in empty msgstr entries in German translations - Add translations for user actions, error messages, and form labels - Ensure all UI strings are consistently translated --- priv/gettext/de/LC_MESSAGES/default.po | 99 ++++++++++++++++++++++---- priv/gettext/default.pot | 75 +++++++++++++++++++ priv/gettext/en/LC_MESSAGES/default.po | 75 +++++++++++++++++++ 3 files changed, 237 insertions(+), 12 deletions(-) diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 9ffcf4a..7177eb1 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -1708,17 +1708,17 @@ msgstr "Warnung: Wechsel von %{old_interval} zu %{new_interval} ist nicht erlaub #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format msgid "A cycle for this period already exists" -msgstr "" +msgstr "Ein Zyklus für diesen Zeitraum existiert bereits" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format msgid "All cycles deleted" -msgstr "" +msgstr "Alle Zyklen gelöscht" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format msgid "Click to edit amount" -msgstr "" +msgstr "Klicken Sie, um den Betrag zu bearbeiten" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format, fuzzy @@ -1733,7 +1733,7 @@ msgstr "Aktueller Zyklus" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format msgid "Create a new cycle manually" -msgstr "" +msgstr "Einen neuen Zyklus manuell erstellen" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format, fuzzy @@ -1778,27 +1778,27 @@ msgstr "Zahlungsfilter" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format msgid "The cycle period will be calculated based on this date and the interval." -msgstr "" +msgstr "Der Zykluszeitraum wird basierend auf diesem Datum und dem Intervall berechnet." #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format msgid "This action cannot be undone." -msgstr "" +msgstr "Diese Aktion kann nicht rückgängig gemacht werden." #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format msgid "Type '%{confirmation}' to confirm" -msgstr "" +msgstr "Geben Sie '%{confirmation}' ein, um zu bestätigen" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format msgid "Warning" -msgstr "" +msgstr "Warnung" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format msgid "You are about to delete all %{count} cycles for this member." -msgstr "" +msgstr "Sie sind dabei, alle %{count} Zyklen für dieses Mitglied zu löschen." #: lib/mv_web/live/member_live/index.html.heex #, elixir-autogen, elixir-format @@ -1813,7 +1813,7 @@ msgstr "Letzter Zyklus Zahlungsstatus" #: lib/mv_web/live/membership_fee_type_live/index.ex #, elixir-autogen, elixir-format msgid "Delete membership fee type" -msgstr "" +msgstr "Mitgliedsbeitragsart löschen" #: lib/mv_web/live/membership_fee_type_live/index.ex #, elixir-autogen, elixir-format, fuzzy @@ -1823,7 +1823,7 @@ msgstr "Mitgliedsbeitragsart bearbeiten" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format msgid "Confirmation text does not match" -msgstr "" +msgstr "Bestätigungstext stimmt nicht überein" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format, fuzzy @@ -1894,7 +1894,7 @@ msgstr "Berechtigungssatz" #: lib/mv_web/live/role_live/show.ex #, elixir-autogen, elixir-format msgid "Role" -msgstr "" +msgstr "Rolle" #: lib/mv_web/live/role_live/show.ex #, elixir-autogen, elixir-format @@ -2021,3 +2021,78 @@ msgstr "Sie haben keine Berechtigung, auf dieses Mitglied zuzugreifen" #, elixir-autogen, elixir-format msgid "You do not have permission to delete this member" msgstr "Sie haben keine Berechtigung, dieses Mitglied zu löschen" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "Custom field value deleted successfully" +msgstr "Benutzerdefiniertes Feldwert erfolgreich gelöscht" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "Custom field value not found" +msgstr "Benutzerdefinierte Felder" + +#: lib/mv_web/live/membership_fee_type_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "Membership fee type not found" +msgstr "Mitgliedsbeitragsart entfernt" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "User %{action} successfully" +msgstr "Mitglied wurde erfolgreich %{action}" + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "User deleted successfully" +msgstr "Rolle erfolgreich gelöscht." + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "User not found" +msgstr "Mitglied nicht gefunden" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to access this custom field value" +msgstr "Sie haben keine Berechtigung, auf dieses Mitglied zuzugreifen" + +#: lib/mv_web/live/membership_fee_type_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to access this membership fee type" +msgstr "Sie haben keine Berechtigung, auf dieses Mitglied zuzugreifen" + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to access this user" +msgstr "Sie haben keine Berechtigung, auf dieses Mitglied zuzugreifen" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to delete this custom field value" +msgstr "Sie haben keine Berechtigung, dieses Mitglied zu löschen" + +#: lib/mv_web/live/membership_fee_type_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to delete this membership fee type" +msgstr "Sie haben keine Berechtigung, dieses Mitglied zu löschen" + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to delete this user" +msgstr "Sie haben keine Berechtigung, dieses Mitglied zu löschen" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "created" +msgstr "erstellt" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "updated" +msgstr "aktualisiert" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "Unknown error" +msgstr "Unbekannter Fehler" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index f333e2a..3fdf543 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -2022,3 +2022,78 @@ msgstr "" #, elixir-autogen, elixir-format msgid "You do not have permission to delete this member" msgstr "" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format +msgid "Custom field value deleted successfully" +msgstr "" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format +msgid "Custom field value not found" +msgstr "" + +#: lib/mv_web/live/membership_fee_type_live/index.ex +#, elixir-autogen, elixir-format +msgid "Membership fee type not found" +msgstr "" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "User %{action} successfully" +msgstr "" + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format +msgid "User deleted successfully" +msgstr "" + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format +msgid "User not found" +msgstr "" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format +msgid "You do not have permission to access this custom field value" +msgstr "" + +#: lib/mv_web/live/membership_fee_type_live/index.ex +#, elixir-autogen, elixir-format +msgid "You do not have permission to access this membership fee type" +msgstr "" + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format +msgid "You do not have permission to access this user" +msgstr "" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format +msgid "You do not have permission to delete this custom field value" +msgstr "" + +#: lib/mv_web/live/membership_fee_type_live/index.ex +#, elixir-autogen, elixir-format +msgid "You do not have permission to delete this membership fee type" +msgstr "" + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format +msgid "You do not have permission to delete this user" +msgstr "" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "created" +msgstr "" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "updated" +msgstr "" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "Unknown error" +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index fdb17ca..968f9d2 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2022,3 +2022,78 @@ msgstr "You do not have permission to access this member" #, elixir-autogen, elixir-format msgid "You do not have permission to delete this member" msgstr "You do not have permission to delete this member" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "Custom field value deleted successfully" +msgstr "" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "Custom field value not found" +msgstr "" + +#: lib/mv_web/live/membership_fee_type_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "Membership fee type not found" +msgstr "" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "User %{action} successfully" +msgstr "" + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "User deleted successfully" +msgstr "" + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "User not found" +msgstr "Member not found" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to access this custom field value" +msgstr "You do not have permission to access this member" + +#: lib/mv_web/live/membership_fee_type_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to access this membership fee type" +msgstr "You do not have permission to access this member" + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to access this user" +msgstr "You do not have permission to access this member" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to delete this custom field value" +msgstr "You do not have permission to delete this member" + +#: lib/mv_web/live/membership_fee_type_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to delete this membership fee type" +msgstr "You do not have permission to delete this member" + +#: lib/mv_web/live/user_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to delete this user" +msgstr "You do not have permission to delete this member" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "created" +msgstr "created" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "updated" +msgstr "updated" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "Unknown error" +msgstr "" From 4aa429e3cb3ea64e0e3027d8dd9e9b00cede0ede Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:05:53 +0100 Subject: [PATCH 62/98] ci: Add check for empty German translations in lint task - Check that all German msgstr entries are filled (excluding header) - Use awk to filter out header msgstr "" entries - Fail lint if any empty translations are found --- Justfile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Justfile b/Justfile index b835cf4..c68c473 100644 --- a/Justfile +++ b/Justfile @@ -32,6 +32,8 @@ lint: mix format --check-formatted mix compile --warnings-as-errors mix credo + # Check that all German translations are filled (UI must be in German) + @bash -c 'for file in priv/gettext/de/LC_MESSAGES/*.po; do awk "/^msgid \"\"$/{header=1; next} /^msgid /{header=0} /^msgstr \"\"$/ && !header{print FILENAME\":\"NR\": \" \$0; exit 1}" "$file" || exit 1; done' mix gettext.extract --check-up-to-date audit: @@ -116,4 +118,4 @@ init-prod-secrets: # Start production environment with Docker Compose start-prod: init-prod-secrets - docker compose -f docker-compose.prod.yml up -d \ No newline at end of file + docker compose -f docker-compose.prod.yml up -d From c1b0336f018c296d5f9046616468e860b9712b5e Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:57:37 +0100 Subject: [PATCH 63/98] fix: Correct Language headers in German .po files --- priv/gettext/de/LC_MESSAGES/default.po | 19 ++++++++++++------- priv/gettext/de/LC_MESSAGES/errors.po | 2 +- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 7177eb1..1c3dfee 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -8,7 +8,7 @@ ## to merge POT files into PO files. msgid "" msgstr "" -"Language: en\n" +"Language: de\n" #: lib/mv_web/components/core_components.ex #: lib/mv_web/live/contribution_period_live/show.ex @@ -1726,9 +1726,9 @@ msgid "Create" msgstr "erstellt" #: lib/mv_web/live/member_live/show/membership_fees_component.ex -#, elixir-autogen, elixir-format, fuzzy +#, elixir-autogen, elixir-format msgid "Create Cycle" -msgstr "Aktueller Zyklus" +msgstr "Zyklus erstellen" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format @@ -1736,14 +1736,14 @@ msgid "Create a new cycle manually" msgstr "Einen neuen Zyklus manuell erstellen" #: lib/mv_web/live/member_live/show/membership_fees_component.ex -#, elixir-autogen, elixir-format, fuzzy +#, elixir-autogen, elixir-format msgid "Cycle Period" -msgstr "Zyklus" +msgstr "Zykluszeitraum" #: lib/mv_web/live/member_live/show/membership_fees_component.ex -#, elixir-autogen, elixir-format, fuzzy +#, elixir-autogen, elixir-format msgid "Cycle created successfully" -msgstr "Zyklen erfolgreich regeneriert" +msgstr "Zyklus erfolgreich erstellt" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format, fuzzy @@ -2096,3 +2096,8 @@ msgstr "aktualisiert" #, elixir-autogen, elixir-format msgid "Unknown error" msgstr "Unbekannter Fehler" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format +msgid "You do not have permission to view custom field values" +msgstr "Sie haben keine Berechtigung, benutzerdefinierte Feldwerte anzuzeigen" diff --git a/priv/gettext/de/LC_MESSAGES/errors.po b/priv/gettext/de/LC_MESSAGES/errors.po index 92d3048..b1d359a 100644 --- a/priv/gettext/de/LC_MESSAGES/errors.po +++ b/priv/gettext/de/LC_MESSAGES/errors.po @@ -8,7 +8,7 @@ ## to merge POT files into PO files. msgid "" msgstr "" -"Language: en\n" +"Language: de\n" ## From Ecto.Changeset.cast/4 msgid "can't be blank" From a6fd5e1c1e19add58e428140c4df0fce22194de0 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:57:39 +0100 Subject: [PATCH 64/98] fix: Replace Ash.read! with error handling in CustomFieldValueLive.Index - Replace Ash.read! with Ash.read and proper error handling in mount/3 --- .../live/custom_field_value_live/index.ex | 33 ++++++++++++++++--- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/lib/mv_web/live/custom_field_value_live/index.ex b/lib/mv_web/live/custom_field_value_live/index.ex index 0847cd6..eea578e 100644 --- a/lib/mv_web/live/custom_field_value_live/index.ex +++ b/lib/mv_web/live/custom_field_value_live/index.ex @@ -75,10 +75,35 @@ defmodule MvWeb.CustomFieldValueLive.Index do def mount(_params, _session, socket) do actor = current_actor(socket) - {:ok, - socket - |> assign(:page_title, "Listing Custom field values") - |> stream(:custom_field_values, Ash.read!(Mv.Membership.CustomFieldValue, actor: actor))} + # Early return if no actor (prevents exceptions in unauthenticated tests) + if is_nil(actor) do + {:ok, + socket + |> assign(:page_title, "Listing Custom field values") + |> stream(:custom_field_values, [])} + else + case Ash.read(Mv.Membership.CustomFieldValue, actor: actor) do + {:ok, custom_field_values} -> + {:ok, + socket + |> assign(:page_title, "Listing Custom field values") + |> stream(:custom_field_values, custom_field_values)} + + {:error, %Ash.Error.Forbidden{}} -> + {:ok, + socket + |> assign(:page_title, "Listing Custom field values") + |> stream(:custom_field_values, []) + |> put_flash(:error, gettext("You do not have permission to view custom field values"))} + + {:error, error} -> + {:ok, + socket + |> assign(:page_title, "Listing Custom field values") + |> stream(:custom_field_values, []) + |> put_flash(:error, format_error(error))} + end + end end @impl true From f18980e80022aec6a7945cf34bc644996b50e916 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:57:42 +0100 Subject: [PATCH 65/98] refactor: Reduce nesting depth in UserLive.Form.load_members_for_linking --- lib/mv_web/live/user_live/form.ex | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/lib/mv_web/live/user_live/form.ex b/lib/mv_web/live/user_live/form.ex index 1cc6f6a..7b02053 100644 --- a/lib/mv_web/live/user_live/form.ex +++ b/lib/mv_web/live/user_live/form.ex @@ -598,20 +598,25 @@ defmodule MvWeb.UserLive.Form do actor = current_actor(socket) - case Ash.read(query, domain: Mv.Membership, actor: actor) do - {:ok, members} -> - # Apply email match filter if user_email is provided - if user_email_str do - Mv.Membership.Member.filter_by_email_match(members, user_email_str) - else - members - end - - {:error, _} -> - [] + # Early return if no actor (prevents exceptions in unauthenticated tests) + if is_nil(actor) do + [] + else + case Ash.read(query, domain: Mv.Membership, actor: actor) do + {:ok, members} -> apply_email_filter(members, user_email_str) + {:error, _} -> [] + end end end + @spec apply_email_filter([Mv.Membership.Member.t()], String.t() | nil) :: + [Mv.Membership.Member.t()] + defp apply_email_filter(members, nil), do: members + + defp apply_email_filter(members, user_email_str) when is_binary(user_email_str) do + Mv.Membership.Member.filter_by_email_match(members, user_email_str) + end + # Extract user-friendly error message from Ash.Error @spec extract_error_message(any()) :: String.t() defp extract_error_message(%Ash.Error.Invalid{errors: errors}) when is_list(errors) do From 7cff550749c20e49fb728de4d7d9253af4dbe71d Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 13 Jan 2026 14:57:44 +0100 Subject: [PATCH 66/98] i18n: Update POT and English translations --- priv/gettext/default.pot | 5 +++++ priv/gettext/en/LC_MESSAGES/default.po | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 3fdf543..91f7bfb 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -2097,3 +2097,8 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Unknown error" msgstr "" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format +msgid "You do not have permission to view custom field values" +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 968f9d2..e2ab73d 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2097,3 +2097,8 @@ msgstr "updated" #, elixir-autogen, elixir-format msgid "Unknown error" msgstr "" + +#: lib/mv_web/live/custom_field_value_live/index.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "You do not have permission to view custom field values" +msgstr "You do not have permission to delete this member" From 93190d558fb246b8c474334c0bb9042a04dabdfa Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 21:03:17 +0100 Subject: [PATCH 67/98] 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 68/98] 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 69/98] 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 70/98] 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 71/98] 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 72/98] 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 73/98] 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 74/98] 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 75/98] 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 76/98] 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 77/98] 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 78/98] 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 79/98] 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 80/98] 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 81/98] 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 82/98] 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