09ec2103c8
Missing closing braces caused Spring to inject the literal placeholder string instead of resolving the property, silently ignoring any app.admin.username / app.admin.password env-var overrides. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
142 lines
7.0 KiB
Markdown
142 lines
7.0 KiB
Markdown
# Backend TODO
|
||
|
||
Findings from architectural review. Ordered roughly by severity.
|
||
|
||
---
|
||
|
||
## Bugs
|
||
|
||
### `@Value` annotation missing closing brace
|
||
**File:** `config/DataInitializer.java:33–37`
|
||
```java
|
||
// Current (broken — Spring may silently fail to resolve the property)
|
||
@Value("${app.admin.username:admin")
|
||
@Value("${app.admin.password:admin123")
|
||
|
||
// Fix
|
||
@Value("${app.admin.username:admin}")
|
||
@Value("${app.admin.password:admin123}")
|
||
```
|
||
Spring's `@Value` SpEL parser requires the closing `}`. Without it, the literal string `"${app.admin.username:admin"` is injected instead of the resolved value. The default credentials will appear to work but env-var overrides will silently be ignored.
|
||
|
||
---
|
||
|
||
### Default credentials logged in plaintext
|
||
**File:** `config/DataInitializer.java:64`
|
||
```java
|
||
log.info("Default Admin erstellt: User='admin', Pass='admin123'");
|
||
```
|
||
The password appears in application logs. Remove the password from the log statement entirely. Log only the username.
|
||
|
||
---
|
||
|
||
## Design Issues
|
||
|
||
### Test data runs in every environment
|
||
**File:** `config/DataInitializer.java:69–143`
|
||
|
||
`initData` seeds 500 fake documents and 4 random persons whenever the database is empty. This runs unconditionally — including in production — and the log message at line 141 claims "50 Personen" when only 4 are created (copy/paste error).
|
||
|
||
**Fix:** Guard the bean with a Spring profile so it only runs locally:
|
||
```java
|
||
@Bean
|
||
@Profile("dev")
|
||
public CommandLineRunner initData(...) { ... }
|
||
```
|
||
Set `spring.profiles.active=dev` in `application.properties` for local use, and ensure the production environment does not set that profile.
|
||
|
||
---
|
||
|
||
### Two redundant permission groups created on startup
|
||
**File:** `config/DataInitializer.java:49–53` and `101–105`
|
||
|
||
`initAdminUser` creates group `"Administrators"` with `{ADMIN, READ_ALL, WRITE_ALL}`.
|
||
`initData` creates group `"Admins"` with `{READ_ALL, WRITE_ALL, ADMIN}` — identical permissions, different name, different bean.
|
||
|
||
Both beans run when the DB is empty, resulting in two duplicate admin groups and the admin user only belonging to the first one. `initData` is also guarded by `personRepo.count() > 0` — so if persons already exist it won't create the second group, making startup behaviour inconsistent.
|
||
|
||
**Fix:** Consolidate into a single `CommandLineRunner` bean or extract group creation into a shared `@PostConstruct` method. Remove `initData`'s group creation entirely (it belongs in `initAdminUser`).
|
||
|
||
---
|
||
|
||
### CSRF protection disabled with no plan to re-enable it
|
||
**File:** `config/SecurityConfig.java:39`
|
||
```java
|
||
.csrf(csrf -> csrf.disable())
|
||
```
|
||
The comment says "for development". HTTP Basic Auth over a cookie-authenticated SvelteKit frontend is still vulnerable to CSRF for mutating endpoints (PUT, DELETE, POST).
|
||
|
||
**Fix:** Either:
|
||
- Re-enable CSRF with `CookieCsrfTokenRepository` and pass the token to the frontend, or
|
||
- Adopt a stateless JWT/session approach and verify `Origin`/`Referer` headers
|
||
|
||
At minimum, document this as a known gap that must be addressed before any external deployment.
|
||
|
||
---
|
||
|
||
### Spring Session tables are created but never used
|
||
**File:** `db/migration/V1__initial_schema.sql` (Spring Session tables), `config/SecurityConfig.java`
|
||
|
||
The schema includes `spring_session` and `spring_session_attributes` tables, but the auth model is stateless HTTP Basic — credentials are re-validated from the database on every request. Spring Session JDBC is wiring itself up but managing nothing meaningful.
|
||
|
||
**Fix:** Decide on one model:
|
||
- **Keep Basic Auth (stateless):** Remove the Spring Session JDBC dependency and tables. Each request re-authenticates — which is fine for a small internal app.
|
||
- **Switch to session-based auth:** Replace Basic Auth with form login, issue a server-side session ID, and let Spring Session manage it. This reduces per-request DB hits.
|
||
|
||
---
|
||
|
||
### `Permission` enum not used consistently — permissions are plain strings
|
||
**File:** `security/Permission.java`, `security/RequirePermission.java`, `security/PermissionAspect.java`
|
||
|
||
`@RequirePermission` takes a `Permission` enum value, but user group permissions are stored and compared as raw `String` values (e.g., `"ADMIN"`, `"READ_ALL"`). The comparison in `PermissionAspect` calls `permission.name()` to convert the enum back to a string for matching — so type-safety is only at the annotation call site, not end-to-end.
|
||
|
||
**Fix:** Make the entire stack type-safe: store permissions as the enum's name (which is already happening) but also parse them back into `Permission` enum values when loading `UserDetails` authorities. This way a typo in the DB is caught at load time rather than silently failing permission checks.
|
||
|
||
---
|
||
|
||
### `MassImportService` provides no status or error feedback
|
||
**File:** `service/MassImportService.java`, `controller/AdminController.java`
|
||
|
||
`/api/admin/trigger-import` returns immediately (async), but there is no way for the admin to know whether the import succeeded, failed, or is still running. Errors during async execution are silently swallowed.
|
||
|
||
**Fix options:**
|
||
- Store import job status in a DB table (`import_jobs`) with state (`RUNNING`, `DONE`, `FAILED`) and expose a `GET /api/admin/import-status` endpoint
|
||
- Alternatively, make the endpoint synchronous since it already blocks on file I/O — only use async if you need true non-blocking behaviour
|
||
|
||
---
|
||
|
||
## Missing Capabilities
|
||
|
||
### No test coverage
|
||
**File:** `src/test/java/org/raddatz/familienarchiv/FamilienarchivApplicationTests.java`
|
||
|
||
The only test is a Spring context load test. No unit or integration tests exist for any service, repository, or controller logic.
|
||
|
||
**Suggested starting points (highest value for effort):**
|
||
1. `DocumentSpecifications` — pure logic, easy to unit test with an in-memory H2 or Testcontainers PostgreSQL
|
||
2. `ExcelService` — parsing logic, test with fixture `.xlsx` files (one exists in `api_tests/`)
|
||
3. `PermissionAspect` — security logic should be tested; use `@WithMockUser` from Spring Security Test
|
||
|
||
---
|
||
|
||
### No API documentation
|
||
There is no OpenAPI/Swagger endpoint. The only documentation is the `.http` files in `backend/api_tests/`.
|
||
|
||
**Fix:** Add `springdoc-openapi-starter-webmvc-ui` to `pom.xml`. It generates `/swagger-ui.html` and `/v3/api-docs` automatically from existing controller annotations with zero additional code.
|
||
|
||
---
|
||
|
||
### `FileService` content-type detection is fragile
|
||
File extension is inferred from the S3 key string. Many document types (TIFF scans, HEIC photos, Word documents) are unhandled and fall back to `application/octet-stream`, forcing a download instead of inline display.
|
||
|
||
**Fix:** Use `java.nio.file.Files.probeContentType()` or Apache Tika for robust MIME detection. Alternatively, store the content-type at upload time in the `Document` entity and retrieve it on download.
|
||
|
||
---
|
||
|
||
### Generic error response is hardcoded in German
|
||
**File:** `controller/GlobalExceptionHandler.java:36`
|
||
```java
|
||
.body(new ErrorResponse("Ein Fehler ist aufgetreten"))
|
||
```
|
||
The fallback error message is hardcoded in German. It should use a locale-aware message source or at minimum be in English as the lingua franca of APIs.
|