Zum Inhalt

Code-Review — Block 7.1a (Tenants-Backend + operator-ui Keycloak-Client)

Branch: platform/block-7-1a-tenants-backend gegen platform/v1.0.0 Review-Scope: 5 Commits, 16 Dateien (+1622/-53), Backend-only. Merge-Blocker-Regel: Findings mit Severity ≥ 80 müssen vor Merge gefixt werden, < 80 werden nach offene-todos.md deferred.

Summary

ID Sev Datei:Zeile Titel Entscheidung
— — — Keine Merge-Blocker —
M1 65 operator_tenants.py:86–108 GET-List-TOCTOU unter Konkurrenz Deferred
M2 60 operator_tenants.py:198–221 changed_fields vor update_tenant — Session-Identity-Trap wenn Service auf ORM-bulk umgestellt wird Deferred
M3 55 tenant_service.py:155 updated_at wird client-seitig in Python gesetzt statt via onupdate=func.now() Deferred
L1 45 drei Route-Dateien Route-lokale Scope-Guards (TODO-Block-7-01) Deferred (schon getrackt)
L2 40 pyproject.toml:22 pydantic[email] zieht email-validator, idna, dnspython in Runtime-Image Deferred
L3 35 operator_tenants.py:91 search ohne ILIKE-Metachar-Escape False-Positive / Deferred
L4 30 operator_tenants.py:95–102 include_deleted für Vendor ohne Test Deferred

Alle Verifications (23/23 unit, 7/7 integration, 12/12 smoke-block7-1a, 12/12 smoke-block3, 25/25 Block-5 Regression, make redeploy-platform) grün. Plan-Alignment ist sauber: Scope-Boundaries (kein Frontend, kein Hard-Delete, kein 2-PC) werden exakt eingehalten.

Critical (≥80)

Keine. Die vier vom Auftraggeber priorisierten Risiken wurden geprüft:

  1. Audit-Delta-Mechanismus (tenant_service.py:189–207, operator_tenants.py:208–220): korrekt. changed_fields wird vor update_tenant aufgerufen — solange get_tenant dieselbe Instanz liefert wie die spätere update_tenant-Mutation, ist der before-Snap stabil (SQLAlchemy Identity-Map innerhalb einer Session garantiert das). Der Route-Code schreibt das Audit-Row nach dem Flush, aber in derselben Transaktion — Ausfall im Audit-Insert rollt die Mutation zurück (503 audit_unavailable aus _platform_audit.py:90). Der if before_delta:-Gate verhindert Leer-Audits bei No-Op-Patches.
  2. updated_at = datetime.now(UTC) vs DB-func.now(): nicht kritisch, aber sub-optimal — siehe M3.
  3. Transaktion/Rollback unter Audit-Failure: sauber. Der request_scoped_session-Contextmanager (engines.py:165–179) wrappt session.begin(), die 503-Exception aus dem Audit-Writer propagiert aus dem Block, SQLAlchemy rollt automatisch zurück. Getestet in Block-5 (25/25 Regression grün).
  4. Slug-Reservation inkl. Race-Fallback: luftdicht. Der pre-check in create_tenant:115 nutzt include_deleted=True. Sollte ein paralleler Create den Check überholen, fängt die DB-Unique-Constraint den Konflikt, der IntegrityError-Handler (:128–133) rollt zurück und konvertiert in TenantSlugConflict → saubere 409. Test 10 im Smoke deckt das ab.

Deferrable (<80)

