mirror of
https://github.com/s-frick/effigenix.git
synced 2026-03-28 18:49:59 +01:00
c2c48a03e8
- Move Java backend to backend/ directory - Create frontend/ directory for TypeScript TUI and future WebUI - Update .gitignore for Node.js and worktrees - Update README.md with new repository structure - Copy documentation to backend/
292 lines
9.5 KiB
Markdown
292 lines
9.5 KiB
Markdown
# Ticket 001 – Code Review: User Management Bounded Context
|
||
|
||
**Datum:** 2026-02-17
|
||
**Commit:** `ec9114a` – feat: add Spring Boot ERP application with user management domain
|
||
**Status:** Offen
|
||
|
||
---
|
||
|
||
## Kritisch
|
||
|
||
### K1 – Aggregates sind nicht immutable
|
||
|
||
**Betroffene Dateien:**
|
||
- `domain/usermanagement/User.java` (Zeilen 22–29)
|
||
- `domain/usermanagement/Role.java` (Zeilen 18–21)
|
||
|
||
**Problem:**
|
||
Felder in `User` und `Role` sind nicht `final`. Business-Methoden wie `lock()`, `unlock()`, `updateEmail()`, `changePassword()` mutieren den internen State direkt statt neue Instanzen oder `Result` zurückzugeben. Die interne `roles`-Collection in `User` (Zeile 25) ist mutable und wird per `assignRole()`/`removeRole()` in-place modifiziert.
|
||
|
||
**Lösung:**
|
||
- Alle Felder `final` machen
|
||
- Business-Methoden geben `Result<UserError, User>` zurück (Copy-on-Write)
|
||
- Interne Collections nur über defensive Kopien exponieren
|
||
|
||
---
|
||
|
||
### K2 – Fehlende Command-Validierung im Application Layer
|
||
|
||
**Betroffene Dateien:**
|
||
- `application/usermanagement/command/CreateUserCommand.java`
|
||
- `application/usermanagement/command/UpdateUserCommand.java`
|
||
- `application/usermanagement/command/AuthenticateCommand.java`
|
||
- `application/usermanagement/command/ChangePasswordCommand.java`
|
||
- `application/usermanagement/command/AssignRoleCommand.java`
|
||
|
||
**Problem:**
|
||
Alle Commands sind reine Records ohne jegliche Validierung. Wird der Application Layer außerhalb des HTTP-Kontexts aufgerufen (z.B. Scheduled Jobs, Message Queues), gibt es keine Input-Checks. Die Validierung liegt ausschließlich in den Web-DTOs (`@Valid`-Annotationen).
|
||
|
||
**Lösung:**
|
||
- Self-validating Commands mit Factory-Methoden, die `Result` zurückgeben
|
||
- Oder Validierung als ersten Schritt im Use Case
|
||
|
||
---
|
||
|
||
### K3 – Authorization fehlt im Application Layer
|
||
|
||
**Betroffene Dateien:**
|
||
- `application/usermanagement/CreateUser.java`
|
||
- `application/usermanagement/LockUser.java`
|
||
- `application/usermanagement/AssignRole.java`
|
||
- (alle Use Cases betroffen)
|
||
|
||
**Problem:**
|
||
`AuthorizationPort` wird im Application Layer nicht verwendet. Autorisierung liegt ausschließlich auf `@PreAuthorize` im Controller. Bei Non-HTTP-Aufrufen wird die Authorization komplett umgangen.
|
||
|
||
**Lösung:**
|
||
- `AuthorizationPort.can(actorId, action)` als ersten Check in jedem Use Case aufrufen
|
||
- Bei fehlender Berechtigung `Result.failure(UserError.Unauthorized(...))` zurückgeben
|
||
|
||
---
|
||
|
||
### K4 – Token-Blacklist Memory Leak
|
||
|
||
**Betroffene Datei:**
|
||
- `infrastructure/security/JwtSessionManager.java` (Zeilen 35–36, 164–168)
|
||
|
||
**Problem:**
|
||
Die Token-Blacklist nutzt `ConcurrentHashMap.newKeySet()` ohne TTL oder Cleanup. Bei laufendem Server wächst die Blacklist unbegrenzt. Nach Server-Restart ist die Blacklist leer – ausgeloggte Tokens werden wieder gültig. Funktioniert nicht in Cluster-Deployments.
|
||
|
||
**Lösung:**
|
||
- Redis-basierte Blacklist mit TTL = Token-Expiration
|
||
- Oder In-Memory mit Scheduled Cleanup abgelaufener Tokens
|
||
|
||
---
|
||
|
||
### K5 – Audit-Bug in ChangePassword
|
||
|
||
**Betroffene Datei:**
|
||
- `application/usermanagement/ChangePassword.java` (Zeile 52)
|
||
|
||
**Problem:**
|
||
Im Fehlerfall wird `AuditEvent.PASSWORD_CHANGED` geloggt statt `PASSWORD_CHANGE_FAILED`. Falsche Audit-Events verfälschen das Security-Monitoring.
|
||
|
||
**Lösung:**
|
||
- Korrektes Event `PASSWORD_CHANGE_FAILED` im Fehlerfall loggen
|
||
- Audit-Events auf Success/Failure-Pfade prüfen
|
||
|
||
---
|
||
|
||
## Mittel
|
||
|
||
### M1 – Fehlende Status-Transitionen-Validierung
|
||
|
||
**Betroffene Datei:**
|
||
- `domain/usermanagement/User.java` (Zeilen 118–132)
|
||
|
||
**Problem:**
|
||
`lock()`, `unlock()`, `deactivate()`, `activate()` validieren den aktuellen Status nicht. Invalide Übergänge sind möglich (z.B. einen inaktiven User sperren). Keine Idempotenz-Checks.
|
||
|
||
**Lösung:**
|
||
- State-Machine oder explizite Guards für erlaubte Übergänge
|
||
- `Result<UserError, User>` als Rückgabetyp
|
||
|
||
---
|
||
|
||
### M2 – Inkonsistente Result-Nutzung in Domain-Methoden
|
||
|
||
**Betroffene Datei:**
|
||
- `domain/usermanagement/User.java` (Zeilen 106–159)
|
||
|
||
**Problem:**
|
||
Manche Business-Methoden geben `Result` zurück (`assignRole()`, `changePassword()`), andere `void` (`removeRole()`, `lock()`, `unlock()`, `updateBranch()`, `updateLastLogin()`). Inkonsistentes API-Design.
|
||
|
||
**Lösung:**
|
||
- Alle Business-Methoden einheitlich auf `Result<UserError, User>` umstellen (siehe K1)
|
||
|
||
---
|
||
|
||
### M3 – RemoveRole bricht Command-Pattern
|
||
|
||
**Betroffene Datei:**
|
||
- `application/usermanagement/RemoveRole.java` (Zeile 44)
|
||
|
||
**Problem:**
|
||
Einziger Use Case, der Raw-Parameter statt eines Command-Objekts akzeptiert:
|
||
```java
|
||
public Result<UserError, UserDTO> execute(String userId, RoleName roleName, ActorId performedBy)
|
||
```
|
||
Alle anderen Use Cases nutzen Command-Records.
|
||
|
||
**Lösung:**
|
||
- `RemoveRoleCommand` Record erstellen und in `execute()` verwenden
|
||
|
||
---
|
||
|
||
### M4 – Refresh Token nicht implementiert
|
||
|
||
**Betroffene Datei:**
|
||
- `infrastructure/security/JwtSessionManager.java` (Zeilen 141–144)
|
||
|
||
**Problem:**
|
||
`refreshSession()` wirft `UnsupportedOperationException`. User-Status (gesperrt/gelöscht) wird beim Token-Refresh nicht geprüft.
|
||
|
||
**Lösung:**
|
||
- Implementierung: User laden, Status verifizieren, neues Token-Paar ausstellen
|
||
- Altes Refresh-Token auf Blacklist setzen
|
||
|
||
---
|
||
|
||
### M5 – Kein Rate Limiting auf Login-Endpoint
|
||
|
||
**Betroffene Datei:**
|
||
- `infrastructure/usermanagement/web/controller/AuthController.java` (Zeile 90)
|
||
|
||
**Problem:**
|
||
`/api/auth/login` hat keinen Brute-Force-Schutz. Angreifer können Credentials unbegrenzt durchprobieren.
|
||
|
||
**Lösung:**
|
||
- Rate Limiting per IP/Username (z.B. Bucket4j, Spring Cloud Gateway)
|
||
- Account-Lockout nach N fehlgeschlagenen Versuchen
|
||
- Exponentielles Backoff
|
||
|
||
---
|
||
|
||
### M6 – CORS deaktiviert statt konfiguriert
|
||
|
||
**Betroffene Datei:**
|
||
- `infrastructure/security/SecurityConfig.java` (Zeile 69)
|
||
|
||
**Problem:**
|
||
CORS ist komplett deaktiviert (`AbstractHttpConfigurer::disable`). TODO-Kommentar im Code, aber keine Konfiguration. Browser-basierte Clients können keine Cross-Origin-Requests senden.
|
||
|
||
**Lösung:**
|
||
- CORS mit expliziten Allowed Origins, Methods und Headers konfigurieren
|
||
- Environment-abhängig (Dev: localhost, Prod: spezifische Domain)
|
||
|
||
---
|
||
|
||
### M7 – Error Response Information Disclosure
|
||
|
||
**Betroffene Datei:**
|
||
- `infrastructure/security/SecurityConfig.java` (Zeilen 100–113)
|
||
|
||
**Problem:**
|
||
Custom Exception Handler gibt `authException.getMessage()` direkt im JSON-Response zurück. Kann interne Implementierungsdetails exponieren.
|
||
|
||
**Lösung:**
|
||
- Generische Fehlermeldungen für Authentication/Authorization Failures
|
||
- Details nur ins Server-Log schreiben
|
||
|
||
---
|
||
|
||
## Niedrig
|
||
|
||
### N1 – Swagger UI öffentlich zugänglich
|
||
|
||
**Betroffene Datei:**
|
||
- `infrastructure/security/SecurityConfig.java` (Zeile 83)
|
||
- `application.yml` (Zeile 56)
|
||
|
||
**Problem:**
|
||
Swagger UI ist standardmäßig ohne Authentifizierung erreichbar. Exponiert alle API-Endpoints mit Parametern.
|
||
|
||
**Lösung:**
|
||
- In Production deaktivieren oder hinter Authentication legen
|
||
- Spring-Profile nutzen: `springdoc.swagger-ui.enabled=${SWAGGER_ENABLED:false}`
|
||
|
||
---
|
||
|
||
### N2 – Default-Admin Credentials in Seed-Daten
|
||
|
||
**Betroffene Datei:**
|
||
- `db/changelog/changes/004-seed-admin-user.sql` (Zeilen 2–3, 26)
|
||
|
||
**Problem:**
|
||
Admin-Credentials `admin/admin123` stehen als Kommentar in der SQL-Datei und als Table-Comment in der Datenbank. Bei versehentlicher DB-Exposure sind Credentials sofort sichtbar.
|
||
|
||
**Lösung:**
|
||
- Seed-Daten nur in Dev-Profil ausführen
|
||
- Table-Comment mit Credentials entfernen
|
||
- Production: Admin über Environment-Variablen oder Init-Script bootstrappen
|
||
|
||
---
|
||
|
||
### N3 – CreateUser erlaubt User ohne Rollen
|
||
|
||
**Betroffene Datei:**
|
||
- `application/usermanagement/CreateUser.java` (Zeile 76)
|
||
|
||
**Problem:**
|
||
Wenn `cmd.roleNames()` leer ist, wird ein User ohne Rollen erstellt. Kein Check, ob mindestens eine Rolle zugewiesen wird.
|
||
|
||
**Lösung:**
|
||
- Validierung in Command oder Use Case: mindestens eine Rolle erforderlich
|
||
|
||
---
|
||
|
||
### N4 – Falscher Error-Typ in Role.addPermission()
|
||
|
||
**Betroffene Datei:**
|
||
- `domain/usermanagement/Role.java` (Zeile 70)
|
||
|
||
**Problem:**
|
||
Bei `null`-Permission wird `UserError.NullRole()` verwendet – semantisch falsch.
|
||
|
||
**Lösung:**
|
||
- Eigenen Error-Typ verwenden oder `IllegalArgumentException` (da interner Programmierfehler)
|
||
|
||
---
|
||
|
||
### N5 – Fehlende Uniqueness-Validierung im Domain Layer
|
||
|
||
**Betroffene Datei:**
|
||
- `domain/usermanagement/User.java` (Zeilen 64–69)
|
||
|
||
**Problem:**
|
||
Username- und Email-Uniqueness wird nur im Application Layer geprüft (Repository-Abfrage). Der Domain Layer dokumentiert diese Invariante nicht explizit.
|
||
|
||
**Lösung:**
|
||
- Domain Service für Uniqueness-Checks oder explizite Dokumentation als Application-Layer-Invariante
|
||
|
||
---
|
||
|
||
### N6 – Repetitiver Switch-Pattern in Use Cases
|
||
|
||
**Betroffene Dateien:**
|
||
- Alle Use Cases im Application Layer
|
||
|
||
**Problem:**
|
||
Das Pattern `switch (repository.method()) { case Failure f -> ...; case Success s -> ...; }` wiederholt sich ~30+ Mal. Hoher Boilerplate-Anteil.
|
||
|
||
**Lösung:**
|
||
- `Result.flatMap()` und `Result.mapError()` konsequent nutzen
|
||
- Helper-Methode für Repository-Error → UserError Mapping
|
||
|
||
---
|
||
|
||
## Checkliste Production-Readiness
|
||
|
||
- [ ] K1: Aggregates immutable machen
|
||
- [ ] K2: Command-Validierung im Application Layer
|
||
- [ ] K3: AuthorizationPort in Use Cases integrieren
|
||
- [ ] K4: Token-Blacklist mit TTL/Redis ersetzen
|
||
- [ ] K5: Audit-Bug in ChangePassword fixen
|
||
- [ ] M4: Refresh Token implementieren
|
||
- [ ] M5: Rate Limiting einrichten
|
||
- [ ] M6: CORS konfigurieren
|
||
- [ ] N1: Swagger in Production absichern
|
||
- [ ] N2: Seed-Daten Production-safe machen
|
||
- [ ] JWT_SECRET, DB_PASSWORD als Environment-Variablen setzen
|
||
- [ ] HTTPS/TLS konfigurieren
|
||
- [ ] Log-Aggregation und Security-Monitoring aufsetzen
|