diff --git a/.drone.yml b/.drone.yml index cca4ca4..0f874a9 100644 --- a/.drone.yml +++ b/.drone.yml @@ -273,7 +273,7 @@ environment: steps: - name: renovate - image: renovate/renovate:42.95 + image: renovate/renovate:42.81 environment: RENOVATE_CONFIG_FILE: "renovate_backend_config.js" RENOVATE_TOKEN: diff --git a/.env.example b/.env.example index d5d35ed..13154f3 100644 --- a/.env.example +++ b/.env.example @@ -11,22 +11,9 @@ PHX_HOST=localhost # Recommended: Association settings ASSOCIATION_NAME="Sportsclub XYZ" -# Optional: Admin user (created/updated on container start via Release.seed_admin) -# In production, set these so the first admin can log in. Change password without redeploy: -# bin/mv eval "Mv.Release.seed_admin()" (with new ADMIN_PASSWORD or ADMIN_PASSWORD_FILE) -# ADMIN_EMAIL=admin@example.com -# ADMIN_PASSWORD=secure-password -# ADMIN_PASSWORD_FILE=/run/secrets/admin_password - # Optional: OIDC Configuration # These have defaults in docker-compose.prod.yml, only override if needed # OIDC_CLIENT_ID=mv # OIDC_BASE_URL=http://localhost:8080/auth/v1 # OIDC_REDIRECT_URI=http://localhost:4001/auth/user/rauthy/callback # OIDC_CLIENT_SECRET=your-rauthy-client-secret - -# Optional: OIDC group → Admin role sync (e.g. Authentik groups from profile scope) -# If OIDC_ADMIN_GROUP_NAME is set, users in that group get Admin role on registration/sign-in. -# OIDC_GROUPS_CLAIM defaults to "groups" (JWT claim name for group list). -# OIDC_ADMIN_GROUP_NAME=admin -# OIDC_GROUPS_CLAIM=groups diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 7e4cee9..c7bcfa6 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) │ ├── group.ex # Group resource │ ├── member_group.ex # MemberGroup join table resource @@ -198,8 +198,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 @@ -1340,8 +1339,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/config/config.exs b/config/config.exs index 6720a5d..64f3604 100644 --- a/config/config.exs +++ b/config/config.exs @@ -58,11 +58,6 @@ config :mv, max_rows: 1000 ] -# OIDC group → role sync (optional). Overridden in runtime.exs from ENV in production. -config :mv, :oidc_role_sync, - admin_group_name: nil, - groups_claim: "groups" - # Configures the endpoint config :mv, MvWeb.Endpoint, url: [host: "localhost"], diff --git a/config/runtime.exs b/config/runtime.exs index f1df5b7..06a2cd8 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -89,11 +89,6 @@ if System.get_env("PHX_SERVER") do config :mv, MvWeb.Endpoint, server: true end -# OIDC group → Admin role sync: read from ENV in all environments (dev/test/prod) -config :mv, :oidc_role_sync, - admin_group_name: System.get_env("OIDC_ADMIN_GROUP_NAME"), - groups_claim: System.get_env("OIDC_GROUPS_CLAIM") || "groups" - if config_env() == :prod do database_url = build_database_url.() diff --git a/docker-compose.yml b/docker-compose.yml index 626f353..4c169b5 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -25,7 +25,7 @@ services: rauthy: container_name: rauthy-dev - image: ghcr.io/sebadob/rauthy:0.34.2 + image: ghcr.io/sebadob/rauthy:0.33.4 environment: - LOCAL_TEST=true - SMTP_URL=mailcrab diff --git a/docs/admin-bootstrap-and-oidc-role-sync.md b/docs/admin-bootstrap-and-oidc-role-sync.md deleted file mode 100644 index b0da019..0000000 --- a/docs/admin-bootstrap-and-oidc-role-sync.md +++ /dev/null @@ -1,54 +0,0 @@ -# Admin Bootstrap and OIDC Role Sync - -## Overview - -- **Admin bootstrap:** In production, no seeds run. The first admin user is created/updated from environment variables in the Docker entrypoint (after migrate, before server). Password can be changed without redeploy via `bin/mv eval "Mv.Release.seed_admin()"`. -- **OIDC role sync:** Optional mapping from OIDC groups (e.g. from Authentik profile scope) to the Admin role. Users in the configured admin group get the Admin role on registration and on each sign-in. - -## Admin Bootstrap (Part A) - -### Environment Variables - -- `ADMIN_EMAIL` – Email of the admin user to create/update. If unset, seed_admin/0 does nothing. -- `ADMIN_PASSWORD` – Password for the admin user. If unset (and no file), no new user is created; if a user with ADMIN_EMAIL already exists (e.g. OIDC-only), their role is set to Admin (no password change). -- `ADMIN_PASSWORD_FILE` – Path to a file containing the password (e.g. Docker secret). - -### Release Task - -- `Mv.Release.seed_admin/0` – Reads ADMIN_EMAIL and password from ADMIN_PASSWORD or ADMIN_PASSWORD_FILE. If both email and password are set: creates or updates the user with the Admin role. If only ADMIN_EMAIL is set: sets the Admin role on an existing user with that email (for OIDC-only admins); does not create a user. Idempotent. - -### Entrypoint - -- rel/overlays/bin/docker-entrypoint.sh – After migrate, runs seed_admin(), then starts the server. - -### Seeds (Dev/Test) - -- priv/repo/seeds.exs – Uses ADMIN_PASSWORD or ADMIN_PASSWORD_FILE when set; otherwise fallback "testpassword" only in dev/test. - -## OIDC Role Sync (Part B) - -### Configuration - -- `OIDC_ADMIN_GROUP_NAME` – OIDC group name that maps to the Admin role. If unset, no role sync. -- `OIDC_GROUPS_CLAIM` – JWT claim name for group list (default "groups"). -- Module: Mv.OidcRoleSyncConfig (oidc_admin_group_name/0, oidc_groups_claim/0). - -### Sync Logic - -- Mv.OidcRoleSync.apply_admin_role_from_user_info(user, user_info) – If admin group configured, sets user role to Admin or Mitglied based on user_info groups. - -### Where It Runs - -1. Registration: register_with_rauthy after_action calls OidcRoleSync. -2. Sign-in: sign_in_with_rauthy prepare after_action calls OidcRoleSync for each user. - -### Internal Action - -- User.set_role_from_oidc_sync – Internal update (role_id only). Used by OidcRoleSync; not exposed. - -## See Also - -- .env.example – Admin and OIDC group env vars. -- lib/mv/release.ex – seed_admin/0. -- lib/mv/oidc_role_sync.ex – Sync implementation. -- docs/oidc-account-linking.md – OIDC account linking. diff --git a/docs/email-sync.md b/docs/email-sync.md index 2f765f0..c191ff4 100644 --- a/docs/email-sync.md +++ b/docs/email-sync.md @@ -4,7 +4,6 @@ 2. **DB constraints** - Prevent duplicates within same table (users.email, members.email) 3. **Custom validations** - Prevent cross-table conflicts only for linked entities 4. **Sync is bidirectional**: User ↔ Member (but User always wins on link) -5. **Linked member email change** - When a member is linked, only administrators or the linked user may change that member's email (Member resource validation `EmailChangePermission`). Because User.email wins on link and changes sync Member → User, allowing anyone to change a linked member's email would overwrite that user's account email; this rule keeps sync under control. --- 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 216c6c9..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 @@ -1025,21 +1012,16 @@ defmodule Mv.Membership.Member do authorize_if expr(id == ^actor(:member_id)) end - # 2. READ/DESTROY: Check permissions only (no :user argument on these actions) - policy action_type([:read, :destroy]) do + # 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) + # - :admin → can CRUD all members (scope :all) + policy action_type([:read, :create, :update, :destroy]) do description "Check permissions from user's role" authorize_if Mv.Authorization.Checks.HasPermission end - - # 3. CREATE/UPDATE: Forbid user link unless admin; then check permissions - # ForbidMemberUserLinkUnlessAdmin: only admins may pass :user (link or unlink via nil/empty). - # HasPermission: :own_data → update linked; :read_only → no update; :normal_user/admin → update all - policy action_type([:create, :update]) do - description "Forbid user link unless admin; then check permissions" - forbid_if Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin - authorize_if Mv.Authorization.Checks.HasPermission - end - + # 4. DEFAULT: Ash implicitly forbids if no policy authorizes (fail-closed) end @@ -1059,8 +1041,6 @@ end - **READ list queries**: No record at strict_check time → bypass with `expr(id == ^actor(:member_id))` needed for auto_filter ✅ - **UPDATE operations**: Changeset contains record → HasPermission evaluates `scope :linked` correctly ✅ -**User–member link:** Only admins may pass the `:user` argument on create_member or update_member (link or unlink via `user: nil`/`user: %{}`). The check uses **argument presence** (key in arguments), not value, to avoid bypass (see [User-Member Linking](#user-member-linking)). - **Permission Matrix:** | Action | Mitglied | Vorstand | Kassenwart | Buchhaltung | Admin | @@ -1155,20 +1135,23 @@ end **Location:** `lib/mv/authorization/role.ex` -**Defense-in-depth:** The Role resource uses `authorizers: [Ash.Policy.Authorizer]` and policies with `Mv.Authorization.Checks.HasPermission`. **Read** is allowed for all permission sets (own_data, read_only, normal_user, admin) via `perm("Role", :read, :all)` in PermissionSets; reading roles is not a security concern. **Create, update, and destroy** are allowed only for admin (admin has full Role CRUD in PermissionSets). Seeds and bootstrap use `authorize?: false` where necessary. - -**Special Protection:** System roles cannot be deleted (validation on destroy). +**Special Protection:** System roles cannot be deleted. ```elixir defmodule Mv.Authorization.Role do - use Ash.Resource, - authorizers: [Ash.Policy.Authorizer] + use Ash.Resource, ... policies do + # Only admin can manage roles policy action_type([:read, :create, :update, :destroy]) do - description "Check permissions from user's role (read all, create/update/destroy admin only)" + description "Check permissions from user's role" authorize_if Mv.Authorization.Checks.HasPermission end + + # DEFAULT: Forbid + policy action_type([:read, :create, :update, :destroy]) do + forbid_if always() + end end # Prevent deletion of system roles @@ -1205,43 +1188,13 @@ end | Action | Mitglied | Vorstand | Kassenwart | Buchhaltung | Admin | |--------|----------|----------|------------|-------------|-------| -| Read | ✅ | ✅ | ✅ | ✅ | ✅ | +| Read | ❌ | ❌ | ❌ | ❌ | ✅ | | Create | ❌ | ❌ | ❌ | ❌ | ✅ | | Update | ❌ | ❌ | ❌ | ❌ | ✅ | | Destroy* | ❌ | ❌ | ❌ | ❌ | ✅ | *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 @@ -2049,10 +2002,7 @@ Users and Members are separate entities that can be linked. Special rules: - A user cannot link themselves to an existing member - A user CAN create a new member and be directly linked to it (self-service) -**Enforcement:** - -- **User side:** The User resource restricts the `update_user` action (which accepts the `member` argument for link/unlink) to admins only via `Mv.Authorization.Checks.ActorIsAdmin`. The UserLive.Form shows the Member-Linking UI and runs member link/unlink on save only when the current user is admin; non-admins use the `:update` action (email only) for profile edit. -- **Member side:** Only admins may set or change the user–member link on **Member** create or update. When creating or updating a member, the `:user` argument (which links the member to a user account) is forbidden for non-admins. This is enforced by `Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin` in the Member resource policies (`forbid_if` before `authorize_if HasPermission`). Non-admins can still create and update members as long as they do **not** pass the `:user` argument. The Member resource uses **`on_missing: :ignore`** for the `:user` relationship on update_member, so **omitting** `:user` from params does **not** change the link (no "unlink by omission"); unlink is only possible by explicitly passing `:user` (e.g. `user: nil`), which is admin-only. +**Enforcement:** The User resource restricts the `update_user` action (which accepts the `member` argument for link/unlink) to admins only via `Mv.Authorization.Checks.ActorIsAdmin`. The UserLive.Form shows the Member-Linking UI and runs member link/unlink on save only when the current user is admin; non-admins use the `:update` action (email only) for profile edit. ### Approach: Separate Ash Actions 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 92b9ef2..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 @@ -187,13 +183,6 @@ defmodule Mv.Accounts.User do require_atomic? false end - # Internal: set role from OIDC group sync (Mv.OidcRoleSync). Bypass policy when context.private.oidc_role_sync. - # Same "at least one admin" validation as update_user (see validations where action_is). - update :set_role_from_oidc_sync do - accept [:role_id] - require_atomic? false - end - # Admin action for direct password changes in admin panel # Uses the official Ash Authentication HashPasswordChange with correct context update :admin_set_password do @@ -258,8 +247,6 @@ defmodule Mv.Accounts.User do end read :sign_in_with_rauthy do - # Single record expected; required for AshAuthentication OAuth2 strategy (returns list of 0 or 1). - get? true argument :user_info, :map, allow_nil?: false argument :oauth_tokens, :map, allow_nil?: false prepare AshAuthentication.Strategy.OAuth2.SignInPreparation @@ -269,27 +256,6 @@ defmodule Mv.Accounts.User do # linked their account via OIDC. Password-only users (oidc_id = nil) # cannot be accessed via OIDC login without password verification. filter expr(oidc_id == get_path(^arg(:user_info), [:sub])) - - # Sync role from OIDC groups after sign-in (e.g. admin group → Admin role) - # get? true can return nil, a single %User{}, or a list; normalize to list for Enum.each - prepare Ash.Resource.Preparation.Builtins.after_action(fn query, result, _context -> - user_info = Ash.Query.get_argument(query, :user_info) || %{} - oauth_tokens = Ash.Query.get_argument(query, :oauth_tokens) || %{} - - users = - case result do - nil -> [] - u when is_struct(u, User) -> [u] - list when is_list(list) -> list - _ -> [] - end - - Enum.each(users, fn user -> - Mv.OidcRoleSync.apply_admin_role_from_user_info(user, user_info, oauth_tokens) - end) - - {:ok, result} - end) end create :register_with_rauthy do @@ -327,18 +293,6 @@ defmodule Mv.Accounts.User do # Sync user email to member when linking (User → Member) change Mv.EmailSync.Changes.SyncUserEmailToMember - - # Sync role from OIDC groups (e.g. admin group → Admin role) after user is created/updated - change fn changeset, _ctx -> - user_info = Ash.Changeset.get_argument(changeset, :user_info) - oauth_tokens = Ash.Changeset.get_argument(changeset, :oauth_tokens) || %{} - - Ash.Changeset.after_action(changeset, fn _cs, record -> - Mv.OidcRoleSync.apply_admin_role_from_user_info(record, user_info, oauth_tokens) - # Return original record so __metadata__.token (from GenerateTokenChange) is preserved - {:ok, record} - end) - end end end @@ -365,13 +319,6 @@ defmodule Mv.Accounts.User do authorize_if Mv.Authorization.Checks.ActorIsAdmin end - # set_role_from_oidc_sync: internal only (called from Mv.OidcRoleSync on registration/sign-in). - # Not exposed in code_interface; only allowed when context.private.oidc_role_sync is set. - bypass action(:set_role_from_oidc_sync) do - description "Internal: OIDC role sync (server-side only)" - authorize_if Mv.Authorization.Checks.OidcRoleSyncContext - end - # UPDATE/DESTROY via HasPermission (evaluates PermissionSets scope) policy action_type([:read, :create, :update, :destroy]) do description "Check permissions from user's role and permission set" @@ -440,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, :set_role_from_oidc_sync])] - # 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.ex b/lib/membership/member.ex index 476501c..7b49c86 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -25,7 +25,6 @@ defmodule Mv.Membership.Member do - Postal code format: exactly 5 digits (German format) - Date validations: join_date not in future, exit_date after join_date - Email uniqueness: prevents conflicts with unlinked users - - Linked member email change: only admins or the linked user may change a linked member's email (see `Mv.Membership.Member.Validations.EmailChangePermission`) ## Full-Text Search Members have a `search_vector` attribute (tsvector) that is automatically @@ -153,18 +152,16 @@ defmodule Mv.Membership.Member do change manage_relationship(:custom_field_values, on_match: :update, on_no_match: :create) - # When :user argument is present and nil/empty, unrelate (admin-only via policy). - # Must run before manage_relationship; on_missing: :ignore then does nothing for nil input. - change Mv.Membership.Member.Changes.UnrelateUserWhenArgumentNil - # Manage the user relationship during member update - # on_missing: :ignore so that omitting :user does NOT unlink (security: only admins may - # change the link; unlink is explicit via user: nil, forbidden for non-admins by policy). change manage_relationship(:user, :user, + # Look up existing user and relate to it on_lookup: :relate, + # Error if user doesn't exist in database on_no_match: :error, + # Error if user is already linked to another member (prevents "stealing") on_match: :error, - on_missing: :ignore + # If no user provided, remove existing relationship (allows user removal) + on_missing: :unrelate ) # Sync member email to user when email changes (Member → User) @@ -314,18 +311,14 @@ defmodule Mv.Membership.Member do authorize_if expr(id == ^actor(:member_id)) end - # READ/DESTROY: Check permissions only (no :user argument on these actions) - policy action_type([:read, :destroy]) do - description "Check permissions from user's role" - authorize_if Mv.Authorization.Checks.HasPermission - end - - # CREATE/UPDATE: Forbid member–user link unless admin, then check permissions - # ForbidMemberUserLinkUnlessAdmin: only admins may pass :user (link or unlink via nil/empty). - # HasPermission: :own_data → update linked; :read_only → no update; :normal_user/admin → update all. - policy action_type([:create, :update]) do - description "Forbid user link unless admin; then check permissions" - forbid_if Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin + # GENERAL: Check permissions from user's role + # HasPermission handles update permissions correctly: + # - :own_data → can update linked member (scope :linked) + # - :read_only → cannot update any member (no update permission) + # - :normal_user → can update all members (scope :all) + # - :admin → can update all members (scope :all) + policy action_type([:read, :create, :update, :destroy]) do + description "Check permissions from user's role and permission set" authorize_if Mv.Authorization.Checks.HasPermission end @@ -388,9 +381,6 @@ defmodule Mv.Membership.Member do # Validates that member email is not already used by another (unlinked) user validate Mv.Membership.Member.Validations.EmailNotUsedByOtherUser - # Only admins or the linked user may change a linked member's email (prevents breaking sync) - validate Mv.Membership.Member.Validations.EmailChangePermission, on: [:update] - # Prevent linking to a user that already has a member # This validation prevents "stealing" users from other members by checking # if the target user is already linked to a different member diff --git a/lib/membership/member/changes/unrelate_user_when_argument_nil.ex b/lib/membership/member/changes/unrelate_user_when_argument_nil.ex deleted file mode 100644 index dc4d097..0000000 --- a/lib/membership/member/changes/unrelate_user_when_argument_nil.ex +++ /dev/null @@ -1,50 +0,0 @@ -defmodule Mv.Membership.Member.Changes.UnrelateUserWhenArgumentNil do - @moduledoc """ - When :user argument is present and nil/empty on update_member, unrelate the current user. - - With on_missing: :ignore, manage_relationship does not unrelate when input is nil/[]. - This change handles explicit unlink (user: nil or user: %{}) by updating the linked - User to set member_id = nil. Only runs when the argument key is present (policy - ForbidMemberUserLinkUnlessAdmin ensures only admins can pass :user). - """ - use Ash.Resource.Change - - @spec change(Ash.Changeset.t(), keyword(), Ash.Resource.Change.context()) :: Ash.Changeset.t() - def change(changeset, _opts, _context) do - if unlink_requested?(changeset) do - unrelate_current_user(changeset) - else - changeset - end - end - - defp unlink_requested?(changeset) do - args = changeset.arguments || %{} - - if Map.has_key?(args, :user) or Map.has_key?(args, "user") do - user_arg = Ash.Changeset.get_argument(changeset, :user) - user_arg == nil or (is_map(user_arg) and map_size(user_arg) == 0) - else - false - end - end - - defp unrelate_current_user(changeset) do - member = changeset.data - actor = Map.get(changeset.context || %{}, :actor) - - case Ash.load(member, :user, domain: Mv.Membership, authorize?: false) do - {:ok, %{user: user}} when not is_nil(user) -> - # User's :update action only accepts [:email]; use :update_user so - # manage_relationship(:member, ..., on_missing: :unrelate) runs and clears member_id. - user - |> Ash.Changeset.for_update(:update_user, %{member: nil}, domain: Mv.Accounts) - |> Ash.update(domain: Mv.Accounts, actor: actor, authorize?: false) - - changeset - - _ -> - changeset - end - end -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/actor.ex b/lib/mv/authorization/actor.ex index edc6b8b..3482043 100644 --- a/lib/mv/authorization/actor.ex +++ b/lib/mv/authorization/actor.ex @@ -1,7 +1,6 @@ defmodule Mv.Authorization.Actor do @moduledoc """ - Helper functions for ensuring User actors have required data loaded - and for querying actor capabilities (e.g. admin, permission set). + Helper functions for ensuring User actors have required data loaded. ## Actor Invariant @@ -28,11 +27,8 @@ defmodule Mv.Authorization.Actor do assign(socket, :current_user, user) end - # Check if actor is admin (policy checks, validations) - if Actor.admin?(actor), do: ... - - # Get permission set name (string or nil) - ps_name = Actor.permission_set_name(actor) + # In tests + user = Actor.ensure_loaded(user) ## Security Note @@ -51,8 +47,6 @@ defmodule Mv.Authorization.Actor do require Logger - alias Mv.Helpers.SystemActor - @doc """ Ensures the actor (User) has their `:role` relationship loaded. @@ -102,45 +96,4 @@ defmodule Mv.Authorization.Actor do actor end end - - @doc """ - Returns the actor's permission set name (string or atom) from their role, or nil. - - Ensures role is loaded (including when role is nil). Supports both atom and - string keys for session/socket assigns. Use for capability checks consistent - with `ActorIsAdmin` and `HasPermission`. - """ - @spec permission_set_name(Mv.Accounts.User.t() | map() | nil) :: String.t() | atom() | nil - def permission_set_name(nil), do: nil - - def permission_set_name(actor) do - actor = actor |> ensure_loaded() |> maybe_load_role() - - get_in(actor, [Access.key(:role), Access.key(:permission_set_name)]) || - get_in(actor, [Access.key("role"), Access.key("permission_set_name")]) - end - - @doc """ - Returns true if the actor is the system user or has the admin permission set. - - Use for validations and policy checks that require admin capability (e.g. - changing a linked member's email). Consistent with `ActorIsAdmin` policy check. - """ - @spec admin?(Mv.Accounts.User.t() | map() | nil) :: boolean() - def admin?(nil), do: false - - def admin?(actor) do - SystemActor.system_user?(actor) or permission_set_name(actor) in ["admin", :admin] - end - - # Load role only when it is nil (e.g. actor from session without role). ensure_loaded/1 - # already handles %Ash.NotLoaded{}, so we do not double-load in the normal Ash path. - defp maybe_load_role(%Mv.Accounts.User{role: nil} = user) do - case Ash.load(user, :role, domain: Mv.Accounts, authorize?: false) do - {:ok, loaded} -> loaded - _ -> user - end - end - - defp maybe_load_role(actor), do: actor end diff --git a/lib/mv/authorization/checks/actor_is_admin.ex b/lib/mv/authorization/checks/actor_is_admin.ex index 413c6c7..2328876 100644 --- a/lib/mv/authorization/checks/actor_is_admin.ex +++ b/lib/mv/authorization/checks/actor_is_admin.ex @@ -1,18 +1,22 @@ defmodule Mv.Authorization.Checks.ActorIsAdmin do @moduledoc """ - Policy check: true when the actor is the system user or has permission_set_name "admin". + Policy check: true when the actor's role has permission_set_name "admin". Used to restrict actions (e.g. User.update_user for member link/unlink) to admins only. - Delegates to `Mv.Authorization.Actor.admin?/1`, which returns true for the system actor - or for a user whose role has permission_set_name "admin". """ use Ash.Policy.SimpleCheck - alias Mv.Authorization.Actor - @impl true def describe(_opts), do: "actor has admin permission set" @impl true - def match?(actor, _context, _opts), do: Actor.admin?(actor) + def match?(nil, _context, _opts), do: false + + def match?(actor, _context, _opts) do + ps_name = + get_in(actor, [Access.key(:role), Access.key(:permission_set_name)]) || + get_in(actor, [Access.key("role"), Access.key("permission_set_name")]) + + ps_name == "admin" + end 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/forbid_member_user_link_unless_admin.ex b/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex deleted file mode 100644 index 1e7cb77..0000000 --- a/lib/mv/authorization/checks/forbid_member_user_link_unless_admin.ex +++ /dev/null @@ -1,71 +0,0 @@ -defmodule Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin do - @moduledoc """ - Policy check: forbids setting or changing the member–user link unless the actor is admin. - - Used on Member create_member and update_member actions. When the `:user` argument - **is present** (key in arguments, regardless of value), only admins may perform the action. - This covers: - - **Linking:** `user: %{id: user_id}` → only admin - - **Unlinking:** explicit `user: nil` or `user: %{}` on update_member → only admin - Non-admin users can create and update members only when they do **not** pass the - `:user` argument; omitting `:user` leaves the relationship unchanged. - - ## Unlink semantics (update_member) - - The Member resource uses `on_missing: :ignore` for the `:user` relationship on update. - So **omitting** `:user` from params does **not** change the link (no "unlink by omission"). - Unlink is only possible by **explicitly** passing `:user` (e.g. `user: nil`), which this - check forbids for non-admins. Admins may link or unlink via the `:user` argument. - - ## Usage - - In Member resource policies, restrict to create/update only: - - policy action_type([:create, :update]) do - forbid_if Mv.Authorization.Checks.ForbidMemberUserLinkUnlessAdmin - authorize_if Mv.Authorization.Checks.HasPermission - end - - ## Behaviour - - - If the `:user` argument **key is not present** → does not forbid. - - If `:user` is present (any value, including nil or %{}) and actor is not admin → forbids. - - If actor is nil → treated as non-admin (forbid when :user present). `Actor.admin?(nil)` is defined and returns false. - - If actor is admin (or system actor) → does not forbid. - """ - use Ash.Policy.Check - - alias Mv.Authorization.Actor - - @impl true - def describe(_opts), do: "forbid setting member–user link unless actor is admin" - - @impl true - def strict_check(actor, authorizer, _opts) do - # Nil actor: treat as non-admin (Actor.admin?(nil) returns false; no crash) - actor = if is_nil(actor), do: nil, else: Actor.ensure_loaded(actor) - - if user_argument_present?(authorizer) and not Actor.admin?(actor) do - {:ok, true} - else - {:ok, false} - end - end - - # Forbid when :user was passed at all (link, unlink via nil/empty, or invalid value). - # Check argument key presence (atom or string) for defense-in-depth. - defp user_argument_present?(authorizer) do - args = get_arguments(authorizer) || %{} - Map.has_key?(args, :user) or Map.has_key?(args, "user") - end - - defp get_arguments(authorizer) do - subject = authorizer.changeset || authorizer.subject - - cond do - is_struct(subject, Ash.Changeset) -> subject.arguments - is_struct(subject, Ash.ActionInput) -> subject.arguments - true -> %{} - end - end -end diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 721cee7..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 @@ -132,10 +131,26 @@ defmodule Mv.Authorization.Checks.HasPermission do resource_name ) do :authorized -> + # For :all scope, authorize directly {:ok, true} {:filter, filter_expr} -> - strict_check_filter_scope(record, filter_expr, actor, resource_name) + # For :own/:linked scope: + # - With a record, evaluate filter against record for strict authorization + # - Without a record (queries/lists), return false + # + # NOTE: Returning false here forces the use of expr-based bypass policies. + # This is necessary because Ash's policy evaluation doesn't reliably call auto_filter + # when strict_check returns :unknown. Instead, resources should use bypass policies + # with expr() directly for filter-based authorization (see User resource). + if record do + evaluate_filter_for_strict_check(filter_expr, actor, record, resource_name) + else + # No record yet (e.g., read/list queries) - deny at strict_check level + # Resources must use expr-based bypass policies for list filtering + # Create: use a dedicated check that does not return a filter (e.g. CustomFieldValueCreateScope) + {:ok, false} + end false -> {:ok, false} @@ -159,15 +174,6 @@ defmodule Mv.Authorization.Checks.HasPermission do end end - # For :own/:linked scope: with record evaluate filter; without record deny (resources use bypass + expr). - defp strict_check_filter_scope(record, filter_expr, actor, resource_name) do - if record do - evaluate_filter_for_strict_check(filter_expr, actor, record, resource_name) - else - {:ok, false} - end - end - @impl true def auto_filter(actor, authorizer, _opts) do resource = authorizer.resource @@ -272,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") @@ -333,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 @@ -354,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/checks/oidc_role_sync_context.ex b/lib/mv/authorization/checks/oidc_role_sync_context.ex deleted file mode 100644 index 1214d75..0000000 --- a/lib/mv/authorization/checks/oidc_role_sync_context.ex +++ /dev/null @@ -1,18 +0,0 @@ -defmodule Mv.Authorization.Checks.OidcRoleSyncContext do - @moduledoc """ - Policy check: true when the action is run from OIDC role sync (context.private.oidc_role_sync). - - Used to allow the internal set_role_from_oidc_sync action only when called by Mv.OidcRoleSync, - which sets context.private.oidc_role_sync when performing the update. - """ - use Ash.Policy.SimpleCheck - - @impl true - def describe(_opts), do: "called from OIDC role sync (context.private.oidc_role_sync)" - - @impl true - def match?(_actor, authorizer, _opts) do - context = Map.get(authorizer, :context) || %{} - get_in(context, [:private, :oidc_role_sync]) == true - end -end diff --git a/lib/mv/authorization/permission_sets.ex b/lib/mv/authorization/permission_sets.ex index b0e7015..ea0ddff 100644 --- a/lib/mv/authorization/permission_sets.ex +++ b/lib/mv/authorization/permission_sets.ex @@ -58,28 +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)] - defp role_read_all, do: [perm("Role", :read, :all)] - @doc """ Returns the list of all valid permission set names. @@ -116,22 +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)] ++ - role_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/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 @@ -148,26 +133,34 @@ 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() ++ - role_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) "/users/:id", "/users/:id/edit", "/users/:id/show/edit", - # Member list + # Member list and CSV export "/members", + "/members/export.csv", # Member detail "/members/:id", # Custom field values overview @@ -184,38 +177,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) - ] ++ - role_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: 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) @@ -223,6 +209,7 @@ defmodule Mv.Authorization.PermissionSets do "/users/:id/edit", "/users/:id/show/edit", "/members", + "/members/export.csv", # Create member "/members/new", "/members/:id", @@ -236,39 +223,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/authorization/role.ex b/lib/mv/authorization/role.ex index 8700a33..9c33e2d 100644 --- a/lib/mv/authorization/role.ex +++ b/lib/mv/authorization/role.ex @@ -37,8 +37,7 @@ defmodule Mv.Authorization.Role do """ use Ash.Resource, domain: Mv.Authorization, - data_layer: AshPostgres.DataLayer, - authorizers: [Ash.Policy.Authorizer] + data_layer: AshPostgres.DataLayer postgres do table "roles" @@ -87,13 +86,6 @@ defmodule Mv.Authorization.Role do end end - policies do - policy action_type([:read, :create, :update, :destroy]) do - description "Role access: read for all permission sets, create/update/destroy for admin only (PermissionSets)" - authorize_if Mv.Authorization.Checks.HasPermission - end - end - validations do validate one_of( :permission_set_name, @@ -181,18 +173,4 @@ defmodule Mv.Authorization.Role do |> Ash.Query.filter(name == "Mitglied") |> Ash.read_one(authorize?: false, domain: Mv.Authorization) end - - @doc """ - Returns the Admin role if it exists. - - Used by release tasks (e.g. seed_admin) and OIDC role sync to assign the admin role. - """ - @spec get_admin_role() :: {:ok, t() | nil} | {:error, term()} - def get_admin_role do - require Ash.Query - - __MODULE__ - |> Ash.Query.filter(name == "Admin") - |> Ash.read_one(authorize?: false, domain: Mv.Authorization) - end end 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 26b26d4..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 @@ -44,29 +40,26 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do defp sync_email(changeset) do Ash.Changeset.around_transaction(changeset, fn cs, callback -> result = callback.(cs) - apply_sync(result) + + with {:ok, record} <- Helpers.extract_record(result), + {:ok, user, member} <- get_user_and_member(record) do + # When called from Member-side, we need to update the member in the result + # When called from User-side, we update the linked member in DB only + case record do + %Mv.Membership.Member{} -> + # Member-side: Override member email in result with user email + Helpers.override_with_linked_email(result, user.email) + + %Mv.Accounts.User{} -> + # User-side: Sync user email to linked member in DB + Helpers.sync_email_to_linked_record(result, member, user.email) + end + else + _ -> result + end end) end - defp apply_sync(result) do - with {:ok, record} <- Helpers.extract_record(result), - {:ok, user, member} <- get_user_and_member(record) do - sync_by_record_type(result, record, user, member) - else - _ -> result - end - end - - # When called from Member-side, we update the member in the result. - # When called from User-side, we sync user email to the linked member in DB. - defp sync_by_record_type(result, %Mv.Membership.Member{}, user, _member) do - Helpers.override_with_linked_email(result, user.email) - end - - defp sync_by_record_type(result, %Mv.Accounts.User{}, user, member) do - Helpers.sync_email_to_linked_record(result, member, user.email) - end - # Retrieves user and member - works for both resource types # Uses system actor via Loader functions defp get_user_and_member(%Mv.Accounts.User{} = user) do diff --git a/lib/mv/email_sync/loader.ex b/lib/mv/email_sync/loader.ex index 31e0468..98f85df 100644 --- a/lib/mv/email_sync/loader.ex +++ b/lib/mv/email_sync/loader.ex @@ -3,15 +3,13 @@ defmodule Mv.EmailSync.Loader do Helper functions for loading linked records in email synchronization. Centralizes the logic for retrieving related User/Member entities. - ## Authorization-independent link checks + ## Authorization - All functions use the **system actor** for the load. Link existence - (linked vs not linked) is therefore determined **independently of the - current request actor**. This is required so that validations (e.g. - `EmailChangePermission`, `EmailNotUsedByOtherUser`) can correctly decide - "member is linked" even when the current user would not have read permission - on the related User. Using the request actor would otherwise allow - treating a linked member as unlinked and bypass the permission rule. + This module runs systemically and uses the system actor for all operations. + This ensures that email synchronization always works, regardless of user permissions. + + All functions use `Mv.Helpers.SystemActor.get_system_actor/0` to bypass + user permission checks, as email sync is a mandatory side effect. """ alias Mv.Helpers alias Mv.Helpers.SystemActor 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/custom_field_value_formatter.ex b/lib/mv/membership/custom_field_value_formatter.ex new file mode 100644 index 0000000..9709353 --- /dev/null +++ b/lib/mv/membership/custom_field_value_formatter.ex @@ -0,0 +1,55 @@ +defmodule Mv.Membership.CustomFieldValueFormatter do + @moduledoc """ + Neutral formatter for custom field values (e.g. CSV export). + + Same logic as the member overview Formatter but without Gettext or web helpers, + so it can be used from the Membership context. For boolean: "Yes"/"No"; + for date: European format (dd.mm.yyyy). + """ + @doc """ + Formats a custom field value for plain text (e.g. CSV). + + Handles nil, Ash.Union, JSONB map, and direct values. Uses custom_field.value_type + for typing. Boolean -> "Yes"/"No", Date -> dd.mm.yyyy. + """ + def format_custom_field_value(nil, _custom_field), do: "" + + def format_custom_field_value(%Ash.Union{value: value, type: type}, custom_field) do + format_value_by_type(value, type, custom_field) + end + + def format_custom_field_value(value, custom_field) when is_map(value) do + type = Map.get(value, "type") || Map.get(value, "_union_type") + val = Map.get(value, "value") || Map.get(value, "_union_value") + format_value_by_type(val, type, custom_field) + end + + def format_custom_field_value(value, custom_field) do + format_value_by_type(value, custom_field.value_type, custom_field) + end + + defp format_value_by_type(value, :string, _), do: to_string(value) + defp format_value_by_type(value, :integer, _), do: to_string(value) + + defp format_value_by_type(value, type, _) when type in [:string, :email] and is_binary(value) do + if String.trim(value) == "", do: "", else: value + end + + defp format_value_by_type(value, :email, _), do: to_string(value) + defp format_value_by_type(value, :boolean, _) when value == true, do: "Yes" + defp format_value_by_type(value, :boolean, _) when value == false, do: "No" + defp format_value_by_type(value, :boolean, _), do: to_string(value) + + defp format_value_by_type(%Date{} = date, :date, _) do + Calendar.strftime(date, "%d.%m.%Y") + end + + defp format_value_by_type(value, :date, _) when is_binary(value) do + case Date.from_iso8601(value) do + {:ok, date} -> Calendar.strftime(date, "%d.%m.%Y") + _ -> value + end + end + + defp format_value_by_type(value, _type, _), do: to_string(value) +end diff --git a/lib/mv/membership/member/validations/email_change_permission.ex b/lib/mv/membership/member/validations/email_change_permission.ex deleted file mode 100644 index 2b1c041..0000000 --- a/lib/mv/membership/member/validations/email_change_permission.ex +++ /dev/null @@ -1,75 +0,0 @@ -defmodule Mv.Membership.Member.Validations.EmailChangePermission do - @moduledoc """ - Validates that only admins or the linked user may change a linked member's email. - - This validation runs on member update when the email attribute is changing. - It allows the change only if: - - The member is not linked to a user, or - - The actor has the admin permission set (via `Mv.Authorization.Actor.admin?/1`), or - - The actor is the user linked to this member (actor.member_id == member.id). - - This prevents non-admins from changing another user's linked member email, - which would sync to that user's account and break email synchronization. - - Missing actor is not allowed; the system actor counts as admin (via `Actor.admin?/1`). - """ - use Ash.Resource.Validation - use Gettext, backend: MvWeb.Gettext, otp_app: :mv - - alias Mv.Authorization.Actor - alias Mv.EmailSync.Loader - - @doc """ - Validates that the actor may change the member's email when the member is linked. - - Only runs when the email attribute is changing (checked inside). Skips when - member is not linked. Allows when actor is admin or owns the linked member. - """ - @impl true - def validate(changeset, _opts, context) do - if Ash.Changeset.changing_attribute?(changeset, :email) do - validate_linked_member_email_change(changeset, context) - else - :ok - end - end - - defp validate_linked_member_email_change(changeset, context) do - linked_user = Loader.get_linked_user(changeset.data) - - if is_nil(linked_user) do - :ok - else - actor = resolve_actor(changeset, context) - member_id = changeset.data.id - - if Actor.admin?(actor) or actor_owns_member?(actor, member_id) do - :ok - else - msg = - dgettext( - "default", - "Only administrators or the linked user can change the email for members linked to users" - ) - - {:error, field: :email, message: msg} - end - end - end - - # Ash stores actor in changeset.context.private.actor; validation context has .actor; some callsites use context.actor - defp resolve_actor(changeset, context) do - ctx = changeset.context || %{} - - get_in(ctx, [:private, :actor]) || - Map.get(ctx, :actor) || - (context && Map.get(context, :actor)) - end - - defp actor_owns_member?(nil, _member_id), do: false - - defp actor_owns_member?(actor, member_id) do - actor_member_id = Map.get(actor, :member_id) || Map.get(actor, "member_id") - actor_member_id == member_id - end -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..f9fba1b 100644 --- a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex +++ b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex @@ -8,8 +8,6 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do This allows creating members with the same email as unlinked users. """ use Ash.Resource.Validation - - alias Mv.EmailSync.Loader alias Mv.Helpers require Logger @@ -34,8 +32,7 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do def validate(changeset, _opts, _context) do email_changing? = Ash.Changeset.changing_attribute?(changeset, :email) - linked_user = Loader.get_linked_user(changeset.data) - linked_user_id = if linked_user, do: linked_user.id, else: nil + linked_user_id = get_linked_user_id(changeset.data) is_linked? = not is_nil(linked_user_id) # Only validate if member is already linked AND email is changing @@ -56,7 +53,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 +73,19 @@ 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) + + defp get_linked_user_id(member_data) do + alias Mv.Helpers.SystemActor + + system_actor = SystemActor.get_system_actor() + opts = Helpers.ash_actor_opts(system_actor) + + case Ash.load(member_data, :user, opts) do + {:ok, %{user: %{id: id}}} -> id + _ -> nil + end + end end diff --git a/lib/mv/membership/member_export.ex b/lib/mv/membership/member_export.ex new file mode 100644 index 0000000..5f771cc --- /dev/null +++ b/lib/mv/membership/member_export.ex @@ -0,0 +1,344 @@ +defmodule Mv.Membership.MemberExport do + @moduledoc """ + Builds member list and column specs for CSV export. + + Used by `MvWeb.MemberExportController`. Does not perform translations; + the controller applies headers (e.g. via `MemberFields.label` / gettext) + and sends the download. + """ + + require Ash.Query + import Ash.Expr + + alias Mv.Membership.CustomField + alias Mv.Membership.Member + alias Mv.Membership.MemberExportSort + alias MvWeb.MemberLive.Index.MembershipFeeStatus + + @member_fields_allowlist (Mv.Constants.member_fields() |> Enum.map(&Atom.to_string/1)) ++ + ["membership_fee_status", "payment_status"] + @computed_export_fields ["membership_fee_status", "payment_status"] + @custom_field_prefix Mv.Constants.custom_field_prefix() + @domain_member_field_strings Mv.Constants.member_fields() |> Enum.map(&Atom.to_string/1) + + @doc """ + Fetches members and column specs for export. + + - `actor` - Ash actor (e.g. current user) + - `parsed` - Map from controller's parse_and_validate (selected_ids, member_fields, etc.) + + Returns `{:ok, members, column_specs}` or `{:error, :forbidden}`. + Column specs have `:kind`, `:key`, and for custom fields `:custom_field`; + the controller adds `:header` and optional computed columns to members before CSV export. + """ + @spec fetch(struct(), map()) :: + {:ok, [struct()], [map()]} | {:error, :forbidden} + def fetch(actor, parsed) do + custom_field_ids_union = + (parsed.custom_field_ids ++ Map.keys(parsed.boolean_filters || %{})) |> Enum.uniq() + + with {:ok, custom_fields_by_id} <- load_custom_fields_by_id(custom_field_ids_union, actor), + {:ok, members} <- load_members(actor, parsed, custom_fields_by_id) do + column_specs = build_column_specs(parsed, custom_fields_by_id) + {:ok, members, column_specs} + end + end + + defp load_custom_fields_by_id([], _actor), do: {:ok, %{}} + + defp load_custom_fields_by_id(custom_field_ids, actor) do + query = + CustomField + |> Ash.Query.filter(expr(id in ^custom_field_ids)) + |> Ash.Query.select([:id, :name, :value_type]) + + case Ash.read(query, actor: actor) do + {:ok, custom_fields} -> + by_id = + Enum.reduce(custom_field_ids, %{}, fn id, acc -> + case Enum.find(custom_fields, fn cf -> to_string(cf.id) == to_string(id) end) do + nil -> acc + cf -> Map.put(acc, id, cf) + end + end) + + {:ok, by_id} + + {:error, %Ash.Error.Forbidden{}} -> + {:error, :forbidden} + end + end + + defp build_column_specs(parsed, custom_fields_by_id) do + member_specs = + Enum.map(parsed.member_fields, fn f -> + if f in parsed.selectable_member_fields do + %{kind: :member_field, key: f} + else + %{kind: :computed, key: String.to_existing_atom(f)} + end + end) + + custom_specs = + parsed.custom_field_ids + |> Enum.map(fn id -> Map.get(custom_fields_by_id, id) end) + |> Enum.reject(&is_nil/1) + |> Enum.map(fn cf -> %{kind: :custom_field, key: cf.id, custom_field: cf} end) + + member_specs ++ custom_specs + end + + defp load_members(actor, parsed, custom_fields_by_id) do + select_fields = + [:id] ++ Enum.map(parsed.selectable_member_fields, &String.to_existing_atom/1) + + custom_field_ids_union = parsed.custom_field_ids ++ Map.keys(parsed.boolean_filters || %{}) + + need_cycles = + parsed.show_current_cycle or parsed.cycle_status_filter != nil or + parsed.computed_fields != [] + + query = + Member + |> Ash.Query.new() + |> Ash.Query.select(select_fields) + |> load_custom_field_values_query(custom_field_ids_union) + |> maybe_load_cycles(need_cycles, parsed.show_current_cycle) + + query = + if parsed.selected_ids != [] do + Ash.Query.filter(query, expr(id in ^parsed.selected_ids)) + else + query + |> apply_search(parsed.query) + |> then(fn q -> + {q, _sort_after_load} = maybe_sort(q, parsed.sort_field, parsed.sort_order) + q + end) + end + + case Ash.read(query, actor: actor) do + {:ok, members} -> + members = + if parsed.selected_ids == [] do + members + |> apply_cycle_status_filter(parsed.cycle_status_filter, parsed.show_current_cycle) + |> MvWeb.MemberLive.Index.apply_boolean_custom_field_filters( + parsed.boolean_filters || %{}, + Map.values(custom_fields_by_id) + ) + else + members + end + + members = + if parsed.selected_ids == [] and sort_after_load?(parsed.sort_field) do + sort_members_by_custom_field( + members, + parsed.sort_field, + parsed.sort_order, + Map.values(custom_fields_by_id) + ) + else + members + end + + {:ok, members} + + {:error, %Ash.Error.Forbidden{}} -> + {:error, :forbidden} + end + end + + defp load_custom_field_values_query(query, []), do: query + + defp load_custom_field_values_query(query, custom_field_ids) do + cfv_query = + Mv.Membership.CustomFieldValue + |> Ash.Query.filter(expr(custom_field_id in ^custom_field_ids)) + |> Ash.Query.load(custom_field: [:id, :name, :value_type]) + + Ash.Query.load(query, custom_field_values: cfv_query) + end + + defp apply_search(query, nil), do: query + defp apply_search(query, ""), do: query + + defp apply_search(query, q) when is_binary(q) do + if String.trim(q) != "" do + Member.fuzzy_search(query, %{query: q}) + else + query + end + end + + defp maybe_sort(query, nil, _order), do: {query, false} + defp maybe_sort(query, _field, nil), do: {query, false} + + defp maybe_sort(query, field, order) when is_binary(field) do + if custom_field_sort?(field) do + {query, true} + else + field_atom = String.to_existing_atom(field) + + if field_atom in (Mv.Constants.member_fields() -- [:notes]) do + {Ash.Query.sort(query, [{field_atom, String.to_existing_atom(order)}]), false} + else + {query, false} + end + end + rescue + ArgumentError -> {query, false} + end + + defp sort_after_load?(field) when is_binary(field), + do: String.starts_with?(field, @custom_field_prefix) + + defp sort_after_load?(_), do: false + + defp sort_members_by_custom_field(members, _field, _order, _custom_fields) when members == [], + do: [] + + defp sort_members_by_custom_field(members, field, order, custom_fields) when is_binary(field) do + id_str = String.trim_leading(field, @custom_field_prefix) + custom_field = Enum.find(custom_fields, fn cf -> to_string(cf.id) == id_str end) + if is_nil(custom_field), do: members + + key_fn = fn member -> + cfv = find_cfv(member, custom_field) + raw = if cfv, do: cfv.value, else: nil + MemberExportSort.custom_field_sort_key(custom_field.value_type, raw) + end + + members + |> Enum.map(fn m -> {m, key_fn.(m)} end) + |> Enum.sort(fn {_, ka}, {_, kb} -> MemberExportSort.key_lt(ka, kb, order) end) + |> Enum.map(fn {m, _} -> m end) + end + + defp find_cfv(member, custom_field) do + (member.custom_field_values || []) + |> Enum.find(fn cfv -> + to_string(cfv.custom_field_id) == to_string(custom_field.id) or + (Map.get(cfv, :custom_field) && + to_string(cfv.custom_field.id) == to_string(custom_field.id)) + end) + end + + defp custom_field_sort?(field), do: String.starts_with?(field, @custom_field_prefix) + + defp maybe_load_cycles(query, false, _show_current), do: query + + defp maybe_load_cycles(query, true, show_current) do + MembershipFeeStatus.load_cycles_for_members(query, show_current) + end + + defp apply_cycle_status_filter(members, nil, _show_current), do: members + + defp apply_cycle_status_filter(members, status, show_current) when status in [:paid, :unpaid] do + MembershipFeeStatus.filter_members_by_cycle_status(members, status, show_current) + end + + defp apply_cycle_status_filter(members, _status, _show_current), do: members + + # Called by controller to build parsed map from raw params (kept here so controller stays thin) + @doc """ + Parses and validates export params (from JSON payload). + + Returns a map with :selected_ids, :member_fields, :selectable_member_fields, + :computed_fields, :custom_field_ids, :query, :sort_field, :sort_order, + :show_current_cycle, :cycle_status_filter, :boolean_filters. + """ + @spec parse_params(map()) :: map() + def parse_params(params) do + member_fields = filter_allowed_member_fields(extract_list(params, "member_fields")) + {selectable_member_fields, computed_fields} = split_member_fields(member_fields) + + %{ + selected_ids: filter_valid_uuids(extract_list(params, "selected_ids")), + member_fields: member_fields, + selectable_member_fields: selectable_member_fields, + computed_fields: computed_fields, + custom_field_ids: filter_valid_uuids(extract_list(params, "custom_field_ids")), + query: extract_string(params, "query"), + sort_field: extract_string(params, "sort_field"), + sort_order: extract_sort_order(params), + show_current_cycle: extract_boolean(params, "show_current_cycle"), + cycle_status_filter: extract_cycle_status_filter(params), + boolean_filters: extract_boolean_filters(params) + } + end + + defp split_member_fields(member_fields) do + selectable = Enum.filter(member_fields, fn f -> f in @domain_member_field_strings end) + computed = Enum.filter(member_fields, fn f -> f in @computed_export_fields end) + {selectable, computed} + end + + defp extract_boolean(params, key) do + case Map.get(params, key) do + true -> true + "true" -> true + _ -> false + end + end + + defp extract_cycle_status_filter(params) do + case Map.get(params, "cycle_status_filter") do + "paid" -> :paid + "unpaid" -> :unpaid + _ -> nil + end + end + + defp extract_boolean_filters(params) do + case Map.get(params, "boolean_filters") do + map when is_map(map) -> + map + |> Enum.filter(fn {k, v} -> is_binary(k) and is_boolean(v) end) + |> Enum.filter(fn {k, _} -> match?({:ok, _}, Ecto.UUID.cast(k)) end) + |> Enum.into(%{}) + + _ -> + %{} + end + end + + defp extract_list(params, key) do + case Map.get(params, key) do + list when is_list(list) -> list + _ -> [] + end + end + + defp extract_string(params, key) do + case Map.get(params, key) do + s when is_binary(s) -> s + _ -> nil + end + end + + defp extract_sort_order(params) do + case Map.get(params, "sort_order") do + "asc" -> "asc" + "desc" -> "desc" + _ -> nil + end + end + + defp filter_allowed_member_fields(field_list) do + allowlist = MapSet.new(@member_fields_allowlist) + + field_list + |> Enum.filter(fn field -> is_binary(field) and MapSet.member?(allowlist, field) end) + |> Enum.uniq() + end + + defp filter_valid_uuids(id_list) when is_list(id_list) do + id_list + |> Enum.filter(fn id -> + is_binary(id) and match?({:ok, _}, Ecto.UUID.cast(id)) + end) + |> Enum.uniq() + end +end diff --git a/lib/mv/membership/member_export_sort.ex b/lib/mv/membership/member_export_sort.ex new file mode 100644 index 0000000..324fb75 --- /dev/null +++ b/lib/mv/membership/member_export_sort.ex @@ -0,0 +1,44 @@ +defmodule Mv.Membership.MemberExportSort do + @moduledoc """ + Type-stable sort keys for CSV export custom-field sorting. + + Used only by `MvWeb.MemberExportController` when sorting members by a custom field + after load. Nil values sort last in ascending order and first in descending order. + String and email comparison is case-insensitive. + """ + @doc """ + Returns a comparable sort key for (value_type, value). + + - Nil: rank 1 so that in asc order nil sorts last, in desc nil sorts first. + - date: chronological (ISO8601 string). + - boolean: false < true (0 < 1). + - integer: numerical order. + - string / email: case-insensitive (downcased). + + Handles Ash.Union in value; value_type is the custom field's value_type atom. + """ + @spec custom_field_sort_key(:string | :integer | :boolean | :date | :email, term()) :: + {0 | 1, term()} + def custom_field_sort_key(_value_type, nil), do: {1, nil} + + def custom_field_sort_key(value_type, %Ash.Union{value: value, type: _type}) do + custom_field_sort_key(value_type, value) + end + + def custom_field_sort_key(:date, %Date{} = d), do: {0, Date.to_iso8601(d)} + def custom_field_sort_key(:boolean, true), do: {0, 1} + def custom_field_sort_key(:boolean, false), do: {0, 0} + def custom_field_sort_key(:integer, v) when is_integer(v), do: {0, v} + def custom_field_sort_key(:string, v) when is_binary(v), do: {0, String.downcase(v)} + def custom_field_sort_key(:email, v) when is_binary(v), do: {0, String.downcase(v)} + def custom_field_sort_key(_value_type, v), do: {0, to_string(v)} + + @doc """ + Returns true if key_a should sort before key_b for the given order. + + "asc" -> nil last; "desc" -> nil first. No reverse of list needed. + """ + @spec key_lt({0 | 1, term()}, {0 | 1, term()}, String.t()) :: boolean() + def key_lt(key_a, key_b, "asc"), do: key_a < key_b + def key_lt(key_a, key_b, "desc"), do: key_b < key_a +end diff --git a/lib/mv/membership/members_csv.ex b/lib/mv/membership/members_csv.ex new file mode 100644 index 0000000..a0fd463 --- /dev/null +++ b/lib/mv/membership/members_csv.ex @@ -0,0 +1,100 @@ +defmodule Mv.Membership.MembersCSV do + @moduledoc """ + Exports members to CSV (RFC 4180) as iodata. + + Uses a column-based API: `export(members, columns)` where each column has + `header` (display string, e.g. from Web layer), `kind` (:member_field | :custom_field | :computed), + and `key` (member attribute name, custom_field id, or computed key). Custom field columns + include a `custom_field` struct for value formatting. Domain code does not use Gettext; + headers and computed values come from the caller (e.g. controller). + """ + alias Mv.Membership.CustomFieldValueFormatter + alias NimbleCSV.RFC4180 + + @doc """ + Exports a list of members to CSV iodata. + + - `members` - List of member structs or maps (with optional `custom_field_values` loaded) + - `columns` - List of column specs: `%{header: String.t(), kind: :member_field | :custom_field | :computed, key: term()}` + For `:custom_field`, also pass `custom_field: %CustomField{}`. Header is used as-is (localized by caller). + + Returns iodata suitable for `IO.iodata_to_binary/1` or sending as response body. + RFC 4180 escaping and formula-injection safe_cell are applied. + """ + @spec export([struct() | map()], [map()]) :: iodata() + def export(members, columns) when is_list(members) do + header = build_header(columns) + rows = Enum.map(members, fn member -> build_row(member, columns) end) + RFC4180.dump_to_iodata([header | rows]) + end + + defp build_header(columns) do + columns + |> Enum.map(fn col -> col.header end) + |> Enum.map(&safe_cell/1) + end + + defp build_row(member, columns) do + columns + |> Enum.map(fn col -> cell_value(member, col) end) + |> Enum.map(&safe_cell/1) + end + + defp cell_value(member, %{kind: :member_field, key: key}) do + key_atom = key_to_atom(key) + value = Map.get(member, key_atom) + format_member_value(value) + end + + defp cell_value(member, %{kind: :custom_field, key: id, custom_field: cf}) do + cfv = get_cfv_by_id(member, id) + + if cfv, + do: CustomFieldValueFormatter.format_custom_field_value(cfv.value, cf), + else: "" + end + + defp cell_value(member, %{kind: :computed, key: key}) do + value = Map.get(member, key_to_atom(key)) + if is_binary(value), do: value, else: "" + end + + defp key_to_atom(k) when is_atom(k), do: k + + defp key_to_atom(k) when is_binary(k) do + try do + String.to_existing_atom(k) + rescue + ArgumentError -> k + end + end + + defp get_cfv_by_id(member, id) do + values = + case Map.get(member, :custom_field_values) do + v when is_list(v) -> v + _ -> [] + end + + id_str = to_string(id) + + Enum.find(values, fn cfv -> + to_string(cfv.custom_field_id) == id_str or + (Map.get(cfv, :custom_field) && to_string(cfv.custom_field.id) == id_str) + end) + end + + @doc false + @spec safe_cell(String.t()) :: String.t() + def safe_cell(s) when is_binary(s) do + if String.starts_with?(s, ["=", "+", "-", "@", "\t"]), do: "'" <> s, else: s + end + + defp format_member_value(nil), do: "" + defp format_member_value(true), do: "true" + defp format_member_value(false), do: "false" + defp format_member_value(%Date{} = d), do: Date.to_iso8601(d) + defp format_member_value(%DateTime{} = dt), do: DateTime.to_iso8601(dt) + defp format_member_value(%NaiveDateTime{} = dt), do: NaiveDateTime.to_iso8601(dt) + defp format_member_value(value), do: to_string(value) +end diff --git a/lib/mv/oidc_role_sync.ex b/lib/mv/oidc_role_sync.ex deleted file mode 100644 index f268154..0000000 --- a/lib/mv/oidc_role_sync.ex +++ /dev/null @@ -1,148 +0,0 @@ -defmodule Mv.OidcRoleSync do - @moduledoc """ - Syncs user role from OIDC user_info (e.g. groups claim → Admin role). - - Used after OIDC registration (register_with_rauthy) and on sign-in so that - users in the configured admin group get the Admin role; others get Mitglied. - Configure via OIDC_ADMIN_GROUP_NAME and OIDC_GROUPS_CLAIM (see OidcRoleSyncConfig). - - Groups are read from user_info (ID token claims) first; if missing or empty, - the access_token from oauth_tokens is decoded as JWT and the groups claim is - read from there (e.g. Rauthy puts groups in the access token when scope - includes "groups"). - - ## JWT access token (security) - - The access_token payload is read without signature verification (peek only). - We rely on the fact that `oauth_tokens` is only ever passed from the - verified OIDC callback (Assent/AshAuthentication after provider token - exchange). If callers passed untrusted or tampered tokens, group claims - could be forged and a user could be assigned the Admin role. Therefore: - do not call this module with user-supplied tokens; it is intended only - for the internal flow from the OIDC callback. - """ - alias Mv.Accounts.User - alias Mv.Authorization.Role - alias Mv.OidcRoleSyncConfig - - @doc """ - Applies Admin or Mitglied role to the user based on OIDC groups claim. - - - If OIDC_ADMIN_GROUP_NAME is not configured: no-op, returns :ok without changing the user. - - If groups (from user_info or access_token) contain the configured admin group: assigns Admin role. - - Otherwise: assigns Mitglied role (downgrade if user was Admin). - - user_info is a map (e.g. from ID token claims); oauth_tokens is optional and may - contain "access_token" (JWT) from which the groups claim is read when not in user_info. - """ - @spec apply_admin_role_from_user_info(User.t(), map(), map() | nil) :: :ok - def apply_admin_role_from_user_info(user, user_info, oauth_tokens \\ nil) - when is_map(user_info) do - admin_group = OidcRoleSyncConfig.oidc_admin_group_name() - - if is_nil(admin_group) or admin_group == "" do - :ok - else - claim = OidcRoleSyncConfig.oidc_groups_claim() - groups = groups_from_user_info(user_info, claim) - - groups = - if Enum.empty?(groups), do: groups_from_access_token(oauth_tokens, claim), else: groups - - target_role = if admin_group in groups, do: :admin, else: :mitglied - set_user_role(user, target_role) - end - end - - defp groups_from_user_info(user_info, claim) do - value = user_info[claim] || user_info[String.to_existing_atom(claim)] - normalize_groups(value) - rescue - ArgumentError -> normalize_groups(user_info[claim]) - end - - defp groups_from_access_token(nil, _claim), do: [] - defp groups_from_access_token(oauth_tokens, _claim) when not is_map(oauth_tokens), do: [] - - defp groups_from_access_token(oauth_tokens, claim) do - access_token = oauth_tokens["access_token"] || oauth_tokens[:access_token] - - if is_binary(access_token) do - case peek_jwt_claims(access_token) do - {:ok, claims} -> - value = claims[claim] || safe_get_atom(claims, claim) - normalize_groups(value) - - _ -> - [] - end - else - [] - end - end - - defp safe_get_atom(map, key) when is_binary(key) do - try do - Map.get(map, String.to_existing_atom(key)) - rescue - ArgumentError -> nil - end - end - - defp safe_get_atom(_map, _key), do: nil - - defp peek_jwt_claims(token) do - parts = String.split(token, ".") - - if length(parts) == 3 do - [_h, payload_b64, _sig] = parts - - case Base.url_decode64(payload_b64, padding: false) do - {:ok, payload} -> Jason.decode(payload) - _ -> :error - end - else - :error - end - end - - defp normalize_groups(nil), do: [] - defp normalize_groups(list) when is_list(list), do: Enum.map(list, &to_string/1) - defp normalize_groups(single) when is_binary(single), do: [single] - defp normalize_groups(_), do: [] - - defp set_user_role(user, :admin) do - case Role.get_admin_role() do - {:ok, %Role{} = role} -> - do_set_role(user, role) - - _ -> - :ok - end - end - - defp set_user_role(user, :mitglied) do - case Role.get_mitglied_role() do - {:ok, %Role{} = role} -> - do_set_role(user, role) - - _ -> - :ok - end - end - - defp do_set_role(user, role) do - if user.role_id == role.id do - :ok - else - user - |> Ash.Changeset.for_update(:set_role_from_oidc_sync, %{role_id: role.id}) - |> Ash.Changeset.set_context(%{private: %{oidc_role_sync: true}}) - |> Ash.update(domain: Mv.Accounts, context: %{private: %{oidc_role_sync: true}}) - |> case do - {:ok, _} -> :ok - {:error, _} -> :ok - end - end - end -end diff --git a/lib/mv/oidc_role_sync_config.ex b/lib/mv/oidc_role_sync_config.ex deleted file mode 100644 index 493a435..0000000 --- a/lib/mv/oidc_role_sync_config.ex +++ /dev/null @@ -1,24 +0,0 @@ -defmodule Mv.OidcRoleSyncConfig do - @moduledoc """ - Runtime configuration for OIDC group → role sync (e.g. admin group → Admin role). - - Reads from Application config `:mv, :oidc_role_sync`: - - `:admin_group_name` – OIDC group name that maps to Admin role (optional; when nil, no sync). - - `:groups_claim` – JWT/user_info claim name for groups (default: `"groups"`). - - Set via ENV in production: OIDC_ADMIN_GROUP_NAME, OIDC_GROUPS_CLAIM (see config/runtime.exs). - """ - @doc "Returns the OIDC group name that maps to Admin role, or nil if not configured." - def oidc_admin_group_name do - get(:admin_group_name) - end - - @doc "Returns the JWT/user_info claim name for groups; defaults to \"groups\"." - def oidc_groups_claim do - get(:groups_claim) || "groups" - end - - defp get(key) do - Application.get_env(:mv, :oidc_role_sync, []) |> Keyword.get(key) - end -end diff --git a/lib/mv/release.ex b/lib/mv/release.ex index 54bc245..c0c2c8a 100644 --- a/lib/mv/release.ex +++ b/lib/mv/release.ex @@ -2,22 +2,9 @@ defmodule Mv.Release do @moduledoc """ Used for executing DB release tasks when run in production without Mix installed. - - ## Tasks - - - `migrate/0` - Runs all pending Ecto migrations. - - `seed_admin/0` - Ensures an admin user exists from ENV (ADMIN_EMAIL, ADMIN_PASSWORD - or ADMIN_PASSWORD_FILE). Idempotent; can be run on every deployment or via shell - to update the admin password without redeploying. """ @app :mv - alias Mv.Accounts - alias Mv.Accounts.User - alias Mv.Authorization.Role - - require Ash.Query - def migrate do load_app() @@ -31,158 +18,6 @@ defmodule Mv.Release do {:ok, _, _} = Ecto.Migrator.with_repo(repo, &Ecto.Migrator.run(&1, :down, to: version)) end - @doc """ - Ensures an admin user exists from ENV (ADMIN_EMAIL, ADMIN_PASSWORD or ADMIN_PASSWORD_FILE). - - Starts the application if not already running (required when called via `bin/mv eval`; - Ash/Telemetry need the running app). Idempotent. - - - If ADMIN_EMAIL is unset: no-op (idempotent). - - If ADMIN_PASSWORD (and ADMIN_PASSWORD_FILE) are unset and the user does not exist: - no user is created (no fallback password in production). - - If both ADMIN_EMAIL and ADMIN_PASSWORD are set: creates or updates the user with - Admin role and the given password. Safe to run on every deployment or via - `bin/mv eval "Mv.Release.seed_admin()"` to change the admin password without redeploying. - """ - def seed_admin do - # Ensure app (and Telemetry/Ash deps) are started when run via bin/mv eval - case Application.ensure_all_started(@app) do - {:ok, _} -> :ok - {:error, {app, reason}} -> raise "Failed to start #{inspect(app)}: #{inspect(reason)}" - end - - admin_email = get_env("ADMIN_EMAIL", nil) - admin_password = get_env_or_file("ADMIN_PASSWORD", nil) - - cond do - is_nil(admin_email) or admin_email == "" -> - :ok - - is_nil(admin_password) or admin_password == "" -> - ensure_admin_role_only(admin_email) - - true -> - ensure_admin_user(admin_email, admin_password) - end - end - - defp ensure_admin_role_only(email) do - case Role.get_admin_role() do - {:ok, nil} -> - :ok - - {:ok, %Role{} = admin_role} -> - case get_user_by_email(email) do - {:ok, %User{} = user} -> - user - |> Ash.Changeset.for_update(:update, %{}) - |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) - |> Ash.update!(authorize?: false) - - :ok - - _ -> - :ok - end - - {:error, _} -> - :ok - end - end - - defp ensure_admin_user(email, password) do - if is_nil(password) or password == "" do - :ok - else - do_ensure_admin_user(email, password) - end - end - - defp do_ensure_admin_user(email, password) do - case Role.get_admin_role() do - {:ok, nil} -> - # Admin role does not exist (e.g. migrations not run); skip - :ok - - {:ok, %Role{} = admin_role} -> - case get_user_by_email(email) do - {:ok, nil} -> - create_admin_user(email, password, admin_role) - - {:ok, user} -> - update_admin_user(user, password, admin_role) - - {:error, _} -> - :ok - end - - {:error, _} -> - :ok - end - end - - defp create_admin_user(email, password, admin_role) do - case Accounts.create_user(%{email: email}, authorize?: false) do - {:ok, user} -> - user - |> Ash.Changeset.for_update(:admin_set_password, %{password: password}) - |> Ash.update!(authorize?: false) - |> then(fn u -> - u - |> Ash.Changeset.for_update(:update, %{}) - |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) - |> Ash.update!(authorize?: false) - end) - - :ok - - {:error, _} -> - :ok - end - end - - defp update_admin_user(user, password, admin_role) do - user - |> Ash.Changeset.for_update(:admin_set_password, %{password: password}) - |> Ash.update!(authorize?: false) - |> then(fn u -> - u - |> Ash.Changeset.for_update(:update, %{}) - |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) - |> Ash.update!(authorize?: false) - end) - - :ok - end - - defp get_user_by_email(email) do - User - |> Ash.Query.filter(email == ^email) - |> Ash.read_one(authorize?: false, domain: Mv.Accounts) - end - - defp get_env(key, default) do - System.get_env(key, default) - end - - defp get_env_or_file(var_name, default) do - file_var = "#{var_name}_FILE" - - case System.get_env(file_var) do - nil -> - System.get_env(var_name, default) - - file_path -> - case File.read(file_path) do - {:ok, content} -> - String.trim_trailing(content) - - {:error, _} -> - default - end - end - end - defp repos do Application.fetch_env!(@app, :ecto_repos) end diff --git a/lib/mv_web/authorization.ex b/lib/mv_web/authorization.ex index d821416..d20be7d 100644 --- a/lib/mv_web/authorization.ex +++ b/lib/mv_web/authorization.ex @@ -97,18 +97,12 @@ defmodule MvWeb.Authorization do @doc """ Checks if user can access a specific page. - Nil-safe: returns false when user is nil (e.g. unauthenticated or layout - assigns regression), so callers do not need to guard. - ## Examples iex> admin = %{role: %{permission_set_name: "admin"}} iex> can_access_page?(admin, "/admin/roles") true - iex> can_access_page?(nil, "/members") - false - iex> mitglied = %{role: %{permission_set_name: "own_data"}} iex> can_access_page?(mitglied, "/members") false diff --git a/lib/mv_web/components/core_components.ex b/lib/mv_web/components/core_components.ex index 9ef8f2b..e74020c 100644 --- a/lib/mv_web/components/core_components.ex +++ b/lib/mv_web/components/core_components.ex @@ -97,13 +97,12 @@ defmodule MvWeb.CoreComponents do <.button navigate={~p"/"}>Home <.button disabled={true}>Disabled """ - attr :rest, :global, include: ~w(href navigate patch method data-testid) + attr :rest, :global, include: ~w(href navigate patch method) attr :variant, :string, values: ~w(primary) attr :disabled, :boolean, default: false, doc: "Whether the button is disabled" slot :inner_block, required: true - def button(assigns) do - rest = assigns.rest + def button(%{rest: rest} = assigns) do variants = %{"primary" => "btn-primary", nil => "btn-primary btn-soft"} assigns = assign(assigns, :class, Map.fetch!(variants, assigns[:variant])) @@ -179,7 +178,8 @@ defmodule MvWeb.CoreComponents do aria-haspopup="menu" aria-expanded={@open} aria-controls={@id} - class="btn" + aria-label={@button_label} + class="btn focus:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:ring-base-content/20" phx-click="toggle_dropdown" phx-target={@phx_target} data-testid="dropdown-button" @@ -233,11 +233,12 @@ defmodule MvWeb.CoreComponents do