diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 2b48de2..0a87836 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -81,8 +81,8 @@ lib/ ├── membership/ # Membership domain │ ├── membership.ex # Domain definition │ ├── member.ex # Member resource -│ ├── custom_field.ex # Custom field (definition) resource │ ├── custom_field_value.ex # Custom field value resource +│ ├── custom_field.ex # CustomFieldValue type resource │ ├── setting.ex # Global settings (singleton resource) │ └── email.ex # Email custom type ├── membership_fees/ # MembershipFees domain @@ -194,8 +194,7 @@ test/ ├── seeds_test.exs # Database seed tests └── support/ # Test helpers ├── conn_case.ex # Controller test helpers - ├── data_case.ex # Data layer test helpers - └── fixtures.ex # Shared test fixtures (Mv.Fixtures) + └── data_case.ex # Data layer test helpers ``` ### 1.2 Module Organization @@ -1248,8 +1247,7 @@ test/ │ └── components/ └── support/ # Test helpers ├── conn_case.ex # Controller test setup - ├── data_case.ex # Database test setup - └── fixtures.ex # Shared test fixtures (Mv.Fixtures) + └── data_case.ex # Database test setup ``` **Test File Naming:** diff --git a/docs/groups-architecture.md b/docs/groups-architecture.md index 735898c..b2316d8 100644 --- a/docs/groups-architecture.md +++ b/docs/groups-architecture.md @@ -4,7 +4,7 @@ **Feature:** Groups Management **Version:** 1.0 **Last Updated:** 2025-01-XX -**Status:** ✅ Implemented (authorization: see [roles-and-permissions-architecture.md](./roles-and-permissions-architecture.md)) +**Status:** Architecture Design - Ready for Implementation --- @@ -412,17 +412,15 @@ lib/ ## Authorization -**Status:** ✅ Implemented. Group and MemberGroup resource policies and PermissionSets are in place. See [roles-and-permissions-architecture.md](./roles-and-permissions-architecture.md) for the full permission matrix and policy patterns. - ### Permission Model (MVP) -**Resource:** `Group` (and `MemberGroup`) +**Resource:** `groups` **Actions:** -- `read` - View groups (all permission sets) -- `create` - Create groups (normal_user and admin) -- `update` - Edit groups (normal_user and admin) -- `destroy` - Delete groups (normal_user and admin) +- `read` - View groups (all users with member read permission) +- `create` - Create groups (admin only) +- `update` - Edit groups (admin only) +- `destroy` - Delete groups (admin only) **Scopes:** - `:all` - All groups (for all permission sets that have read access) @@ -444,7 +442,7 @@ lib/ **Own Data Permission Set:** - `read` action on `Group` resource with `:all` scope - granted -**Note:** All permission sets use `:all` scope for groups. Groups are considered public information that all users with member read permission can view. normal_user and admin can manage (create/update/destroy) groups. +**Note:** All permission sets use `:all` scope for groups. Groups are considered public information that all users with member read permission can view. Only admins can manage (create/update/destroy) groups. ### Member-Group Association Permissions diff --git a/docs/membership-fee-architecture.md b/docs/membership-fee-architecture.md index 6c81169..4a290b7 100644 --- a/docs/membership-fee-architecture.md +++ b/docs/membership-fee-architecture.md @@ -334,18 +334,20 @@ lib/ ### Permission System Integration -**Status:** ✅ Implemented. See [roles-and-permissions-architecture.md](./roles-and-permissions-architecture.md) for the full permission matrix and policy patterns. +**See:** [roles-and-permissions-architecture.md](./roles-and-permissions-architecture.md) -**PermissionSets (lib/mv/authorization/permission_sets.ex):** +**Required Permissions:** -- **MembershipFeeType:** All permission sets can read (:all); only admin has create/update/destroy (:all). -- **MembershipFeeCycle:** All can read (:all); read_only has read only; normal_user and admin have read + create + update + destroy (:all). -- **Manual "Regenerate Cycles" (UI + server):** The "Regenerate Cycles" button in the member detail view is shown to users who have MembershipFeeCycle create permission (normal_user and admin). UI access is gated by `can_create_cycle`. The LiveView handler also enforces `can?(:create, MembershipFeeCycle)` server-side before running regeneration (so e.g. a read_only user cannot trigger it via DevTools). Regeneration runs with system actor. +- `MembershipFeeType.create/update/destroy` - Admin only +- `MembershipFeeType.read` - Admin, Treasurer, Board +- `MembershipFeeCycle.update` (status changes) - Admin, Treasurer +- `MembershipFeeCycle.read` - Admin, Treasurer, Board, Own member -**Resource Policies:** +**Policy Patterns:** -- **MembershipFeeType** (`lib/membership_fees/membership_fee_type.ex`): `authorizers: [Ash.Policy.Authorizer]`, single policy with `HasPermission` for read/create/update/destroy. -- **MembershipFeeCycle** (`lib/membership_fees/membership_fee_cycle.ex`): Same pattern; update includes mark_as_paid, mark_as_suspended, mark_as_unpaid. +- Use existing HasPermission check +- Leverage existing roles (Admin, Kassenwart) +- Member can read own cycles (linked via member_id) ### LiveView Integration @@ -355,7 +357,7 @@ lib/ 2. MembershipFeeCycle table component (member detail view) - Implemented as `MvWeb.MemberLive.Show.MembershipFeesComponent` - Displays all cycles in a table with status management - - Allows changing cycle status, editing amounts, and manually regenerating cycles (normal_user and admin) + - Allows changing cycle status, editing amounts, and regenerating cycles 3. Settings form section (admin) 4. Member list column (membership fee status) diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md index 92ad3c5..dbf2353 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -97,10 +97,6 @@ Control CRUD operations on: - CustomFieldValue (custom field values) - CustomField (custom field definitions) - Role (role management) -- Group (group definitions; read all, create/update/destroy normal_user and admin) -- MemberGroup (member–group associations; own_data read :linked, read_only read :all, normal_user/admin create/destroy) -- MembershipFeeType (fee type definitions; all read, admin-only create/update/destroy) -- MembershipFeeCycle (fee cycles; own_data read :linked, read_only read :all, normal_user/admin read+create+update+destroy; manual "Regenerate Cycles" for normal_user and admin) **4. Page-Level Permissions** @@ -109,7 +105,6 @@ Control access to LiveView pages: - Show pages (detail views) - Form pages (create/edit) - Admin pages -- Settings pages: `/settings` and `/membership_fee_settings` are admin-only (explicit in PermissionSets) **5. Granular Scopes** @@ -126,8 +121,6 @@ Three scope levels for permissions: - **Linked Member Email:** Only admins can edit email of member linked to user - **System Roles:** "Mitglied" role cannot be deleted (is_system_role flag) - **User-Member Linking:** Only admins can link/unlink users and members -- **User Role Assignment:** Only admins can change a user's role (via `update_user` with `role_id`). Last-admin validation ensures at least one user keeps the Admin role. -- **Settings Pages:** `/settings` and `/membership_fee_settings` are admin-only (explicit in PermissionSets pages). **7. UI Consistency** @@ -691,12 +684,6 @@ Quick reference table showing what each permission set allows: | **CustomFieldValue** (all) | - | R | R, C, U, D | R, C, U, D | | **CustomField** (all) | R | R | R | R, C, U, D | | **Role** (all) | - | - | - | R, C, U, D | -| **Group** (all) | R | R | R, C, U, D | R, C, U, D | -| **MemberGroup** (linked) | R | - | - | - | -| **MemberGroup** (all) | - | R | R, C, D | R, C, D | -| **MembershipFeeType** (all) | R | R | R | R, C, U, D | -| **MembershipFeeCycle** (linked) | R | - | - | - | -| **MembershipFeeCycle** (all) | - | R | R, C, U, D | R, C, U, D | **Legend:** R=Read, C=Create, U=Update, D=Destroy @@ -1208,36 +1195,6 @@ end *Cannot destroy if `is_system_role=true` -### User Role Assignment (Admin-Only) - -**Location:** `lib/accounts/user.ex` (update_user action), `lib/mv_web/live/user_live/form.ex` - -Only admins can change a user's role. The `update_user` action accepts `role_id`; the User form shows a role dropdown when `can?(actor, :update, Mv.Authorization.Role)`. **Last-admin validation:** If the only non-system admin tries to change their role, the change is rejected with "At least one user must keep the Admin role." (System user is excluded from the admin count.) See [User-Member Linking](#user-member-linking) for the same admin-only pattern. - -### Group Resource Policies - -**Location:** `lib/membership/group.ex` - -Policies use `HasPermission` for read/create/update/destroy. All permission sets can read; normal_user and admin can create, update, destroy. No bypass (scope :all only in PermissionSets). - -### MemberGroup Resource Policies - -**Location:** `lib/membership/member_group.ex` - -Bypass for read restricted to own_data (MemberGroupReadLinkedForOwnData check: own_data only, filter `member_id == actor.member_id`); HasPermission for read (read_only/normal_user/admin :all) and create/destroy (normal_user + admin only). Admin with member_id set still gets :all from HasPermission (bypass does not apply). - -### MembershipFeeType Resource Policies - -**Location:** `lib/membership_fees/membership_fee_type.ex` - -Policies use `HasPermission` for read/create/update/destroy. All permission sets can read; only admin can create, update, destroy. - -### MembershipFeeCycle Resource Policies - -**Location:** `lib/membership_fees/membership_fee_cycle.ex` - -Bypass for read restricted to own_data (MembershipFeeCycleReadLinkedForOwnData: own_data only, filter `member_id == actor.member_id`); HasPermission for read (read_only/normal_user/admin :all) and create/update/destroy. own_data can only read cycles of the linked member; read_only can read all; normal_user and admin can read, create, update, and destroy (including mark_as_paid and manual "Regenerate Cycles"; UI button when `can_create_cycle`). Regenerate-cycles handler enforces `can?(:create, MembershipFeeCycle)` server-side. - --- ## Page Permission System diff --git a/docs/roles-and-permissions-implementation-plan.md b/docs/roles-and-permissions-implementation-plan.md index 95db031..23b045c 100644 --- a/docs/roles-and-permissions-implementation-plan.md +++ b/docs/roles-and-permissions-implementation-plan.md @@ -78,11 +78,10 @@ Stored in database `roles` table, each referencing a `permission_set_name`: - ✅ Hardcoded PermissionSets module with 4 permission sets - ✅ Role database table and CRUD interface - ✅ Custom Ash Policy Check (`HasPermission`) that reads from PermissionSets -- ✅ Policies on all resources (Member, User, CustomFieldValue, CustomField, Role, Group, MemberGroup, MembershipFeeType, MembershipFeeCycle) -- ✅ Page-level permissions via Phoenix Plug (including admin-only `/settings` and `/membership_fee_settings`) +- ✅ Policies on all resources (Member, User, CustomFieldValue, CustomField, Role) +- ✅ Page-level permissions via Phoenix Plug - ✅ UI authorization helpers for conditional rendering - ✅ Special case: Member email validation for linked users -- ✅ User role assignment: admin-only `role_id` in update_user; Last-Admin validation; role dropdown in User form when `can?(actor, :update, Role)` - ✅ Seed data for 5 roles **Benefits of Hardcoded Approach:** diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 034177a..f792973 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -8,9 +8,6 @@ defmodule Mv.Accounts.User do extensions: [AshAuthentication], authorizers: [Ash.Policy.Authorizer] - require Ash.Query - import Ash.Expr - postgres do table "users" repo Mv.Repo @@ -149,10 +146,9 @@ defmodule Mv.Accounts.User do update :update_user do description "Updates a user and manages the optional member relationship. To change an existing member link, first remove it (set member to nil), then add the new one." - - # Accept email and role_id (role_id only used by admins; policy restricts update_user to admins). - # member_id is NOT in accept list - use argument :member for relationship management. - accept [:email, :role_id] + # Only accept email directly - member_id is NOT in accept list + # This prevents direct foreign key manipulation, forcing use of manage_relationship + accept [:email] # Allow member to be passed as argument for relationship management argument :member, :map, allow_nil?: true @@ -391,63 +387,6 @@ defmodule Mv.Accounts.User do end end - # Last-admin: prevent the only admin from leaving the admin role (at least one admin required). - # Only block when the user is leaving admin (target role is not admin). Switching between - # two admin roles (e.g. "Admin" and "Superadmin" both with permission_set_name "admin") is allowed. - validate fn changeset, _context -> - if Ash.Changeset.changing_attribute?(changeset, :role_id) do - new_role_id = Ash.Changeset.get_attribute(changeset, :role_id) - - if is_nil(new_role_id) do - :ok - else - current_role_id = changeset.data.role_id - - current_role = - Mv.Authorization.Role - |> Ash.get!(current_role_id, authorize?: false) - - new_role = - Mv.Authorization.Role - |> Ash.get!(new_role_id, authorize?: false) - - # Only block when current user is admin and target role is not admin (leaving admin) - if current_role.permission_set_name == "admin" and - new_role.permission_set_name != "admin" do - admin_role_ids = - Mv.Authorization.Role - |> Ash.Query.for_read(:read) - |> Ash.Query.filter(expr(permission_set_name == "admin")) - |> Ash.read!(authorize?: false) - |> Enum.map(& &1.id) - - # Count only non-system users with admin role (system user is for internal ops) - system_email = Mv.Helpers.SystemActor.system_user_email() - - count = - Mv.Accounts.User - |> Ash.Query.for_read(:read) - |> Ash.Query.filter(expr(role_id in ^admin_role_ids)) - |> Ash.Query.filter(expr(email != ^system_email)) - |> Ash.count!(authorize?: false) - - if count <= 1 do - {:error, - field: :role_id, message: "At least one user must keep the Admin role."} - else - :ok - end - else - :ok - end - end - else - :ok - end - end, - on: [:update], - where: [action_is(:update_user)] - # Prevent modification of the system actor user (required for internal operations). # Block update/destroy on UI-exposed actions only; :update_internal is used by bootstrap/tests. validate fn changeset, _context -> diff --git a/lib/membership/group.ex b/lib/membership/group.ex index d468166..14aadc8 100644 --- a/lib/membership/group.ex +++ b/lib/membership/group.ex @@ -36,8 +36,7 @@ defmodule Mv.Membership.Group do """ use Ash.Resource, domain: Mv.Membership, - data_layer: AshPostgres.DataLayer, - authorizers: [Ash.Policy.Authorizer] + data_layer: AshPostgres.DataLayer require Ash.Query alias Mv.Helpers @@ -64,13 +63,6 @@ defmodule Mv.Membership.Group do end end - policies do - policy action_type([:read, :create, :update, :destroy]) do - description "Check permissions from role (all can read; normal_user and admin can create/update/destroy)" - authorize_if Mv.Authorization.Checks.HasPermission - end - end - validations do validate present(:name) @@ -144,7 +136,7 @@ defmodule Mv.Membership.Group do query = Mv.Membership.Group |> Ash.Query.filter(fragment("LOWER(?) = LOWER(?)", name, ^name)) - |> Helpers.query_exclude_id(exclude_id) + |> maybe_exclude_id(exclude_id) opts = Helpers.ash_actor_opts(actor) @@ -163,4 +155,7 @@ defmodule Mv.Membership.Group do :ok end end + + defp maybe_exclude_id(query, nil), do: query + defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) end diff --git a/lib/membership/member_group.ex b/lib/membership/member_group.ex index 22a1f70..5d29bda 100644 --- a/lib/membership/member_group.ex +++ b/lib/membership/member_group.ex @@ -39,8 +39,7 @@ defmodule Mv.Membership.MemberGroup do """ use Ash.Resource, domain: Mv.Membership, - data_layer: AshPostgres.DataLayer, - authorizers: [Ash.Policy.Authorizer] + data_layer: AshPostgres.DataLayer require Ash.Query @@ -57,26 +56,6 @@ defmodule Mv.Membership.MemberGroup do end end - # Authorization: read uses bypass for :linked (own_data only) then HasPermission for :all; - # create/destroy use HasPermission (normal_user + admin only). - # Single check: own_data gets filter via auto_filter; admin does not match, gets :all from HasPermission. - policies do - bypass action_type(:read) do - description "own_data: read only member_groups where member_id == actor.member_id" - authorize_if Mv.Authorization.Checks.MemberGroupReadLinkedForOwnData - end - - policy action_type(:read) do - description "Check read permission from role (read_only/normal_user/admin :all)" - authorize_if Mv.Authorization.Checks.HasPermission - end - - policy action_type([:create, :destroy]) do - description "Check create/destroy from role (normal_user + admin only)" - authorize_if Mv.Authorization.Checks.HasPermission - end - end - validations do validate present(:member_id) validate present(:group_id) @@ -139,7 +118,7 @@ defmodule Mv.Membership.MemberGroup do query = Mv.Membership.MemberGroup |> Ash.Query.filter(member_id == ^member_id and group_id == ^group_id) - |> Helpers.query_exclude_id(exclude_id) + |> maybe_exclude_id(exclude_id) opts = Helpers.ash_actor_opts(actor) @@ -156,4 +135,7 @@ defmodule Mv.Membership.MemberGroup do :ok end end + + defp maybe_exclude_id(query, nil), do: query + defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) end diff --git a/lib/membership/setting.ex b/lib/membership/setting.ex index bb7d122..4ba0794 100644 --- a/lib/membership/setting.ex +++ b/lib/membership/setting.ex @@ -155,15 +155,12 @@ defmodule Mv.Membership.Setting do on: [:create, :update] # Validate default_membership_fee_type_id exists if set - validate fn changeset, context -> + validate fn changeset, _context -> fee_type_id = Ash.Changeset.get_attribute(changeset, :default_membership_fee_type_id) if fee_type_id do - # Check existence only; action is already restricted by policy (e.g. admin). - opts = [domain: Mv.MembershipFees, authorize?: false] - - case Ash.get(Mv.MembershipFees.MembershipFeeType, fee_type_id, opts) do + case Ash.get(Mv.MembershipFees.MembershipFeeType, fee_type_id) do {:ok, _} -> :ok diff --git a/lib/membership_fees/changes/set_membership_fee_start_date.ex b/lib/membership_fees/changes/set_membership_fee_start_date.ex index 0e9cf00..a2e1ad0 100644 --- a/lib/membership_fees/changes/set_membership_fee_start_date.ex +++ b/lib/membership_fees/changes/set_membership_fee_start_date.ex @@ -31,12 +31,12 @@ defmodule Mv.MembershipFees.Changes.SetMembershipFeeStartDate do alias Mv.MembershipFees.CalendarCycles @impl true - def change(changeset, _opts, context) do + def change(changeset, _opts, _context) do # Only calculate if membership_fee_start_date is not already set if has_start_date?(changeset) do changeset else - calculate_and_set_start_date(changeset, context) + calculate_and_set_start_date(changeset) end end @@ -56,13 +56,10 @@ defmodule Mv.MembershipFees.Changes.SetMembershipFeeStartDate do end end - defp calculate_and_set_start_date(changeset, context) do - actor = Map.get(context || %{}, :actor) - opts = if actor, do: [actor: actor], else: [] - + defp calculate_and_set_start_date(changeset) do with {:ok, join_date} <- get_join_date(changeset), {:ok, membership_fee_type_id} <- get_membership_fee_type_id(changeset), - {:ok, interval} <- get_interval(membership_fee_type_id, opts), + {:ok, interval} <- get_interval(membership_fee_type_id), {:ok, include_joining_cycle} <- get_include_joining_cycle() do start_date = calculate_start_date(join_date, interval, include_joining_cycle) Ash.Changeset.force_change_attribute(changeset, :membership_fee_start_date, start_date) @@ -121,8 +118,8 @@ defmodule Mv.MembershipFees.Changes.SetMembershipFeeStartDate do end end - defp get_interval(membership_fee_type_id, opts) do - case Ash.get(Mv.MembershipFees.MembershipFeeType, membership_fee_type_id, opts) do + defp get_interval(membership_fee_type_id) do + case Ash.get(Mv.MembershipFees.MembershipFeeType, membership_fee_type_id) do {:ok, %{interval: interval}} -> {:ok, interval} {:error, _} -> {:error, :membership_fee_type_not_found} end diff --git a/lib/membership_fees/changes/validate_same_interval.ex b/lib/membership_fees/changes/validate_same_interval.ex index 0ad32a1..8c1efb4 100644 --- a/lib/membership_fees/changes/validate_same_interval.ex +++ b/lib/membership_fees/changes/validate_same_interval.ex @@ -19,9 +19,9 @@ defmodule Mv.MembershipFees.Changes.ValidateSameInterval do use Ash.Resource.Change @impl true - def change(changeset, _opts, context) do + def change(changeset, _opts, _context) do if changing_membership_fee_type?(changeset) do - validate_interval_match(changeset, context) + validate_interval_match(changeset) else changeset end @@ -33,10 +33,9 @@ defmodule Mv.MembershipFees.Changes.ValidateSameInterval do end # Validate that the new type has the same interval as the current type - defp validate_interval_match(changeset, context) do + defp validate_interval_match(changeset) do current_type_id = get_current_type_id(changeset) new_type_id = get_new_type_id(changeset) - actor = Map.get(context || %{}, :actor) cond do # If no current type, allow any change (first assignment) @@ -49,13 +48,13 @@ defmodule Mv.MembershipFees.Changes.ValidateSameInterval do # Both types exist - validate intervals match true -> - validate_intervals_match(changeset, current_type_id, new_type_id, actor) + validate_intervals_match(changeset, current_type_id, new_type_id) end end # Validates that intervals match when both types exist - defp validate_intervals_match(changeset, current_type_id, new_type_id, actor) do - case get_intervals(current_type_id, new_type_id, actor) do + defp validate_intervals_match(changeset, current_type_id, new_type_id) do + case get_intervals(current_type_id, new_type_id) do {:ok, current_interval, new_interval} -> if current_interval == new_interval do changeset @@ -86,16 +85,11 @@ defmodule Mv.MembershipFees.Changes.ValidateSameInterval do end end - # Get intervals for both types (actor required for authorization when resource has policies) - defp get_intervals(current_type_id, new_type_id, actor) do + # Get intervals for both types + defp get_intervals(current_type_id, new_type_id) do alias Mv.MembershipFees.MembershipFeeType - opts = if actor, do: [actor: actor], else: [] - - case { - Ash.get(MembershipFeeType, current_type_id, opts), - Ash.get(MembershipFeeType, new_type_id, opts) - } do + case {Ash.get(MembershipFeeType, current_type_id), Ash.get(MembershipFeeType, new_type_id)} do {{:ok, current_type}, {:ok, new_type}} -> {:ok, current_type.interval, new_type.interval} diff --git a/lib/membership_fees/membership_fee_cycle.ex b/lib/membership_fees/membership_fee_cycle.ex index f0dd1a7..4d9c8b7 100644 --- a/lib/membership_fees/membership_fee_cycle.ex +++ b/lib/membership_fees/membership_fee_cycle.ex @@ -28,8 +28,7 @@ defmodule Mv.MembershipFees.MembershipFeeCycle do """ use Ash.Resource, domain: Mv.MembershipFees, - data_layer: AshPostgres.DataLayer, - authorizers: [Ash.Policy.Authorizer] + data_layer: AshPostgres.DataLayer postgres do table "membership_fee_cycles" @@ -84,19 +83,6 @@ defmodule Mv.MembershipFees.MembershipFeeCycle do end end - # READ: bypass for own_data (:linked) then HasPermission for :all; create/update/destroy: HasPermission only. - policies do - bypass action_type(:read) do - description "own_data: read only cycles where member_id == actor.member_id" - authorize_if Mv.Authorization.Checks.MembershipFeeCycleReadLinkedForOwnData - end - - policy action_type([:read, :create, :update, :destroy]) do - description "Check permissions from role (all read; normal_user and admin create/update/destroy)" - authorize_if Mv.Authorization.Checks.HasPermission - end - end - attributes do uuid_v7_primary_key :id diff --git a/lib/membership_fees/membership_fee_type.ex b/lib/membership_fees/membership_fee_type.ex index 8ec9467..498ff75 100644 --- a/lib/membership_fees/membership_fee_type.ex +++ b/lib/membership_fees/membership_fee_type.ex @@ -24,8 +24,7 @@ defmodule Mv.MembershipFees.MembershipFeeType do """ use Ash.Resource, domain: Mv.MembershipFees, - data_layer: AshPostgres.DataLayer, - authorizers: [Ash.Policy.Authorizer] + data_layer: AshPostgres.DataLayer postgres do table "membership_fee_types" @@ -62,13 +61,6 @@ defmodule Mv.MembershipFees.MembershipFeeType do end end - policies do - policy action_type([:read, :create, :update, :destroy]) do - description "Check permissions from role (all can read, only admin can create/update/destroy)" - authorize_if Mv.Authorization.Checks.HasPermission - end - end - validations do # Prevent interval changes after creation validate fn changeset, _context -> diff --git a/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex b/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex index 72cc10c..0e693e1 100644 --- a/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex +++ b/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex @@ -81,7 +81,7 @@ defmodule Mv.Accounts.User.Validations.EmailNotUsedByOtherMember do query = Mv.Membership.Member |> Ash.Query.filter(email == ^to_string(email)) - |> Mv.Helpers.query_exclude_id(exclude_member_id) + |> maybe_exclude_id(exclude_member_id) system_actor = SystemActor.get_system_actor() opts = Helpers.ash_actor_opts(system_actor) @@ -101,4 +101,7 @@ defmodule Mv.Accounts.User.Validations.EmailNotUsedByOtherMember do :ok end end + + defp maybe_exclude_id(query, nil), do: query + defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) end diff --git a/lib/mv/authorization/checks/actor_permission_set_is.ex b/lib/mv/authorization/checks/actor_permission_set_is.ex deleted file mode 100644 index deb9382..0000000 --- a/lib/mv/authorization/checks/actor_permission_set_is.ex +++ /dev/null @@ -1,44 +0,0 @@ -defmodule Mv.Authorization.Checks.ActorPermissionSetIs do - @moduledoc """ - Policy check: true when the actor's role has the given permission_set_name. - - Used to restrict bypass policies (e.g. MemberGroup read by member_id) to actors - with a specific permission set (e.g. "own_data") so that admin with member_id - still gets :all scope from HasPermission, not the bypass filter. - - ## Usage - - # In a resource policy (both conditions must hold for the bypass) - bypass action_type(:read) do - authorize_if expr(member_id == ^actor(:member_id)) - authorize_if {Mv.Authorization.Checks.ActorPermissionSetIs, permission_set_name: "own_data"} - end - - ## Options - - - `:permission_set_name` (required) - String or atom, e.g. `"own_data"` or `:own_data` - """ - use Ash.Policy.SimpleCheck - - alias Mv.Authorization.Actor - - @impl true - def describe(opts) do - name = opts[:permission_set_name] || "?" - "actor has permission set #{name}" - end - - @impl true - def match?(actor, _context, opts) do - case opts[:permission_set_name] do - nil -> - false - - expected -> - case Actor.permission_set_name(actor) do - nil -> false - actual -> to_string(expected) == to_string(actual) - end - end - end -end diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 1139c3c..774e767 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -50,7 +50,6 @@ defmodule Mv.Authorization.Checks.HasPermission do - **:linked** - Filters based on resource type: - 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) - - MemberGroup: `member_id == actor.member_id` (MemberGroup.member_id → Member.id → User.member_id) ## Error Handling @@ -279,28 +278,36 @@ defmodule Mv.Authorization.Checks.HasPermission do # For :own scope with User resource: id == actor.id # For :linked scope with Member resource: id == actor.member_id defp evaluate_filter_for_strict_check(_filter_expr, actor, record, resource_name) do - result = - case {resource_name, record} do - # Scope :own - {"User", %{id: user_id}} when not is_nil(user_id) -> - user_id == actor.id + case {resource_name, record} do + {"User", %{id: user_id}} when not is_nil(user_id) -> + # Check if this user's ID matches the actor's ID (scope :own) + if user_id == actor.id do + {:ok, true} + else + {:ok, false} + end - # Scope :linked - {"Member", %{id: member_id}} when not is_nil(member_id) -> - member_id == actor.member_id + {"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) -> - cfv_member_id == actor.member_id + {"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 - {"MemberGroup", %{member_id: mg_member_id}} when not is_nil(mg_member_id) -> - mg_member_id == actor.member_id - - _ -> - :unknown - end - - out = if result == :unknown, do: {:ok, :unknown}, else: {:ok, result} - out + _ -> + # 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") @@ -340,20 +347,24 @@ defmodule Mv.Authorization.Checks.HasPermission do defp apply_scope(:linked, actor, resource_name) do case resource_name do "Member" -> - # User.member_id → Member.id (inverse relationship). Filter: member.id == actor.member_id - linked_filter_by_member_id(actor, :id) + # User.member_id → Member.id (inverse relationship) + # Filter: member.id == actor.member_id + # If actor has no member_id, return no results (use false or impossible condition) + if is_nil(actor.member_id) do + {:filter, expr(false)} + else + {:filter, expr(id == ^actor.member_id)} + end "CustomFieldValue" -> # CustomFieldValue.member_id → Member.id → User.member_id - linked_filter_by_member_id(actor, :member_id) - - "MemberGroup" -> - # MemberGroup.member_id → Member.id → User.member_id (own linked member's group associations) - linked_filter_by_member_id(actor, :member_id) - - "MembershipFeeCycle" -> - # MembershipFeeCycle.member_id → Member.id → User.member_id (own linked member's cycles) - linked_filter_by_member_id(actor, :member_id) + # Filter: custom_field_value.member_id == actor.member_id + # If actor has no member_id, return no results + if is_nil(actor.member_id) do + {:filter, expr(false)} + else + {:filter, expr(member_id == ^actor.member_id)} + end _ -> # Fallback for other resources @@ -361,17 +372,6 @@ defmodule Mv.Authorization.Checks.HasPermission do end end - # Returns {:filter, expr(false)} if actor has no member_id; otherwise {:filter, expr(field == ^actor.member_id)}. - # Used for :linked scope on Member (field :id), CustomFieldValue and MemberGroup (field :member_id). - defp linked_filter_by_member_id(actor, _field) when is_nil(actor.member_id) do - {:filter, expr(false)} - end - - defp linked_filter_by_member_id(actor, :id), do: {:filter, expr(id == ^actor.member_id)} - - defp linked_filter_by_member_id(actor, :member_id), - do: {:filter, expr(member_id == ^actor.member_id)} - # Log authorization failures for debugging (lazy evaluation) defp log_auth_failure(actor, resource, action, reason) do Logger.debug(fn -> diff --git a/lib/mv/authorization/checks/member_group_read_linked_for_own_data.ex b/lib/mv/authorization/checks/member_group_read_linked_for_own_data.ex deleted file mode 100644 index a553fde..0000000 --- a/lib/mv/authorization/checks/member_group_read_linked_for_own_data.ex +++ /dev/null @@ -1,63 +0,0 @@ -defmodule Mv.Authorization.Checks.MemberGroupReadLinkedForOwnData do - @moduledoc """ - Policy check for MemberGroup read: true only when actor has permission set "own_data" - AND record.member_id == actor.member_id. - - Used in a bypass so that own_data gets the linked filter (via auto_filter for list queries), - while admin with member_id does not match and gets :all from HasPermission. - - - With a record (e.g. get by id): returns true only when own_data and member_id match. - - Without a record (list query): strict_check returns false; auto_filter adds filter when own_data. - """ - use Ash.Policy.Check - - alias Mv.Authorization.Checks.ActorPermissionSetIs - - @impl true - def type, do: :filter - - @impl true - def describe(_opts), - do: "own_data can read only member_groups where member_id == actor.member_id" - - @impl true - def strict_check(actor, authorizer, _opts) do - record = get_record_from_authorizer(authorizer) - is_own_data = ActorPermissionSetIs.match?(actor, authorizer, permission_set_name: "own_data") - - cond do - # List query + own_data: return :unknown so authorizer applies auto_filter (keyword list) - is_nil(record) and is_own_data -> - {:ok, :unknown} - - is_nil(record) -> - {:ok, false} - - not is_own_data -> - {:ok, false} - - record.member_id == actor.member_id -> - {:ok, true} - - true -> - {:ok, false} - end - end - - @impl true - def auto_filter(actor, _authorizer, _opts) do - if ActorPermissionSetIs.match?(actor, nil, permission_set_name: "own_data") && - Map.get(actor, :member_id) do - [member_id: actor.member_id] - else - [] - end - end - - defp get_record_from_authorizer(authorizer) do - case authorizer.subject do - %{data: data} when not is_nil(data) -> data - _ -> nil - end - end -end diff --git a/lib/mv/authorization/checks/membership_fee_cycle_read_linked_for_own_data.ex b/lib/mv/authorization/checks/membership_fee_cycle_read_linked_for_own_data.ex deleted file mode 100644 index 092558c..0000000 --- a/lib/mv/authorization/checks/membership_fee_cycle_read_linked_for_own_data.ex +++ /dev/null @@ -1,62 +0,0 @@ -defmodule Mv.Authorization.Checks.MembershipFeeCycleReadLinkedForOwnData do - @moduledoc """ - Policy check for MembershipFeeCycle read: true only when actor has permission set "own_data" - AND record.member_id == actor.member_id. - - Used in a bypass so that own_data gets the linked filter (via auto_filter for list queries), - while admin with member_id does not match and gets :all from HasPermission. - - - With a record (e.g. get by id): returns true only when own_data and member_id match. - - Without a record (list query): return :unknown so authorizer applies auto_filter. - """ - use Ash.Policy.Check - - alias Mv.Authorization.Checks.ActorPermissionSetIs - - @impl true - def type, do: :filter - - @impl true - def describe(_opts), - do: "own_data can read only membership_fee_cycles where member_id == actor.member_id" - - @impl true - def strict_check(actor, authorizer, _opts) do - record = get_record_from_authorizer(authorizer) - is_own_data = ActorPermissionSetIs.match?(actor, authorizer, permission_set_name: "own_data") - - cond do - is_nil(record) and is_own_data -> - {:ok, :unknown} - - is_nil(record) -> - {:ok, false} - - not is_own_data -> - {:ok, false} - - record.member_id == actor.member_id -> - {:ok, true} - - true -> - {:ok, false} - end - end - - @impl true - def auto_filter(actor, _authorizer, _opts) do - if ActorPermissionSetIs.match?(actor, nil, permission_set_name: "own_data") && - Map.get(actor, :member_id) do - [member_id: actor.member_id] - else - [] - end - end - - defp get_record_from_authorizer(authorizer) do - case authorizer.subject do - %{data: data} when not is_nil(data) -> data - _ -> nil - end - end -end diff --git a/lib/mv/authorization/permission_sets.ex b/lib/mv/authorization/permission_sets.ex index 9a5f7a7..858748d 100644 --- a/lib/mv/authorization/permission_sets.ex +++ b/lib/mv/authorization/permission_sets.ex @@ -58,27 +58,6 @@ defmodule Mv.Authorization.PermissionSets do pages: [String.t()] } - # DRY helpers for shared resource permission lists (used in own_data, read_only, normal_user, admin) - defp perm(resource, action, scope), - do: %{resource: resource, action: action, scope: scope, granted: true} - - # All four CRUD actions for a resource with scope :all (used for admin) - defp perm_all(resource), - do: [ - perm(resource, :read, :all), - perm(resource, :create, :all), - perm(resource, :update, :all), - perm(resource, :destroy, :all) - ] - - # User: read/update own credentials only (all non-admin sets allow password changes) - defp user_own_credentials, do: [perm("User", :read, :own), perm("User", :update, :own)] - - defp group_read_all, do: [perm("Group", :read, :all)] - defp custom_field_read_all, do: [perm("CustomField", :read, :all)] - defp membership_fee_type_read_all, do: [perm("MembershipFeeType", :read, :all)] - defp membership_fee_cycle_read_all, do: [perm("MembershipFeeCycle", :read, :all)] - @doc """ Returns the list of all valid permission set names. @@ -115,21 +94,29 @@ defmodule Mv.Authorization.PermissionSets do def get_permissions(:own_data) do %{ - resources: - user_own_credentials() ++ - [ - perm("Member", :read, :linked), - perm("Member", :update, :linked), - perm("CustomFieldValue", :read, :linked), - perm("CustomFieldValue", :update, :linked), - perm("CustomFieldValue", :create, :linked), - perm("CustomFieldValue", :destroy, :linked) - ] ++ - custom_field_read_all() ++ - group_read_all() ++ - [perm("MemberGroup", :read, :linked)] ++ - membership_fee_type_read_all() ++ - [perm("MembershipFeeCycle", :read, :linked)], + resources: [ + # User: Can read/update own credentials only + # IMPORTANT: "read_only" refers to member data, NOT user credentials. + # All permission sets grant User.update :own to allow password changes. + %{resource: "User", action: :read, scope: :own, granted: true}, + %{resource: "User", action: :update, scope: :own, granted: true}, + + # Member: Can read/update linked member + %{resource: "Member", action: :read, scope: :linked, granted: true}, + %{resource: "Member", action: :update, scope: :linked, granted: true}, + + # CustomFieldValue: Can read/update/create/destroy custom field values of linked member + %{resource: "CustomFieldValue", action: :read, scope: :linked, granted: true}, + %{resource: "CustomFieldValue", action: :update, scope: :linked, granted: true}, + %{resource: "CustomFieldValue", action: :create, scope: :linked, granted: true}, + %{resource: "CustomFieldValue", action: :destroy, scope: :linked, granted: true}, + + # CustomField: Can read all (needed for forms) + %{resource: "CustomField", action: :read, scope: :all, granted: true}, + + # Group: Can read all (needed for viewing groups) + %{resource: "Group", action: :read, scope: :all, granted: true} + ], pages: [ # No "/" - Mitglied must not see member index at root (same content as /members). # Own profile (sidebar links to /users/:id) and own user edit @@ -146,17 +133,25 @@ defmodule Mv.Authorization.PermissionSets do def get_permissions(:read_only) do %{ - resources: - user_own_credentials() ++ - [ - perm("Member", :read, :all), - perm("CustomFieldValue", :read, :all) - ] ++ - custom_field_read_all() ++ - group_read_all() ++ - [perm("MemberGroup", :read, :all)] ++ - membership_fee_type_read_all() ++ - membership_fee_cycle_read_all(), + resources: [ + # User: Can read/update own credentials only + # IMPORTANT: "read_only" refers to member data, NOT user credentials. + # All permission sets grant User.update :own to allow password changes. + %{resource: "User", action: :read, scope: :own, granted: true}, + %{resource: "User", action: :update, scope: :own, granted: true}, + + # Member: Can read all members, no modifications + %{resource: "Member", action: :read, scope: :all, granted: true}, + + # CustomFieldValue: Can read all custom field values + %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, + + # CustomField: Can read all + %{resource: "CustomField", action: :read, scope: :all, granted: true}, + + # Group: Can read all + %{resource: "Group", action: :read, scope: :all, granted: true} + ], pages: [ "/", # Own profile (sidebar links to /users/:id; redirect target must be allowed) @@ -181,37 +176,31 @@ defmodule Mv.Authorization.PermissionSets do def get_permissions(:normal_user) do %{ - resources: - user_own_credentials() ++ - [ - perm("Member", :read, :all), - perm("Member", :create, :all), - perm("Member", :update, :all), - # destroy intentionally omitted for safety - perm("CustomFieldValue", :read, :all), - perm("CustomFieldValue", :create, :all), - perm("CustomFieldValue", :update, :all), - perm("CustomFieldValue", :destroy, :all) - ] ++ - custom_field_read_all() ++ - [ - perm("Group", :read, :all), - perm("Group", :create, :all), - perm("Group", :update, :all), - perm("Group", :destroy, :all) - ] ++ - [ - perm("MemberGroup", :read, :all), - perm("MemberGroup", :create, :all), - perm("MemberGroup", :destroy, :all) - ] ++ - membership_fee_type_read_all() ++ - [ - perm("MembershipFeeCycle", :read, :all), - perm("MembershipFeeCycle", :create, :all), - perm("MembershipFeeCycle", :update, :all), - perm("MembershipFeeCycle", :destroy, :all) - ], + resources: [ + # User: Can read/update own credentials only + # IMPORTANT: "read_only" refers to member data, NOT user credentials. + # All permission sets grant User.update :own to allow password changes. + %{resource: "User", action: :read, scope: :own, granted: true}, + %{resource: "User", action: :update, scope: :own, granted: true}, + + # Member: Full CRUD except destroy (safety) + %{resource: "Member", action: :read, scope: :all, granted: true}, + %{resource: "Member", action: :create, scope: :all, granted: true}, + %{resource: "Member", action: :update, scope: :all, granted: true}, + # Note: destroy intentionally omitted for safety + + # CustomFieldValue: Full CRUD + %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :create, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :update, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :destroy, scope: :all, granted: true}, + + # CustomField: Read only (admin manages definitions) + %{resource: "CustomField", action: :read, scope: :all, granted: true}, + + # Group: Can read all + %{resource: "Group", action: :read, scope: :all, granted: true} + ], pages: [ "/", # Own profile (sidebar links to /users/:id; redirect target must be allowed) @@ -232,39 +221,52 @@ defmodule Mv.Authorization.PermissionSets do "/custom_field_values/:id/edit", # Groups overview "/groups", - # Create group - "/groups/new", # Group detail - "/groups/:slug", - # Edit group - "/groups/:slug/edit" + "/groups/:slug" ] } end def get_permissions(:admin) do - # MemberGroup has no :update action in the domain; use read/create/destroy only - member_group_perms = [ - perm("MemberGroup", :read, :all), - perm("MemberGroup", :create, :all), - perm("MemberGroup", :destroy, :all) - ] - %{ - resources: - perm_all("User") ++ - perm_all("Member") ++ - perm_all("CustomFieldValue") ++ - perm_all("CustomField") ++ - perm_all("Role") ++ - perm_all("Group") ++ - member_group_perms ++ - perm_all("MembershipFeeType") ++ - perm_all("MembershipFeeCycle"), + resources: [ + # User: Full management including other users + %{resource: "User", action: :read, scope: :all, granted: true}, + %{resource: "User", action: :create, scope: :all, granted: true}, + %{resource: "User", action: :update, scope: :all, granted: true}, + %{resource: "User", action: :destroy, scope: :all, granted: true}, + + # Member: Full CRUD + %{resource: "Member", action: :read, scope: :all, granted: true}, + %{resource: "Member", action: :create, scope: :all, granted: true}, + %{resource: "Member", action: :update, scope: :all, granted: true}, + %{resource: "Member", action: :destroy, scope: :all, granted: true}, + + # CustomFieldValue: Full CRUD + %{resource: "CustomFieldValue", action: :read, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :create, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :update, scope: :all, granted: true}, + %{resource: "CustomFieldValue", action: :destroy, scope: :all, granted: true}, + + # CustomField: Full CRUD (admin manages custom field definitions) + %{resource: "CustomField", action: :read, scope: :all, granted: true}, + %{resource: "CustomField", action: :create, scope: :all, granted: true}, + %{resource: "CustomField", action: :update, scope: :all, granted: true}, + %{resource: "CustomField", action: :destroy, scope: :all, granted: true}, + + # Role: Full CRUD (admin manages roles) + %{resource: "Role", action: :read, scope: :all, granted: true}, + %{resource: "Role", action: :create, scope: :all, granted: true}, + %{resource: "Role", action: :update, scope: :all, granted: true}, + %{resource: "Role", action: :destroy, scope: :all, granted: true}, + + # Group: Full CRUD (admin manages groups) + %{resource: "Group", action: :read, scope: :all, granted: true}, + %{resource: "Group", action: :create, scope: :all, granted: true}, + %{resource: "Group", action: :update, scope: :all, granted: true}, + %{resource: "Group", action: :destroy, scope: :all, granted: true} + ], pages: [ - # Explicit admin-only pages (for clarity and future restrictions) - "/settings", - "/membership_fee_settings", # Wildcard: Admin can access all pages "*" ] 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 624692b..eb6770c 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 @@ -27,10 +27,6 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do Modified changeset with email synchronization applied, or original changeset if recursion detected. """ - # Ash 3.12+ calls this to decide whether to run the change in certain contexts. - @impl true - def has_change?, do: true - @impl true def change(changeset, _opts, context) do # Only recursion protection needed - trigger logic is in `where` clauses diff --git a/lib/mv/helpers.ex b/lib/mv/helpers.ex index ae22e13..e20db67 100644 --- a/lib/mv/helpers.ex +++ b/lib/mv/helpers.ex @@ -5,8 +5,6 @@ defmodule Mv.Helpers do Provides utilities that are not specific to a single domain or layer. """ - require Ash.Query - @doc """ Converts an actor to Ash options list for authorization. Returns empty list if actor is nil. @@ -26,22 +24,4 @@ defmodule Mv.Helpers do @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] - - @doc """ - Returns the query unchanged if `exclude_id` is nil; otherwise adds a filter `id != ^exclude_id`. - - Used in uniqueness validations that must exclude the current record (e.g. name uniqueness - on update, duplicate association checks). Call with the record's primary key to exclude it - from the result set. - - ## Examples - - query - |> Ash.Query.filter(name == ^name) - |> Mv.Helpers.query_exclude_id(current_id) - - """ - @spec query_exclude_id(Ash.Query.t(), String.t() | nil) :: Ash.Query.t() - def query_exclude_id(query, nil), do: query - def query_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) end 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 1297515..1ee8ab0 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 @@ -56,7 +56,7 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do query = Mv.Accounts.User |> Ash.Query.filter(email == ^email) - |> Mv.Helpers.query_exclude_id(exclude_user_id) + |> maybe_exclude_id(exclude_user_id) system_actor = SystemActor.get_system_actor() opts = Helpers.ash_actor_opts(system_actor) @@ -76,4 +76,7 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do :ok end end + + defp maybe_exclude_id(query, nil), do: query + defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) end diff --git a/lib/mv_web/components/core_components.ex b/lib/mv_web/components/core_components.ex index 9ef8f2b..59e300e 100644 --- a/lib/mv_web/components/core_components.ex +++ b/lib/mv_web/components/core_components.ex @@ -545,9 +545,6 @@ defmodule MvWeb.CoreComponents do attr :label, :string attr :class, :string attr :col_click, :any, doc: "optional column-specific click handler that overrides row_click" - - attr :sort_field, :any, - doc: "optional; when equal to table sort_field, aria-sort is set on this th" end slot :action, doc: "the slot for showing user actions in the last table column" @@ -563,13 +560,7 @@ defmodule MvWeb.CoreComponents do - +
- {col[:label]} - {col[:label]} <.live_component module={MvWeb.Components.SortHeaderComponent} @@ -655,16 +646,6 @@ defmodule MvWeb.CoreComponents do """ end - defp table_th_aria_sort(col, sort_field, sort_order) do - col_sort = Map.get(col, :sort_field) - - if not is_nil(col_sort) and col_sort == sort_field and sort_order in [:asc, :desc] do - if sort_order == :asc, do: "ascending", else: "descending" - else - nil - end - end - @doc """ Renders a data list. diff --git a/lib/mv_web/components/table_components.ex b/lib/mv_web/components/table_components.ex index 6b3c060..ed94994 100644 --- a/lib/mv_web/components/table_components.ex +++ b/lib/mv_web/components/table_components.ex @@ -20,6 +20,7 @@ defmodule MvWeb.TableComponents do type="button" phx-click="sort" phx-value-field={@field} + aria-sort={aria_sort(@sort_field, @sort_order, @field)} class="flex items-center gap-1 hover:underline focus:outline-none" > {@label} @@ -32,4 +33,12 @@ defmodule MvWeb.TableComponents do """ end + + defp aria_sort(current_field, current_order, this_field) do + cond do + current_field != this_field -> "none" + current_order == :asc -> "ascending" + true -> "descending" + end + end end diff --git a/lib/mv_web/helpers/user_helpers.ex b/lib/mv_web/helpers/user_helpers.ex deleted file mode 100644 index 2f9741c..0000000 --- a/lib/mv_web/helpers/user_helpers.ex +++ /dev/null @@ -1,58 +0,0 @@ -defmodule MvWeb.Helpers.UserHelpers do - @moduledoc """ - Helper functions for user-related display in the web layer. - - Provides utilities for showing authentication status without exposing - sensitive attributes (e.g. hashed_password). - """ - - @doc """ - Returns whether the user has password authentication set. - - Only returns true when `hashed_password` is a non-empty string. This avoids - treating `nil`, empty string, or forbidden/redacted values (e.g. when the - attribute is not visible to the actor) as "has password". - - ## Examples - - iex> user = %{hashed_password: nil} - iex> MvWeb.Helpers.UserHelpers.has_password?(user) - false - - iex> user = %{hashed_password: "$2b$12$..."} - iex> MvWeb.Helpers.UserHelpers.has_password?(user) - true - - iex> user = %{hashed_password: ""} - iex> MvWeb.Helpers.UserHelpers.has_password?(user) - false - """ - @spec has_password?(map() | struct()) :: boolean() - def has_password?(user) when is_map(user) do - case Map.get(user, :hashed_password) do - hash when is_binary(hash) and byte_size(hash) > 0 -> true - _ -> false - end - end - - @doc """ - Returns whether the user is linked via OIDC/SSO (has a non-empty oidc_id). - - ## Examples - - iex> user = %{oidc_id: nil} - iex> MvWeb.Helpers.UserHelpers.has_oidc?(user) - false - - iex> user = %{oidc_id: "sub-from-rauthy"} - iex> MvWeb.Helpers.UserHelpers.has_oidc?(user) - true - """ - @spec has_oidc?(map() | struct()) :: boolean() - def has_oidc?(user) when is_map(user) do - case Map.get(user, :oidc_id) do - id when is_binary(id) and byte_size(id) > 0 -> true - _ -> false - end - end -end diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index f9588c0..b72add6 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -177,8 +177,7 @@ defmodule MvWeb.MemberLive.Form do phx-change="validate" value={@form[:membership_fee_type_id].value || ""} > - <%!-- No "None" option: a membership fee type is required (validated in Member resource). --%> - + <%= for fee_type <- @available_fee_types do %>