From fecf98dc0ec02298a632641eb09c649529bb334f Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 23:12:07 +0100 Subject: [PATCH 1/2] Security: Fix critical deny-filter bug and improve authorization CRITICAL FIX: Deny-filter was allowing all records instead of denying Fix: User validation in Member now uses actor from changeset.context --- ...iff-main-aus-lese-dir-die-roles-and-pe.txt | 751 ++++++++++++++++++ CLAUDE.md | 100 +++ learning.md | 480 +++++++++++ lib/membership/member.ex | 15 +- lib/mv/authorization/checks/has_permission.ex | 20 +- .../has_permission_integration_test.exs | 4 +- 6 files changed, 1356 insertions(+), 14 deletions(-) create mode 100644 2026-01-08-fhre-git-diff-main-aus-lese-dir-die-roles-and-pe.txt create mode 100644 CLAUDE.md create mode 100644 learning.md diff --git a/2026-01-08-fhre-git-diff-main-aus-lese-dir-die-roles-and-pe.txt b/2026-01-08-fhre-git-diff-main-aus-lese-dir-die-roles-and-pe.txt new file mode 100644 index 0000000..267caad --- /dev/null +++ b/2026-01-08-fhre-git-diff-main-aus-lese-dir-die-roles-and-pe.txt @@ -0,0 +1,751 @@ + + * ▐▛███▜▌ * Claude Code v2.0.76 +* ▝▜█████▛▘ * Sonnet 4.5 · Claude Pro + * ▘▘ ▝▝ * /home/user/LIT/mitgliederverwaltung/mitgliederverwaltung + +> führe git diff main aus, lese dir die roles-and-permissions docs durch und + erstelle ein code review vom aktuellen branch + +● Ich führe die Aufgaben parallel aus: + +● Bash(git diff main) + ⎿ diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/aut + horization/checks/has_permission.ex + new file mode 100644 + … +656 lines (ctrl+o to expand) + + +● Search(pattern: "**/docs/**/roles*.md") + ⎿  Found 3 files (ctrl+o to expand) + +● Search(pattern: "**/docs/**/permission*.md") + ⎿  Found 0 files + +● Read(docs/roles-and-permissions-implementation-plan.md) + ⎿  Read 1654 lines + +● Read(docs/roles-and-permissions-architecture.md) + ⎿  Read 2503 lines + +● Read(docs/roles-and-permissions-overview.md) + ⎿  Read 507 lines + +● Ich habe den git diff und die Dokumentation gelesen. Jetzt erstelle ich das Code Review: + +● Updated plan + ⎿  /plan to preview + +> /status + +> /model + +● Ich verifiziere noch die Member-Resource, um die Relationship-Frage zu klären: + +● Search(pattern: "has_one :user|belongs_to :user|user_id", path: + "lib/mv/membership/member.ex", output_mode: "content") + ⎿  Error searching files + +● Search(pattern: "**/member.ex") + ⎿  Found 1 file (ctrl+o to expand) + +● Read(lib/membership/member.ex) + ⎿  Read 1276 lines + +● Die Member Resource hat tatsächlich has_one :user (Zeile 517). Die HasPermission Implementierung ist korrekt. Ich aktualisiere das Review: + +● Updated plan + ⎿  /plan to preview + +● Updated plan + ⎿  /plan to preview + +● Ich habe das Code Review erstellt und verifiziert. Hier ist die Zusammenfassung: + + Code Review: HasPermission Policy Check + + Gesamtbewertung: Gut ✅ + + Positiv: + - Korrekte Ash.Policy.Check Implementierung mit strict_check/3 und auto_filter/3 + - Umfassende Fehlerbehandlung (nil actor, nil role, invalid permission_set_name) + - Gute Test Coverage (~25 Tests) + - Die Relationship-Pfade sind korrekt - Member hat has_one :user (lib/membership/member.ex:517) + + Kleinere Punkte: + - Dokumentation zwischen Moduledoc und Architecture Doc sollte vereinheitlicht werden + - Die Architektur-Dokumentation beschreibt user_id, die Implementierung nutzt user.id (funktional äquivalent, aber inkonsistent dokumentiert) + + Merge-Empfehlung: ✅ Kann gemerged werden + + --- + Das vollständige Review befindet sich in /home/agent/.claude/plans/moonlit-soaring-coral.md. + ⎿  User rejected Claude's plan: + ╭─────────────────────────────────────────────────────────────────────────╮ + │ Code Review: HasPermission Policy Check (Issue #6) │ + │ │ + │ Branch: feature/343_haspermission │ + │ Base: main │ + │ Reviewer: Claude Code │ + │ │ + │ --- │ + │ Zusammenfassung │ + │ │ + │ Der Branch implementiert den HasPermission Policy Check (Issue #6 aus │ + │ dem Implementation Plan). Dies ist eine zentrale Komponente des Rollen- │ + │ und Berechtigungssystems, die Berechtigungen aus dem PermissionSets │ + │ Modul ausliest und auf Ash Policies anwendet. │ + │ │ + │ Geänderte Dateien: │ + │ - lib/mv/authorization/checks/has_permission.ex (neu, 239 Zeilen) │ + │ - test/mv/authorization/checks/has_permission_test.exs (neu, 264 Zeilen)│ + │ - test/mv/authorization/checks/has_permission_integration_test.exs (neu,│ + │ 87 Zeilen) │ + │ │ + │ --- │ + │ Positives │ + │ │ + │ 1. Saubere Architektur │ + │ │ + │ - Korrekte Implementierung von Ash.Policy.Check mit strict_check/3 und │ + │ auto_filter/3 │ + │ - Gute Separation of Concerns - Berechtigungslogik delegiert an │ + │ PermissionSets Modul │ + │ - Klare Modulstruktur mit Helper-Funktionen zur Reduktion von Nesting │ + │ │ + │ 2. Fehlerbehandlung │ + │ │ + │ - Umfassende Behandlung von Edge Cases (nil actor, nil role, invalid │ + │ permission_set_name) │ + │ - Graceful degradation - Fehler führen zu {:ok, false} statt Crashes │ + │ - Debug-Logging für Autorisierungsfehler (lazy evaluation mit │ + │ Logger.debug(fn -> ... end)) │ + │ │ + │ 3. Scope-Implementierung │ + │ │ + │ - Korrekte Implementierung aller drei Scopes: :all, :own, :linked │ + │ - Wichtig: Die :linked Scope-Implementierung für CustomFieldValue │ + │ traversiert korrekt die Relationship (member.user.id) │ + │ - Die Filter-Expressions verwenden korrekt Ash.Expr │ + │ │ + │ 4. Test Coverage │ + │ │ + │ - Umfassende Unit Tests für alle Szenarien │ + │ - Separate Integration Tests für Filter-Expression-Struktur │ + │ - Tests für Error Handling und Logging │ + │ │ + │ --- │ + │ Kritische Punkte │ + │ │ + │ 1. VERIFIZIERT: Relationship-Pfade in apply_scope/3 sind korrekt │ + │ │ + │ Die Implementierung verwendet: │ + │ # Member (Zeile 191) │ + │ {:filter, expr(user.id == ^actor.id)} │ + │ │ + │ # CustomFieldValue (Zeile 196) │ + │ {:filter, expr(member.user.id == ^actor.id)} │ + │ │ + │ Verifiziert: Member hat has_one :user, Mv.Accounts.User │ + │ (lib/membership/member.ex:517) │ + │ │ + │ Die Architektur-Dokumentation beschreibt member.user_id == actor.id, │ + │ aber die Implementierung nutzt bewusst die Relationship user.id. Beides │ + │ ist funktional äquivalent, da Ash die Relationship auflöst. │ + │ │ + │ HINWEIS: Der Commit db0a1870 ("fix: correct relationship filter paths") │ + │ zeigt, dass dies eine bewusste Design-Entscheidung war, um Ash │ + │ Relationship-Traversal zu nutzen statt direkter Foreign Key-Vergleiche. │ + │ │ + │ 2. Moduledoc entspricht nicht der Architektur-Dokumentation │ + │ │ + │ Der @moduledoc beschreibt die :linked Scope für Member als: │ + │ - Member: member.user.id == actor.id (via has_one :user relationship) │ + │ │ + │ Die Architektur-Dokumentation sagt jedoch: │ + │ - Member: member.user_id == actor.id (direct relationship) │ + │ │ + │ Empfehlung: Dokumentation vereinheitlichen. │ + │ │ + │ 3. FEHLER: match?/3 nicht implementiert │ + │ │ + │ Der Implementation Plan (Issue #6) spezifiziert: │ + │ 4. Implement `match?/3` - the core authorization logic │ + │ │ + │ Die Implementierung verwendet stattdessen strict_check/3 und │ + │ auto_filter/3. │ + │ │ + │ Bewertung: Dies ist kein Fehler - strict_check/3 und auto_filter/3 sind │ + │ die korrekten Callbacks für Ash.Policy.Check. Die Dokumentation im │ + │ Implementation Plan ist veraltet. Die Implementierung ist korrekt. │ + │ │ + │ 4. Error-Return-Typen │ + │ │ + │ Der Implementation Plan spezifiziert Error-Returns wie {:error, │ + │ :no_actor}, aber die Implementierung gibt {:ok, false} zurück. │ + │ │ + │ Bewertung: Korrekt. Ash.Policy.Check erwartet {:ok, true | false | │ + │ :unknown}, nicht {:error, ...}. Die Implementierung ist konform mit der │ + │ Ash API. │ + │ │ + │ --- │ + │ Verbesserungsvorschläge │ + │ │ + │ 1. Code: Fallback in apply_scope(:linked, ...) könnte problematisch sein│ + │ │ + │ # Zeile 199-200 │ + │ _ -> │ + │ {:filter, expr(user.id == ^actor.id or user_id == ^actor.id)} │ + │ │ + │ Dieser Fallback könnte zu unerwarteten Ergebnissen führen, wenn ein │ + │ Resource weder user noch user_id hat. │ + │ │ + │ Empfehlung: Explizite Fehlerbehandlung statt Fallback oder Logging wenn │ + │ der Fallback verwendet wird. │ + │ │ + │ 2. Tests: Integration Tests sind minimal │ + │ │ + │ Die Integration Tests (has_permission_integration_test.exs) testen nur │ + │ die Struktur der Filter-Expressions, nicht deren Korrektheit in echten │ + │ Queries. │ + │ │ + │ Empfehlung: Nach Implementierung der Resource Policies (Issues #7-#10) │ + │ sollten End-to-End Tests hinzugefügt werden. │ + │ │ + │ 3. Tests: Fehlende Tests für describe/1 │ + │ │ + │ Der Test für describe/1 (Zeile 23-27) testet nur, dass ein String │ + │ zurückgegeben wird, nicht den Inhalt. │ + │ │ + │ Minimal: Akzeptabel, da describe/1 nur für Logging/Debugging verwendet │ + │ wird. │ + │ │ + │ 4. Performance: Logger.debug könnte optimiert werden │ + │ │ + │ Die Logging-Funktion log_auth_failure/4 erstellt einen String auch wenn │ + │ Debug-Logging deaktiviert ist. │ + │ │ + │ Aktuell: Die Implementierung verwendet bereits lazy evaluation mit │ + │ Logger.debug(fn -> ... end). Gut! │ + │ │ + │ --- │ + │ Übereinstimmung mit Architektur-Dokumentation │ + │ │ + │ | Requirement | Status | Kommentar │ + │ | │ + │ |--------------------------------------------|--------|-----------------│ + │ ----------------| │ + │ | Implements Ash.Policy.Check | ✅ | Korrekt │ + │ | │ + │ | Reads actor.role.permission_set_name | ✅ | Korrekt │ + │ | │ + │ | Calls PermissionSets.get_permissions/1 | ✅ | Korrekt │ + │ | │ + │ | Applies scope :all | ✅ | Returns │ + │ :authorized | │ + │ | Applies scope :own | ✅ | Filter auf id ==│ + │ ^actor.id | │ + │ | Applies scope :linked for Member | ⚠️ | Verwendet │ + │ user.id statt user_id | │ + │ | Applies scope :linked for CustomFieldValue | ✅ | Traversiert │ + │ korrekt | │ + │ | Error handling graceful | ✅ | Keine Crashes │ + │ | │ + │ | Logging for failures | ✅ | Logger.debug mit│ + │ lazy eval | │ + │ │ + │ --- │ + │ Test Coverage Analyse │ + │ │ + │ | Test-Kategorie | Anzahl Tests | Bewertung | │ + │ |--------------------------------|--------------|-------------| │ + │ | Permission Lookup | 5 | ✅ Gut | │ + │ | Scope :all | 2 | ✅ Gut | │ + │ | Scope :own | 1 | ⚠️ Minimal | │ + │ | Scope :linked | 2 | ⚠️ Minimal | │ + │ | Error Handling | 7 | ✅ Sehr gut | │ + │ | Logging | 2 | ✅ Gut | │ + │ | Resource Name Extraction | 2 | ✅ Gut | │ + │ | Integration (Filter Structure) | 4 | ✅ Gut | │ + │ │ + │ Gesamtbewertung Tests: ~25 Tests - entspricht den Erwartungen für Issue │ + │ #6 │ + │ │ + │ --- │ + │ Fazit │ + │ │ + │ Gesamtbewertung: Gut (mit Hinweisen) │ + │ │ + │ Die Implementierung ist solide und entspricht weitgehend den │ + │ Anforderungen aus der Architektur-Dokumentation. Die wichtigsten Punkte:│ + │ │ + │ Positiv: │ + │ - Korrekte Ash.Policy.Check Implementierung │ + │ - Umfassende Fehlerbehandlung │ + │ - Gute Test Coverage │ + │ - Sauberer, lesbarer Code │ + │ │ + │ Zu Klären: │ + │ 1. Relationship-Pfade (user.id vs user_id) - scheint bewusst so │ + │ implementiert (siehe Commit-Message), sollte aber dokumentiert werden │ + │ 2. Dokumentation zwischen Code und Architecture Doc vereinheitlichen │ + │ │ + │ Merge-Empfehlung: ✅ Kann gemerged werden, nachdem die Dokumentation │ + │ aktualisiert wurde │ + │ │ + │ --- │ + │ Checkliste für Merge │ + │ │ + │ - ~~Verify: Member hat has_one :user Relationship~~ → Bestätigt │ + │ (lib/membership/member.ex:517) │ + │ - Update: Moduledoc oder Architecture Doc für Konsistenz │ + │ - Run: just test bestätigt alle Tests bestehen │ + │ - Run: just lint zeigt keine Fehler │ + │ │ + │ │ + │ │ + │ │ + │ │ + │ │ + │ │ + │ │ + │ │ + │ │ + │ │ + │ │ + │ │ + │ │ + │ │ + │ │ + ╰─────────────────────────────────────────────────────────────────────────╯ + +> Erstelle einen sehr expliziten Plan in plan.md um den Member Resource +Policies Issue umzusetzen und gleichzeitig die Verbesserungen aus dem Review + zu übernehmen. + +● Ich lese zuerst die relevanten Dateien, um einen präzisen Plan zu erstellen: + +● Read(lib/mv/authorization/checks/has_permission.ex) + ⎿  Read 240 lines + +● Search(pattern: "policies do", path: "lib/membership/member.ex", output_mode: + "content") + ⎿  Found 0 lines + +● Read(lib/mv/authorization/permission_sets.ex) + ⎿  Read 295 lines + +● Jetzt erstelle ich den expliziten Implementation Plan: + +● Updated plan + ⎿  /plan to preview + ⎿  You've hit your limit · resets 7pm (UTC) + /upgrade or /extra-usage to finish what you're working on. + +> /model + ⎿ Set model to sonnet (claude-sonnet-4-5-20250929) + +> /plan + ⎿ Current Plan + /home/agent/.claude/plans/moonlit-soaring-coral.md + + # Implementation Plan: Member Resource Policies + Review-Verbesserungen + + **Issue:** #7 (Member Resource Policies) + Code Review Verbesserungen + **Branch:** `feature/343_haspermission` + **Abhängigkeiten:** Issue #6 (HasPermission) ✅ bereits implementiert + + --- + + ## Übersicht + + Dieser Plan implementiert: + 1. **Member Resource Policies** gemäß Issue #7 + 2. **Dokumentations-Synchronisation** aus dem Code Review + 3. **Fallback-Verbesserung** in HasPermission für robustere Fehlerbehandlung + + --- + + ## Teil 1: Member Resource Policies + + ### Datei: `lib/membership/member.ex` + + **Position:** Nach `relationships do ... end` Block (ca. Zeile 528), VOR + `calculations do` + + #### 1.1 Policies Block hinzufügen + + ```elixir + policies do + # ========================================================================= + # Policy Order (WICHTIG!): + # 1. Special Case: User kann immer eigenes verknüpftes Mitglied + lesen/bearbeiten + # 2. General: HasPermission Check für alle anderen Fälle + # 3. Default: Forbid (implizit durch Ash wenn keine Policy matched) + # ========================================================================= + + # SPECIAL CASE: User kann sein verknüpftes Mitglied immer lesen/bearbeiten + # Diese Policy greift VOR dem HasPermission Check + # Verwendet Ash Relationship Traversal: user.id == actor.id + policy action_type([:read, :update]) do + description "Users can always access their linked member" + authorize_if expr(user.id == ^actor(:id)) + end + + # GENERAL: Alle anderen Operationen via HasPermission Check + # Delegiert an PermissionSets Modul für scope-basierte Autorisierung + policy action_type([:read, :create, :update, :destroy]) do + description "Check permissions from user's role via PermissionSets" + authorize_if Mv.Authorization.Checks.HasPermission + end + end + ``` + + #### 1.2 Actor-Preparation für Role-Loading + + **Position:** Nach `policies do ... end` Block + + ```elixir + # Ensure actor has role preloaded for policy checks + preparations do + prepare build(actor_context: [role: [:permission_set_name]]) + end + ``` + + **ALTERNATIV** (falls Ash 3.0 andere Syntax benötigt): + Die Role muss im Actor bereits geladen sein. Prüfen ob `Mv.Accounts.User` + bereits `:role` preloaded. + + --- + + ## Teil 2: Dokumentations-Synchronisation + + ### 2.1 Datei: `lib/mv/authorization/checks/has_permission.ex` + + **Zeile 23-25:** Moduledoc aktualisieren für Konsistenz mit Code + + ```elixir + # VORHER: + # - Member: member.user.id == actor.id (via has_one :user relationship) + # - CustomFieldValue: custom_field_value.member.user.id == actor.id (traverses + member → user relationship!) + + # NACHHER (konsistent mit tatsächlichem Code): + ## Scope Behavior + + - **:all** - Authorizes without filtering (returns all records) + - **:own** - Filters to records where record.id == actor.id + - **:linked** - Filters based on resource type using Ash relationship traversal: + - Member: `user.id == actor.id` (traverses has_one :user relationship) + - CustomFieldValue: `member.user.id == actor.id` (traverses belongs_to :member + → has_one :user) + - Other resources: Falls back to `user.id == actor.id or user_id == actor.id` + ``` + + ### 2.2 Datei: `docs/roles-and-permissions-architecture.md` + + **Zeilen 799-812:** apply_scope Dokumentation aktualisieren + + ```markdown + # VORHER: + # - Member: member.user_id == actor.id + # - CustomFieldValue: custom_field_value.member.user_id == actor.id + + # NACHHER (konsistent mit Implementation): + - Member: `user.id == actor.id` (uses Ash relationship traversal) + - CustomFieldValue: `member.user.id == actor.id` (traverses relationships) + ``` + + --- + + ## Teil 3: Fallback-Verbesserung in HasPermission + + ### Datei: `lib/mv/authorization/checks/has_permission.ex` + + **Zeilen 205-208:** Fallback expliziter machen + + ```elixir + # VORHER: + _ -> + # Fallback for other resources: try user relationship first, then user_id + {:filter, expr(user.id == ^actor.id or user_id == ^actor.id)} + + # NACHHER (mit Logging): + _ -> + # Fallback for other resources: try user relationship first, then user_id + # This fallback is intentionally permissive - logs warning for investigation + Logger.warning(fn -> + "HasPermission using fallback :linked filter for resource: #{resource_name}. + " <> + "Consider adding explicit case for this resource." + end) + {:filter, expr(user.id == ^actor.id or user_id == ^actor.id)} + ``` + + --- + + ## Teil 4: Tests für Member Policies + + ### Neue Datei: `test/membership/member_policies_test.exs` + + ```elixir + defmodule Mv.Membership.MemberPoliciesTest do + @moduledoc """ + Tests for Member resource policies. + + Verifies that: + - Special case: Users can access linked member + - HasPermission scopes work correctly for each permission set + """ + use Mv.DataCase, async: true + + alias Mv.Membership + alias Mv.Accounts + + # Test Fixtures + defp create_user_with_role(permission_set_name) do + # Create role + {:ok, role} = Mv.Authorization.create_role(%{ + name: "Test Role #{System.unique_integer()}", + permission_set_name: permission_set_name + }) + + # Create user with role + {:ok, user} = Accounts.create_user(%{ + email: "test-#{System.unique_integer()}@example.com", + password: "password123!", + role_id: role.id + }) + + # Preload role + Ash.load!(user, :role) + end + + defp create_member_linked_to(user) do + {:ok, member} = Membership.create_member(%{ + email: user.email, + user: %{id: user.id} + }) + member + end + + defp create_unlinked_member do + {:ok, member} = Membership.create_member(%{ + email: "unlinked-#{System.unique_integer()}@example.com" + }) + member + end + + describe "Special Case: Linked Member Access" do + test "user can read their linked member regardless of role" do + user = create_user_with_role("own_data") + member = create_member_linked_to(user) + + result = Membership.get_member(member.id, actor: user) + + assert {:ok, returned_member} = result + assert returned_member.id == member.id + end + + test "user can update their linked member regardless of role" do + user = create_user_with_role("own_data") + member = create_member_linked_to(user) + + result = Membership.update_member(member, %{notes: "Updated"}, actor: + user) + + assert {:ok, updated_member} = result + assert updated_member.notes == "Updated" + end + end + + describe "own_data permission set (Mitglied)" do + test "cannot read unlinked member" do + user = create_user_with_role("own_data") + _linked_member = create_member_linked_to(user) + unlinked_member = create_unlinked_member() + + result = Membership.get_member(unlinked_member.id, actor: user) + + # Should return not found or forbidden + assert {:error, _} = result + end + + test "cannot create member" do + user = create_user_with_role("own_data") + + result = Membership.create_member(%{ + email: "new@example.com" + }, actor: user) + + assert {:error, %Ash.Error.Forbidden{}} = result + end + end + + describe "read_only permission set (Vorstand)" do + test "can read all members" do + user = create_user_with_role("read_only") + _member1 = create_unlinked_member() + _member2 = create_unlinked_member() + + result = Membership.list_members(actor: user) + + assert {:ok, members} = result + assert length(members) >= 2 + end + + test "cannot create member" do + user = create_user_with_role("read_only") + + result = Membership.create_member(%{ + email: "new@example.com" + }, actor: user) + + assert {:error, %Ash.Error.Forbidden{}} = result + end + + test "cannot update member" do + user = create_user_with_role("read_only") + member = create_unlinked_member() + + result = Membership.update_member(member, %{notes: "Updated"}, actor: + user) + + assert {:error, %Ash.Error.Forbidden{}} = result + end + end + + describe "normal_user permission set (Kassenwart)" do + test "can read all members" do + user = create_user_with_role("normal_user") + _member = create_unlinked_member() + + result = Membership.list_members(actor: user) + + assert {:ok, _members} = result + end + + test "can create member" do + user = create_user_with_role("normal_user") + + result = Membership.create_member(%{ + email: "new-#{System.unique_integer()}@example.com" + }, actor: user) + + assert {:ok, _member} = result + end + + test "can update any member" do + user = create_user_with_role("normal_user") + member = create_unlinked_member() + + result = Membership.update_member(member, %{notes: "Updated"}, actor: + user) + + assert {:ok, updated} = result + assert updated.notes == "Updated" + end + + test "cannot destroy member" do + user = create_user_with_role("normal_user") + member = create_unlinked_member() + + result = Membership.destroy_member(member, actor: user) + + assert {:error, %Ash.Error.Forbidden{}} = result + end + end + + describe "admin permission set" do + test "can perform all CRUD operations" do + user = create_user_with_role("admin") + + # Create + {:ok, member} = Membership.create_member(%{ + email: "admin-test-#{System.unique_integer()}@example.com" + }, actor: user) + + # Read + {:ok, _} = Membership.get_member(member.id, actor: user) + + # Update + {:ok, _} = Membership.update_member(member, %{notes: "Admin"}, actor: + user) + + # Destroy + {:ok, _} = Membership.destroy_member(member, actor: user) + end + end + end + ``` + + --- + + ## Ausführungsreihenfolge + + ### Schritt 1: HasPermission Verbesserungen (5 min) + 1. Öffne `lib/mv/authorization/checks/has_permission.ex` + 2. Update Moduledoc (Zeilen 19-26) + 3. Add Logging zum Fallback (Zeilen 205-208) + + ### Schritt 2: Member Policies hinzufügen (10 min) + 1. Öffne `lib/membership/member.ex` + 2. Finde Position nach `relationships do ... end` (ca. Zeile 528) + 3. Füge `policies do ... end` Block ein + 4. Optional: Prüfe ob actor preparation nötig + + ### Schritt 3: Dokumentation aktualisieren (5 min) + 1. Update `docs/roles-and-permissions-architecture.md` Zeilen 799-812 + + ### Schritt 4: Tests schreiben (15 min) + 1. Erstelle `test/membership/member_policies_test.exs` + 2. Implementiere Test-Fixtures + 3. Schreibe Tests für alle Permission Sets + + ### Schritt 5: Validierung (5 min) + 1. `just test test/membership/member_policies_test.exs` + 2. `just test test/mv/authorization/` (bestehende Tests) + 3. `just lint` + + --- + + ## Dateien die geändert werden + + | Datei | Änderung | + |-------|----------| + | `lib/membership/member.ex` | `policies do` Block hinzufügen (~15 Zeilen) | + | `lib/mv/authorization/checks/has_permission.ex` | Moduledoc + Fallback-Logging + (~10 Zeilen) | + | `docs/roles-and-permissions-architecture.md` | Scope-Dokumentation + aktualisieren (~5 Zeilen) | + | `test/membership/member_policies_test.exs` | Neue Datei (~150 Zeilen) | + + --- + + ## Risiken und Mitigationen + + | Risiko | Mitigation | + |--------|------------| + | Policy-Order falsch | Special Case MUSS vor General stehen | + | Actor ohne Role | HasPermission handhabt das bereits graceful | + | Bestehende Tests brechen | Tests mit `actor: user` Parameter aktualisieren | + | Performance | Minimal - Policies werden nur bei Queries evaluiert | + + --- + + ## Akzeptanzkriterien (aus Issue #7) + + - [x] Policies block added to Member resource + - [ ] All CRUD actions protected by HasPermission + - [ ] Special case: User can always access linked member + - [ ] Policy order is correct (specific before general) + - [ ] Actor preloads :role relationship + - [ ] All policies tested + + + +> /status + ⎿ Status dialog dismissed + + + diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..8b1d231 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,100 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## Project Overview + +**Mila** is a self-hostable membership management system for small to mid-sized clubs, built with Elixir/Phoenix and the Ash Framework. + +**Tech Stack:** +- Elixir 1.18.3 / Erlang/OTP 27.3.4 +- Phoenix 1.8 with LiveView 1.1 +- Ash Framework 3.0 with AshPostgres, AshAuthentication, AshPhoenix +- PostgreSQL 17 (dev) / 16 (prod) +- Tailwind CSS 4 + DaisyUI +- Just (task runner) + +## Essential Commands + +```bash +# Development +just run # Start full dev environment (DB + app) +just test # Run test suite +just test path/to/test.exs # Run single test file +just test path/to/test.exs:42 # Run single test at line +just lint # Format check + compile warnings + credo + gettext +just audit # Security audit (sobelow + deps.audit + hex.audit) +just format # Auto-format code +just ci-dev # Run lint + audit + test (pre-commit check) + +# Database +just reset-database # Reset dev and test databases +just migrate-database # Run migrations (mix ash.setup) + +# Migrations +just regen-migrations name # Rollback untracked migrations and regenerate +mix ash.codegen --name description # Generate new migration + +# Gettext (i18n) +just gettext # Extract and merge translations +``` + +## Architecture + +### Ash Domains (lib/) + +The codebase follows domain-driven design using Ash Framework: + +- **`lib/accounts/`** - User authentication domain (AshAuthentication) + - `User` - Authenticated users with OIDC/password strategies + - `Token` - Auth tokens + +- **`lib/membership/`** - Core membership domain + - `Member` - Club members with personal information + - `CustomField` - Admin-defined custom fields + - `CustomFieldValue` - Values for custom fields per member + - `Setting` - Application settings + +- **`lib/membership_fees/`** - Fee management domain + - `MembershipFeeType` - Fee type definitions + - `MembershipFeeCycle` - Fee cycles + +- **`lib/mv/authorization/`** - Authorization domain (in development) + - `Role` - User roles referencing permission sets + - `PermissionSets` - Permission definitions + +- **`lib/mv/`** - Core application modules + - `email_sync/` - Email synchronization between User and Member + - `repo.ex`, `application.ex`, `secrets.ex` + +- **`lib/mv_web/`** - Phoenix web layer + - `live/` - LiveView modules + - `components/` - Reusable UI components + - `controllers/` - HTTP controllers + +### Key Patterns + +**Ash Resources:** Each resource defines its own actions, validations, policies, and code interface. Use the domain's code interface for all operations: +```elixir +Mv.Membership.create_member(attrs) # Not Ash.create(Member, attrs) +``` + +**Email Sync:** User and Member emails are synchronized - User email is the source of truth. + +**LiveView:** Primary UI pattern. Controllers are thin; delegate to Ash domains. + +## Testing + +Tests mirror lib/ structure in `test/`. Use: +- `Mv.DataCase` for database tests +- `MvWeb.ConnCase` for controller/LiveView tests +- `async: true` when possible for parallel execution + +## Important Notes + +- Run `just ci-dev` before each commit to catch issues +- All modules require `@moduledoc` documentation (enforced by Credo) +- Follow existing Ash DSL section ordering (see `.formatter.exs` Spark config) +- Commit messages use conventional format: `feat:`, `fix:`, `docs:`, `refactor:`, `test:`, `chore:` +- Refer to `CODE_GUIDELINES.md` for detailed coding standards +- Refer to `docs/` for feature-specific documentation diff --git a/learning.md b/learning.md new file mode 100644 index 0000000..4ef23f6 --- /dev/null +++ b/learning.md @@ -0,0 +1,480 @@ +# Learning: Ash Framework, Policies, Phoenix & Elixir + +## Session Overview +This document captures key learnings from implementing authorization policies for the `Member` resource using Ash Framework 3.0. + +--- + +## 1. Ash Framework Policies + +### 1.1 Enabling Policies +- **Critical:** Policies require the `Ash.Policy.Authorizer` extension to be explicitly enabled +- Must be added in the resource definition: +```elixir +use Ash.Resource, + domain: Mv.Membership, + data_layer: AshPostgres.DataLayer, + authorizers: [Ash.Policy.Authorizer] # Required for policies! +``` +- Without this, the `policies` DSL block will not be available and will cause compilation errors + +### 1.2 Action Types vs Action Names +- **Key Discovery:** Policies match on **action types**, not specific action names +- Action Types: `:create`, `:read`, `:update`, `:destroy` +- Action Names: `:create_member`, `:update_member`, `:read_member`, etc. + +**Problem:** +```elixir +# In HasPermission check - WRONG: +action = authorizer.subject.action.name # Returns :create_member +``` + +**Solution:** +```elixir +# In HasPermission check - CORRECT: +action = authorizer.subject.action_type # Returns :create +``` + +**Why this matters:** The `PermissionSets` module defines permissions by action type (`:create`, `:read`, etc.), not by specific action names. Extracting the action name instead of the action type caused permission lookups to fail. + +### 1.3 Policy Structure +```elixir +policies do + # Policy order matters! Specific before general + + # Special cases first (optional) + policy action_type(:read) do + description "Special case for linked members" + authorize_if SomeCustomCheck + end + + # General authorization + policy action_type([:read, :create, :update, :destroy]) do + description "Standard permission check" + authorize_if HasPermission + end +end +``` + +### 1.4 Policy Directives +- `authorize_if`: Grants access if check passes +- `forbid_if`: Denies access if check passes +- `bypass`: Allows access if check passes, skipping remaining policies +- `policy do ... end`: Groups multiple checks together + +### 1.5 Policy Evaluation Results +Custom policy checks can return: +- `{:ok, true}`: Authorization granted +- `{:ok, false}`: Authorization denied +- `{:ok, :unknown}`: Let other policies evaluate + +**Use Case:** The `NoActor` check returns `:unknown` when an actor is present, allowing other policies to take over: +```elixir +def strict_check(actor, _authorizer, _opts) do + if is_nil(actor) do + {:ok, true} # Allow if no actor (for seeds) + else + {:ok, :unknown} # Let other policies decide + end +end +``` + +--- + +## 2. Custom Policy Checks + +### 2.1 Structure +```elixir +defmodule Mv.Authorization.Checks.MyCheck do + use Ash.Policy.Check + + @impl true + def describe(_opts), do: "description for debugging" + + @impl true + def strict_check(actor, authorizer, _opts) do + # Runtime authorization logic + {:ok, true | false | :unknown} + end + + @impl true + def auto_filter(actor, authorizer, _opts) do + # Query filter for list operations + nil | {:filter, expr(...)} + end +end +``` + +### 2.2 strict_check vs auto_filter +- **`strict_check`:** Called for single record operations (get, create, update, destroy) +- **`auto_filter`:** Called for list/query operations to filter results + +**Critical:** Both must be implemented correctly for comprehensive authorization! + +**Example from HasPermission:** +```elixir +def strict_check(actor, authorizer, _opts) do + # Check single record authorization + has_permission?(actor, authorizer) +end + +def auto_filter(actor, authorizer, _opts) do + # Return filter for list queries + case get_scope(actor, authorizer) do + :all -> nil # No filter needed + :linked -> {:filter, expr(id == ^actor.member_id)} + _ -> {:error, :forbidden} + end +end +``` + +--- + +## 3. Inverse Relationships & Authorization + +### 3.1 The Problem +When implementing the "linked member" special case, initial attempts failed: +```elixir +# WRONG: Tries to traverse from Member to User +policy action_type(:read) do + authorize_if expr(user.id == ^actor(:id)) +end +``` + +**Error:** `user` relationship is not loaded/available in this context + +### 3.2 Understanding the Relationship +``` +User (member_id) ──FK──> Member (id) + ^ | + └──── has_one :user ─┘ (inverse) +``` + +- `User` table has `member_id` column (foreign key) +- `Member` has `has_one :user` (inverse relationship) +- **No foreign key on Member table pointing to User!** + +### 3.3 The Solution +Check the relationship from the correct direction: +```elixir +# CORRECT: Check if actor's member_id matches this member's id +policy action_type(:read) do + authorize_if expr(^actor(:member_id) == id) +end +``` + +### 3.4 Applying to Scopes +In `HasPermission.apply_scope/3` for `:linked` scope: + +```elixir +case resource_name do + "Member" -> + # Member has_one :user (inverse), FK is on User side + {:filter, expr(id == ^actor.member_id)} + + "CustomFieldValue" -> + # CustomFieldValue belongs_to :member + # User.member_id → Member.id → CustomFieldValue.member_id + {:filter, expr(member.id == ^actor.member_id)} +end +``` + +**Key Insight:** Always check which table has the foreign key! + +--- + +## 4. SAT Solver for Policy Strictness + +### 4.1 The Warning +``` +No SAT solver is available, so some policy strictness checks cannot be run +``` + +### 4.2 The Solution +Add a SAT solver dependency to `mix.exs`: +```elixir +{:picosat_elixir, "~> 0.1", only: [:dev, :test]} +``` + +### 4.3 Why It's Needed +Ash uses a SAT solver to verify that policies don't have logical contradictions or unreachable branches. This is a compile-time safety check. + +--- + +## 5. Testing Authorization + +### 5.1 Actor in Tests +**Critical:** Authorization requires an actor to be passed to all operations + +```elixir +# Create with actor +{:ok, member} = Membership.create_member(%{...}, actor: user) + +# Read with actor +{:ok, member} = Ash.get(Mv.Membership.Member, id, actor: user, domain: Mv.Membership) + +# List with actor +members = Membership.list_members!(actor: user) +``` + +### 5.2 Reloading Actor After Relationships Change +**Problem:** After linking a member to a user, the `user` struct in memory doesn't have the updated `member_id`. + +```elixir +# Link member to user +{:ok, member} = member + |> Ash.Changeset.for_update(:update_member, %{}) + |> Ash.Changeset.manage_relationship(:user, user, type: :append_and_remove) + |> Ash.update(actor: admin_user) + +# CRITICAL: Reload user to get updated member_id! +user = Ash.get!(Mv.Accounts.User, user.id, domain: Mv.Accounts) +# Now user.member_id is populated +``` + +### 5.3 Test Fixtures with Admin Actor +When creating test fixtures, use an admin actor to bypass authorization: + +```elixir +defp create_member_linked_to(user) do + # Create admin for fixture setup + admin_user = create_admin_user() + + # Create member as admin + {:ok, member} = Membership.create_member(%{...}, actor: admin_user) + + # Link member as admin + {:ok, member} = member + |> Ash.Changeset.for_update(:update_member, %{}) + |> Ash.Changeset.manage_relationship(:user, user, type: :append_and_remove) + |> Ash.update(actor: admin_user) + + member +end +``` + +--- + +## 6. Database Seeding with Authorization + +### 6.1 The Problem +Once policies are active, seed scripts fail because they don't have an authenticated actor: + +```elixir +# In priv/repo/seeds.exs +Membership.create_member!(%{...}) # Ash.Error.Forbidden! +``` + +### 6.2 Solution: NoActor Check +Create a custom check that allows actions without an actor: + +```elixir +defmodule Mv.Authorization.Checks.NoActor do + use Ash.Policy.Check + + def describe(_opts), do: "allows if no actor is present" + + def strict_check(actor, _authorizer, _opts) do + if is_nil(actor) do + {:ok, true} # Allow seeding + else + {:ok, :unknown} # Let other policies decide + end + end + + def auto_filter(_actor, _authorizer, _opts), do: nil +end +``` + +Then add to policies: +```elixir +policy action_type(:create) do + description "Allow seeding without actor" + authorize_if NoActor +end +``` + +--- + +## 7. Debugging Policies + +### 7.1 Enable Policy Breakdowns +Add to `config/test.exs`: +```elixir +config :ash, :policies, show_policy_breakdowns?: true +``` + +This shows detailed policy evaluation in test output. + +### 7.2 NDJSON Logging +Structure for debug logs: +```elixir +defp log_debug(event, data) do + log_entry = %{ + location: "has_permission.ex", + message: event, + data: data, + timestamp: System.system_time(:millisecond), + sessionId: "debug-session" + } + + line = Jason.encode!(log_entry) <> "\n" + File.write!(".cursor/debug.log", line, [:append]) +end +``` + +### 7.3 Strategic Log Placement +Place logs at: +1. **Function entry** with parameters +2. **Before critical operations** (e.g., before permission lookup) +3. **After critical operations** (e.g., after permission lookup with result) +4. **Branch execution** (which if/else path was taken) +5. **Function exit** with return value + +### 7.4 Log Analysis Workflow +1. Delete old log file: `truncate -s 0 .cursor/debug.log` +2. Run failing test +3. Read log file line by line +4. Trace execution flow +5. Identify where actual behavior diverges from expected + +--- + +## 8. Key Ash Concepts + +### 8.1 Resources vs Domains +- **Resource:** Data model with attributes, relationships, actions (like `Member`) +- **Domain:** Logical grouping of resources (like `Membership`, `Accounts`) +- Domain must be specified in queries: `domain: Mv.Membership` + +### 8.2 Actions +- **Create actions:** `:create`, `:create_member` +- **Read actions:** `:read`, `:get_member`, `:list_members` +- **Update actions:** `:update`, `:update_member` +- **Destroy actions:** `:destroy`, `:delete_member` + +Each action has a **type** (one of the 4 above) and a **name** (specific) + +### 8.3 Changesets +All modifications go through changesets: +```elixir +member +|> Ash.Changeset.for_update(:update_member, %{first_name: "New"}) +|> Ash.Changeset.manage_relationship(:user, user, type: :append_and_remove) +|> Ash.update(actor: current_user) +``` + +--- + +## 9. Elixir Patterns + +### 9.1 Pattern Matching in Policy Checks +```elixir +def strict_check(%{role: %{permissions: permissions}}, authorizer, _opts) do + # Actor has role with permissions +end + +def strict_check(_actor, _authorizer, _opts) do + # Actor doesn't have role/permissions + {:ok, false} +end +``` + +### 9.2 With Expressions for Clean Authorization +```elixir +with %{role: %{permissions: permissions}} <- actor, + action <- get_action_from_authorizer(authorizer), + true <- check_permission(permissions, resource_name, action) do + {:ok, true} +else + _ -> {:ok, false} +end +``` + +--- + +## 10. Common Pitfalls & Solutions + +| Pitfall | Symptom | Solution | +|---------|---------|----------| +| Missing `Ash.Policy.Authorizer` | `undefined function policies/1` | Add `authorizers: [Ash.Policy.Authorizer]` | +| Using action name instead of type | `check_permission_not_found` | Use `authorizer.subject.action_type` not `.action.name` | +| Wrong relationship direction | Policy expression fails | Check which table has the FK | +| Actor not reloaded | `member_id` is `nil` after link | Reload actor from DB after relationship changes | +| Missing `auto_filter` | List queries return 0 results | Implement `auto_filter` in custom checks | +| Seeds fail after adding policies | `Ash.Error.Forbidden` in seeds | Add `NoActor` check or provide admin actor | + +--- + +## 11. Session Metrics + +- **Duration:** Multiple hours of debugging +- **Primary Issues Fixed:** 3 major (policies not loading, action type mismatch, inverse relationship) +- **Debug Iterations:** ~10+ test runs with progressive instrumentation +- **Files Modified:** 6 (member.ex, has_permission.ex, tests, config, mix.exs, seeds) +- **Tests Created:** 15+ covering all permission sets +- **Lines of Debug Logging Added:** ~50+ (across multiple files) + +--- + +## 12. Best Practices Established + +1. **Always enable policy authorizer explicitly** in resource definition +2. **Match on action types, not names** in policies and permission checks +3. **Understand relationship direction** before writing authorization filters +4. **Reload actors after relationship changes** in tests +5. **Implement both `strict_check` and `auto_filter`** for comprehensive authorization +6. **Use `NoActor` check for seeding** instead of disabling policies +7. **Add SAT solver dependency** for policy validation +8. **Enable policy breakdowns in test config** for debugging +9. **Use NDJSON logging for complex debugging** with strategic placement +10. **Test all permission sets systematically** (own_data, read_only, normal_user, admin) + +--- + +## 13. Architecture Decisions + +### 13.1 HasPermission as Central Check +Rather than spreading authorization logic across multiple policy checks, we centralized it in `HasPermission`: +- Single source of truth for permission logic +- Permission sets defined in one module (`PermissionSets`) +- Scopes (`:all`, `:linked`) applied consistently + +### 13.2 Policy Order +Keep policies simple with clear order: +1. Special cases first (if any) +2. General HasPermission check +3. Implicit forbid (by Ash) + +### 13.3 Minimal Special Cases +Initially tried multiple special case policies for linked members. **Simplified to just use HasPermission with scope handling:** +- Less complex policy block +- Easier to reason about +- Scope logic centralized in one place + +--- + +## 14. Future Considerations + +1. **Performance:** Monitor query performance with authorization filters on large datasets +2. **Caching:** Consider caching permission lookups if they become a bottleneck +3. **Audit Logging:** Add audit trail for authorization decisions (who accessed what) +4. **Dynamic Permissions:** Current system uses static permission sets; might need dynamic permissions per user in future +5. **Policy Testing:** Consider property-based testing for policies to catch edge cases +6. **Documentation:** Keep permission set documentation in sync with code + +--- + +## 15. Resources & Documentation + +- Ash Framework Policies: https://hexdocs.pm/ash/policies.html +- Ash Policy Authorizer: https://hexdocs.pm/ash/Ash.Policy.Authorizer.html +- Ash Policy Check: https://hexdocs.pm/ash/Ash.Policy.Check.html +- AshPostgres: https://hexdocs.pm/ash_postgres/ +- Elixir Pattern Matching: https://elixir-lang.org/getting-started/pattern-matching.html + +--- + +**Document Last Updated:** 2026-01-08 +**Session ID:** debug-session-member-policies +**Status:** Debugging in progress - investigating list query filtering issues diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 28cc712..27c1ce4 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -298,12 +298,12 @@ defmodule Mv.Membership.Member do # Authorization Policies # Order matters: Most specific policies first, then general permission check policies do - # SYSTEM OPERATIONS: Allow CRUD operations without actor + # SYSTEM OPERATIONS: Allow CRUD operations without actor (TEST ENVIRONMENT ONLY) # In test: All operations allowed (for test fixtures) - # In production: Only :create and :read allowed (enforced by NoActor.check) - # :read is needed for internal Ash lookups (e.g., relationship validation during user creation). + # 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 (seeds, tests, internal lookups)" + description "Allow system operations without actor (test environment only)" authorize_if Mv.Authorization.Checks.NoActor end @@ -397,8 +397,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 diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 3512ef0..68d83d1 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -131,13 +131,12 @@ defmodule Mv.Authorization.Checks.HasPermission do cond do is_nil(actor) -> # No actor - deny access (fail-closed) - # Return filter that never matches (using impossible condition) - # This ensures no records are returned when actor is missing - [id: {:not, {:in, []}}] + # Return filter that never matches (id IN [] = never matches) + deny_filter() is_nil(action) -> # Cannot determine action - deny access (fail-closed) - [id: {:not, {:in, []}}] + deny_filter() true -> auto_filter_with_permissions(actor, resource, action) @@ -169,16 +168,23 @@ defmodule Mv.Authorization.Checks.HasPermission do false -> # No permission - deny access (fail-closed) - # Return filter that never matches (using impossible condition) - [id: {:not, {:in, []}}] + deny_filter() end else _ -> # Error case (no role, invalid permission set, etc.) - deny access (fail-closed) - [id: {:not, {:in, []}}] + deny_filter() end end + # Helper function to return a filter that never matches (deny all records) + # Used when authorization should be denied (fail-closed) + # Returns [id: {:in, []}] which means "id IN []" - never matches (correct deny-all) + # NOTE: [id: {:not, {:in, []}}] would be "NOT (id IN [])" = true for all IDs (allow-all) - WRONG! + defp deny_filter do + [id: {:in, []}] + end + # Helper to extract action type from authorizer # CRITICAL: Must use action_type, not action.name! # Action types: :create, :read, :update, :destroy diff --git a/test/mv/authorization/checks/has_permission_integration_test.exs b/test/mv/authorization/checks/has_permission_integration_test.exs index dad68ea..9ef5fd2 100644 --- a/test/mv/authorization/checks/has_permission_integration_test.exs +++ b/test/mv/authorization/checks/has_permission_integration_test.exs @@ -28,7 +28,7 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do end describe "Filter Expression Structure - :linked scope" do - test "Member filter uses user.id relationship path" do + test "Member filter uses actor.member_id (inverse relationship)" do actor = create_actor_with_role("own_data", member_id: "member-123") authorizer = create_authorizer(Mv.Membership.Member, :read) @@ -42,7 +42,7 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do assert is_list(filter) or is_map(filter) end - test "CustomFieldValue filter uses member.user.id relationship path" do + test "CustomFieldValue filter uses actor.member_id (via member relationship)" do actor = create_actor_with_role("own_data", member_id: "member-123") authorizer = create_authorizer(Mv.Membership.CustomFieldValue, :read) From 05cbd833bccbe1e2c285ee0d23e51eb49e3be25e Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 23:12:07 +0100 Subject: [PATCH 2/2] Security: Fix critical deny-filter bug and improve authorization CRITICAL FIX: Deny-filter was allowing all records instead of denying Fix: User validation in Member now uses actor from changeset.context --- ...-_membership_fee_features_e886dc4b.plan.md | 318 ------------------ lib/membership/member.ex | 15 +- lib/mv/authorization/checks/has_permission.ex | 20 +- .../has_permission_integration_test.exs | 4 +- 4 files changed, 25 insertions(+), 332 deletions(-) delete mode 100644 code_review_fixes_-_membership_fee_features_e886dc4b.plan.md 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/lib/membership/member.ex b/lib/membership/member.ex index 28cc712..27c1ce4 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -298,12 +298,12 @@ defmodule Mv.Membership.Member do # Authorization Policies # Order matters: Most specific policies first, then general permission check policies do - # SYSTEM OPERATIONS: Allow CRUD operations without actor + # SYSTEM OPERATIONS: Allow CRUD operations without actor (TEST ENVIRONMENT ONLY) # In test: All operations allowed (for test fixtures) - # In production: Only :create and :read allowed (enforced by NoActor.check) - # :read is needed for internal Ash lookups (e.g., relationship validation during user creation). + # 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 (seeds, tests, internal lookups)" + description "Allow system operations without actor (test environment only)" authorize_if Mv.Authorization.Checks.NoActor end @@ -397,8 +397,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 diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 3512ef0..68d83d1 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -131,13 +131,12 @@ defmodule Mv.Authorization.Checks.HasPermission do cond do is_nil(actor) -> # No actor - deny access (fail-closed) - # Return filter that never matches (using impossible condition) - # This ensures no records are returned when actor is missing - [id: {:not, {:in, []}}] + # Return filter that never matches (id IN [] = never matches) + deny_filter() is_nil(action) -> # Cannot determine action - deny access (fail-closed) - [id: {:not, {:in, []}}] + deny_filter() true -> auto_filter_with_permissions(actor, resource, action) @@ -169,16 +168,23 @@ defmodule Mv.Authorization.Checks.HasPermission do false -> # No permission - deny access (fail-closed) - # Return filter that never matches (using impossible condition) - [id: {:not, {:in, []}}] + deny_filter() end else _ -> # Error case (no role, invalid permission set, etc.) - deny access (fail-closed) - [id: {:not, {:in, []}}] + deny_filter() end end + # Helper function to return a filter that never matches (deny all records) + # Used when authorization should be denied (fail-closed) + # Returns [id: {:in, []}] which means "id IN []" - never matches (correct deny-all) + # NOTE: [id: {:not, {:in, []}}] would be "NOT (id IN [])" = true for all IDs (allow-all) - WRONG! + defp deny_filter do + [id: {:in, []}] + end + # Helper to extract action type from authorizer # CRITICAL: Must use action_type, not action.name! # Action types: :create, :read, :update, :destroy diff --git a/test/mv/authorization/checks/has_permission_integration_test.exs b/test/mv/authorization/checks/has_permission_integration_test.exs index dad68ea..9ef5fd2 100644 --- a/test/mv/authorization/checks/has_permission_integration_test.exs +++ b/test/mv/authorization/checks/has_permission_integration_test.exs @@ -28,7 +28,7 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do end describe "Filter Expression Structure - :linked scope" do - test "Member filter uses user.id relationship path" do + test "Member filter uses actor.member_id (inverse relationship)" do actor = create_actor_with_role("own_data", member_id: "member-123") authorizer = create_authorizer(Mv.Membership.Member, :read) @@ -42,7 +42,7 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do assert is_list(filter) or is_map(filter) end - test "CustomFieldValue filter uses member.user.id relationship path" do + test "CustomFieldValue filter uses actor.member_id (via member relationship)" do actor = create_actor_with_role("own_data", member_id: "member-123") authorizer = create_authorizer(Mv.Membership.CustomFieldValue, :read)