diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 5bee497..987be42 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -690,16 +690,9 @@ end **Authorization Bootstrap Patterns:** -Three mechanisms exist for bypassing standard authorization: +Two mechanisms exist for bypassing standard authorization: -1. **NoActor** (test only) - Allows operations without actor in test environment - ```elixir - # Automatically enabled in tests via config/test.exs - # Policies use: bypass action_type(...) do authorize_if NoActor end - member = create_member(%{name: "Test"}) # Works in tests - ``` - -2. **system_actor** (systemic operations) - Admin user for operations that must always succeed +1. **system_actor** (systemic operations) - Admin user for operations that must always succeed ```elixir # Good: Systemic operation system_actor = SystemActor.get_system_actor() @@ -709,7 +702,7 @@ Three mechanisms exist for bypassing standard authorization: # Never use system_actor for user-initiated actions! ``` -3. **authorize?: false** (bootstrap only) - Skips policies for circular dependencies +2. **authorize?: false** (bootstrap only) - Skips policies for circular dependencies ```elixir # Good: Bootstrap (seeds, SystemActor loading) Accounts.create_user!(%{email: admin_email}, authorize?: false) @@ -719,10 +712,10 @@ Three mechanisms exist for bypassing standard authorization: ``` **Decision Guide:** -- Use **NoActor** for test fixtures (automatic via config) -- Use **system_actor** for email sync, cycle generation, validations +- Use **system_actor** for email sync, cycle generation, validations, and test fixtures - Use **authorize?: false** only for bootstrap (seeds, circular dependencies) - Always document why `authorize?: false` is necessary +- **Note:** NoActor bypass was removed to prevent masking authorization bugs in tests **See also:** `docs/roles-and-permissions-architecture.md` (Authorization Bootstrap Patterns section) @@ -1702,65 +1695,54 @@ case Ash.read(Mv.Membership.Member, actor: actor) do end ``` -### 5.1a NoActor Pattern - Test Environment Only +### 5.1a Authorization in Tests -**IMPORTANT:** The `Mv.Authorization.Checks.NoActor` check is **ONLY for test environment**. It must NEVER be used in production. +**IMPORTANT:** All tests must explicitly provide an actor for Ash operations. The NoActor bypass has been removed to prevent masking authorization bugs. -**What NoActor Does:** +**Test Fixtures:** -- Allows CRUD operations without an actor in **test environment only** -- Denies all operations without an actor in **production/dev** (fail-closed) -- Uses compile-time config check to prevent accidental production use (release-safe) - -**Security Guards:** +All test fixtures use `system_actor` for authorization: ```elixir -# config/test.exs -config :mv, :allow_no_actor_bypass, true - -# lib/mv/authorization/checks/no_actor.ex -# Compile-time check from config (release-safe, no Mix.env) -@allow_no_actor_bypass Application.compile_env(:mv, :allow_no_actor_bypass, false) - -# Uses compile-time flag only (no runtime Mix.env needed) -def match?(nil, _context, _opts) do - @allow_no_actor_bypass # true in test, false in prod/dev +# test/support/fixtures.ex +def member_fixture(attrs \\ %{}) do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + + attrs + |> Enum.into(%{...}) + |> Mv.Membership.create_member(actor: system_actor) end ``` -**Why This Pattern Exists:** +**Why Explicit Actors in Tests:** -- Test fixtures often need to create resources without an actor -- Production operations MUST always have an actor for security -- Config-based guard (not Mix.env) ensures release-safety -- Defaults to `false` (fail-closed) if config not set +- Prevents masking authorization bugs +- Makes authorization requirements explicit +- Tests fail if authorization is broken (fail-fast) +- Consistent with production code patterns -**NEVER Use NoActor in Production:** +**Using system_actor in Tests:** ```elixir -# ❌ BAD - Don't do this in production code -Ash.create!(Member, attrs) # No actor - will fail in prod - -# ✅ GOOD - Use admin actor for system operations -admin_user = get_admin_user() -Ash.create!(Member, attrs, actor: admin_user) -``` - -**Alternative: System Actor Pattern** - -For production system operations, use the System Actor Pattern (see Section 3.3) instead of NoActor: - -```elixir -# System operations in production -system_actor = get_system_actor() +# ✅ GOOD - Explicit actor in tests +system_actor = Mv.Helpers.SystemActor.get_system_actor() Ash.create!(Member, attrs, actor: system_actor) + +# ❌ BAD - Missing actor (will fail) +Ash.create!(Member, attrs) # Forbidden error! ``` -**Testing:** +**For Bootstrap Operations:** -- NoActor tests verify the compile-time config guard -- Production safety is guaranteed by config (only set in test.exs, defaults to false) -- See `test/mv/authorization/checks/no_actor_test.exs` +Use `authorize?: false` only for bootstrap scenarios (seeds, SystemActor initialization): + +```elixir +# ✅ GOOD - Bootstrap only +Accounts.create_user!(%{email: admin_email}, authorize?: false) + +# ❌ BAD - Never use in tests for normal operations +Ash.create!(Member, attrs, authorize?: false) # Never do this! +``` ### 5.2 Password Security diff --git a/config/test.exs b/config/test.exs index b48c408..b47c764 100644 --- a/config/test.exs +++ b/config/test.exs @@ -12,7 +12,10 @@ config :mv, Mv.Repo, port: System.get_env("TEST_POSTGRES_PORT", "5000"), database: "mv_test#{System.get_env("MIX_TEST_PARTITION")}", pool: Ecto.Adapters.SQL.Sandbox, - pool_size: System.schedulers_online() * 4 + pool_size: System.schedulers_online() * 8, + queue_target: 5000, + queue_interval: 1000, + timeout: 30_000 # We don't run a server during test. If one is required, # you can enable the server option below. diff --git a/docs/policy-bypass-vs-haspermission.md b/docs/policy-bypass-vs-haspermission.md index 8a65c6f..31bb737 100644 --- a/docs/policy-bypass-vs-haspermission.md +++ b/docs/policy-bypass-vs-haspermission.md @@ -262,7 +262,7 @@ The bypass is not a design choice but a **technical necessity** due to Ash's pol - ✅ UPDATE operations via HasPermission with `scope :own` - ✅ Admin operations via HasPermission with `scope :all` - ✅ AshAuthentication bypass (registration/login) -- ✅ NoActor bypass (test environment) +- ✅ Tests use system_actor for authorization **Key Tests Proving Pattern:** diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md index bc1b75c..8934688 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -946,12 +946,7 @@ defmodule Mv.Accounts.User do authorize_if always() end - # 2. NoActor Bypass (test environment only, for test fixtures) - bypass action_type([:create, :read, :update, :destroy]) do - authorize_if Mv.Authorization.Checks.NoActor - end - - # 3. SPECIAL CASE: Users can always READ their own account + # 2. SPECIAL CASE: Users can always READ their own account # Bypass needed for list queries (expr() triggers auto_filter in Ash) # UPDATE is handled by HasPermission below (scope :own works with changesets) bypass action_type(:read) do @@ -959,7 +954,7 @@ defmodule Mv.Accounts.User do authorize_if expr(id == ^actor(:id)) end - # 4. GENERAL: Check permissions from user's role + # 3. GENERAL: Check permissions from user's role # - :own_data → can UPDATE own user (scope :own via HasPermission) # - :read_only → can UPDATE own user (scope :own via HasPermission) # - :normal_user → can UPDATE own user (scope :own via HasPermission) @@ -969,7 +964,7 @@ defmodule Mv.Accounts.User do authorize_if Mv.Authorization.Checks.HasPermission end - # 5. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed) + # 4. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed) end # ... @@ -1007,12 +1002,7 @@ defmodule Mv.Membership.Member do use Ash.Resource, ... policies do - # 1. NoActor Bypass (test environment only, for test fixtures) - bypass action_type([:create, :read, :update, :destroy]) do - authorize_if Mv.Authorization.Checks.NoActor - end - - # 2. SPECIAL CASE: Users can always READ their linked member + # 1. SPECIAL CASE: Users can always READ their linked member # Bypass needed for list queries (expr() triggers auto_filter in Ash) # UPDATE is handled by HasPermission below (scope :linked works with changesets) bypass action_type(:read) do @@ -1020,7 +1010,7 @@ defmodule Mv.Membership.Member do authorize_if expr(id == ^actor(:member_id)) end - # 3. GENERAL: Check permissions from role + # 2. GENERAL: Check permissions from role # - :own_data → can UPDATE linked member (scope :linked via HasPermission) # - :read_only → can READ all members (scope :all), no update permission # - :normal_user → can CRUD all members (scope :all) @@ -2629,45 +2619,16 @@ This section clarifies three different mechanisms for bypassing standard authori ### Overview -The codebase uses three authorization bypass mechanisms: +The codebase uses two authorization bypass mechanisms: -1. **NoActor** - Test-only bypass (compile-time secured) -2. **system_actor** - Admin user for systemic operations -3. **authorize?: false** - Bootstrap bypass for circular dependencies +1. **system_actor** - Admin user for systemic operations +2. **authorize?: false** - Bootstrap bypass for circular dependencies -**All three are necessary and serve different purposes.** +**Both are necessary and serve different purposes.** -### 1. NoActor Check +**Note:** The NoActor bypass has been removed to prevent masking authorization bugs in tests. All tests now explicitly use `system_actor` for authorization. -**Purpose:** Allows CRUD operations without actor in test environment only. - -**Implementation:** -```elixir -# lib/mv/authorization/checks/no_actor.ex -@allow_no_actor_bypass Application.compile_env(:mv, :allow_no_actor_bypass, false) - -def match?(nil, _context, _opts) do - @allow_no_actor_bypass # true in test.exs, false elsewhere -end -``` - -**Security:** -- Compile-time flag (not runtime `Mix.env()` check) -- Default: false (fail-closed) -- Only enabled in `config/test.exs` - -**Use Case:** Test fixtures without verbose actor setup: -```elixir -# With NoActor (test environment only) -member = create_member(%{name: "Test"}) - -# Production behavior (NoActor returns false) -member = create_member(%{name: "Test"}, actor: user) -``` - -**Trade-off:** May mask tests that should fail without actor. Mitigated by explicit policy tests (e.g., `test/mv/accounts/user_policies_test.exs`). - -### 2. System Actor +### 1. System Actor **Purpose:** Admin user for systemic operations that must always succeed regardless of user permissions. @@ -2708,7 +2669,7 @@ end - Consistent authorization flow - Testable -### 3. authorize?: false +### 2. authorize?: false **Purpose:** Skip policies for bootstrap scenarios with circular dependencies. @@ -2759,21 +2720,17 @@ Mv.Authorization.Role ### Comparison -| Aspect | NoActor | system_actor | authorize?: false | -|--------|---------|--------------|-------------------| -| **Environment** | Test only | All | All | -| **Actor** | nil | Admin user | nil | -| **Policies** | Bypassed | Evaluated | Skipped | -| **Audit Trail** | No | Yes (system@mila.local) | No | -| **Use Case** | Test fixtures | Systemic operations | Bootstrap | -| **Explicit?** | Policy bypass | Function call | Query option | +| Aspect | system_actor | authorize?: false | +|--------|--------------|-------------------| +| **Environment** | All | All | +| **Actor** | Admin user | nil | +| **Policies** | Evaluated | Skipped | +| **Audit Trail** | Yes (system@mila.local) | No | +| **Use Case** | Systemic operations, test fixtures | Bootstrap | +| **Explicit?** | Function call | Query option | ### Decision Guide -**Use NoActor when:** -- ✅ Writing test fixtures -- ✅ Compile-time guard ensures test-only - **Use system_actor when:** - ✅ Systemic operation must always succeed - ✅ Email synchronization @@ -2789,7 +2746,7 @@ Mv.Authorization.Role **DON'T:** - ❌ Use `authorize?: false` for user-initiated actions - ❌ Use `authorize?: false` when `system_actor` would work -- ❌ Enable NoActor outside test environment +- ❌ Skip actor in tests (always use system_actor) ### The Circular Dependency Problem @@ -2873,7 +2830,8 @@ end - Enhanced edge case documentation **Changes from V2.0:** -- Added "Authorization Bootstrap Patterns" section explaining NoActor, system_actor, and authorize?: false +- Added "Authorization Bootstrap Patterns" section explaining system_actor and authorize?: false +- Removed NoActor bypass (all tests now use system_actor for explicit authorization) --- diff --git a/docs/roles-and-permissions-implementation-plan.md b/docs/roles-and-permissions-implementation-plan.md index 33b1702..23b045c 100644 --- a/docs/roles-and-permissions-implementation-plan.md +++ b/docs/roles-and-permissions-implementation-plan.md @@ -542,7 +542,7 @@ Following the same pattern as Member resource: 1. ✅ Open `lib/accounts/user.ex` 2. ✅ Add `policies` block 3. ✅ Add AshAuthentication bypass (registration/login without actor) -4. ✅ Add NoActor bypass (test environment only) +4. ✅ ~~Add NoActor bypass (test environment only)~~ **REMOVED** - NoActor bypass was removed to prevent masking authorization bugs. All tests now use `system_actor`. 5. ✅ Add bypass for READ: Allow user to always read their own account ```elixir bypass action_type(:read) do @@ -556,10 +556,11 @@ Following the same pattern as Member resource: **Policy Order:** 1. ✅ AshAuthentication bypass (registration/login) -2. ✅ NoActor bypass (test environment) -3. ✅ Bypass: User can READ own account (id == actor.id) -4. ✅ HasPermission: General permission check (UPDATE uses scope :own, admin uses scope :all) -5. ✅ Default: Ash implicitly forbids (fail-closed) +2. ✅ Bypass: User can READ own account (id == actor.id) +3. ✅ HasPermission: General permission check (UPDATE uses scope :own, admin uses scope :all) +4. ✅ Default: Ash implicitly forbids (fail-closed) + +**Note:** NoActor bypass was removed. All tests now use `system_actor` for authorization. **Why Bypass for READ but not UPDATE?** @@ -574,7 +575,7 @@ This ensures `scope :own` in PermissionSets is actually used (not redundant). - ✅ User can always update own credentials (via HasPermission with scope :own) - ✅ Only admin can read/update other users (scope :all) - ✅ Only admin can destroy users (scope :all) -- ✅ Policy order is correct (AshAuth → NoActor → Bypass READ → HasPermission) +- ✅ Policy order is correct (AshAuth → Bypass READ → HasPermission) - ✅ Actor preloads :role relationship - ✅ All tests pass (30/31 pass, 1 skipped) @@ -584,7 +585,7 @@ This ensures `scope :own` in PermissionSets is actually used (not redundant). - ✅ 31 tests total: 30 passing, 1 skipped (AshAuthentication edge case) - ✅ Tests for all 4 permission sets: own_data, read_only, normal_user, admin - ✅ Tests for AshAuthentication bypass (registration/login) -- ✅ Tests for NoActor bypass (test environment) +- ✅ Tests use system_actor for authorization (NoActor bypass removed) - ✅ Tests verify scope :own is used for UPDATE (not redundant) --- diff --git a/docs/user-resource-policies-implementation-summary.md b/docs/user-resource-policies-implementation-summary.md index c85d3d7..c939c6b 100644 --- a/docs/user-resource-policies-implementation-summary.md +++ b/docs/user-resource-policies-implementation-summary.md @@ -22,18 +22,13 @@ policies do authorize_if always() end - # 2. NoActor Bypass (test environment only) - bypass action_type([:create, :read, :update, :destroy]) do - authorize_if Mv.Authorization.Checks.NoActor - end - - # 3. Bypass for READ (list queries via auto_filter) + # 2. Bypass for READ (list queries via auto_filter) bypass action_type(:read) do description "Users can always read their own account" authorize_if expr(id == ^actor(:id)) end - # 4. HasPermission for all operations (uses scope from PermissionSets) + # 3. HasPermission for all operations (uses scope from PermissionSets) policy action_type([:read, :create, :update, :destroy]) do description "Check permissions from user's role and permission set" authorize_if Mv.Authorization.Checks.HasPermission @@ -51,7 +46,7 @@ end - ✅ CREATE operations (admin only) - ✅ DESTROY operations (admin only) - ✅ AshAuthentication bypass (registration/login) -- ✅ NoActor bypass (test environment) +- ✅ Tests use system_actor for authorization --- @@ -190,7 +185,7 @@ mix test test/mv/accounts/user_policies_test.exs \ **Test Environment:** - ✅ Operations without actor work in test environment -- ✅ NoActor bypass correctly detects compile-time environment +- ✅ All tests explicitly use system_actor for authorization --- diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 08d1130..badbd72 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -275,12 +275,6 @@ defmodule Mv.Accounts.User do authorize_if always() end - # NoActor bypass (test fixtures only, see no_actor.ex) - bypass action_type([:create, :read, :update, :destroy]) do - description "Allow system operations without actor (test environment only)" - authorize_if Mv.Authorization.Checks.NoActor - end - # READ bypass for list queries (scope :own via expr) bypass action_type(:read) do description "Users can always read their own account" diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 650cf43..f2f27c0 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -303,15 +303,6 @@ 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 (TEST ENVIRONMENT ONLY) - # In test: All operations allowed (for test fixtures) - # 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 (test environment only)" - 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 @@ -402,21 +393,11 @@ 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 - # Check if authorization is disabled in the parent operation's context - # Access private context where authorize? flag is stored - authorize? = - case get_in(changeset.context, [:private, :authorize?]) do - false -> false - _ -> true - end - - # Pass actor and authorize? to ensure proper authorization (User might have policies in future) - case Ash.get(Mv.Accounts.User, user_id, actor: actor, authorize?: authorize?) do + # This is an integrity check, not a user authorization check + # Use authorize?: false to bypass policies for this internal validation query + # This ensures the validation always works regardless of actor availability + # (consistent with MembershipFeeType destroy validations) + case Ash.get(Mv.Accounts.User, user_id, authorize?: false) do # User is free to be linked {:ok, %{member_id: nil}} -> :ok @@ -429,6 +410,9 @@ defmodule Mv.Membership.Member do # User is linked to a different member - prevent "stealing" {:error, field: :user, message: "User is already linked to another member"} + {:error, %Ash.Error.Query.NotFound{}} -> + {:error, field: :user, message: "User not found"} + {:error, _} -> {:error, field: :user, message: "User not found"} end diff --git a/lib/membership_fees/membership_fee_type.ex b/lib/membership_fees/membership_fee_type.ex index 01ae625..498ff75 100644 --- a/lib/membership_fees/membership_fee_type.ex +++ b/lib/membership_fees/membership_fee_type.ex @@ -85,10 +85,11 @@ defmodule Mv.MembershipFees.MembershipFeeType do if changeset.action_type == :destroy do require Ash.Query + # Integrity check: count members without authorization (systemic operation) member_count = Mv.Membership.Member |> Ash.Query.filter(membership_fee_type_id == ^changeset.data.id) - |> Ash.count!() + |> Ash.count!(authorize?: false) if member_count > 0 do {:error, @@ -108,10 +109,11 @@ defmodule Mv.MembershipFees.MembershipFeeType do if changeset.action_type == :destroy do require Ash.Query + # Integrity check: count cycles without authorization (systemic operation) cycle_count = Mv.MembershipFees.MembershipFeeCycle |> Ash.Query.filter(membership_fee_type_id == ^changeset.data.id) - |> Ash.count!() + |> Ash.count!(authorize?: false) if cycle_count > 0 do {:error, @@ -131,10 +133,11 @@ defmodule Mv.MembershipFees.MembershipFeeType do if changeset.action_type == :destroy do require Ash.Query + # Integrity check: count settings without authorization (systemic operation) setting_count = Mv.Membership.Setting |> Ash.Query.filter(default_membership_fee_type_id == ^changeset.data.id) - |> Ash.count!() + |> Ash.count!(authorize?: false) if setting_count > 0 do {:error, diff --git a/lib/mv/constants.ex b/lib/mv/constants.ex index 73bfcd9..4ef355d 100644 --- a/lib/mv/constants.ex +++ b/lib/mv/constants.ex @@ -19,6 +19,12 @@ defmodule Mv.Constants do @custom_field_prefix "custom_field_" + @boolean_filter_prefix "bf_" + + @max_boolean_filters 50 + + @max_uuid_length 36 + @email_validator_checks [:html_input, :pow] def member_fields, do: @member_fields @@ -33,6 +39,42 @@ defmodule Mv.Constants do """ def custom_field_prefix, do: @custom_field_prefix + @doc """ + Returns the prefix used for boolean custom field filter URL parameters. + + ## Examples + + iex> Mv.Constants.boolean_filter_prefix() + "bf_" + """ + def boolean_filter_prefix, do: @boolean_filter_prefix + + @doc """ + Returns the maximum number of boolean custom field filters allowed per request. + + This limit prevents DoS attacks by restricting the number of filter parameters + that can be processed in a single request. + + ## Examples + + iex> Mv.Constants.max_boolean_filters() + 50 + """ + def max_boolean_filters, do: @max_boolean_filters + + @doc """ + Returns the maximum length of a UUID string (36 characters including hyphens). + + UUIDs in standard format (e.g., "550e8400-e29b-41d4-a716-446655440000") are + exactly 36 characters long. This constant is used for input validation. + + ## Examples + + iex> Mv.Constants.max_uuid_length() + 36 + """ + def max_uuid_length, do: @max_uuid_length + @doc """ Returns the email validator checks used for EctoCommons.EmailValidator. diff --git a/lib/mv/helpers/system_actor.ex b/lib/mv/helpers/system_actor.ex index 7a8ab8b..565c2ef 100644 --- a/lib/mv/helpers/system_actor.ex +++ b/lib/mv/helpers/system_actor.ex @@ -271,11 +271,12 @@ defmodule Mv.Helpers.SystemActor do end # Finds admin role in existing roles + # SECURITY: Uses authorize?: false for bootstrap role lookup. @spec find_admin_role() :: {:ok, Mv.Authorization.Role.t()} | {:error, :not_found} defp find_admin_role do alias Mv.Authorization - case Authorization.list_roles() do + case Authorization.list_roles(authorize?: false) do {:ok, roles} -> case Enum.find(roles, &(&1.permission_set_name == "admin")) do nil -> {:error, :not_found} @@ -305,16 +306,20 @@ defmodule Mv.Helpers.SystemActor do end # Attempts to create admin role + # SECURITY: Uses authorize?: false for bootstrap role creation. @spec create_admin_role() :: {:ok, Mv.Authorization.Role.t()} | {:error, :already_exists | term()} defp create_admin_role do alias Mv.Authorization - case Authorization.create_role(%{ - name: "Admin", - description: "Administrator with full access", - permission_set_name: "admin" - }) do + case Authorization.create_role( + %{ + name: "Admin", + description: "Administrator with full access", + permission_set_name: "admin" + }, + authorize?: false + ) do {:ok, role} -> {:ok, role} @@ -327,11 +332,12 @@ defmodule Mv.Helpers.SystemActor do end # Finds existing admin role after creation attempt failed due to race condition + # SECURITY: Uses authorize?: false for bootstrap role lookup. @spec find_existing_admin_role() :: Mv.Authorization.Role.t() | no_return() defp find_existing_admin_role do alias Mv.Authorization - case Authorization.list_roles() do + case Authorization.list_roles(authorize?: false) do {:ok, roles} -> Enum.find(roles, &(&1.permission_set_name == "admin")) || raise "Admin role should exist but was not found" @@ -350,14 +356,22 @@ defmodule Mv.Helpers.SystemActor do defp create_system_user_with_role(admin_role) do alias Mv.Accounts + # SECURITY: Uses authorize?: false for bootstrap user creation. + # This is necessary because we're creating the system actor itself, + # which would otherwise be needed for authorization (chicken-and-egg). + # This is safe because: + # 1. Only creates system user with known email + # 2. Only called during system actor initialization (bootstrap) + # 3. Once created, all subsequent operations use proper authorization Accounts.create_user!(%{email: system_user_email_config()}, upsert?: true, - upsert_identity: :unique_email + upsert_identity: :unique_email, + authorize?: false ) |> Ash.Changeset.for_update(:update, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) - |> Ash.update!() - |> Ash.load!(:role, domain: Mv.Accounts) + |> Ash.update!(authorize?: false) + |> Ash.load!(:role, domain: Mv.Accounts, authorize?: false) end # Finds a user by email address @@ -376,9 +390,12 @@ defmodule Mv.Helpers.SystemActor do end # Loads a user with their role preloaded (required for authorization) + # SECURITY: Uses authorize?: false for bootstrap role loading. + # This is necessary because loading the role is part of system actor initialization, + # which would otherwise require an actor (chicken-and-egg). @spec load_user_with_role(Mv.Accounts.User.t()) :: Mv.Accounts.User.t() | no_return() defp load_user_with_role(user) do - case Ash.load(user, :role, domain: Mv.Accounts) do + case Ash.load(user, :role, domain: Mv.Accounts, authorize?: false) do {:ok, user_with_role} -> validate_admin_role(user_with_role) diff --git a/lib/mv/membership/import/member_csv.ex b/lib/mv/membership/import/member_csv.ex index d56c56e..f2e7591 100644 --- a/lib/mv/membership/import/member_csv.ex +++ b/lib/mv/membership/import/member_csv.ex @@ -512,7 +512,10 @@ defmodule Mv.Membership.Import.MemberCSV do member_attrs_with_cf end - case Mv.Membership.create_member(final_attrs) do + # Use system_actor for CSV imports (systemic operation) + system_actor = Mv.Helpers.SystemActor.get_system_actor() + + case Mv.Membership.create_member(final_attrs, actor: system_actor) do {:ok, member} -> {:ok, member} diff --git a/lib/mv_web/live/components/member_filter_component.ex b/lib/mv_web/live/components/member_filter_component.ex new file mode 100644 index 0000000..9286ace --- /dev/null +++ b/lib/mv_web/live/components/member_filter_component.ex @@ -0,0 +1,444 @@ +defmodule MvWeb.Components.MemberFilterComponent do + @moduledoc """ + Provides the MemberFilter Live-Component. + + A DaisyUI dropdown filter for filtering members by payment status and boolean custom fields. + Uses radio inputs in a segmented control pattern (join + btn) for tri-state boolean filters. + + ## Design Decisions + + - Uses `div` panel instead of `ul.menu/li` structure to avoid DaisyUI menu styles + (padding, display, hover, font sizes) that would interfere with form controls. + - Filter controls are form elements (fieldset with legend, radio inputs), not menu items. + Uses semantic `
` and `` for proper accessibility and form structure. + - Dropdown stays open when clicking filter segments to allow multiple filter changes. + - Uses `phx-change` on form for radio inputs instead of individual `phx-click` events. + + ## Props + - `:cycle_status_filter` - Current payment filter state: `nil` (all), `:paid`, or `:unpaid` + - `:boolean_custom_fields` - List of boolean custom fields to display + - `:boolean_filters` - Map of active boolean filters: `%{custom_field_id => true | false}` + - `:id` - Component ID (required) + - `:member_count` - Number of filtered members to display in badge (optional, default: 0) + + ## Events + - Sends `{:payment_filter_changed, filter}` to parent when payment filter changes + - Sends `{:boolean_filter_changed, custom_field_id, filter_value}` to parent when boolean filter changes + """ + use MvWeb, :live_component + + @impl true + def mount(socket) do + {:ok, assign(socket, :open, false)} + end + + @impl true + def update(assigns, socket) do + socket = + socket + |> assign(:id, assigns.id) + |> assign(:cycle_status_filter, assigns[:cycle_status_filter]) + |> assign(:boolean_custom_fields, assigns[:boolean_custom_fields] || []) + |> assign(:boolean_filters, assigns[:boolean_filters] || %{}) + |> assign(:member_count, assigns[:member_count] || 0) + + {:ok, socket} + end + + @impl true + def render(assigns) do + ~H""" +
+ + + + +
+ """ + end + + @impl true + def handle_event("toggle_dropdown", _params, socket) do + {:noreply, assign(socket, :open, !socket.assigns.open)} + end + + @impl true + def handle_event("close_dropdown", _params, socket) do + {:noreply, assign(socket, :open, false)} + end + + @impl true + def handle_event("update_filters", params, socket) do + # Parse payment filter + payment_filter = + case Map.get(params, "payment_filter") do + "paid" -> :paid + "unpaid" -> :unpaid + _ -> nil + end + + # Parse boolean custom field filters (including nil values for "all") + custom_boolean_filters_parsed = + params + |> Map.get("custom_boolean", %{}) + |> Enum.reduce(%{}, fn {custom_field_id_str, value_str}, acc -> + filter_value = parse_tri_state(value_str) + Map.put(acc, custom_field_id_str, filter_value) + end) + + # Update payment filter if changed + if payment_filter != socket.assigns.cycle_status_filter do + send(self(), {:payment_filter_changed, payment_filter}) + end + + # Update boolean filters - send events for each changed filter + current_filters = socket.assigns.boolean_filters + + # Process all custom field filters from form (including those set to "all"/nil) + # Radio buttons in a group always send a value, so all active filters are in the form + Enum.each(custom_boolean_filters_parsed, fn {custom_field_id_str, new_value} -> + current_value = Map.get(current_filters, custom_field_id_str) + + # Only send event if value actually changed + if current_value != new_value do + send(self(), {:boolean_filter_changed, custom_field_id_str, new_value}) + end + end) + + # Don't close dropdown - allow multiple filter changes + {:noreply, socket} + end + + @impl true + def handle_event("reset_filters", _params, socket) do + # Send single message to reset all filters at once (performance optimization) + # This avoids N×2 load_members() calls when resetting multiple filters + send(self(), {:reset_all_filters, nil, %{}}) + + # Close dropdown after reset + {:noreply, assign(socket, :open, false)} + end + + # Parse tri-state filter value: "all" | "true" | "false" -> nil | true | false + defp parse_tri_state("true"), do: true + defp parse_tri_state("false"), do: false + defp parse_tri_state("all"), do: nil + defp parse_tri_state(_), do: nil + + # Get display label for button + defp button_label(cycle_status_filter, boolean_custom_fields, boolean_filters) do + # If payment filter is active, show payment filter label + if cycle_status_filter do + payment_filter_label(cycle_status_filter) + else + # Otherwise show boolean filter labels + boolean_filter_label(boolean_custom_fields, boolean_filters) + end + end + + # Get payment filter label + defp payment_filter_label(nil), do: gettext("All") + defp payment_filter_label(:paid), do: gettext("Paid") + defp payment_filter_label(:unpaid), do: gettext("Unpaid") + + # Get boolean filter label (comma-separated list of active filter names) + defp boolean_filter_label(_boolean_custom_fields, boolean_filters) + when map_size(boolean_filters) == 0 do + gettext("All") + end + + defp boolean_filter_label(boolean_custom_fields, boolean_filters) do + # Get names of active boolean filters + active_filter_names = + boolean_filters + |> Enum.map(fn {custom_field_id_str, _value} -> + Enum.find(boolean_custom_fields, fn cf -> to_string(cf.id) == custom_field_id_str end) + end) + |> Enum.filter(&(&1 != nil)) + |> Enum.map(& &1.name) + + # Join with comma and truncate if too long + label = Enum.join(active_filter_names, ", ") + truncate_label(label, 30) + end + + # Truncate label if longer than max_length + defp truncate_label(label, max_length) when byte_size(label) <= max_length, do: label + + defp truncate_label(label, max_length) do + String.slice(label, 0, max_length) <> "..." + end + + # Count active boolean filters + defp active_boolean_filters_count(boolean_filters) do + map_size(boolean_filters) + end + + # Get CSS classes for payment filter label based on current state + defp payment_filter_label_class(current_filter, expected_value) do + base_classes = "join-item btn btn-sm" + is_active = current_filter == expected_value + + cond do + # All button (nil expected) + expected_value == nil -> + if is_active do + "#{base_classes} btn-active" + else + "#{base_classes} btn" + end + + # Paid button + expected_value == :paid -> + if is_active do + "#{base_classes} btn-success btn-active" + else + "#{base_classes} btn" + end + + # Unpaid button + expected_value == :unpaid -> + if is_active do + "#{base_classes} btn-error btn-active" + else + "#{base_classes} btn" + end + + true -> + "#{base_classes} btn-outline" + end + end + + # Get CSS classes for boolean filter label based on current state + defp boolean_filter_label_class(boolean_filters, custom_field_id, expected_value) do + base_classes = "join-item btn btn-sm" + current_value = Map.get(boolean_filters, to_string(custom_field_id)) + is_active = current_value == expected_value + + cond do + # All button (nil expected) + expected_value == nil -> + if is_active do + "#{base_classes} btn-active" + else + "#{base_classes} btn" + end + + # True button + expected_value == true -> + if is_active do + "#{base_classes} btn-success btn-active" + else + "#{base_classes} btn" + end + + # False button + expected_value == false -> + if is_active do + "#{base_classes} btn-error btn-active" + else + "#{base_classes} btn" + end + + true -> + "#{base_classes} btn-outline" + end + end +end diff --git a/lib/mv_web/live/components/payment_filter_component.ex b/lib/mv_web/live/components/payment_filter_component.ex deleted file mode 100644 index 9caaa1f..0000000 --- a/lib/mv_web/live/components/payment_filter_component.ex +++ /dev/null @@ -1,147 +0,0 @@ -defmodule MvWeb.Components.PaymentFilterComponent do - @moduledoc """ - Provides the PaymentFilter Live-Component. - - A dropdown filter for filtering members by cycle payment status (paid/unpaid/all). - Uses DaisyUI dropdown styling and sends filter changes to parent LiveView. - Filter is based on cycle status (last or current cycle, depending on cycle view toggle). - - ## Props - - `:cycle_status_filter` - Current filter state: `nil` (all), `:paid`, or `:unpaid` - - `:id` - Component ID (required) - - `:member_count` - Number of filtered members to display in badge (optional, default: 0) - - ## Events - - Sends `{:payment_filter_changed, filter}` to parent when filter changes - """ - use MvWeb, :live_component - - @impl true - def mount(socket) do - {:ok, assign(socket, :open, false)} - end - - @impl true - def update(assigns, socket) do - socket = - socket - |> assign(:id, assigns.id) - |> assign(:cycle_status_filter, assigns[:cycle_status_filter]) - |> assign(:member_count, assigns[:member_count] || 0) - - {:ok, socket} - end - - @impl true - def render(assigns) do - ~H""" -
- - - -
- """ - end - - @impl true - def handle_event("toggle_dropdown", _params, socket) do - {:noreply, assign(socket, :open, !socket.assigns.open)} - end - - @impl true - def handle_event("close_dropdown", _params, socket) do - {:noreply, assign(socket, :open, false)} - end - - @impl true - def handle_event("select_filter", %{"filter" => filter_str}, socket) do - filter = parse_filter(filter_str) - - # Close dropdown and notify parent - socket = assign(socket, :open, false) - send(self(), {:payment_filter_changed, filter}) - - {:noreply, socket} - end - - # Parse filter string to atom - defp parse_filter("paid"), do: :paid - defp parse_filter("unpaid"), do: :unpaid - defp parse_filter(_), do: nil - - # Get display label for current filter - defp filter_label(nil), do: gettext("All") - defp filter_label(:paid), do: gettext("Paid") - defp filter_label(:unpaid), do: gettext("Unpaid") -end diff --git a/lib/mv_web/live/member_live/index.ex b/lib/mv_web/live/member_live/index.ex index 2cf7392..50b0cfa 100644 --- a/lib/mv_web/live/member_live/index.ex +++ b/lib/mv_web/live/member_live/index.ex @@ -28,6 +28,7 @@ defmodule MvWeb.MemberLive.Index do use MvWeb, :live_view require Ash.Query + require Logger import Ash.Expr import MvWeb.LiveHelpers, only: [current_actor: 1] @@ -41,6 +42,15 @@ defmodule MvWeb.MemberLive.Index do # Prefix used in sort field names for custom fields (e.g., "custom_field_") @custom_field_prefix Mv.Constants.custom_field_prefix() + # Prefix used for boolean custom field filter URL parameters (e.g., "bf_") + @boolean_filter_prefix Mv.Constants.boolean_filter_prefix() + + # Maximum number of boolean custom field filters allowed per request (DoS protection) + @max_boolean_filters Mv.Constants.max_boolean_filters() + + # Maximum length of UUID string (36 characters including hyphens) + @max_uuid_length Mv.Constants.max_uuid_length() + # Member fields that are loaded for the overview # Uses constants from Mv.Constants to ensure consistency # Note: :id is always included for member identification @@ -72,6 +82,12 @@ defmodule MvWeb.MemberLive.Index do |> Ash.Query.sort(name: :asc) |> Ash.read!(actor: actor) + # Load boolean custom fields (filtered and sorted from all_custom_fields) + boolean_custom_fields = + all_custom_fields + |> Enum.filter(&(&1.value_type == :boolean)) + |> Enum.sort_by(& &1.name, :asc) + # Load settings once to avoid N+1 queries settings = case Membership.get_settings() do @@ -101,10 +117,12 @@ defmodule MvWeb.MemberLive.Index do |> assign_new(:sort_field, fn -> :first_name end) |> assign_new(:sort_order, fn -> :asc end) |> assign(:cycle_status_filter, nil) + |> assign(:boolean_custom_field_filters, %{}) |> assign(:selected_members, MapSet.new()) |> assign(:settings, settings) |> assign(:custom_fields_visible, custom_fields_visible) |> assign(:all_custom_fields, all_custom_fields) + |> assign(:boolean_custom_fields, boolean_custom_fields) |> assign(:all_available_fields, all_available_fields) |> assign(:user_field_selection, initial_selection) |> assign( @@ -218,7 +236,8 @@ defmodule MvWeb.MemberLive.Index do socket.assigns.sort_field, socket.assigns.sort_order, socket.assigns.cycle_status_filter, - new_show_current + new_show_current, + socket.assigns.boolean_custom_field_filters ) new_path = ~p"/members?#{query_params}" @@ -332,7 +351,8 @@ defmodule MvWeb.MemberLive.Index do existing_field_query, existing_sort_query, socket.assigns.cycle_status_filter, - socket.assigns.show_current_cycle + socket.assigns.show_current_cycle, + socket.assigns.boolean_custom_field_filters ) # Set the new path with params @@ -361,7 +381,77 @@ defmodule MvWeb.MemberLive.Index do socket.assigns.sort_field, socket.assigns.sort_order, filter, - socket.assigns.show_current_cycle + socket.assigns.show_current_cycle, + socket.assigns.boolean_custom_field_filters + ) + + new_path = ~p"/members?#{query_params}" + + {:noreply, + push_patch(socket, + to: new_path, + replace: true + )} + end + + @impl true + def handle_info({:boolean_filter_changed, custom_field_id_str, filter_value}, socket) do + # Update boolean filters map + updated_filters = + if filter_value == nil do + # Remove filter if nil (All option selected) + Map.delete(socket.assigns.boolean_custom_field_filters, custom_field_id_str) + else + # Add or update filter + Map.put(socket.assigns.boolean_custom_field_filters, custom_field_id_str, filter_value) + end + + socket = + socket + |> assign(:boolean_custom_field_filters, updated_filters) + |> load_members() + |> update_selection_assigns() + + # Build the URL with all params including new filter + query_params = + build_query_params( + socket.assigns.query, + socket.assigns.sort_field, + socket.assigns.sort_order, + socket.assigns.cycle_status_filter, + socket.assigns.show_current_cycle, + updated_filters + ) + + new_path = ~p"/members?#{query_params}" + + {:noreply, + push_patch(socket, + to: new_path, + replace: true + )} + end + + @impl true + def handle_info({:reset_all_filters, cycle_status_filter, boolean_filters}, socket) do + # Reset all filters at once (performance optimization) + # This avoids N×2 load_members() calls when resetting multiple filters + socket = + socket + |> assign(:cycle_status_filter, cycle_status_filter) + |> assign(:boolean_custom_field_filters, boolean_filters) + |> load_members() + |> update_selection_assigns() + + # Build the URL with all params including reset filters + query_params = + build_query_params( + socket.assigns.query, + socket.assigns.sort_field, + socket.assigns.sort_order, + cycle_status_filter, + socket.assigns.show_current_cycle, + boolean_filters ) new_path = ~p"/members?#{query_params}" @@ -448,6 +538,9 @@ defmodule MvWeb.MemberLive.Index do """ @impl true def handle_params(params, _url, socket) do + # Build signature BEFORE updates to detect if anything actually changed + prev_sig = build_signature(socket) + # Parse field selection from URL url_selection = FieldSelection.parse_from_url(params) @@ -471,23 +564,68 @@ defmodule MvWeb.MemberLive.Index do visible_member_fields = FieldVisibility.get_visible_member_fields(final_selection) visible_custom_fields = FieldVisibility.get_visible_custom_fields(final_selection) + # Apply all updates socket = socket |> maybe_update_search(params) |> maybe_update_sort(params) |> maybe_update_cycle_status_filter(params) + |> maybe_update_boolean_filters(params) |> maybe_update_show_current_cycle(params) |> assign(:query, params["query"]) |> assign(:user_field_selection, final_selection) |> assign(:member_fields_visible, visible_member_fields) |> assign(:visible_custom_field_ids, extract_custom_field_ids(visible_custom_fields)) - |> load_members() - |> prepare_dynamic_cols() - |> update_selection_assigns() + + # Build signature AFTER updates + next_sig = build_signature(socket) + + # Only load members if signature changed (optimization: avoid duplicate loads) + # OR if members haven't been loaded yet (first handle_params call after mount) + socket = + if prev_sig == next_sig && Map.has_key?(socket.assigns, :members) do + # Nothing changed AND members already loaded, skip expensive load_members() call + socket + |> prepare_dynamic_cols() + |> update_selection_assigns() + else + # Signature changed OR members not loaded yet, reload members + socket + |> load_members() + |> prepare_dynamic_cols() + |> update_selection_assigns() + end {:noreply, socket} end + # Builds a signature tuple representing all filter/sort parameters that affect member loading. + # + # This signature is used to detect if member data needs to be reloaded when handle_params + # is called. If the signature hasn't changed, we can skip the expensive load_members() call. + # + # Returns a tuple containing all relevant parameters: + # - query: Search query string + # - sort_field: Field to sort by + # - sort_order: Sort direction (:asc or :desc) + # - cycle_status_filter: Payment filter (:paid, :unpaid, or nil) + # - show_current_cycle: Whether to show current cycle + # - boolean_custom_field_filters: Map of active boolean filters + # - user_field_selection: Map of user's field visibility selections + # - visible_custom_field_ids: List of visible custom field IDs (affects which custom fields are loaded) + defp build_signature(socket) do + { + socket.assigns.query, + socket.assigns.sort_field, + socket.assigns.sort_order, + socket.assigns.cycle_status_filter, + socket.assigns.show_current_cycle, + socket.assigns.boolean_custom_field_filters, + socket.assigns.user_field_selection, + socket.assigns[:visible_custom_field_ids] || [] + } + end + # Prepares dynamic column definitions for custom fields that should be shown in the overview. # # Creates a list of column definitions, each containing: @@ -586,7 +724,8 @@ defmodule MvWeb.MemberLive.Index do field_str, Atom.to_string(order), socket.assigns.cycle_status_filter, - socket.assigns.show_current_cycle + socket.assigns.show_current_cycle, + socket.assigns.boolean_custom_field_filters ) new_path = ~p"/members?#{query_params}" @@ -616,7 +755,8 @@ defmodule MvWeb.MemberLive.Index do socket.assigns.sort_field, socket.assigns.sort_order, socket.assigns.cycle_status_filter, - socket.assigns.show_current_cycle + socket.assigns.show_current_cycle, + socket.assigns.boolean_custom_field_filters ) |> maybe_add_field_selection(socket.assigns[:user_field_selection]) @@ -634,12 +774,14 @@ defmodule MvWeb.MemberLive.Index do # Builds URL query parameters map including all filter/sort state. # Converts cycle_status_filter atom to string for URL. + # Adds boolean custom field filters as bf_=true|false. defp build_query_params( query, sort_field, sort_order, cycle_status_filter, - show_current_cycle + show_current_cycle, + boolean_filters ) do field_str = if is_atom(sort_field) do @@ -670,11 +812,19 @@ defmodule MvWeb.MemberLive.Index do end # Add show_current_cycle if true - if show_current_cycle do - Map.put(base_params, "show_current_cycle", "true") - else - base_params - end + base_params = + if show_current_cycle do + Map.put(base_params, "show_current_cycle", "true") + else + base_params + end + + # Add boolean custom field filters + Enum.reduce(boolean_filters, base_params, fn {custom_field_id, filter_value}, acc -> + param_key = "#{@boolean_filter_prefix}#{custom_field_id}" + param_value = if filter_value == true, do: "true", else: "false" + Map.put(acc, param_key, param_value) + end) end # Loads members from the database with custom field values and applies search/sort/payment filters. @@ -704,9 +854,32 @@ defmodule MvWeb.MemberLive.Index do |> Ash.Query.new() |> Ash.Query.select(@overview_fields) - # Load custom field values for visible custom fields (based on user selection) + # Load custom field values for visible custom fields AND active boolean filters + # This ensures boolean filters work even when the custom field is not visible in overview visible_custom_field_ids = socket.assigns[:visible_custom_field_ids] || [] - query = load_custom_field_values(query, visible_custom_field_ids) + + # Get IDs of active boolean filters (whitelisted against boolean_custom_fields) + # Convert boolean_custom_fields list to map for efficient lookup (consistent with maybe_update_boolean_filters) + boolean_custom_fields_map = + socket.assigns.boolean_custom_fields + |> Map.new(fn cf -> {to_string(cf.id), cf} end) + + active_boolean_filter_ids = + socket.assigns.boolean_custom_field_filters + |> Map.keys() + |> Enum.filter(fn id_str -> + # Validate UUID format and check against whitelist + String.length(id_str) <= @max_uuid_length && + match?({:ok, _}, Ecto.UUID.cast(id_str)) && + Map.has_key?(boolean_custom_fields_map, id_str) + end) + + # Union of visible IDs and active filter IDs + ids_to_load = + (visible_custom_field_ids ++ active_boolean_filter_ids) + |> Enum.uniq() + + query = load_custom_field_values(query, ids_to_load) # Load membership fee cycles for status display query = MembershipFeeStatus.load_cycles_for_members(query, socket.assigns.show_current_cycle) @@ -726,7 +899,9 @@ defmodule MvWeb.MemberLive.Index do # Errors in handle_params are handled by Phoenix LiveView actor = current_actor(socket) - members = Ash.read!(query, actor: actor) + {time_microseconds, members} = :timer.tc(fn -> Ash.read!(query, actor: actor) end) + time_milliseconds = time_microseconds / 1000 + Logger.info("Ash.read! in load_members/1 took #{time_milliseconds} ms") # Custom field values are already filtered at the database level in load_custom_field_values/2 # No need for in-memory filtering anymore @@ -739,6 +914,14 @@ defmodule MvWeb.MemberLive.Index do socket.assigns.show_current_cycle ) + # Apply boolean custom field filters if set + members = + apply_boolean_custom_field_filters( + members, + socket.assigns.boolean_custom_field_filters, + socket.assigns.all_custom_fields + ) + # Sort in memory if needed (for custom fields) members = if sort_after_load do @@ -1133,6 +1316,142 @@ defmodule MvWeb.MemberLive.Index do defp determine_cycle_status_filter("unpaid"), do: :unpaid defp determine_cycle_status_filter(_), do: nil + # Updates boolean custom field filters from URL parameters if present. + # + # Parses all URL parameters with prefix @boolean_filter_prefix and validates them: + # - Extracts custom field ID from parameter name (explicitly removes prefix) + # - Validates filter value using determine_boolean_filter/1 + # - Whitelisting: Only custom field IDs that exist and have value_type: :boolean + # - Security: Limits to maximum @max_boolean_filters filters to prevent DoS attacks + # - Security: Validates UUID length (max @max_uuid_length characters) + # + # Returns socket with updated :boolean_custom_field_filters assign. + defp maybe_update_boolean_filters(socket, params) do + # Get all boolean custom fields for whitelisting (keyed by ID as string for consistency) + boolean_custom_fields = + socket.assigns.all_custom_fields + |> Enum.filter(&(&1.value_type == :boolean)) + |> Map.new(fn cf -> {to_string(cf.id), cf} end) + + # Parse all boolean filter parameters + # Security: Use reduce_while to abort early after @max_boolean_filters to prevent DoS attacks + # This protects CPU/Parsing costs, not just memory/state + # We count processed parameters (not just valid filters) to protect against parsing DoS + prefix_length = String.length(@boolean_filter_prefix) + + {filters, total_processed} = + params + |> Enum.filter(fn {key, _value} -> String.starts_with?(key, @boolean_filter_prefix) end) + |> Enum.reduce_while({%{}, 0}, fn {key, value_str}, {acc, count} -> + if count >= @max_boolean_filters do + {:halt, {acc, count}} + else + new_acc = + process_boolean_filter_param( + key, + value_str, + prefix_length, + boolean_custom_fields, + acc + ) + + # Increment counter for each processed parameter (DoS protection) + # Note: We count processed params, not just valid filters, to protect parsing costs + {:cont, {new_acc, count + 1}} + end + end) + + # Log warning if we hit the limit + if total_processed >= @max_boolean_filters do + Logger.warning( + "Boolean filter limit reached: processed #{total_processed} parameters, accepted #{map_size(filters)} valid filters (max: #{@max_boolean_filters})" + ) + end + + assign(socket, :boolean_custom_field_filters, filters) + end + + # Processes a single boolean filter parameter from URL params. + # + # Validates the parameter and adds it to the accumulator if valid. + # Returns the accumulator unchanged if validation fails. + defp process_boolean_filter_param( + key, + value_str, + prefix_length, + boolean_custom_fields, + acc + ) do + # Extract custom field ID from parameter name (explicitly remove prefix) + # This is more secure than String.replace_prefix which only removes first occurrence + custom_field_id_str = String.slice(key, prefix_length, String.length(key) - prefix_length) + + # Validate custom field ID length (UUIDs are max @max_uuid_length characters) + # This provides an additional security layer beyond UUID format validation + if String.length(custom_field_id_str) > @max_uuid_length do + acc + else + validate_and_add_boolean_filter( + custom_field_id_str, + value_str, + boolean_custom_fields, + acc + ) + end + end + + # Validates UUID format and custom field existence, then adds filter if valid. + defp validate_and_add_boolean_filter( + custom_field_id_str, + value_str, + boolean_custom_fields, + acc + ) do + case Ecto.UUID.cast(custom_field_id_str) do + {:ok, _custom_field_id} -> + add_boolean_filter_if_valid( + custom_field_id_str, + value_str, + boolean_custom_fields, + acc + ) + + :error -> + acc + end + end + + # Adds boolean filter to accumulator if custom field exists and value is valid. + defp add_boolean_filter_if_valid( + custom_field_id_str, + value_str, + boolean_custom_fields, + acc + ) do + if Map.has_key?(boolean_custom_fields, custom_field_id_str) do + case determine_boolean_filter(value_str) do + nil -> acc + filter_value -> Map.put(acc, custom_field_id_str, filter_value) + end + else + acc + end + end + + # Determines valid boolean filter value from URL parameter. + # + # SECURITY: This function whitelists allowed filter values. Only "true" and "false" + # are accepted - all other input (including malicious strings) falls back to nil. + # This ensures no raw user input is ever passed to filter functions. + # + # Returns: + # - `true` for "true" string + # - `false` for "false" string + # - `nil` for any other value + defp determine_boolean_filter("true"), do: true + defp determine_boolean_filter("false"), do: false + defp determine_boolean_filter(_), do: nil + # Updates show_current_cycle from URL parameters if present. defp maybe_update_show_current_cycle(socket, %{"show_current_cycle" => "true"}) do assign(socket, :show_current_cycle, true) @@ -1166,7 +1485,166 @@ defmodule MvWeb.MemberLive.Index do values when is_list(values) -> Enum.find(values, fn cfv -> cfv.custom_field_id == custom_field.id or - (cfv.custom_field && cfv.custom_field.id == custom_field.id) + (match?(%{custom_field: %{id: _}}, cfv) && cfv.custom_field.id == custom_field.id) + end) + + _ -> + nil + end + end + + # Extracts the boolean value from a member's custom field value. + # + # Handles different value formats: + # - `%Ash.Union{value: value, type: :boolean}` - Extracts value from union + # - Map format with `"type"` and `"value"` keys - Extracts from map + # - Map format with `"_union_type"` and `"_union_value"` keys - Extracts from map + # + # Returns: + # - `true` if the custom field value is boolean true + # - `false` if the custom field value is boolean false + # - `nil` if no custom field value exists, value is nil, or value is not boolean + # + # Examples: + # get_boolean_custom_field_value(member, boolean_field) -> true + # get_boolean_custom_field_value(member, non_existent_field) -> nil + def get_boolean_custom_field_value(member, custom_field) do + case get_custom_field_value(member, custom_field) do + nil -> + nil + + cfv -> + extract_boolean_value(cfv.value) + end + end + + # Extracts boolean value from custom field value, handling different formats. + # + # Handles: + # - `%Ash.Union{value: value, type: :boolean}` - Union struct format + # - Map with `"type"` and `"value"` keys - JSONB map format + # - Map with `"_union_type"` and `"_union_value"` keys - Alternative map format + # - Direct boolean value - Primitive boolean + # + # Returns `true`, `false`, or `nil`. + defp extract_boolean_value(%Ash.Union{value: value, type: :boolean}) do + extract_boolean_value(value) + end + + defp extract_boolean_value(value) when is_map(value) do + # Handle map format from JSONB + type = Map.get(value, "type") || Map.get(value, "_union_type") + val = Map.get(value, "value") || Map.get(value, "_union_value") + + if type == "boolean" or type == :boolean do + extract_boolean_value(val) + else + nil + end + end + + defp extract_boolean_value(value) when is_boolean(value), do: value + defp extract_boolean_value(nil), do: nil + defp extract_boolean_value(_), do: nil + + # Applies boolean custom field filters to a list of members. + # + # Filters members based on boolean custom field values. Only members that match + # ALL active filters (AND logic) are returned. + # + # Parameters: + # - `members` - List of Member resources with loaded custom_field_values + # - `filters` - Map of `%{custom_field_id_string => true | false}` + # - `all_custom_fields` - List of all CustomField resources (for validation) + # + # Returns: + # - Filtered list of members that match all active filters + # - All members if filters map is empty + # - Filters with non-existent custom field IDs are ignored + # + # Examples: + # apply_boolean_custom_field_filters(members, %{"uuid-123" => true}, all_custom_fields) -> [member1, ...] + # apply_boolean_custom_field_filters(members, %{}, all_custom_fields) -> members + def apply_boolean_custom_field_filters(members, filters, _all_custom_fields) + when map_size(filters) == 0 do + members + end + + def apply_boolean_custom_field_filters(members, filters, all_custom_fields) do + # Build a map of valid boolean custom field IDs (as strings) for quick lookup + valid_custom_field_ids = + all_custom_fields + |> Enum.filter(&(&1.value_type == :boolean)) + |> MapSet.new(fn cf -> to_string(cf.id) end) + + # Filter out invalid custom field IDs from filters + valid_filters = + Enum.filter(filters, fn {custom_field_id_str, _value} -> + MapSet.member?(valid_custom_field_ids, custom_field_id_str) + end) + |> Enum.into(%{}) + + # If no valid filters remain, return all members + if map_size(valid_filters) == 0 do + members + else + Enum.filter(members, fn member -> + matches_all_filters?(member, valid_filters) + end) + end + end + + # Checks if a member matches all active boolean filters. + # + # A member matches a filter if: + # - The filter value is `true` and the member's custom field value is `true` + # - The filter value is `false` and the member's custom field value is `false` + # + # Members without a custom field value or with `nil` value do not match any filter. + # + # Returns `true` if all filters match, `false` otherwise. + defp matches_all_filters?(member, filters) do + Enum.all?(filters, fn {custom_field_id_str, filter_value} -> + matches_filter?(member, custom_field_id_str, filter_value) + end) + end + + # Checks if a member matches a specific boolean filter. + # + # Finds the custom field value by ID and checks if the member's boolean value + # matches the filter value. + # + # Returns: + # - `true` if the member's boolean value matches the filter value + # - `false` if no custom field value exists (member is filtered out) + # - `false` if value is nil or values don't match + defp matches_filter?(member, custom_field_id_str, filter_value) do + case find_custom_field_value_by_id(member, custom_field_id_str) do + nil -> + false + + cfv -> + boolean_value = extract_boolean_value(cfv.value) + boolean_value == filter_value + end + end + + # Finds a custom field value by custom field ID string. + # + # Searches through the member's custom_field_values to find one matching + # the given custom field ID. + # + # Returns the CustomFieldValue or nil. + defp find_custom_field_value_by_id(member, custom_field_id_str) do + case member.custom_field_values do + nil -> + nil + + values when is_list(values) -> + Enum.find(values, fn cfv -> + to_string(cfv.custom_field_id) == custom_field_id_str or + (match?(%{custom_field: %{id: _}}, cfv) && + to_string(cfv.custom_field.id) == custom_field_id_str) end) _ -> @@ -1221,8 +1699,11 @@ defmodule MvWeb.MemberLive.Index do # # Note: Mailto URLs have length limits that vary by email client. # For large selections, consider using export functionality instead. + # + # Handles case where members haven't been loaded yet (e.g., when signature didn't change in handle_params). defp update_selection_assigns(socket) do - members = socket.assigns.members + # Handle case where members haven't been loaded yet (e.g., when signature didn't change) + members = socket.assigns[:members] || [] selected_members = socket.assigns.selected_members selected_count = diff --git a/lib/mv_web/live/member_live/index.html.heex b/lib/mv_web/live/member_live/index.html.heex index b2af205..394db2c 100644 --- a/lib/mv_web/live/member_live/index.html.heex +++ b/lib/mv_web/live/member_live/index.html.heex @@ -37,9 +37,11 @@ placeholder={gettext("Search...")} /> <.live_component - module={MvWeb.Components.PaymentFilterComponent} - id="payment-filter" + module={MvWeb.Components.MemberFilterComponent} + id="member-filter" cycle_status_filter={@cycle_status_filter} + boolean_custom_fields={@boolean_custom_fields} + boolean_filters={@boolean_custom_field_filters} member_count={length(@members)} />