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 deleted file mode 100644 index 267caad..0000000 --- a/2026-01-08-fhre-git-diff-main-aus-lese-dir-die-roles-and-pe.txt +++ /dev/null @@ -1,751 +0,0 @@ - - * ▐▛███▜▌ * 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 deleted file mode 100644 index 8b1d231..0000000 --- a/CLAUDE.md +++ /dev/null @@ -1,100 +0,0 @@ -# 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/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/learning.md b/learning.md deleted file mode 100644 index 4ef23f6..0000000 --- a/learning.md +++ /dev/null @@ -1,480 +0,0 @@ -# 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