Bernhard Müller
cfc32b3a7d
- Add TariffInviteServiceTest, GlobalExceptionHandlerTest, setup-test.ts - Add compose plans/specs docs - Extend .gitignore with *.xlsx, *.ps1, *.http, *.py, *.docx - Remove stale test data files and scripts from working tree
134 lines
7.2 KiB
Markdown
134 lines
7.2 KiB
Markdown
# Spec: Code-Review-Fixes — Kritische und hohe Befunde
|
|
|
|
## Zusammenfassung
|
|
|
|
Behebung von 3 kritischen und 5 hohen Code-Review-Findings im EEG Portal Backend. Die kritischen Finds betreffen Sicherheitslücken (fehlende Authorization, IDOR) und inkonsistente Fehlerbehandlung. Die hohen Finds betreffen Code-Qualität, Konsistenz und ein Produktionsrisiko.
|
|
|
|
## Ausgangslage
|
|
|
|
Ein Code-Review hat 8 Befunde identifiziert:
|
|
|
|
| # | Schwere | Befund |
|
|
|---|---------|--------|
|
|
| 1 | KRITISCH | `EnergyCommunityAdminController` fehlt `@PreAuthorize` — Admin-Endpunkte ohne Rollenprüfung |
|
|
| 2 | KRITISCH | `NotificationService.markAsRead` hat IDOR — jede User kann beliebige Notifications lesen |
|
|
| 3 | KRITISCH | `GlobalExceptionHandler` fehlt `MethodArgumentNotValidException`-Handler — @Valid-Fehler → 500 statt 400 |
|
|
| 4 | HOCH | Inkonsistente Exception-Strategy — `ResponseStatusException` bypassed `GlobalExceptionHandler` |
|
|
| 5 | HOCH | `TariffService` Code-Duplizierung — ~30 Zeilen validierende Logik doppelt |
|
|
| 6 | HOCH | `AtNumberAlreadyExistsException`-Handler verwirft Exception-Message |
|
|
| 7 | HOCH | `MeteringPoint.memberships` — `CascadeType.ALL` ohne `orphanRemoval` (latentes Risiko) |
|
|
| 8 | HOCH | `ddl-auto: update` als Prod-Default — kann Schema still ändern |
|
|
|
|
## Befund-Detail
|
|
|
|
### 1. Fehlende @PreAuthorize auf EnergyCommunityAdminController (KRITISCH)
|
|
|
|
**Datei:** `eeg_backend/.../community/api/EnergyCommunityAdminController.java:18-21`
|
|
|
|
**Ist-Zustand:** Kein `@PreAuthorize` auf Klassenebene. Endpunkte unter `/api/admin/energy-communities` sind nur durch URL-Muster gesichert (SecurityFilterChain), nicht durch annotation-basierte Authorization. Im Vergleich: `AdminIamController` hat `@PreAuthorize("hasRole('ADMIN')")` auf Klassenebene.
|
|
|
|
**Soll-Zustand:** `@PreAuthorize("hasRole('ADMIN')")` auf Klassenebene, konsistent mit `AdminIamController`.
|
|
|
|
**Risiko:** Jeder authentifizierte MEMBER kann Admin-Endpoints aufrufen (Community erstellen, bearbeiten, löschen), sofern der SecurityFilterChain keine explizite URL-Prüfung hat.
|
|
|
|
### 2. IDOR in NotificationService.markAsRead (KRITISCH)
|
|
|
|
**Dateien:**
|
|
- `eeg_backend/.../common/api/NotificationController.java:33-38`
|
|
- `eeg_backend/.../common/service/NotificationService.java:39-45`
|
|
|
|
**Ist-Zustand:** `markAsRead(UUID id)` nimmt nur die Notification-ID, kein `@CurrentUserId`. Der Service macht keinen Ownership-Check. Andere Methoden im selben Controller (`getNotifications`, `getUnreadCount`, `markAllAsRead`) verwenden korrekt `@CurrentUserId`.
|
|
|
|
**Soll-Zustand:** Controller fügt `@CurrentUserId String userId` Parameter hinzu. Service-Signatur wird `markAsRead(UUID id, UUID userId)`. Ownership-Check: `notification.getUserId().equals(userId)`, sonst `AccessDeniedException`.
|
|
|
|
### 3. Fehlender MethodArgumentNotValidException-Handler (KRITISCH)
|
|
|
|
**Datei:** `eeg_backend/.../common/exception/GlobalExceptionHandler.java`
|
|
|
|
**Ist-Zustand:** Kein `@ExceptionHandler(MethodArgumentNotValidException.class)`. Spring's Default-Handler liefert ein eigenes JSON-Format (field errors, global errors, object name) statt des projektspezifischen `ErrorResponse(message, status)`. @Valid-Fehler produzieren inkonsistente Responses.
|
|
|
|
**Soll-Zustand:** Handler extrahiert `BindingResult`-Fehler, formatiert sie als komma-separierte Nachricht, gibt 400 BAD_REQUEST mit `ErrorResponse` zurück.
|
|
|
|
### 4. Inkonsistente Exception-Strategy (HOCH)
|
|
|
|
**Dateien:**
|
|
- `eeg_backend/.../iam/service/AdminIamService.java:53-66` (`rejectUser`)
|
|
- `eeg_backend/.../community/service/EnergyCommunityService.java:53-73` (`update`, `delete`)
|
|
|
|
**Ist-Zustand:**
|
|
- `AdminIamService.approveUser`: wirft `IllegalArgumentException`/`IllegalStateException` → GlobalExceptionHandler → 400/409
|
|
- `AdminIamService.rejectUser`: wirft `ResponseStatusException` → bypassed GlobalExceptionHandler → Spring-Default
|
|
- `EnergyCommunityService.getCommunityById`: `IllegalArgumentException` → 400
|
|
- `EnergyCommunityService.update`: `ResponseStatusException(NOT_FOUND)` → bypassed
|
|
- `EnergyCommunityService.delete`: `IllegalArgumentException` + `ResponseStatusException(CONFLICT)` → gemischt
|
|
|
|
**Soll-Zustand:** Einheitlich `IllegalArgumentException` (400) und `IllegalStateException` (409) in Services. `ResponseStatusException` nur in Controllern (wie `AdminIamController.approveUser` already macht).
|
|
|
|
**Mapping:** IAE → 400 (GlobalExceptionHandler:39-45), ISE → 409 (GlobalExceptionHandler:47-53).
|
|
|
|
### 5. TariffService Code-Duplizierung (HOCH)
|
|
|
|
**Datei:** `eeg_backend/.../tariff/service/TariffService.java:74-197`
|
|
|
|
**Ist-Zustand:** `createUserTariff` (Zeilen 76-126) und `updateUserTariff` (Zeilen 160-197) teilen ~30 Zeilen identische Validierung:
|
|
- source != target MeteringPoint
|
|
- Beide Points vorhanden (findByById)
|
|
- Beide Points ACTIVE (MakoState)
|
|
- Community-Tarif vorhanden
|
|
- Preis <= Maximalpreis
|
|
- Beide User aktive Members der Community
|
|
|
|
**Soll-Zustand:** Private Methode `validateUserTariffRequest(UUID communityId, UserTariffRequest request)` extrahieren. `createUserTariff` ruft sie plus Duplicate-Tarif-Check und Invite-Check auf. `updateUserTariff` ruft sie plus Ownership-Check auf.
|
|
|
|
### 6. AtNumberAlreadyExistsException-Handler verwirft Message (HOCH)
|
|
|
|
**Datei:** `eeg_backend/.../common/exception/GlobalExceptionHandler.java:55-60`
|
|
|
|
**Ist-Zustand:** Handler gibt hardcoded `"Zählerpunkt existiert bereits"` zurück, obwohl `AtNumberAlreadyExistsException` eine Message mit der AT-Nummer enthält.
|
|
|
|
**Soll-Zustand:** `ex.getMessage()` verwenden, wie bei allen anderen Handlern.
|
|
|
|
### 7. MeteringPoint.memberships ohne orphanRemoval (HOCH)
|
|
|
|
**Datei:** `eeg_backend/.../community/domain/MeteringPoint.java:51-52`
|
|
|
|
**Ist-Zustand:** `@OneToMany(mappedBy = "meteringPoint", cascade = CascadeType.ALL)` — kein `orphanRemoval`. Wenn ein Membership aus der Liste entfernt wird, wird es nicht automatisch aus der DB gelöscht.
|
|
|
|
**Soll-Zustand:** `orphanRemoval = true` hinzufügen. Defensive Absicherung, auch wenn aktuell kein Delete-Endpoint existiert.
|
|
|
|
### 8. ddl-auto: update als Prod-Default (HOCH)
|
|
|
|
**Datei:** `eeg_backend/src/main/resources/application-prod.yml:9`
|
|
|
|
**Ist-Zustand:** `ddl-auto: ${JPA_DDL_AUTO:update}` — Default `update` kann in Produktion Spalten still hinzufügen/ändern, aber nie löschen.
|
|
|
|
**Soll-Zustand:** Default auf `validate` ändern. Schema-Änderungen nur über Migrationen (Flyway/Liquibase) oder manuell.
|
|
|
|
## Technische Abhängigkeiten
|
|
|
|
```
|
|
Finding 3 (Handler-Ergänzung) ─┐
|
|
Finding 6 (AtNumber-Handler) ─┤── Finding 4 (Exception-Unification)
|
|
Finding 1 (PreAuthorize) ─┤
|
|
Finding 2 (IDOR-Fix) ─┤
|
|
Finding 5 (Refactoring) ─┤
|
|
Finding 7 (orphanRemoval) ─┤
|
|
Finding 8 (ddl-auto) ─┘
|
|
```
|
|
|
|
Finding 3 und 6 müssen VOR Finding 4 kommen, da die Exception-Unification auf den bestehenden Handlern aufbaut. Alle anderen Findings sind unabhängig voneinander.
|
|
|
|
## Scope
|
|
|
|
### Im Scope
|
|
- Nur Backend-Änderungen (Java, YAML)
|
|
- Bestehende Tests aktualisieren (nicht neue Test-Suiten erstellen, wo Tests existieren)
|
|
- Conventional Commits
|
|
|
|
### Nicht im Scope
|
|
- Frontend-Änderungen
|
|
- Flyway/Liquibase-Migration (nur ddl-auto Default ändern)
|
|
- MeteringPoint-Lösch-Endpoint erstellen (nur orphanRemoval als Absicherung)
|
|
- Audit-Logging
|
|
- Rollenbasierte SecurityFilterChain-Änderungen
|