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
This commit is contained in:
parent
b3eb6c9223
commit
42a463f422
GPG key ID:
1020A035E5DD0824
4 changed files with 25 additions and 332 deletions
|
|
@ -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
|
||||
|
||||
|
|
@ -300,12 +300,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
|
||||
|
||||
|
|
@ -399,8 +399,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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue