diff --git a/Justfile b/Justfile index b835cf4..c68c473 100644 --- a/Justfile +++ b/Justfile @@ -32,6 +32,8 @@ lint: mix format --check-formatted mix compile --warnings-as-errors mix credo + # Check that all German translations are filled (UI must be in German) + @bash -c 'for file in priv/gettext/de/LC_MESSAGES/*.po; do awk "/^msgid \"\"$/{header=1; next} /^msgid /{header=0} /^msgstr \"\"$/ && !header{print FILENAME\":\"NR\": \" \$0; exit 1}" "$file" || exit 1; done' mix gettext.extract --check-up-to-date audit: @@ -116,4 +118,4 @@ init-prod-secrets: # Start production environment with Docker Compose start-prod: init-prod-secrets - docker compose -f docker-compose.prod.yml up -d \ No newline at end of file + docker compose -f docker-compose.prod.yml up -d diff --git a/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md b/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md deleted file mode 100644 index eebf419..0000000 --- a/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md +++ /dev/null @@ -1,318 +0,0 @@ ---- -name: Code Review Fixes - Membership Fee Features -overview: Umsetzung der validen Code Review Punkte aus beiden Reviews mit Priorisierung nach Kritikalität. Fokus auf Transaktionssicherheit, Code-Qualität, Performance und UX-Verbesserungen. -todos: - - id: fix-after-action-tasks - content: "after_action mit Task.start → after_transaction + Task.Supervisor: Task.Supervisor zu application.ex hinzufügen, after_action Hooks in after_transaction umwandeln, Task.Supervisor.async_nolink verwenden" - status: pending - - id: reduce-code-duplication - content: "Code-Duplikation reduzieren: handle_cycle_generation/2 private Funktion extrahieren, alle drei Stellen (Create, Type Change, Date Change) verwenden" - status: pending - dependencies: - - fix-after-action-tasks - - id: fix-join-date-validation - content: "join_date Validierung: Entweder Validierung wieder hinzufügen (validate compare(:join_date, less_than_or_equal_to: &Date.utc_today/0)) oder Dokumentation anpassen" - status: pending - - id: fix-load-cycles-docs - content: "load_cycles_for_members: Entweder Dokumentation korrigieren (ehrlich machen) oder echte Filterung implementieren (z.B. nur letzte 2 Intervalle)" - status: pending - - id: fix-get-current-cycle-sort - content: "get_current_cycle nondeterministisch: Vor List.first() nach cycle_start sortieren (desc) in MembershipFeeHelpers.get_current_cycle" - status: pending - - id: fix-n1-query-member-count - content: "N+1 Query beheben: Aggregate auf MembershipFeeType definieren oder member_count einmalig vorab laden und in assigns cachen" - status: pending - - id: fix-assign-new-stale - content: "assign_new → assign: In MembershipFeesComponent.update/2 immer assign(:cycles, cycles) und assign(:available_fee_types, available_fee_types) setzen" - status: pending - - id: fix-regenerating-flag - content: "@regenerating auf true setzen: Direkt beim Event-Start in handle_event(\"regenerate_cycles\", ...) socket |> assign(:regenerating, true) setzen" - status: pending - - id: fix-create-cycle-parsing - content: "Create-cycle parsing Fix: Decimal.parse explizit behandeln und {:error, :invalid_amount} zurückgeben statt :error" - status: pending - - id: fix-delete-all-atomic - content: "Delete all cycles atomar: Bulk Delete Query verwenden (Ash bulk destroy oder Query-basiert) statt Enum.map" - status: pending - - id: improve-async-error-handling - content: "Fehlerbehandlung bei async Tasks: Strukturierte Error-Logs mit Context, optional Retry-Mechanismus oder Event-System für Benachrichtigung" - status: pending - - id: improve-format-currency - content: "format_currency Robustheit: Number.Currency verwenden oder robusteres Pattern Matching + Tests für Edge Cases (negative Zahlen, sehr große Zahlen)" - status: pending - - id: add-missing-typespecs - content: "Fehlende Typespecs: @spec für SetDefaultMembershipFeeType.change/3 hinzufügen" - status: pending - - id: fix-race-condition - content: "Potenzielle Race Condition: Prüfen ob Ash doppelte Auslösung verhindert, ggf. Logik anpassen (beide Änderungen in einem Hook zusammenfassen)" - status: pending - - id: extract-magic-values - content: "Magic Numbers/Strings: Application.get_env(:mv, :sql_sandbox, false) in Konstante/Helper extrahieren (z.B. Mv.Config.sql_sandbox?/0)" - status: pending - - id: fix-domain-consistency - content: "Domain-Konsistenz: Überall in MembershipFeesComponent domain: MembershipFees explizit angeben" - status: pending - - id: fix-test-helper - content: "Test-Helper Fix: create_cycle/3 Helper - Cleanup nur einmal im Setup oder gezielt nur auto-generierte löschen" - status: pending - - id: fix-date-utc-today-param - content: "Date.utc_today() Parameter: today Parameter durchgeben in get_cycle_status_for_member und Helper-Funktionen" - status: pending - - id: fix-ui-locale-input - content: "UI/Locale Input Fix: type=\"number\" → type=\"text\" + inputmode=\"decimal\" + serverseitig \",\" → \".\" normalisieren" - status: pending - - id: fix-delete-confirmation - content: "Delete-all-Confirmation robuster: String.trim() + case-insensitive Vergleich oder \"type DELETE\" Pattern" - status: pending - - id: fix-warning-state - content: "Warning-State Fix: Bei Decimal.parse(:error) explizit hide_amount_warning(socket) aufrufen" - status: pending - - id: fix-double-toggle - content: "Toggle entfernen: Toggle-Button im Spalten-Header entfernen (nur in Toolbar behalten)" - status: pending - - id: fix-format-consistency - content: "Format-Konsistenz: Inputs ebenfalls auf Komma ausrichten oder serverseitig normalisieren" - status: pending - dependencies: - - fix-ui-locale-input ---- - -# Code Review Fixes - Membership Fee Features - -## Kritische Probleme (Müssen vor Merge behoben werden) - -### 1. after_action mit Task.start - Transaktionsprobleme - -**Dateien:** `lib/membership/member.ex` (Zeilen 142, 279) - -**Problem:** `Task.start/1` wird innerhalb von `after_action` Hooks verwendet. `after_action` läuft innerhalb der DB-Transaktion, daher: - -- Tasks sehen möglicherweise noch nicht committed state -- Tasks werden auch bei Rollback gestartet -- Keine Supervision → Memory Leaks möglich - -**Lösung:** - -- `after_transaction` Hook verwenden (Ash Best Practice) -- `Task.Supervisor` zum Supervision Tree hinzufügen (`lib/mv/application.ex`) -- `Task.Supervisor.async_nolink/3` statt `Task.start/1` verwenden - -**Betroffene Stellen:** - -- Member Creation (Zeile 116-164) -- Join/Exit Date Change (Zeile 250-301) - -### 2. Code-Duplikation in Cycle-Generation-Logik - -**Datei:** `lib/membership/member.ex` - -**Problem:** Cycle-Generation-Logik ist dreimal dupliziert (Create, Type Change, Date Change) - -**Lösung:** Extrahiere in private Funktion `handle_cycle_generation/2` - -## Wichtige Probleme (Sollten behoben werden) - -### 3. join_date Validierung entfernt, aber Dokumentation behauptet Gegenteil - -**Datei:** `lib/membership/member.ex` (Zeile 27, 516-518) - -**Problem:** Dokumentation sagt "join_date not in future", aber Validierung fehlt - -**Lösung:** Dokumentation anpassen - -### 4. load_cycles_for_members overpromises - -**Datei:** `lib/mv_web/member_live/index/membership_fee_status.ex` (Zeile 36-40) - -**Problem:** Dokumentation sagt "Only loads the relevant cycle per member" und "Filters cycles at database level", aber lädt alle Cycles - -**Lösung:** echte Filterung implementieren (z.B. nur letzte 2 Intervalle) - -### 5. get_current_cycle nondeterministisch - -**Datei:** `lib/mv_web/helpers/membership_fee_helpers.ex` (Zeile 178-182) - -**Problem:** `List.first()` ohne explizite Sortierung → Ergebnis hängt von Reihenfolge ab - -**Lösung:** Vor `List.first()` nach `cycle_start` sortieren (desc) - -### 6. N+1 Query durch get_member_count - -**Datei:** `lib/mv_web/live/membership_fee_type_live/index.ex` (Zeile 134-140) - -**Problem:** `get_member_count/1` wird pro Row aufgerufen → N+1 Query - -**Lösung:** Aggregate auf MembershipFeeType definieren oder einmalig vorab laden - -### 7. assign_new kann stale werden - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 402-403) - -**Problem:** `assign_new(:cycles, ...)` und `assign_new(:available_fee_types, ...)` werden nur gesetzt, wenn Assign noch nicht existiert - -**Lösung:** In `update/2` immer `assign(:cycles, cycles)` / `assign(:available_fee_types, available_fee_types)` setzen - -### 8. @regenerating wird nie auf true gesetzt - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 526-561) - -**Problem:** `regenerating` wird nur auf `false` gesetzt, nie auf `true` → Button/Spinner werden nie disabled - -**Lösung:** Direkt beim Event-Start `socket |> assign(:regenerating, true)` setzen - -### 9. Create-cycle parsing: invalid amount zeigt falsche Fehlermeldung - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 748-812) - -**Problem:** `Decimal.parse/1` gibt `:error` zurück, aber `with` behandelt es als `:error` → landet in "Invalid date format" Branch - -**Lösung:** Explizit `{:error, :invalid_amount}` zurückgeben: - -```elixir -amount = case Decimal.parse(amount_str) do - {d, _} -> {:ok, d} - :error -> {:error, :invalid_amount} -end -``` - -### 10. Delete all cycles: nicht atomar - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 666-714) - -**Problem:** `Enum.map(cycles, &Ash.destroy/1)` → nicht atomar, teilweise gelöscht möglich - -**Lösung:** Bulk Delete Query verwenden (Ash bulk destroy oder Query-basiert) - -### 11. Fehlerbehandlung bei async Tasks - -**Datei:** `lib/membership/member.ex` - -**Problem:** Bei Fehlern in async Tasks wird nur geloggt, aber der Benutzer erhält keine Rückmeldung. Die Member-Aktion wird als erfolgreich zurückgegeben, auch wenn die Cycle-Generierung fehlschlägt. Keine Retry-Logik oder Monitoring. - -**Lösung:** - -- Für kritische Fälle: synchron ausführen oder Retry-Mechanismus implementieren -- Für nicht-kritische Fälle: Event-System für spätere Benachrichtigung -- Strukturierte Error-Logs mit Context -- Optional: Error-Tracking (Sentry, etc.) - -### 12. format_currency Robustheit - -**Datei:** `lib/mv_web/helpers/membership_fee_helpers.ex` (Zeilen 27-51) - -**Problem:** Die Funktion verwendet String-Manipulation für Formatierung. Edge Cases könnten problematisch sein (z.B. sehr große Zahlen, negative Werte). - -**Lösung:** - -- `Number.Currency` oder ähnliche Bibliothek verwenden -- Oder: Robusteres Pattern Matching für Edge Cases -- Tests für Edge Cases hinzufügen (negative Zahlen, sehr große Zahlen) - -### 13. Fehlende Typespecs - -**Datei:** `lib/membership/member/changes/set_default_membership_fee_type.ex` - -**Problem:** Keine `@spec` für die `change/3` Funktion. - -**Lösung:** Typespecs hinzufügen für bessere Dokumentation und Dialyzer-Support. - -### 14. Potenzielle Race Condition - -**Datei:** `lib/membership/member.ex` (Zeile 250-301) - -**Problem:** Wenn `join_date` und `exit_date` gleichzeitig geändert werden, könnte die Cycle-Generierung zweimal ausgelöst werden (einmal pro Änderung). - -**Lösung:** Prüfen, ob Ash dies bereits verhindert, oder Logik anpassen (z.B. beide Änderungen in einem Hook zusammenfassen). - -### 15. Magic Numbers/Strings - -**Problem:** `Application.get_env(:mv, :sql_sandbox, false)` wird mehrfach verwendet. - -**Lösung:** Extrahiere in Konstante oder Helper-Funktion (z.B. `Mv.Config.sql_sandbox?/0`). - -## Mittlere Probleme (Nice-to-have) - -### 16. Inconsistent use of domain - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 819-821) - -**Problem:** Einige Actions verwenden `domain: MembershipFees`, andere nicht - -**Lösung:** Konsistent `domain` überall verwenden - -### 17. Tests: create_cycle/3 löscht jedes Mal alle Cycles - -**Datei:** `test/mv_web/member_live/index/membership_fee_status_test.exs` (Zeile 45-52) - -**Problem:** Helper löscht vor jedem Create alle Cycles → Tests prüfen nicht, was sie denken - -**Lösung:** Cleanup nur einmal im Setup oder gezielt nur auto-generierte löschen - -### 18. Tests/Design: Date.utc_today() macht Tests flaky - -**Problem:** Tests hängen von `Date.utc_today()` ab → nicht deterministisch - -**Lösung:** `today` Parameter durchgeben (z.B. `get_cycle_status_for_member(member, show_current, today \\ Date.utc_today())`) - -### 19. UI/Locale: input type="number" + Decimal/Komma - -**Problem:** `type="number"` funktioniert nicht zuverlässig mit Komma als Dezimaltrenner - -**Lösung:** `type="text"` + `inputmode="decimal"` + serverseitig "," → "." normalisieren - -### 20. Delete-all-Confirmation: String-Vergleich ist fragil - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 296-298) - -**Problem:** String-Vergleich gegen `gettext("Yes")` und `"Yes"` → fragil bei Whitespace/Locale - -**Lösung:** `String.trim()` + case-insensitive Vergleich oder "type DELETE" Pattern - -### 21. MembershipFeeType Form: Warning-State kann hängen bleiben - -**Datei:** `lib/mv_web/live/membership_fee_type_live/form.ex` (Zeile 367-378) - -**Problem:** Bei `Decimal.parse(:error)` wird nur `socket` zurückgegeben → Warning kann stehen bleiben - -**Lösung:** Bei `:error` explizit `hide_amount_warning(socket)` aufrufen - -### 22. UI/UX: Toggle ist doppelt vorhanden - -**Datei:** `lib/mv_web/live/member_live/index.html.heex` (Zeile 45-72, 284-296) - -**Problem:** Toggle-Button sowohl in Toolbar als auch im Spalten-Header - -**Lösung:** Toggle im Spalten-Header entfernen (nur in Toolbar behalten) - -### 23. Konsistenz: format_currency vs Inputs - -**Problem:** `format_currency` formatiert deutsch (Komma), aber Inputs erwarten Punkt - -**Lösung:** Inputs ebenfalls auf Komma ausrichten oder serverseitig normalisieren - -## Implementierungsreihenfolge - -1. **Kritisch:** after_action → after_transaction + Task.Supervisor -2. **Kritisch:** Code-Duplikation reduzieren -3. **Wichtig:** join_date Validierung/Dokumentation -4. **Wichtig:** load_cycles_for_members Dokumentation/Implementierung -5. **Wichtig:** get_current_cycle Sortierung -6. **Wichtig:** N+1 Query beheben -7. **Wichtig:** assign_new → assign -8. **Wichtig:** @regenerating auf true setzen -9. **Wichtig:** Create-cycle parsing Fix -10. **Wichtig:** Delete all cycles atomar -11. **Wichtig:** Fehlerbehandlung bei async Tasks -12. **Wichtig:** format_currency Robustheit -13. **Wichtig:** Fehlende Typespecs -14. **Wichtig:** Potenzielle Race Condition prüfen/beheben -15. **Wichtig:** Magic Numbers/Strings extrahieren -16. **Mittel:** Domain-Konsistenz -17. **Mittel:** Test-Helper Fix -18. **Mittel:** Date.utc_today() Parameter -19. **Mittel:** UI/Locale Fixes -20. **Mittel:** String-Vergleich robuster -21. **Mittel:** Warning-State Fix -22. **Mittel:** Toggle entfernen -23. **Mittel:** Format-Konsistenz - diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md index b44604b..8c89af7 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -110,8 +110,8 @@ Control access to LiveView pages: Three scope levels for permissions: - **:own** - Only records where `record.id == user.id` (for User resource) - **:linked** - Only records linked to user via relationships - - Member: `member.user_id == user.id` - - CustomFieldValue: `custom_field_value.member.user_id == user.id` + - Member: `id == user.member_id` (User.member_id → Member.id, inverse relationship) + - CustomFieldValue: `member_id == user.member_id` (traverses Member → User relationship) - **:all** - All records, no filtering **6. Special Cases** @@ -714,8 +714,8 @@ defmodule Mv.Authorization.Checks.HasPermission do - **:all** - Authorizes without filtering (returns all records) - **:own** - Filters to records where record.id == actor.id - **:linked** - Filters based on resource type: - - Member: member.user_id == actor.id - - CustomFieldValue: custom_field_value.member.user_id == actor.id (traverses relationship!) + - Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship) + - CustomFieldValue: `member_id == actor.member_id` (CustomFieldValue.member_id → Member.id → User.member_id) ## Error Handling @@ -799,12 +799,14 @@ defmodule Mv.Authorization.Checks.HasPermission do defp apply_scope(:linked, actor, resource_name) do case resource_name do "Member" -> - # Member.user_id == actor.id (direct relationship) - {:filter, expr(user_id == ^actor.id)} + # User.member_id → Member.id (inverse relationship) + # Filter: member.id == actor.member_id + {:filter, expr(id == ^actor.member_id)} "CustomFieldValue" -> - # CustomFieldValue.member.user_id == actor.id (traverse through member!) - {:filter, expr(member.user_id == ^actor.id)} + # CustomFieldValue.member_id → Member.id → User.member_id + # Filter: custom_field_value.member_id == actor.member_id + {:filter, expr(member_id == ^actor.member_id)} _ -> # Fallback for other resources: try direct user_id @@ -918,7 +920,7 @@ end **Location:** `lib/mv/membership/member.ex` -**Special Case:** Users can always access their linked member (where `member.user_id == user.id`). +**Special Case:** Users can always READ their linked member (where `id == user.member_id`). ```elixir defmodule Mv.Membership.Member do @@ -978,10 +980,10 @@ defmodule Mv.Membership.CustomFieldValue do policies do # SPECIAL CASE: Users can access custom field values of their linked member - # Note: This traverses the member relationship! + # Note: This uses member_id relationship (CustomFieldValue.member_id → Member.id → User.member_id) policy action_type([:read, :update]) do description "Users can access custom field values of their linked member" - authorize_if expr(member.user_id == ^actor(:id)) + authorize_if expr(member_id == ^actor(:member_id)) end # GENERAL: Check permissions from role diff --git a/docs/roles-and-permissions-overview.md b/docs/roles-and-permissions-overview.md index 86e7273..61aef9c 100644 --- a/docs/roles-and-permissions-overview.md +++ b/docs/roles-and-permissions-overview.md @@ -294,7 +294,9 @@ Each Permission Set contains: **:own** - Only records where id == actor.id - Example: User can read their own User record -**:linked** - Only records where user_id == actor.id +**:linked** - Only records linked to actor via relationships +- Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship) +- CustomFieldValue: `member_id == actor.member_id` (traverses Member → User relationship) - Example: User can read Member linked to their account **:all** - All records without restriction diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 6ae9307..31d51cc 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -34,10 +34,12 @@ defmodule Mv.Membership.Member do """ use Ash.Resource, domain: Mv.Membership, - data_layer: AshPostgres.DataLayer + data_layer: AshPostgres.DataLayer, + authorizers: [Ash.Policy.Authorizer] require Ash.Query import Ash.Expr + alias Mv.Helpers require Logger # Module constants @@ -116,11 +118,12 @@ defmodule Mv.Membership.Member do # Only runs if membership_fee_type_id is set # Note: Cycle generation runs asynchronously to not block the action, # but in test environment it runs synchronously for DB sandbox compatibility - change after_transaction(fn _changeset, result, _context -> + change after_transaction(fn changeset, result, _context -> case result do {:ok, member} -> if member.membership_fee_type_id && member.join_date do - handle_cycle_generation(member) + actor = Map.get(changeset.context, :actor) + handle_cycle_generation(member, actor: actor) end {:error, _} -> @@ -191,7 +194,9 @@ defmodule Mv.Membership.Member do Ash.Changeset.changing_attribute?(changeset, :membership_fee_type_id) if fee_type_changed && member.membership_fee_type_id && member.join_date do - case regenerate_cycles_on_type_change(member) do + actor = Map.get(changeset.context, :actor) + + case regenerate_cycles_on_type_change(member, actor: actor) do {:ok, notifications} -> # Return notifications to Ash - they will be sent automatically after commit {:ok, member, notifications} @@ -223,7 +228,8 @@ defmodule Mv.Membership.Member do exit_date_changed = Ash.Changeset.changing_attribute?(changeset, :exit_date) if (join_date_changed || exit_date_changed) && member.membership_fee_type_id do - handle_cycle_generation(member) + actor = Map.get(changeset.context, :actor) + handle_cycle_generation(member, actor: actor) end {:error, _} -> @@ -294,6 +300,41 @@ defmodule Mv.Membership.Member do end end + # Authorization Policies + # Order matters: Most specific policies first, then general permission check + policies do + # SYSTEM OPERATIONS: Allow CRUD operations without actor (TEST ENVIRONMENT ONLY) + # In test: All operations allowed (for test fixtures) + # In production/dev: ALL operations denied without actor (fail-closed for security) + # NoActor.check uses compile-time environment detection to prevent security issues + bypass action_type([:create, :read, :update, :destroy]) do + description "Allow system operations without actor (test environment only)" + authorize_if Mv.Authorization.Checks.NoActor + end + + # SPECIAL CASE: Users can always READ their linked member + # This allows users with ANY permission set to read their own linked member + # Check using the inverse relationship: User.member_id → Member.id + bypass action_type(:read) do + description "Users can always read member linked to their account" + authorize_if expr(id == ^actor(:member_id)) + end + + # GENERAL: Check permissions from user's role + # HasPermission handles update permissions correctly: + # - :own_data → can update linked member (scope :linked) + # - :read_only → cannot update any member (no update permission) + # - :normal_user → can update all members (scope :all) + # - :admin → can update all members (scope :all) + policy action_type([:read, :create, :update, :destroy]) do + description "Check permissions from user's role and permission set" + authorize_if Mv.Authorization.Checks.HasPermission + end + + # DEFAULT: Forbid if no policy matched + # Ash implicitly forbids if no policy authorized + end + @doc """ Filters members list based on email match priority. @@ -361,8 +402,13 @@ defmodule Mv.Membership.Member do user_id = user_arg[:id] current_member_id = changeset.data.id + # Get actor from changeset context for authorization + # If no actor is present, this will fail in production (fail-closed) + actor = Map.get(changeset.context || %{}, :actor) + # Check the current state of the user in the database - case Ash.get(Mv.Accounts.User, user_id) do + # Pass actor to ensure proper authorization (User might have policies in future) + case Ash.get(Mv.Accounts.User, user_id, actor: actor) do # User is free to be linked {:ok, %{member_id: nil}} -> :ok @@ -737,33 +783,37 @@ defmodule Mv.Membership.Member do # Returns {:ok, notifications} or {:error, reason} where notifications are collected # to be sent after transaction commits @doc false - def regenerate_cycles_on_type_change(member) do + def regenerate_cycles_on_type_change(member, opts \\ []) do today = Date.utc_today() lock_key = :erlang.phash2(member.id) + actor = Keyword.get(opts, :actor) # Use advisory lock to prevent concurrent deletion and regeneration # This ensures atomicity when multiple updates happen simultaneously if Mv.Repo.in_transaction?() do - regenerate_cycles_in_transaction(member, today, lock_key) + regenerate_cycles_in_transaction(member, today, lock_key, actor: actor) else - regenerate_cycles_new_transaction(member, today, lock_key) + regenerate_cycles_new_transaction(member, today, lock_key, actor: actor) end end # Already in transaction: use advisory lock directly # Returns {:ok, notifications} - notifications should be returned to after_action hook - defp regenerate_cycles_in_transaction(member, today, lock_key) do + defp regenerate_cycles_in_transaction(member, today, lock_key, opts) do + actor = Keyword.get(opts, :actor) Ecto.Adapters.SQL.query!(Mv.Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - do_regenerate_cycles_on_type_change(member, today, skip_lock?: true) + do_regenerate_cycles_on_type_change(member, today, skip_lock?: true, actor: actor) end # Not in transaction: start new transaction with advisory lock # Returns {:ok, notifications} - notifications should be sent by caller (e.g., via after_action) - defp regenerate_cycles_new_transaction(member, today, lock_key) do + defp regenerate_cycles_new_transaction(member, today, lock_key, opts) do + actor = Keyword.get(opts, :actor) + Mv.Repo.transaction(fn -> Ecto.Adapters.SQL.query!(Mv.Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - case do_regenerate_cycles_on_type_change(member, today, skip_lock?: true) do + case do_regenerate_cycles_on_type_change(member, today, skip_lock?: true, actor: actor) do {:ok, notifications} -> # Return notifications - they will be sent by the caller notifications @@ -785,6 +835,7 @@ defmodule Mv.Membership.Member do require Ash.Query skip_lock? = Keyword.get(opts, :skip_lock?, false) + actor = Keyword.get(opts, :actor) # Find all unpaid cycles for this member # We need to check cycle_end for each cycle using its own interval @@ -794,10 +845,21 @@ defmodule Mv.Membership.Member do |> Ash.Query.filter(status == :unpaid) |> Ash.Query.load([:membership_fee_type]) - case Ash.read(all_unpaid_cycles_query) do + result = + if actor do + Ash.read(all_unpaid_cycles_query, actor: actor) + else + Ash.read(all_unpaid_cycles_query) + end + + case result do {:ok, all_unpaid_cycles} -> cycles_to_delete = filter_future_cycles(all_unpaid_cycles, today) - delete_and_regenerate_cycles(cycles_to_delete, member.id, today, skip_lock?: skip_lock?) + + delete_and_regenerate_cycles(cycles_to_delete, member.id, today, + skip_lock?: skip_lock?, + actor: actor + ) {:error, reason} -> {:error, reason} @@ -826,13 +888,14 @@ defmodule Mv.Membership.Member do # Returns {:ok, notifications} or {:error, reason} defp delete_and_regenerate_cycles(cycles_to_delete, member_id, today, opts) do skip_lock? = Keyword.get(opts, :skip_lock?, false) + actor = Keyword.get(opts, :actor) if Enum.empty?(cycles_to_delete) do # No cycles to delete, just regenerate - regenerate_cycles(member_id, today, skip_lock?: skip_lock?) + regenerate_cycles(member_id, today, skip_lock?: skip_lock?, actor: actor) else case delete_cycles(cycles_to_delete) do - :ok -> regenerate_cycles(member_id, today, skip_lock?: skip_lock?) + :ok -> regenerate_cycles(member_id, today, skip_lock?: skip_lock?, actor: actor) {:error, reason} -> {:error, reason} end end @@ -858,11 +921,13 @@ defmodule Mv.Membership.Member do # Returns {:ok, notifications} - notifications should be returned to after_action hook defp regenerate_cycles(member_id, today, opts) do skip_lock? = Keyword.get(opts, :skip_lock?, false) + actor = Keyword.get(opts, :actor) case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member( member_id, today: today, - skip_lock?: skip_lock? + skip_lock?: skip_lock?, + actor: actor ) do {:ok, _cycles, notifications} when is_list(notifications) -> {:ok, notifications} @@ -876,21 +941,25 @@ defmodule Mv.Membership.Member do # based on environment (test vs production) # This function encapsulates the common logic for cycle generation # to avoid code duplication across different hooks - defp handle_cycle_generation(member) do + defp handle_cycle_generation(member, opts) do + actor = Keyword.get(opts, :actor) + if Mv.Config.sql_sandbox?() do - handle_cycle_generation_sync(member) + handle_cycle_generation_sync(member, actor: actor) else - handle_cycle_generation_async(member) + handle_cycle_generation_async(member, actor: actor) end end # Runs cycle generation synchronously (for test environment) - defp handle_cycle_generation_sync(member) do + defp handle_cycle_generation_sync(member, opts) do require Logger + actor = Keyword.get(opts, :actor) case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member( member.id, - today: Date.utc_today() + today: Date.utc_today(), + actor: actor ) do {:ok, cycles, notifications} -> send_notifications_if_any(notifications) @@ -902,9 +971,11 @@ defmodule Mv.Membership.Member do end # Runs cycle generation asynchronously (for production environment) - defp handle_cycle_generation_async(member) do + defp handle_cycle_generation_async(member, opts) do + actor = Keyword.get(opts, :actor) + Task.Supervisor.async_nolink(Mv.TaskSupervisor, fn -> - case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member.id) do + case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member.id, actor: actor) do {:ok, cycles, notifications} -> send_notifications_if_any(notifications) log_cycle_generation_success(member, cycles, notifications, sync: false) @@ -1156,15 +1227,18 @@ defmodule Mv.Membership.Member do custom_field_values_arg = Ash.Changeset.get_argument(changeset, :custom_field_values) if is_nil(custom_field_values_arg) do - extract_existing_values(changeset.data) + extract_existing_values(changeset.data, changeset) else extract_argument_values(custom_field_values_arg) end end # Extracts custom field values from existing member data (update scenario) - defp extract_existing_values(member_data) do - case Ash.load(member_data, :custom_field_values) do + defp extract_existing_values(member_data, changeset) do + actor = Map.get(changeset.context, :actor) + opts = Helpers.ash_actor_opts(actor) + + case Ash.load(member_data, :custom_field_values, opts) do {:ok, %{custom_field_values: existing_values}} -> Enum.reduce(existing_values, %{}, &extract_value_from_cfv/2) diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 345d6e4..18ffe5b 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -21,8 +21,8 @@ defmodule Mv.Authorization.Checks.HasPermission do - **:all** - Authorizes without filtering (returns all records) - **:own** - Filters to records where record.id == actor.id - **:linked** - Filters based on resource type: - - Member: member.user.id == actor.id (via has_one :user relationship) - - CustomFieldValue: custom_field_value.member.user.id == actor.id (traverses member → user relationship!) + - Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship) + - CustomFieldValue: `member_id == actor.member_id` (CustomFieldValue.member_id → Member.id → User.member_id) ## Error Handling @@ -59,6 +59,7 @@ defmodule Mv.Authorization.Checks.HasPermission do def strict_check(actor, authorizer, _opts) do resource = authorizer.resource action = get_action_from_authorizer(authorizer) + record = get_record_from_authorizer(authorizer) cond do is_nil(actor) -> @@ -76,12 +77,12 @@ defmodule Mv.Authorization.Checks.HasPermission do {:ok, false} true -> - strict_check_with_permissions(actor, resource, action) + strict_check_with_permissions(actor, resource, action, record) end end # Helper function to reduce nesting depth - defp strict_check_with_permissions(actor, resource, action) do + defp strict_check_with_permissions(actor, resource, action, record) do with %{role: %{permission_set_name: ps_name}} when not is_nil(ps_name) <- actor, {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name), permissions <- PermissionSets.get_permissions(ps_atom), @@ -93,9 +94,15 @@ defmodule Mv.Authorization.Checks.HasPermission do actor, resource_name ) do - :authorized -> {:ok, true} - {:filter, _} -> {:ok, :unknown} - false -> {:ok, false} + :authorized -> + {:ok, true} + + {:filter, filter_expr} -> + # For strict_check on single records, evaluate the filter against the record + evaluate_filter_for_strict_check(filter_expr, actor, record, resource_name) + + false -> + {:ok, false} end else %{role: nil} -> @@ -122,9 +129,17 @@ defmodule Mv.Authorization.Checks.HasPermission do action = get_action_from_authorizer(authorizer) cond do - is_nil(actor) -> nil - is_nil(action) -> nil - true -> auto_filter_with_permissions(actor, resource, action) + is_nil(actor) -> + # No actor - deny access (fail-closed) + # Return filter that never matches (expr(false) = match none) + deny_filter() + + is_nil(action) -> + # Cannot determine action - deny access (fail-closed) + deny_filter() + + true -> + auto_filter_with_permissions(actor, resource, action) end end @@ -141,21 +156,97 @@ defmodule Mv.Authorization.Checks.HasPermission do actor, resource_name ) do - :authorized -> nil - {:filter, filter_expr} -> filter_expr - false -> nil + :authorized -> + # :all scope - allow all records (no filter) + # Return empty keyword list (no filtering) + [] + + {:filter, filter_expr} -> + # :linked or :own scope - apply filter + # filter_expr is a keyword list from expr(...), return it directly + filter_expr + + false -> + # No permission - deny access (fail-closed) + deny_filter() end else + _ -> + # Error case (no role, invalid permission set, etc.) - deny access (fail-closed) + deny_filter() + end + end + + # Helper function to return a filter that never matches (deny all records) + # Used when authorization should be denied (fail-closed) + # + # Using `expr(false)` avoids depending on the primary key being named `:id`. + # This is more robust than [id: {:in, []}] which assumes the primary key is `:id`. + defp deny_filter do + expr(false) + end + + # Helper to extract action type from authorizer + # CRITICAL: Must use action_type, not action.name! + # Action types: :create, :read, :update, :destroy + # Action names: :create_member, :update_member, etc. + # PermissionSets uses action types, not action names + # + # Prefer authorizer.action.type (stable API) over authorizer.subject (varies by context) + defp get_action_from_authorizer(authorizer) do + # Primary: Use authorizer.action.type (stable API) + case Map.get(authorizer, :action) do + %{type: action_type} when action_type in [:create, :read, :update, :destroy] -> + action_type + + _ -> + # Fallback: Try authorizer.subject (for compatibility with different Ash versions/contexts) + case Map.get(authorizer, :subject) do + %{action_type: action_type} when action_type in [:create, :read, :update, :destroy] -> + action_type + + %{action: %{type: action_type}} + when action_type in [:create, :read, :update, :destroy] -> + action_type + + _ -> + nil + end + end + end + + # Helper to extract record from authorizer for strict_check + defp get_record_from_authorizer(authorizer) do + case authorizer.subject do + %{data: data} when not is_nil(data) -> data _ -> nil end end - # Helper to extract action from authorizer - defp get_action_from_authorizer(authorizer) do - case authorizer.subject do - %{action: %{name: action}} -> action - %{action: action} when is_atom(action) -> action - _ -> nil + # Evaluate filter expression for strict_check on single records + # For :linked scope with Member resource: id == actor.member_id + defp evaluate_filter_for_strict_check(_filter_expr, actor, record, resource_name) do + case {resource_name, record} do + {"Member", %{id: member_id}} when not is_nil(member_id) -> + # Check if this member's ID matches the actor's member_id + if member_id == actor.member_id do + {:ok, true} + else + {:ok, false} + end + + {"CustomFieldValue", %{member_id: cfv_member_id}} when not is_nil(cfv_member_id) -> + # Check if this CFV's member_id matches the actor's member_id + if cfv_member_id == actor.member_id do + {:ok, true} + else + {:ok, false} + end + + _ -> + # For other cases or when record is not available, return :unknown + # This will cause Ash to use auto_filter instead + {:ok, :unknown} end end @@ -190,21 +281,24 @@ defmodule Mv.Authorization.Checks.HasPermission do end # Scope: linked - Filter based on user relationship (resource-specific!) - # Uses Ash relationships: Member has_one :user, CustomFieldValue belongs_to :member + # IMPORTANT: Understand the relationship direction! + # - User belongs_to :member (User.member_id → Member.id) + # - Member has_one :user (inverse, no FK on Member) defp apply_scope(:linked, actor, resource_name) do case resource_name do "Member" -> - # Member has_one :user → filter by user.id == actor.id - {:filter, expr(user.id == ^actor.id)} + # User.member_id → Member.id (inverse relationship) + # Filter: member.id == actor.member_id + {:filter, expr(id == ^actor.member_id)} "CustomFieldValue" -> - # CustomFieldValue belongs_to :member → member has_one :user - # Traverse: custom_field_value.member.user.id == actor.id - {:filter, expr(member.user.id == ^actor.id)} + # CustomFieldValue.member_id → Member.id → User.member_id + # Filter: custom_field_value.member_id == actor.member_id + {:filter, expr(member_id == ^actor.member_id)} _ -> - # Fallback for other resources: try user relationship first, then user_id - {:filter, expr(user.id == ^actor.id or user_id == ^actor.id)} + # Fallback for other resources + {:filter, expr(user_id == ^actor.id)} end end diff --git a/lib/mv/authorization/checks/no_actor.ex b/lib/mv/authorization/checks/no_actor.ex new file mode 100644 index 0000000..f5eebb7 --- /dev/null +++ b/lib/mv/authorization/checks/no_actor.ex @@ -0,0 +1,74 @@ +defmodule Mv.Authorization.Checks.NoActor do + @moduledoc """ + Custom Ash Policy Check that allows actions when no actor is present. + + **IMPORTANT:** This check ONLY works in test environment for security reasons. + In production/dev, ALL operations without an actor are denied. + + ## Security Note + + This check uses compile-time environment detection to prevent accidental + security issues in production. In production, ALL operations (including :create + and :read) will be denied if no actor is present. + + For seeds and system operations in production, use an admin actor instead: + + admin_user = get_admin_user() + Ash.create!(resource, attrs, actor: admin_user) + + ## Usage in Policies + + policies do + # Allow system operations without actor (TEST ENVIRONMENT ONLY) + # In test: All operations allowed + # In production: ALL operations denied (fail-closed) + bypass action_type([:create, :read, :update, :destroy]) do + authorize_if NoActor + end + + # Check permissions when actor is present + policy action_type([:read, :create, :update, :destroy]) do + authorize_if HasPermission + end + end + + ## Behavior + + - In test environment: Returns `true` when actor is nil (allows all operations) + - In production/dev: Returns `false` when actor is nil (denies all operations - fail-closed) + - Returns `false` when actor is present (delegates to other policies) + """ + + use Ash.Policy.SimpleCheck + + # Compile-time check: Only allow no-actor bypass in test environment + @allow_no_actor_bypass Mix.env() == :test + # Alternative (if you want to control via config): + # @allow_no_actor_bypass Application.compile_env(:mv, :allow_no_actor_bypass, false) + + @impl true + def describe(_opts) do + if @allow_no_actor_bypass do + "allows actions when no actor is present (test environment only)" + else + "denies all actions when no actor is present (production/dev - fail-closed)" + end + end + + @impl true + def match?(nil, _context, _opts) do + # Actor is nil + if @allow_no_actor_bypass do + # Test environment: Allow all operations + true + else + # Production/dev: Deny all operations (fail-closed for security) + false + end + end + + def match?(_actor, _context, _opts) do + # Actor is present - don't match (let other policies decide) + false + end +end diff --git a/lib/mv/email_sync/changes/sync_member_email_to_user.ex b/lib/mv/email_sync/changes/sync_member_email_to_user.ex index 48c7955..0c0d8f7 100644 --- a/lib/mv/email_sync/changes/sync_member_email_to_user.ex +++ b/lib/mv/email_sync/changes/sync_member_email_to_user.ex @@ -41,8 +41,10 @@ defmodule Mv.EmailSync.Changes.SyncMemberEmailToUser do Ash.Changeset.around_transaction(changeset, fn cs, callback -> result = callback.(cs) + actor = Map.get(changeset.context, :actor) + with {:ok, member} <- Helpers.extract_record(result), - linked_user <- Loader.get_linked_user(member) do + linked_user <- Loader.get_linked_user(member, actor) do Helpers.sync_email_to_linked_record(result, linked_user, new_email) else _ -> result diff --git a/lib/mv/email_sync/changes/sync_user_email_to_member.ex b/lib/mv/email_sync/changes/sync_user_email_to_member.ex index 7148067..54829a4 100644 --- a/lib/mv/email_sync/changes/sync_user_email_to_member.ex +++ b/lib/mv/email_sync/changes/sync_user_email_to_member.ex @@ -33,7 +33,17 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do if Map.get(context, :syncing_email, false) do changeset else - sync_email(changeset) + # Ensure actor is in changeset context - get it from context if available + actor = Map.get(changeset.context, :actor) || Map.get(context, :actor) + + changeset_with_actor = + if actor && !Map.has_key?(changeset.context, :actor) do + Ash.Changeset.put_context(changeset, :actor, actor) + else + changeset + end + + sync_email(changeset_with_actor) end end @@ -42,7 +52,7 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do result = callback.(cs) with {:ok, record} <- Helpers.extract_record(result), - {:ok, user, member} <- get_user_and_member(record) do + {:ok, user, member} <- get_user_and_member(record, cs) do # When called from Member-side, we need to update the member in the result # When called from User-side, we update the linked member in DB only case record do @@ -61,15 +71,19 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do end # Retrieves user and member - works for both resource types - defp get_user_and_member(%Mv.Accounts.User{} = user) do - case Loader.get_linked_member(user) do + defp get_user_and_member(%Mv.Accounts.User{} = user, changeset) do + actor = Map.get(changeset.context, :actor) + + case Loader.get_linked_member(user, actor) do nil -> {:error, :no_member} member -> {:ok, user, member} end end - defp get_user_and_member(%Mv.Membership.Member{} = member) do - case Loader.load_linked_user!(member) do + defp get_user_and_member(%Mv.Membership.Member{} = member, changeset) do + actor = Map.get(changeset.context, :actor) + + case Loader.load_linked_user!(member, actor) do {:ok, user} -> {:ok, user, member} error -> error end diff --git a/lib/mv/email_sync/loader.ex b/lib/mv/email_sync/loader.ex index ecb1038..91927fb 100644 --- a/lib/mv/email_sync/loader.ex +++ b/lib/mv/email_sync/loader.ex @@ -2,15 +2,30 @@ defmodule Mv.EmailSync.Loader do @moduledoc """ Helper functions for loading linked records in email synchronization. Centralizes the logic for retrieving related User/Member entities. + + ## Authorization + + This module runs systemically and accepts optional actor parameters. + When called from hooks/changes, actor is extracted from changeset context. + When called directly, actor should be provided for proper authorization. + + All functions accept an optional `actor` parameter that is passed to Ash operations + to ensure proper authorization checks are performed. """ + alias Mv.Helpers @doc """ Loads the member linked to a user, returns nil if not linked or on error. - """ - def get_linked_member(%{member_id: nil}), do: nil - def get_linked_member(%{member_id: id}) do - case Ash.get(Mv.Membership.Member, id) do + Accepts optional actor for authorization. + """ + def get_linked_member(user, actor \\ nil) + def get_linked_member(%{member_id: nil}, _actor), do: nil + + def get_linked_member(%{member_id: id}, actor) do + opts = Helpers.ash_actor_opts(actor) + + case Ash.get(Mv.Membership.Member, id, opts) do {:ok, member} -> member {:error, _} -> nil end @@ -18,9 +33,13 @@ defmodule Mv.EmailSync.Loader do @doc """ Loads the user linked to a member, returns nil if not linked or on error. + + Accepts optional actor for authorization. """ - def get_linked_user(member) do - case Ash.load(member, :user) do + def get_linked_user(member, actor \\ nil) do + opts = Helpers.ash_actor_opts(actor) + + case Ash.load(member, :user, opts) do {:ok, %{user: user}} -> user {:error, _} -> nil end @@ -29,9 +48,13 @@ defmodule Mv.EmailSync.Loader do @doc """ Loads the user linked to a member, returning an error tuple if not linked. Useful when a link is required for the operation. + + Accepts optional actor for authorization. """ - def load_linked_user!(member) do - case Ash.load(member, :user) do + def load_linked_user!(member, actor \\ nil) do + opts = Helpers.ash_actor_opts(actor) + + case Ash.load(member, :user, opts) do {:ok, %{user: user}} when not is_nil(user) -> {:ok, user} {:ok, _} -> {:error, :no_linked_user} {:error, _} = error -> error diff --git a/lib/mv/helpers.ex b/lib/mv/helpers.ex new file mode 100644 index 0000000..e20db67 --- /dev/null +++ b/lib/mv/helpers.ex @@ -0,0 +1,27 @@ +defmodule Mv.Helpers do + @moduledoc """ + Shared helper functions used across the Mv application. + + Provides utilities that are not specific to a single domain or layer. + """ + + @doc """ + Converts an actor to Ash options list for authorization. + Returns empty list if actor is nil. + + This helper ensures consistent actor handling across all Ash operations + in the application, whether called from LiveViews, changes, validations, + or other contexts. + + ## Examples + + opts = ash_actor_opts(actor) + Ash.read(query, opts) + + opts = ash_actor_opts(nil) + # => [] + """ + @spec ash_actor_opts(Mv.Accounts.User.t() | nil) :: keyword() + def ash_actor_opts(nil), do: [] + def ash_actor_opts(actor) when not is_nil(actor), do: [actor: actor] +end diff --git a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex index a14bc0b..e1bbc4e 100644 --- a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex +++ b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex @@ -8,6 +8,7 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do This allows creating members with the same email as unlinked users. """ use Ash.Resource.Validation + alias Mv.Helpers @doc """ Validates email uniqueness across linked Member-User pairs. @@ -29,7 +30,8 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do def validate(changeset, _opts, _context) do email_changing? = Ash.Changeset.changing_attribute?(changeset, :email) - linked_user_id = get_linked_user_id(changeset.data) + actor = Map.get(changeset.context || %{}, :actor) + linked_user_id = get_linked_user_id(changeset.data, actor) is_linked? = not is_nil(linked_user_id) # Only validate if member is already linked AND email is changing @@ -38,19 +40,21 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do if should_validate? do new_email = Ash.Changeset.get_attribute(changeset, :email) - check_email_uniqueness(new_email, linked_user_id) + check_email_uniqueness(new_email, linked_user_id, actor) else :ok end end - defp check_email_uniqueness(email, exclude_user_id) do + defp check_email_uniqueness(email, exclude_user_id, actor) do query = Mv.Accounts.User |> Ash.Query.filter(email == ^email) |> maybe_exclude_id(exclude_user_id) - case Ash.read(query) do + opts = Helpers.ash_actor_opts(actor) + + case Ash.read(query, opts) do {:ok, []} -> :ok @@ -65,8 +69,10 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do defp maybe_exclude_id(query, nil), do: query defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) - defp get_linked_user_id(member_data) do - case Ash.load(member_data, :user) do + defp get_linked_user_id(member_data, actor) do + opts = Helpers.ash_actor_opts(actor) + + case Ash.load(member_data, :user, opts) do {:ok, %{user: %{id: id}}} -> id _ -> nil end diff --git a/lib/mv/membership_fees/cycle_generator.ex b/lib/mv/membership_fees/cycle_generator.ex index 7d6c798..0d17dd3 100644 --- a/lib/mv/membership_fees/cycle_generator.ex +++ b/lib/mv/membership_fees/cycle_generator.ex @@ -28,6 +28,15 @@ defmodule Mv.MembershipFees.CycleGenerator do Uses PostgreSQL advisory locks to prevent race conditions when generating cycles for the same member concurrently. + ## Authorization + + This module runs systemically and accepts optional actor parameters. + When called from hooks/changes, actor is extracted from changeset context. + When called directly, actor should be provided for proper authorization. + + All functions accept an optional `actor` parameter in the `opts` keyword list + that is passed to Ash operations to ensure proper authorization checks are performed. + ## Examples # Generate cycles for a single member @@ -77,7 +86,9 @@ defmodule Mv.MembershipFees.CycleGenerator do def generate_cycles_for_member(member_or_id, opts \\ []) def generate_cycles_for_member(member_id, opts) when is_binary(member_id) do - case load_member(member_id) do + actor = Keyword.get(opts, :actor) + + case load_member(member_id, actor: actor) do {:ok, member} -> generate_cycles_for_member(member, opts) {:error, reason} -> {:error, reason} end @@ -87,25 +98,27 @@ defmodule Mv.MembershipFees.CycleGenerator do today = Keyword.get(opts, :today, Date.utc_today()) skip_lock? = Keyword.get(opts, :skip_lock?, false) - do_generate_cycles_with_lock(member, today, skip_lock?) + do_generate_cycles_with_lock(member, today, skip_lock?, opts) end # Generate cycles with lock handling # Returns {:ok, cycles, notifications} - notifications are never sent here, # they should be returned to the caller (e.g., via after_action hook) - defp do_generate_cycles_with_lock(member, today, true = _skip_lock?) do + defp do_generate_cycles_with_lock(member, today, true = _skip_lock?, opts) do # Lock already set by caller (e.g., regenerate_cycles_on_type_change) # Just generate cycles without additional locking - do_generate_cycles(member, today) + actor = Keyword.get(opts, :actor) + do_generate_cycles(member, today, actor: actor) end - defp do_generate_cycles_with_lock(member, today, false) do + defp do_generate_cycles_with_lock(member, today, false, opts) do lock_key = :erlang.phash2(member.id) + actor = Keyword.get(opts, :actor) Repo.transaction(fn -> Ecto.Adapters.SQL.query!(Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - case do_generate_cycles(member, today) do + case do_generate_cycles(member, today, actor: actor) do {:ok, cycles, notifications} -> # Return cycles and notifications - do NOT send notifications here # They will be sent by the caller (e.g., via after_action hook) @@ -222,21 +235,33 @@ defmodule Mv.MembershipFees.CycleGenerator do # Private functions - defp load_member(member_id) do - Member - |> Ash.Query.filter(id == ^member_id) - |> Ash.Query.load([:membership_fee_type, :membership_fee_cycles]) - |> Ash.read_one() - |> case do + defp load_member(member_id, opts) do + actor = Keyword.get(opts, :actor) + + query = + Member + |> Ash.Query.filter(id == ^member_id) + |> Ash.Query.load([:membership_fee_type, :membership_fee_cycles]) + + result = + if actor do + Ash.read_one(query, actor: actor) + else + Ash.read_one(query) + end + + case result do {:ok, nil} -> {:error, :member_not_found} {:ok, member} -> {:ok, member} {:error, reason} -> {:error, reason} end end - defp do_generate_cycles(member, today) do + defp do_generate_cycles(member, today, opts) do + actor = Keyword.get(opts, :actor) + # Reload member with relationships to ensure fresh data - case load_member(member.id) do + case load_member(member.id, actor: actor) do {:ok, member} -> cond do is_nil(member.membership_fee_type_id) -> @@ -246,7 +271,7 @@ defmodule Mv.MembershipFees.CycleGenerator do {:error, :no_join_date} true -> - generate_missing_cycles(member, today) + generate_missing_cycles(member, today, actor: actor) end {:error, reason} -> @@ -254,7 +279,8 @@ defmodule Mv.MembershipFees.CycleGenerator do end end - defp generate_missing_cycles(member, today) do + defp generate_missing_cycles(member, today, opts) do + actor = Keyword.get(opts, :actor) fee_type = member.membership_fee_type interval = fee_type.interval amount = fee_type.amount @@ -270,7 +296,7 @@ defmodule Mv.MembershipFees.CycleGenerator do # Only generate if start_date <= end_date if start_date && Date.compare(start_date, end_date) != :gt do cycle_starts = generate_cycle_starts(start_date, end_date, interval) - create_cycles(cycle_starts, member.id, fee_type.id, amount) + create_cycles(cycle_starts, member.id, fee_type.id, amount, actor: actor) else {:ok, [], []} end @@ -365,7 +391,8 @@ defmodule Mv.MembershipFees.CycleGenerator do end end - defp create_cycles(cycle_starts, member_id, fee_type_id, amount) do + defp create_cycles(cycle_starts, member_id, fee_type_id, amount, opts) do + actor = Keyword.get(opts, :actor) # Always use return_notifications?: true to collect notifications # Notifications will be returned to the caller, who is responsible for # sending them (e.g., via after_action hook returning {:ok, result, notifications}) @@ -380,7 +407,7 @@ defmodule Mv.MembershipFees.CycleGenerator do } handle_cycle_creation_result( - Ash.create(MembershipFeeCycle, attrs, return_notifications?: true), + Ash.create(MembershipFeeCycle, attrs, return_notifications?: true, actor: actor), cycle_start ) end) diff --git a/lib/mv_web/live/custom_field_live/form_component.ex b/lib/mv_web/live/custom_field_live/form_component.ex index 172cfd3..bf2e882 100644 --- a/lib/mv_web/live/custom_field_live/form_component.ex +++ b/lib/mv_web/live/custom_field_live/form_component.ex @@ -91,7 +91,9 @@ defmodule MvWeb.CustomFieldLive.FormComponent do @impl true def handle_event("save", %{"custom_field" => custom_field_params}, socket) do - case AshPhoenix.Form.submit(socket.assigns.form, params: custom_field_params) do + actor = MvWeb.LiveHelpers.current_actor(socket) + + case MvWeb.LiveHelpers.submit_form(socket.assigns.form, custom_field_params, actor) do {:ok, custom_field} -> action = case socket.assigns.form.source.type do diff --git a/lib/mv_web/live/custom_field_value_live/form.ex b/lib/mv_web/live/custom_field_value_live/form.ex index 4db2bed..599ce1d 100644 --- a/lib/mv_web/live/custom_field_value_live/form.ex +++ b/lib/mv_web/live/custom_field_value_live/form.ex @@ -32,6 +32,9 @@ defmodule MvWeb.CustomFieldValueLive.Form do """ use MvWeb, :live_view + on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} + import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] + @impl true def render(assigns) do ~H""" @@ -172,8 +175,9 @@ defmodule MvWeb.CustomFieldValueLive.Form do page_title = action <> " " <> "Custom field value" # Load all CustomFields and Members for the selection fields - custom_fields = Ash.read!(Mv.Membership.CustomField) - members = Ash.read!(Mv.Membership.Member) + actor = current_actor(socket) + custom_fields = Ash.read!(Mv.Membership.CustomField, actor: actor) + members = Ash.read!(Mv.Membership.Member, actor: actor) {:ok, socket @@ -224,7 +228,9 @@ defmodule MvWeb.CustomFieldValueLive.Form do custom_field_value_params end - case AshPhoenix.Form.submit(socket.assigns.form, params: updated_params) do + actor = current_actor(socket) + + case submit_form(socket.assigns.form, updated_params, actor) do {:ok, custom_field_value} -> notify_parent({:saved, custom_field_value}) diff --git a/lib/mv_web/live/custom_field_value_live/index.ex b/lib/mv_web/live/custom_field_value_live/index.ex index b52fd96..eea578e 100644 --- a/lib/mv_web/live/custom_field_value_live/index.ex +++ b/lib/mv_web/live/custom_field_value_live/index.ex @@ -23,6 +23,9 @@ defmodule MvWeb.CustomFieldValueLive.Index do """ use MvWeb, :live_view + on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} + import MvWeb.LiveHelpers, only: [current_actor: 1] + @impl true def render(assigns) do ~H""" @@ -70,17 +73,85 @@ defmodule MvWeb.CustomFieldValueLive.Index do @impl true def mount(_params, _session, socket) do - {:ok, - socket - |> assign(:page_title, "Listing Custom field values") - |> stream(:custom_field_values, Ash.read!(Mv.Membership.CustomFieldValue))} + actor = current_actor(socket) + + # Early return if no actor (prevents exceptions in unauthenticated tests) + if is_nil(actor) do + {:ok, + socket + |> assign(:page_title, "Listing Custom field values") + |> stream(:custom_field_values, [])} + else + case Ash.read(Mv.Membership.CustomFieldValue, actor: actor) do + {:ok, custom_field_values} -> + {:ok, + socket + |> assign(:page_title, "Listing Custom field values") + |> stream(:custom_field_values, custom_field_values)} + + {:error, %Ash.Error.Forbidden{}} -> + {:ok, + socket + |> assign(:page_title, "Listing Custom field values") + |> stream(:custom_field_values, []) + |> put_flash(:error, gettext("You do not have permission to view custom field values"))} + + {:error, error} -> + {:ok, + socket + |> assign(:page_title, "Listing Custom field values") + |> stream(:custom_field_values, []) + |> put_flash(:error, format_error(error))} + end + end end @impl true def handle_event("delete", %{"id" => id}, socket) do - custom_field_value = Ash.get!(Mv.Membership.CustomFieldValue, id) - Ash.destroy!(custom_field_value) + actor = MvWeb.LiveHelpers.current_actor(socket) - {:noreply, stream_delete(socket, :custom_field_values, custom_field_value)} + case Ash.get(Mv.Membership.CustomFieldValue, id, actor: actor) do + {:ok, custom_field_value} -> + case Ash.destroy(custom_field_value, actor: actor) do + :ok -> + {:noreply, + socket + |> stream_delete(:custom_field_values, custom_field_value) + |> put_flash(:info, gettext("Custom field value deleted successfully"))} + + {:error, %Ash.Error.Forbidden{}} -> + {:noreply, + put_flash( + socket, + :error, + gettext("You do not have permission to delete this custom field value") + )} + + {:error, error} -> + {:noreply, put_flash(socket, :error, format_error(error))} + end + + {:error, %Ash.Error.Query.NotFound{}} -> + {:noreply, put_flash(socket, :error, gettext("Custom field value not found"))} + + {:error, %Ash.Error.Forbidden{} = _error} -> + {:noreply, + put_flash( + socket, + :error, + gettext("You do not have permission to access this custom field value") + )} + + {:error, error} -> + {:noreply, put_flash(socket, :error, format_error(error))} + end + end + + defp format_error(%Ash.Error.Invalid{errors: errors}) do + Enum.map_join(errors, ", ", fn %{message: message} -> message end) + end + + defp format_error(error) do + inspect(error) end end diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index 9bce04b..0a968d6 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -79,7 +79,9 @@ defmodule MvWeb.GlobalSettingsLive do @impl true def handle_event("save", %{"setting" => setting_params}, socket) do - case AshPhoenix.Form.submit(socket.assigns.form, params: setting_params) do + actor = MvWeb.LiveHelpers.current_actor(socket) + + case MvWeb.LiveHelpers.submit_form(socket.assigns.form, setting_params, actor) do {:ok, _updated_settings} -> # Reload settings from database to ensure all dependent data is updated {:ok, fresh_settings} = Membership.get_settings() diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index 0a05e1f..07d4e2d 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -21,6 +21,10 @@ defmodule MvWeb.MemberLive.Form do """ use MvWeb, :live_view + on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} + + import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] + alias Mv.MembershipFees alias Mv.MembershipFees.MembershipFeeType alias MvWeb.Helpers.MembershipFeeHelpers @@ -172,7 +176,7 @@ defmodule MvWeb.MemberLive.Form do