From 21dbdbe366c4d264d0102dd6f673f874e8cd8f1d Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 29 Jan 2026 13:53:55 +0100 Subject: [PATCH] fix: CustomField policies, no system-actor fallback, guidelines - Tests and UI pass actor for CustomField create/read/destroy; seeds use actor - Member required-custom-fields validation uses context.actor only (no fallback) - CODE_GUIDELINES: add rule forbidding system-actor fallbacks --- CODE_GUIDELINES.md | 31 ++++++++++++++++++ lib/membership/member.ex | 7 ++-- lib/membership/membership.ex | 18 ++++++++--- .../live/custom_field_live/index_component.ex | 23 ++++++++++--- lib/mv_web/live/global_settings_live.ex | 1 + lib/mv_web/live/member_live/form.ex | 2 +- priv/repo/seeds.exs | 6 ++-- .../member_filter_component_test.exs | 12 ++++--- .../live/custom_field_live/deletion_test.exs | 32 +++++++++++-------- test/mv_web/member_live/index_test.exs | 27 +++++++++------- 10 files changed, 116 insertions(+), 43 deletions(-) diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 28c454b..eb3e4ed 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -683,6 +683,13 @@ end - Falls back to admin user from seeds if system user doesn't exist - Should NEVER be used for user-initiated actions (only systemic operations) +**DO NOT use system actor as a fallback:** + +- **Never** fall back to `Mv.Helpers.SystemActor.get_system_actor()` when an actor is missing or nil (e.g. in validations, changes, or when reading from context). +- Fallbacks hide bugs (callers forget to pass actor) and can cause privilege escalation (unauthenticated or low-privilege paths run with system rights). +- If no actor is available, fail explicitly (validation error, Forbidden, or clear error message). Fix the caller to pass the correct actor instead of adding a fallback. +- Use system actor only where the operation is **explicitly** a systemic operation (see list above); never as a "safety net" when actor is absent. + **User Mode vs System Mode:** - **User Mode**: User-initiated actions use the actual user actor, policies are enforced @@ -1658,6 +1665,30 @@ end ## 5. Security Guidelines +### 5.0 No system-actor fallbacks (mandatory) + +**Do not use the system actor as a fallback when an actor is missing.** + +Examples of forbidden patterns: + +```elixir +# ❌ FORBIDDEN - Fallback to system actor when actor is nil +actor = Map.get(changeset.context, :actor) || Mv.Helpers.SystemActor.get_system_actor() + +# ❌ FORBIDDEN - "Safety" fallback in validations, changes, or helpers +actor = opts[:actor] || Mv.Helpers.SystemActor.get_system_actor() + +# ❌ FORBIDDEN - Default actor in function options +def list_something(opts \\ []) do + actor = Keyword.get(opts, :actor) || Mv.Helpers.SystemActor.get_system_actor() + # ... +end +``` + +**Why:** Fallbacks hide missing-actor bugs and can lead to privilege escalation (e.g. a request without actor would run with system privileges). Always require the caller to pass the actor for user-facing or context-dependent operations; if none is available, return an error or fail validation instead of using the system actor. + +**Allowed:** Use the system actor only where the operation is **by design** a systemic operation (e.g. email sync, seeds, test fixtures, background jobs) and you explicitly call `SystemActor.get_system_actor()` at that call site—never as a fallback when `actor` is nil or absent. + ### 5.1 Authentication & Authorization **Use AshAuthentication:** diff --git a/lib/membership/member.ex b/lib/membership/member.ex index da61146..6de7c77 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -471,11 +471,12 @@ defmodule Mv.Membership.Member do end end - # Validate required custom fields - validate fn changeset, _ -> + # Validate required custom fields (actor from validation context only; no fallback) + validate fn changeset, context -> provided_values = provided_custom_field_values(changeset) + actor = context.actor - case Mv.Membership.list_required_custom_fields() do + case Mv.Membership.list_required_custom_fields(actor: actor) do {:ok, required_custom_fields} -> missing_fields = missing_required_fields(required_custom_fields, provided_values) diff --git a/lib/membership/membership.ex b/lib/membership/membership.ex index 47d2f0e..3f84194 100644 --- a/lib/membership/membership.ex +++ b/lib/membership/membership.ex @@ -155,23 +155,31 @@ defmodule Mv.Membership do Lists only required custom fields. This is an optimized version that filters at the database level instead of - loading all custom fields and filtering in memory. + loading all custom fields and filtering in memory. Requires an actor for + authorization (CustomField read policy). + + ## Options + + - `:actor` - Required. The actor for authorization (e.g. current user). + All roles can read CustomField; actor must have a valid role. ## Returns - `{:ok, required_custom_fields}` - List of required custom fields - - `{:error, error}` - Error reading custom fields + - `{:error, error}` - Error reading custom fields (e.g. Forbidden when no actor) ## Examples - iex> {:ok, required_fields} = Mv.Membership.list_required_custom_fields() + iex> {:ok, required_fields} = Mv.Membership.list_required_custom_fields(actor: actor) iex> Enum.all?(required_fields, & &1.required) true """ - def list_required_custom_fields do + def list_required_custom_fields(opts \\ []) do + actor = Keyword.get(opts, :actor) + Mv.Membership.CustomField |> Ash.Query.filter(expr(required == true)) - |> Ash.read(domain: __MODULE__) + |> Ash.read(domain: __MODULE__, actor: actor) end @doc """ diff --git a/lib/mv_web/live/custom_field_live/index_component.ex b/lib/mv_web/live/custom_field_live/index_component.ex index 5f26f78..ddde324 100644 --- a/lib/mv_web/live/custom_field_live/index_component.ex +++ b/lib/mv_web/live/custom_field_live/index_component.ex @@ -176,6 +176,13 @@ defmodule MvWeb.CustomFieldLive.IndexComponent do """ end + defp stream_custom_fields(actor) do + case Ash.read(Mv.Membership.CustomField, actor: actor) do + {:ok, custom_fields} -> custom_fields + {:error, _} -> [] + end + end + @impl true def update(assigns, socket) do # Track previous show_form state to detect when form is closed @@ -207,7 +214,7 @@ defmodule MvWeb.CustomFieldLive.IndexComponent do |> assign_new(:show_delete_modal, fn -> false end) |> assign_new(:custom_field_to_delete, fn -> nil end) |> assign_new(:slug_confirmation, fn -> "" end) - |> stream(:custom_fields, Ash.read!(Mv.Membership.CustomField), reset: true)} + |> stream(:custom_fields, stream_custom_fields(assigns[:actor]), reset: true)} end @impl true @@ -226,7 +233,8 @@ defmodule MvWeb.CustomFieldLive.IndexComponent do @impl true def handle_event("edit_custom_field", %{"id" => id}, socket) do - custom_field = Ash.get!(Mv.Membership.CustomField, id) + actor = socket.assigns[:actor] + custom_field = Ash.get!(Mv.Membership.CustomField, id, actor: actor) # Only send event if form was not already open if not socket.assigns[:show_form] do @@ -242,7 +250,13 @@ defmodule MvWeb.CustomFieldLive.IndexComponent do @impl true def handle_event("prepare_delete", %{"id" => id}, socket) do - custom_field = Ash.get!(Mv.Membership.CustomField, id, load: [:assigned_members_count]) + actor = socket.assigns[:actor] + + custom_field = + Ash.get!(Mv.Membership.CustomField, id, + load: [:assigned_members_count], + actor: actor + ) {:noreply, socket @@ -259,9 +273,10 @@ defmodule MvWeb.CustomFieldLive.IndexComponent do @impl true def handle_event("confirm_delete", _params, socket) do custom_field = socket.assigns.custom_field_to_delete + actor = socket.assigns[:actor] if socket.assigns.slug_confirmation == custom_field.slug do - case Ash.destroy(custom_field) do + case Ash.destroy(custom_field, actor: actor) do :ok -> send(self(), {:custom_field_deleted, custom_field}) diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index 0fbcbbe..7b29cae 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -130,6 +130,7 @@ defmodule MvWeb.GlobalSettingsLive do :if={@active_editing_section != :member_fields} module={MvWeb.CustomFieldLive.IndexComponent} id="custom-fields-component" + actor={@current_user} /> diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index 2f3a0ff..b72add6 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -226,7 +226,7 @@ defmodule MvWeb.MemberLive.Form do def mount(params, _session, socket) do # current_user should be set by on_mount hooks (LiveUserAuth + LiveHelpers) actor = current_actor(socket) - {:ok, custom_fields} = Mv.Membership.list_custom_fields() + {:ok, custom_fields} = Mv.Membership.list_custom_fields(actor: actor) initial_custom_field_values = Enum.map(custom_fields, fn cf -> diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index 43ffcaf..71ae3ff 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -118,10 +118,12 @@ for attrs <- [ required: false } ] do + # Bootstrap: no admin user yet; CustomField create requires admin, so skip authorization Membership.create_custom_field!( attrs, upsert?: true, - upsert_identity: :unique_name + upsert_identity: :unique_name, + authorize?: false ) end @@ -594,7 +596,7 @@ end) # Create sample custom field values for some members all_members = Ash.read!(Membership.Member, actor: admin_user_with_role) -all_custom_fields = Ash.read!(Membership.CustomField) +all_custom_fields = Ash.read!(Membership.CustomField, actor: admin_user_with_role) # Helper function to find custom field by name find_field = fn name -> Enum.find(all_custom_fields, &(&1.name == name)) end diff --git a/test/mv_web/components/member_filter_component_test.exs b/test/mv_web/components/member_filter_component_test.exs index a3a3846..485475a 100644 --- a/test/mv_web/components/member_filter_component_test.exs +++ b/test/mv_web/components/member_filter_component_test.exs @@ -16,8 +16,10 @@ defmodule MvWeb.Components.MemberFilterComponentTest do alias Mv.Membership.CustomField - # Helper to create a boolean custom field + # Helper to create a boolean custom field (uses system_actor - only admin can create) defp create_boolean_custom_field(attrs \\ %{}) do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + default_attrs = %{ name: "test_boolean_#{System.unique_integer([:positive])}", value_type: :boolean @@ -27,11 +29,13 @@ defmodule MvWeb.Components.MemberFilterComponentTest do CustomField |> Ash.Changeset.for_create(:create, attrs) - |> Ash.create!() + |> Ash.create!(actor: system_actor) end - # Helper to create a non-boolean custom field + # Helper to create a non-boolean custom field (uses system_actor - only admin can create) defp create_string_custom_field(attrs \\ %{}) do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + default_attrs = %{ name: "test_string_#{System.unique_integer([:positive])}", value_type: :string @@ -41,7 +45,7 @@ defmodule MvWeb.Components.MemberFilterComponentTest do CustomField |> Ash.Changeset.for_create(:create, attrs) - |> Ash.create!() + |> Ash.create!(actor: system_actor) end describe "rendering" do diff --git a/test/mv_web/live/custom_field_live/deletion_test.exs b/test/mv_web/live/custom_field_live/deletion_test.exs index 9610b24..2490601 100644 --- a/test/mv_web/live/custom_field_live/deletion_test.exs +++ b/test/mv_web/live/custom_field_live/deletion_test.exs @@ -20,8 +20,9 @@ defmodule MvWeb.CustomFieldLive.DeletionTest do setup do system_actor = Mv.Helpers.SystemActor.get_system_actor() + admin_role = Mv.Fixtures.role_fixture("admin") - # Create admin user for testing + # Create admin user for testing (must have admin role to read/manage CustomField) {:ok, user} = Mv.Accounts.User |> Ash.Changeset.for_create(:register_with_password, %{ @@ -30,8 +31,17 @@ defmodule MvWeb.CustomFieldLive.DeletionTest do }) |> Ash.create(actor: system_actor) - conn = log_in_user(build_conn(), user) - %{conn: conn, user: user} + {:ok, user} = + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) + |> Ash.update(actor: system_actor) + + user_with_role = Ash.load!(user, :role, domain: Mv.Accounts, actor: system_actor) + conn = log_in_user(build_conn(), user_with_role) + # Use English locale so "Delete" link text matches in assertions + conn = Plug.Test.init_test_session(conn, Map.put(conn.private.plug_session, "locale", "en")) + %{conn: conn, user: user_with_role} end describe "delete button and modal" do @@ -107,9 +117,8 @@ defmodule MvWeb.CustomFieldLive.DeletionTest do |> element("#delete-custom-field-modal form") |> render_change(%{"slug" => custom_field.slug}) - # Confirm button should be enabled now (no disabled attribute) - html = render(view) - refute html =~ ~r/disabled(?:=""|(?!\w))/ + # Confirm button should be enabled now (no disabled attribute on the confirm button) + refute has_element?(view, "#delete-custom-field-modal button.btn-error[disabled]") end test "delete button is disabled when slug doesn't match", %{conn: conn} do @@ -126,9 +135,8 @@ defmodule MvWeb.CustomFieldLive.DeletionTest do |> element("#delete-custom-field-modal form") |> render_change(%{"slug" => "wrong-slug"}) - # Button should be disabled - html = render(view) - assert html =~ ~r/disabled(?:=""|(?!\w))/ + # Confirm button should be disabled + assert has_element?(view, "#delete-custom-field-modal button.btn-error[disabled]") end end @@ -186,10 +194,8 @@ defmodule MvWeb.CustomFieldLive.DeletionTest do |> element("#delete-custom-field-modal form") |> render_change(%{"slug" => "wrong-slug"}) - # Button should be disabled and we cannot click it - # The test verifies that the button is properly disabled in the UI - html = render(view) - assert html =~ ~r/disabled(?:=""|(?!\w))/ + # Confirm button should be disabled and we cannot click it + assert has_element?(view, "#delete-custom-field-modal button.btn-error[disabled]") # Custom field should still exist since deletion couldn't proceed system_actor = Mv.Helpers.SystemActor.get_system_actor() diff --git a/test/mv_web/member_live/index_test.exs b/test/mv_web/member_live/index_test.exs index 0624c77..7d14a7f 100644 --- a/test/mv_web/member_live/index_test.exs +++ b/test/mv_web/member_live/index_test.exs @@ -750,8 +750,10 @@ defmodule MvWeb.MemberLive.IndexTest do describe "boolean custom field filters" do alias Mv.Membership.CustomField - # Helper to create a boolean custom field + # Helper to create a boolean custom field (uses system actor for authorization) defp create_boolean_custom_field(attrs \\ %{}) do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + default_attrs = %{ name: "test_boolean_#{System.unique_integer([:positive])}", value_type: :boolean @@ -761,11 +763,13 @@ defmodule MvWeb.MemberLive.IndexTest do CustomField |> Ash.Changeset.for_create(:create, attrs) - |> Ash.create!() + |> Ash.create!(actor: system_actor) end - # Helper to create a non-boolean custom field + # Helper to create a non-boolean custom field (uses system actor for authorization) defp create_string_custom_field(attrs \\ %{}) do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + default_attrs = %{ name: "test_string_#{System.unique_integer([:positive])}", value_type: :string @@ -775,7 +779,7 @@ defmodule MvWeb.MemberLive.IndexTest do CustomField |> Ash.Changeset.for_create(:create, attrs) - |> Ash.create!() + |> Ash.create!(actor: system_actor) end test "mount initializes boolean_custom_field_filters as empty map", %{conn: conn} do @@ -1016,6 +1020,7 @@ defmodule MvWeb.MemberLive.IndexTest do test "handle_params removes filter when custom field is deleted", %{conn: conn} do conn = conn_with_oidc_user(conn) + system_actor = Mv.Helpers.SystemActor.get_system_actor() boolean_field = create_boolean_custom_field() # Set up filter via URL @@ -1026,8 +1031,8 @@ defmodule MvWeb.MemberLive.IndexTest do filters_before = state_before.socket.assigns.boolean_custom_field_filters assert filters_before[boolean_field.id] == true - # Delete the custom field - Ash.destroy!(boolean_field) + # Delete the custom field (requires actor with destroy permission) + Ash.destroy!(boolean_field, actor: system_actor) # Navigate again - filter should be removed since custom field no longer exists {:ok, view2, _html} = @@ -1328,7 +1333,7 @@ defmodule MvWeb.MemberLive.IndexTest do members = [member_with_true, member_with_false, member_without_value] filters = %{to_string(boolean_field.id) => true} - all_custom_fields = Mv.Membership.CustomField |> Ash.read!() + all_custom_fields = Mv.Membership.CustomField |> Ash.read!(actor: system_actor) result = MvWeb.MemberLive.Index.apply_boolean_custom_field_filters( @@ -1378,7 +1383,7 @@ defmodule MvWeb.MemberLive.IndexTest do members = [member_with_true, member_with_false, member_without_value] filters = %{to_string(boolean_field.id) => false} - all_custom_fields = Mv.Membership.CustomField |> Ash.read!() + all_custom_fields = Mv.Membership.CustomField |> Ash.read!(actor: system_actor) result = MvWeb.MemberLive.Index.apply_boolean_custom_field_filters( @@ -1417,7 +1422,7 @@ defmodule MvWeb.MemberLive.IndexTest do members = [member1, member2] filters = %{} - all_custom_fields = Mv.Membership.CustomField |> Ash.read!() + all_custom_fields = Mv.Membership.CustomField |> Ash.read!(actor: system_actor) result = MvWeb.MemberLive.Index.apply_boolean_custom_field_filters( @@ -1507,7 +1512,7 @@ defmodule MvWeb.MemberLive.IndexTest do to_string(boolean_field2.id) => true } - all_custom_fields = Mv.Membership.CustomField |> Ash.read!() + all_custom_fields = Mv.Membership.CustomField |> Ash.read!(actor: system_actor) result = MvWeb.MemberLive.Index.apply_boolean_custom_field_filters( @@ -1538,7 +1543,7 @@ defmodule MvWeb.MemberLive.IndexTest do members = [member] filters = %{fake_id => true} - all_custom_fields = Mv.Membership.CustomField |> Ash.read!() + all_custom_fields = Mv.Membership.CustomField |> Ash.read!(actor: system_actor) result = MvWeb.MemberLive.Index.apply_boolean_custom_field_filters(