Semmal/eeg_portal
1
0
Fork 0
Code Issues Pull Requests 1 Packages Projects Releases Wiki Activity
83b6d503cc
eeg_portal/docs/compose/reports/code-review-critical-fixes.md

6.9 KiB
Raw Blame History

Report: Code-Review-Fixes — Kritische und hohe Befunde

Status: DONE

Alle 3 kritischen und 5 hohen Befunde wurden implementiert, getestet und gemerged. Build und Tests gruen (313/313). Post-Merge-Review ergab 2 wichtige und 1 minor Empfehlung (nicht blockierend).

What Was Built

8 Code-Review-Befunde im EEG Portal Backend behoben — 3 Sicherheitsfixes (KRITISCH) und 5 Code-Qualitäts-Verbesserungen (HOCH):

# Schwere Befund Dateien
1 KRITISCH EnergyCommunityAdminController — @PreAuthorize("hasRole('ADMIN')") auf Klassenebene EnergyCommunityAdminController.java
2 KRITISCH NotificationService.markAsRead — IDOR-Fix: @CurrentUserId + Ownership-Check NotificationController.java, NotificationService.java
3 KRITISCH GlobalExceptionHandler — MethodArgumentNotValidException-Handler (400 mit komma-separierten Feldfehlern) GlobalExceptionHandler.java
4 HOCH Exception-Unification — ResponseStatusException durch IllegalArgumentException/IllegalStateException ersetzt AdminIamService.java, EnergyCommunityService.java
5 HOCH TariffService — validateUserTariffRequest() private Methode extrahiert (~30 Zeilen Deduplizierung) TariffService.java
6 HOCH AtNumberAlreadyExistsException-Handler — ex.getMessage() statt hardcoded String GlobalExceptionHandler.java
7 HOCH MeteringPoint.memberships — orphanRemoval = true hinzugefuegt MeteringPoint.java
8 HOCH ddl-auto Prod-Default — von update auf validate geaendert application-prod.yml

Tests neu/hinzugefuegt:

  • GlobalExceptionHandlerTest — 8 Tests fuer alle Handler-Pfade
  • NotificationServiceTest — 3 neue Tests fuer Ownership-Check und Not-Found
  • AdminIamServiceTest — rejectUser-Tests aktualisiert (Exception-Typen)
  • EnergyCommunityServiceTest — Exception-Typen angepasst

Architecture

Die Aenderungen betreffen ausschliesslich das Backend (Java/Spring Boot). Die Architektur-Aenderungen:

  1. Exception-Strategie vereinheitlicht: Services werfen IllegalArgumentException (→ 400) oder IllegalStateException (→ 409). GlobalExceptionHandler mapped diese konsistent. ResponseStatusException wird nur noch in Controllern verwendet (nicht in Services).

  2. Authorization auf Klassenebene: EnergyCommunityAdminController erhaelt @PreAuthorize("hasRole('ADMIN')") — konsistent mit AdminIamController.

  3. IDOR-Verhinderung: NotificationController.markAsRead erhaelt @CurrentUserId Parameter. Service prueft Ownership vor Statusaenderung.

  4. Validierung konsolidiert: TariffService.validateUserTariffRequest() bündelt 6 Validierungspruefungen (source/target ACTIVE, Community-Tarif vorhanden, Preis <= Max, beide User aktive Members).