M1 — GET-List-TOCTOU unter paralleler Mutation (Severity 65)

  • Datei:Zeile: src/kora_platform/api/routes/operator_tenants.py:86–108
  • Problem: list_tenants lädt erst count, dann eine separate SELECT- Query. Zwischen beiden kann ein paralleler Create/Delete die total- Zahl inkonsistent zu items machen. Für die Operator-UI ist das kosmetisch (Pagination-Flicker), bei sehr schnellen CRUD-Loops könnte eine Seite mit total=16, items=15 ausgeliefert werden.
  • Fix: Beide Queries in eine SELECT mit Window-Function (COUNT(*) OVER ()) konsolidieren — ein Roundtrip statt zwei, identischer Snapshot.
  • Deferral-Rationale: Operator-UI rendert max. 50 Zeilen in niedrigem Traffic. Kosmetischer Defekt, nicht datenschädlich. Trigger: sobald das Tenant-Volumen > 1000 oder die UI live-filter-driven ist.

M2 — Session-Identity-Trap bei künftigem ORM-bulk-update (Severity 60)

  • Datei:Zeile: src/kora_platform/api/routes/operator_tenants.py:198–221
  • Problem: before = get_tenant(...) und update_tenant(...) teilen dieselbe Session. changed_fields(before, payload) liest die alten Werte aus der ORM-Instanz bevor update_tenant setattr aufruft — funktioniert heute, weil update_tenant die Attribute auf derselben Instanz mutiert. Wenn der Service später auf session.execute(update(...) umgestellt wird (z. B. wegen Bulk-Patches), ist die before-Instanz dieselbe Identity, aber die Werte könnten per session.refresh bereits überschrieben worden sein.
  • Fix: before_snapshot = {f: getattr(before, f) for f in TenantUpdate.model_fields} direkt nach dem get_tenant-Call, dann changed_fields gegen das dict statt gegen die ORM-Instanz laufen lassen.
  • Deferral-Rationale: Bei heutiger Implementierung kein Bug, nur Refactoring-Fragilität. Trigger: wenn update_tenant auf Core-SQL wechselt (Block 12 Batch-Import).

M3 — updated_at per Python statt DB-onupdate (Severity 55)

  • Datei:Zeile: src/kora_platform/services/tenant_service.py:155, ebenfalls :181
  • Problem: tenant.updated_at = datetime.now(UTC) verwendet die Wall-Clock des API-Containers. Bei Clock-Skew zwischen API-Host und Postgres-Host können Audit-Timestamps (platform_audit_log.occurred_at = DB-now()) und tenants.updated_at divergieren. Außerdem konsistenzinkonsistent zu TenantPackage.configured_at (DB-default).
  • Fix: Im SQLA-Modell onupdate=func.now() auf updated_at setzen und die Python-Zuweisung streichen. Das verlangt aber eine Migration, die die bestehende Column um den Server-Trigger erweitert.
  • Deferral-Rationale: Kein Daten-Integritätsrisiko; die Drift wäre unter Single-Host-Docker-Deployment ≤ 1s. Trigger: Multi-Host-Deploy oder explizite Konsistenz-Policy (z. B. Block 12 Audit-Harmonisierung).

L1 — Route-lokale Scope-Guard-Duplikation (Severity 45)

  • Datei:Zeile: operator_tenants.py:52–65, tenant_modules.py:~50, chatbot_templates.py:~50
  • Problem: Drei Routes definieren _require_operator / _require_operator_or_vendor nahezu identisch. Bei Rollenmodell- Änderung (z. B. operator-editor vs operator-admin-Split) drei Stellen zu touchen.
  • Fix: kora_platform/api/dependencies/scope_guards.py mit FastAPI- Dependencies, per Depends(require_operator) injiziert.
  • Deferral-Rationale: Schon als TODO-Block-7-01 getrackt mit Konsolidierungs-Trigger nach Block 7.4 (wenn alle Operator-Routes existieren und das Pattern sich stabilisiert hat).

L2 — pydantic[email] Transitive-Dep-Footprint (Severity 40)

  • Datei:Zeile: pyproject.toml:22
  • Problem: pydantic[email] zieht email-validator → idna → dnspython. dnspython alleine sind ~2 MB Wheel + Attack-Surface (kann DNS-Queries triggern, wenn check_deliverability aktiv ist). Für Operator-UI-CRUD ist der RFC-5322-parser plus Regex-Fallback des DB-Layers ausreichend.
  • Fix: EmailStr durch str mit einem lean Regex-Validator (r"[^@\s]+@[^@\s]+\.[^@\s]+") ersetzen. Hebt drei Transitives.
  • Deferral-Rationale: email-validator hat Standard-mäßig check_deliverability=False, macht also keine DNS-Lookups. Attack- Surface gering. Trigger: wenn Supply-Chain-Review vor v1.0.0-Release Runtime-Deps reduzieren will.

L3 — ILIKE-Metachar-Escape in search (Severity 35)

  • Datei:Zeile: operator_tenants.py:91 → tenant_service.py:72–76
  • Problem: search wird als %{search}% in ILIKE eingebettet; SQL- Injection ist durch :bind-Parameter ausgeschlossen, aber %, _, \ im Suchstring werden als SQL-Wildcards interpretiert. Ein User, der nach einem Slug mit Underscore sucht, bekommt False-Matches.
  • Fix: Vor dem Wrap search.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_"), dann ILIKE mit ESCAPE '\'.
  • Deferral-Rationale: Operator-seitig genutzte Admin-Suche, keine Injection, nur UX-Glitch. Slugs enthalten per Regex keine Underscores/Prozente. Trigger: wenn search auch auf display_name mit Userinput-Kundendaten arbeitet (Block 7.4).

L4 — include_deleted für Vendor ohne Integrationstest (Severity 30)

  • Datei:Zeile: operator_tenants.py:95–102
  • Problem: Der Vendor darf include_deleted=true passieren; das Feature ist nirgends in den 7 Integrationstests abgedeckt.
  • Fix: test_vendor_include_deleted_lists_soft_deleted ergänzen.
  • Deferral-Rationale: Pfad ist trivial, das Risiko ≈ null. Trigger: Block 7.4, wenn die UI das Flag tatsächlich anklickt.

Looks good

  • Plan-Alignment: Alle acht Scope-Punkte aus dem Auftrag wurden eins-zu-eins umgesetzt. Keine unangekündigten Abweichungen.
  • Service-Layer: Die Entscheidung, dass der Service flusht aber nicht committet und der Route das Audit + Commit orchestriert, folgt konsistent dem Block-5/6-ModuleService-Pattern. Docstring-Header (tenant_service.py:1–17) erklärt das explizit.
  • Race-Fallback beim Slug-Conflict: pre-check + DB-constraint + IntegrityError-Konverter sind das komplette Paket. Lehrbuch-sauber.
  • was_fresh-Flag für Soft-Delete: elegant — Idempotenz ohne Doppel-Audit-Rows, Tests 9+10 im Smoke decken beide Pfade.
  • BYPASSRLS für Operator-Writes auf tenants: korrekt, weil die Tabelle FORCE ROW LEVEL SECURITY trägt und operatoren über alle Tenants hinweg schreiben müssen. Kommentar in der Route-Datei (:5–10) macht die Begründung reviewer-freundlich explizit.
  • Ops-Run-Hygiene: Smoke-Script purged Stale-Slugs idempotent (smoke-block7-1a.sh:39–40 und :210–211), Integration-Tests nutzen cleanup-Fixtures, Unit-Tests rollen pro Test zurück.
  • Keycloak-operator-ui-Client: PKCE S256, keine Direct-Grants, kein Service-Account, Frontchannel-Logout, saubere Trennung prod vs dev-Redirects. patch-dev-redirects.sh ist idempotent und purgt die Redirect-Set-Union korrekt.
  • Audit-Rename: Pure Rename, 4 Call-Sites aktualisiert, alte Datei sauber gelöscht (commit 95037f6).
  • Smoke-Block-3-Migration: Die beiden Tests (6, 8) gegen den neuen Pfad umzustellen statt den alten Stub zu behalten, ist die richtige Entscheidung — verhindert Doppel-Wartung.