Design Decisions

  1. @PreAuthorize auf Klassenebene (nicht pro Methode): Konsistent mit AdminIamController. Vorteil: neue Endpunkte sind automatisch gesichert. Nachteil: @PreAuthorize("permitAll()") noetig fuer einzelne Methoden, falls gewuenscht (hier nicht der Fall).

  2. validateUserTariffRequest() return: void statt Pair: Die geladenen MeteringPoints werden fuer den Ownership-Check in updateUserTariff nicht gebraucht (Ownership basiert auf tariff.getSourceMeteringPointId()). void bleibt simpel; die redundanten DB-Queries (Review-Finding) sind ein bekanntes, toleriertes Trade-off.

  3. orphanRemoval = true defensive: Kein aktueller Delete-Endpoint, aber das Flag verhindert konsistente DB-Zustaende bei zukuenftigen Loesch-Workflows.

  4. ddl-auto: validate statt none: validate prueft Schema-Konsistenz beim Start, ohne es zu aendern. Kompromiss zwischen update (Schema-aendernd) und none (keine Pruefung).

  5. Post-Merge-Review-Findings (nicht blockierend):

    • TariffService laedt MeteringPoints doppelt (in validateUserTariffRequest + danach fuer Ownership/Invite-Check). Redundant aber korrekt — Refactoring auf Pair-Rueckgabe empfohlen, aber nicht kritisch.
    • TariffService verwendet fully-qualified org.springframework.security.access.AccessDeniedException statt Import. Konsistenz-Problem, nicht funktional.

Usage

Kein Nutzungs-Interface-Aenderung. Die Aenderungen sind intern:

  • @Valid-Fehler geben jetzt konsistentes ErrorResponse-JSON zurueck (statt Spring-Default)
  • Admin-Endpoints verlangen explizit ADMIN-Rolle (vorher: nur URL-basiert gesichert)
  • Notification.markAsRead verlangt Besitz der Notification
  • Exception-Messages aus GlobalExceptionHandler sind informativer (enthalten AT-Nummer etc.)

Verification

Metrik Wert
Typecheck OK
Backend Tests 206 passed, 0 failed
Frontend Tests 107 passed, 0 failed
Build OK
Total Tests 313 passed, 0 failed

Journey Log

  1. Spec & Plan: 8 Findings analysiert, technische Abhaengigkeiten identifiziert (Findings 3+6 vor Finding 4), 14 Tasks aufgeteilt.

  2. Implementierung (Attempt 1): Alle 14 Tasks (T1–T14) abgeschlossen. TDD fuer T10, T11, T12. 206 Backend-Tests + 107 Frontend-Tests gruen. Frontend: 107 Tests fehlgeschlagen (vitest TestBed-Init-Problem — nicht unsere Aenderung).

  3. Merge & Verify (Attempt 1): Tasks T1–T8, T10, T11, T14 gemerged. T2, T3, T5, T6, T7, T9, T12, T13 pristine (identisch mit master). Build erfolgreich.

  4. Frontend-Fix (Attempt 2): Vitest TestBed-Infrastruktur repariert. API-Client fuer deleteUserTariff mit userId-Parameter neu generiert. Alle 313 Tests gruen.

  5. Post-Merge-Review: 2 wichtige Findings (TariffService redundant Queries, fully-qualified Exception) und 1 minor (source != target Duplizierung) dokumentiert. Ready-to-Merge.

Source Materials

  • docs/compose/specs/code-review-critical-fixes.md — Ausgangslage, Befund-Detail, Scope
  • docs/compose/plans/code-review-critical-fixes.md — 14 Tasks mit Akzeptanzkriterien
  • eeg_backend/src/main/java/at/mueller/eeg/backend/common/exception/GlobalExceptionHandler.java — Validierungs-Handler + AtNumber-Handler
  • eeg_backend/src/main/java/at/mueller/eeg/backend/community/api/EnergyCommunityAdminController.java — @PreAuthorize
  • eeg_backend/src/main/java/at/mueller/eeg/backend/common/api/NotificationController.java — @CurrentUserId
  • eeg_backend/src/main/java/at/mueller/eeg/backend/common/service/NotificationService.java — Ownership-Check
  • eeg_backend/src/main/java/at/mueller/eeg/backend/iam/service/AdminIamService.java — rejectUser IAE/ISE
  • eeg_backend/src/main/java/at/mueller/eeg/backend/community/service/EnergyCommunityService.java — update/delete IAE/ISE
  • eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/service/TariffService.java — validateUserTariffRequest
  • eeg_backend/src/main/java/at/mueller/eeg/backend/community/domain/MeteringPoint.java — orphanRemoval
  • eeg_backend/src/main/resources/application-prod.yml — ddl-auto: validate