fix(user): rename yaml key username→email so admin seed reads APP_ADMIN_USERNAME (#513) #516

marcel · 2026-05-11T16:55:17+02:00

marcel commented

2026-05-11 16:55:17 +02:00

Summary

Closes #513.

UserDataInitializer reads @Value("${app.admin.email:...}"), but application.yaml exposed the env var under app.admin.username. The keys never connected — APP_ADMIN_USERNAME was silently ignored and the admin user was always seeded with the hardcoded default admin@familyarchive.local.

For production this is HIGH severity: per DEPLOYMENT.md §3.5 and ADR-011, the admin password is permanently locked on first deploy. With the bug, the lock-in is on the dev defaults instead of the operator's strong password — an operator setting PROD_APP_ADMIN_USERNAME=admin@archiv.raddatz.cloud gets a false sense of security; the admin email is actually admin@familyarchive.local.

(Discovered alongside #514 while debugging the staging login flow — the password key was already correctly bound, only the email key was wrong.)

Fix

Rename the yaml key from username: to email: so the Spring property app.admin.email actually exists. Keep env-var name APP_ADMIN_USERNAME (matches existing Gitea secrets + DEPLOYMENT.md §3.3). Default value updated to an email-shape.

Added AdminSeedPropertyKeyTest (Binder pattern, no Spring context): verifies both app.admin.email and app.admin.password resolve from the yaml. Confirmed red without the fix, green with it.

Test plan after merge

AdminSeedPropertyKeyTest passes
Redeploy production / staging — backend logs show Default Admin erstellt: Email='<APP_ADMIN_USERNAME value>'
Existing staging admin (already seeded with the dev default) needs a separate cleanup decision (wipe staging DB OR live with admin@familyarchive.local on staging only); prod must land this PR before the v1.0.0 tag.

🤖 Generated with Claude Code

## Summary Closes #513. `UserDataInitializer` reads `@Value("${app.admin.email:...}")`, but `application.yaml` exposed the env var under `app.admin.username`. The keys never connected — `APP_ADMIN_USERNAME` was silently ignored and the admin user was always seeded with the hardcoded default `admin@familyarchive.local`. For production this is HIGH severity: per `DEPLOYMENT.md §3.5` and ADR-011, the admin password is **permanently locked on first deploy**. With the bug, the lock-in is on the dev defaults instead of the operator's strong password — an operator setting `PROD_APP_ADMIN_USERNAME=admin@archiv.raddatz.cloud` gets a false sense of security; the admin email is actually `admin@familyarchive.local`. (Discovered alongside #514 while debugging the staging login flow — the password key was already correctly bound, only the email key was wrong.) ## Fix Rename the yaml key from `username:` to `email:` so the Spring property `app.admin.email` actually exists. Keep env-var name `APP_ADMIN_USERNAME` (matches existing Gitea secrets + DEPLOYMENT.md §3.3). Default value updated to an email-shape. Added `AdminSeedPropertyKeyTest` (Binder pattern, no Spring context): verifies both `app.admin.email` and `app.admin.password` resolve from the yaml. Confirmed red without the fix, green with it. ## Test plan after merge - [x] `AdminSeedPropertyKeyTest` passes - [ ] Redeploy production / staging — backend logs show `Default Admin erstellt: Email='<APP_ADMIN_USERNAME value>'` - [ ] Existing staging admin (already seeded with the dev default) needs a separate cleanup decision (wipe staging DB OR live with `admin@familyarchive.local` on staging only); prod must land this PR **before** the v1.0.0 tag. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel commented

2026-05-11 16:56:24 +02:00

Markus Keller — Senior Application Architect

Verdict: ⚠️ Approved with concerns

This is the right fix. The env-var contract (APP_ADMIN_USERNAME) is deliberately preserved so existing Gitea secrets and DEPLOYMENT.md §3.3 keep working, while the internal Spring property gets renamed to match what UserDataInitializer actually reads. Pragmatic, minimal, surgical — exactly what a hot-path config bug needs before a v1.0.0 cut.

The inline comment on the yaml block is good — it documents the intentional name asymmetry (env-var keeps USERNAME, internal key becomes email) and links to #513 so the next reader doesn't "fix" it back.

Blockers

None at the architectural layer.

Concerns

Hidden scope: docker-compose.prod.yml changes are not in the PR description. The diff also adds restart: "no" and create-buckets: condition: service_completed_successfully to backend's depends_on. The comments reference #510, not #513, so this is a separate concern piggybacking on the admin-seed fix. Either split into its own commit/PR or update the PR body and title to declare the scope honestly. Atomic commits is one of our standing rules.
ADR-011 references the wrong key. If ADR-011 documents the first-deploy lockout and uses app.admin.username anywhere, update it now while you remember. Doc drift is a blocker per my review rubric — please confirm ADR-011 (and DEPLOYMENT.md §3.5) still reads correctly after this rename.
The env-var/property naming asymmetry is now permanent. APP_ADMIN_USERNAME → app.admin.email is the cheaper path for v1.0.0, agreed. But add it to the GLOSSARY or DEPLOYMENT.md as a known wart — "the env var historically said USERNAME, the property is email, both refer to the admin login email." Future operators will trip over this otherwise.

Suggestion

The Binder test is a clean architectural pattern — context-less yaml binding to pin contract-as-data. Consider extracting a small YamlBinderTestSupport helper if a second contract test follows; right now one occurrence isn't enough to justify the abstraction, so the duplication-of-three rule says wait.

## Markus Keller — Senior Application Architect **Verdict: ⚠️ Approved with concerns** This is the right fix. The env-var contract (`APP_ADMIN_USERNAME`) is deliberately preserved so existing Gitea secrets and `DEPLOYMENT.md §3.3` keep working, while the internal Spring property gets renamed to match what `UserDataInitializer` actually reads. Pragmatic, minimal, surgical — exactly what a hot-path config bug needs before a v1.0.0 cut. The inline comment on the yaml block is good — it documents the intentional name asymmetry (env-var keeps `USERNAME`, internal key becomes `email`) and links to #513 so the next reader doesn't "fix" it back. ### Blockers None at the architectural layer. ### Concerns 1. **Hidden scope: `docker-compose.prod.yml` changes are not in the PR description.** The diff also adds `restart: "no"` and `create-buckets: condition: service_completed_successfully` to backend's depends_on. The comments reference #510, not #513, so this is a separate concern piggybacking on the admin-seed fix. Either split into its own commit/PR or update the PR body and title to declare the scope honestly. Atomic commits is one of our standing rules. 2. **ADR-011 references the wrong key.** If ADR-011 documents the first-deploy lockout and uses `app.admin.username` anywhere, update it now while you remember. Doc drift is a blocker per my review rubric — please confirm ADR-011 (and DEPLOYMENT.md §3.5) still reads correctly after this rename. 3. **The env-var/property naming asymmetry is now permanent.** `APP_ADMIN_USERNAME → app.admin.email` is the cheaper path for v1.0.0, agreed. But add it to the GLOSSARY or DEPLOYMENT.md as a known wart — "the env var historically said USERNAME, the property is `email`, both refer to the admin login email." Future operators will trip over this otherwise. ### Suggestion The Binder test is a clean architectural pattern — context-less yaml binding to pin contract-as-data. Consider extracting a small `YamlBinderTestSupport` helper if a second contract test follows; right now one occurrence isn't enough to justify the abstraction, so the duplication-of-three rule says wait.

marcel commented

2026-05-11 16:56:35 +02:00

Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Clean, minimal, surgical fix. The test came first, the test was red without the change, the test is green with it — that's the TDD contract honored. The test itself reads well: descriptive names (admin_email_key_binds_from_yaml), AssertJ with .as(...) describing what's being verified, one logical assertion per test, and the orElseThrow failure message is the kind that saves a future debugger 20 minutes.

The inline yaml comment is the right why comment — it documents the intentional asymmetry between env-var name and Spring property name so the next reader doesn't "clean up" the inconsistency.

Blockers

Scope creep — docker-compose.prod.yml is not in the PR description. The diff adds restart: "no" and create-buckets: service_completed_successfully dependency. Those comments reference #510, not #513. Atomic commits rule: one logical change per commit. Either land the compose changes as a separate commit on this branch with its own message, or push them to a separate PR. Right now the commit history won't tell the truth about what this PR did.

Suggestions

Test class is package-private and has no @DisplayName — minor. The other tests in the codebase mostly use the same style, so I'm fine either way.
UserDataInitializer should pin the property name as a constant. Right now @Value("${app.admin.email:...}") is the only place that defines the contract, and it lives as a string literal. Consider extracting public static final String ADMIN_EMAIL_PROPERTY = "app.admin.email"; and referencing it from both the @Value (via SpEL) and the test. That makes the contract grep-able from one place. Not a blocker — @Value SpEL with a constant is awkward syntax — but flag for follow-up.
The test verifies the key exists; it does not verify UserDataInitializer actually reads it. A future refactor that changes UserDataInitializer to read app.admin.adminEmail would make this test pass while the bug returns. A second, narrower test that boots UserDataInitializer with a MockEnvironment setting app.admin.email=foo@bar and asserts the resulting AppUser.email == "foo@bar" would close that gap. Nice-to-have, not a blocker — the current test catches the specific bug from #513.

## Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** Clean, minimal, surgical fix. The test came first, the test was red without the change, the test is green with it — that's the TDD contract honored. The test itself reads well: descriptive names (`admin_email_key_binds_from_yaml`), AssertJ with `.as(...)` describing what's being verified, one logical assertion per test, and the `orElseThrow` failure message is the kind that saves a future debugger 20 minutes. The inline yaml comment is the right *why* comment — it documents the intentional asymmetry between env-var name and Spring property name so the next reader doesn't "clean up" the inconsistency. ### Blockers 1. **Scope creep — `docker-compose.prod.yml` is not in the PR description.** The diff adds `restart: "no"` and `create-buckets: service_completed_successfully` dependency. Those comments reference #510, not #513. Atomic commits rule: one logical change per commit. Either land the compose changes as a separate commit on this branch with its own message, or push them to a separate PR. Right now the commit history won't tell the truth about what this PR did. ### Suggestions 1. **Test class is package-private and has no `@DisplayName`** — minor. The other tests in the codebase mostly use the same style, so I'm fine either way. 2. **`UserDataInitializer` should pin the property name as a constant.** Right now `@Value("${app.admin.email:...}")` is the only place that defines the contract, and it lives as a string literal. Consider extracting `public static final String ADMIN_EMAIL_PROPERTY = "app.admin.email";` and referencing it from both the `@Value` (via SpEL) and the test. That makes the contract grep-able from one place. Not a blocker — `@Value` SpEL with a constant is awkward syntax — but flag for follow-up. 3. **The test verifies the key exists; it does not verify `UserDataInitializer` actually reads it.** A future refactor that changes `UserDataInitializer` to read `app.admin.adminEmail` would make this test pass while the bug returns. A second, narrower test that boots `UserDataInitializer` with a `MockEnvironment` setting `app.admin.email=foo@bar` and asserts the resulting `AppUser.email == "foo@bar"` would close that gap. Nice-to-have, not a blocker — the current test catches the specific bug from #513.

marcel commented

2026-05-11 16:56:47 +02:00

Tobias Wendt — DevOps & Platform Engineer

Verdict: 🚫 Changes requested

The application.yaml fix is correct and the env-var asymmetry decision is the right one — preserving APP_ADMIN_USERNAME means I don't have to touch Gitea secrets or rotate the production env-file. Good call.

But this PR title and body claim "rename a yaml key + add a test." The actual diff modifies docker-compose.prod.yml too. That's where I have to push back.

Blockers

Out-of-scope docker-compose.prod.yml changes. The PR introduces:
- restart: "no" on create-buckets
- create-buckets: condition: service_completed_successfully on the backend's depends_on
The inline comments reference #510, not #513. This is a #510 change riding on a #513 PR. From an ops/audit perspective this is poison: if I need to revert PR #516 because the admin-seed fix had a regression, I also revert two production compose changes that were unrelated. Conversely, if I revert because of a compose-orchestration regression, I roll back the admin-seed fix. The two concerns are coupled in a commit they shouldn't be coupled in.

Fix: split. Land the yaml-key fix as #516, land the compose changes as their own PR (or at minimum their own commit with a #510-scoped message). Atomic commits.
Was the compose change tested? service_completed_successfully on a one-shot is the right pattern (I'd recommend the same), but it needs verification with docker compose -f docker-compose.yml -f docker-compose.prod.yml up -d --wait on a clean volume set. If create-buckets ever fails idempotently on a re-run, backend will now refuse to start. Please confirm a re-deploy on a system where buckets already exist still succeeds — the bucket-create script needs to exit 0 on "already exists."

Notes (positive)

APP_ADMIN_USERNAME preserved → existing prod env-file and Gitea secret stay valid. Zero ops migration cost. Good.
Default value updated from admin to admin@familienarchiv.local — email-shaped default matches what UserDataInitializer expects. Sane.
The yaml comment block linking back to #513 and DEPLOYMENT.md §3.3 is the kind of inline documentation I want — future me will thank present you.

Suggestion (post-merge)

Once this lands, run a deploy verification: docker compose ... up -d --wait, then docker logs archiv-app 2>&1 | grep "Default Admin" — confirm the log line shows the operator's email, not the dev default. That should be in the test plan as an explicit step before tagging v1.0.0.

## Tobias Wendt — DevOps & Platform Engineer **Verdict: 🚫 Changes requested** The application.yaml fix is correct and the env-var asymmetry decision is the right one — preserving `APP_ADMIN_USERNAME` means I don't have to touch Gitea secrets or rotate the production env-file. Good call. But this PR title and body claim "rename a yaml key + add a test." The actual diff modifies `docker-compose.prod.yml` too. That's where I have to push back. ### Blockers 1. **Out-of-scope `docker-compose.prod.yml` changes.** The PR introduces: - `restart: "no"` on `create-buckets` - `create-buckets: condition: service_completed_successfully` on the backend's depends_on The inline comments reference `#510`, **not** `#513`. This is a `#510` change riding on a `#513` PR. From an ops/audit perspective this is poison: if I need to revert PR #516 because the admin-seed fix had a regression, I also revert two production compose changes that were unrelated. Conversely, if I revert because of a compose-orchestration regression, I roll back the admin-seed fix. The two concerns are coupled in a commit they shouldn't be coupled in. **Fix**: split. Land the yaml-key fix as #516, land the compose changes as their own PR (or at minimum their own commit with a `#510`-scoped message). Atomic commits. 2. **Was the compose change tested?** `service_completed_successfully` on a one-shot is the right pattern (I'd recommend the same), but it needs verification with `docker compose -f docker-compose.yml -f docker-compose.prod.yml up -d --wait` on a clean volume set. If `create-buckets` ever fails idempotently on a re-run, backend will now refuse to start. Please confirm a re-deploy on a system where buckets already exist still succeeds — the bucket-create script needs to exit 0 on "already exists." ### Notes (positive) - `APP_ADMIN_USERNAME` preserved → existing prod env-file and Gitea secret stay valid. Zero ops migration cost. Good. - Default value updated from `admin` to `admin@familienarchiv.local` — email-shaped default matches what `UserDataInitializer` expects. Sane. - The yaml comment block linking back to `#513` and `DEPLOYMENT.md §3.3` is the kind of inline documentation I want — future me will thank present you. ### Suggestion (post-merge) Once this lands, run a deploy verification: `docker compose ... up -d --wait`, then `docker logs archiv-app 2>&1 | grep "Default Admin"` — confirm the log line shows the operator's email, not the dev default. That should be in the test plan as an explicit step before tagging v1.0.0.

marcel commented

2026-05-11 16:57:01 +02:00

Elicit — Requirements Engineer & Business Analyst

Verdict: ⚠️ Approved with concerns

The PR description is unusually strong: it states the why (silent env-var ignore → dev-default admin in production), the measurable harm (first-deploy password lock-in per ADR-011 + DEPLOYMENT.md §3.5), the scope boundary (env-var name unchanged, only property name moves), and a test plan with concrete post-deploy verification steps. This is how a bugfix issue should look.

Concerns

The "Test plan after merge" has an unresolved branch and no owner. The third checkbox says:

Existing staging admin (already seeded with the dev default) needs a separate cleanup decision (wipe staging DB OR live with admin@familyarchive.local on staging only)

This is an open question that blocks the v1.0.0 tag's success criteria. Convert it to a Gitea issue (or sub-issue of #513) with: option A description, option B description, decision owner, decision deadline (must be before tag), and the trigger condition for each option. As written, this risks falling between the cracks during the deploy.
Acceptance criterion missing in Given-When-Then form. The bug is described prose-style. The implicit AC is:

Given an operator sets APP_ADMIN_USERNAME=admin@archiv.raddatz.cloud in the prod env-file,
When the backend boots for the first time on a fresh database,
Then the seeded admin user's email is admin@archiv.raddatz.cloud, not the application.yaml default.

The unit test covers half of this (the property binds) but not the integration half (the seeded AppUser actually carries that email). Recommend adding that AC explicitly to issue #513 and tracking whether the post-deploy log-check (Default Admin erstellt: Email='...') satisfies it.
PR scope/description mismatch. The diff touches docker-compose.prod.yml (restart policy + dependency on create-buckets). The PR body does not mention this. Either update the PR description to include the compose changes (and reference #510 alongside #513), or split. As a requirements artifact, the PR's "Summary" should match the diff.

Strengths to preserve

HIGH severity rationale is concrete and measurable (locked-in password, ADR + DEPLOYMENT.md citations, a real env-var example).
Discovery context (found while debugging #514) is noted — useful for the post-mortem / lessons-learned register.
Test plan distinguishes "passes" (✓) from "to verify" (☐) — keeps the unresolved work visible.

Suggestion

When you cut v1.0.0, add a one-line entry to a "production readiness checklist" issue (or DEPLOYMENT.md §3.5): "Verified that APP_ADMIN_USERNAME is read into the seeded admin's email field via post-deploy log inspection." Closes the loop between the fix and the production-deploy gate.

## Elicit — Requirements Engineer & Business Analyst **Verdict: ⚠️ Approved with concerns** The PR description is unusually strong: it states the *why* (silent env-var ignore → dev-default admin in production), the *measurable harm* (first-deploy password lock-in per ADR-011 + DEPLOYMENT.md §3.5), the *scope boundary* (env-var name unchanged, only property name moves), and a test plan with concrete post-deploy verification steps. This is how a bugfix issue should look. ### Concerns 1. **The "Test plan after merge" has an unresolved branch and no owner.** The third checkbox says: > Existing staging admin (already seeded with the dev default) needs a separate cleanup decision (wipe staging DB OR live with `admin@familyarchive.local` on staging only) This is an open question that *blocks the v1.0.0 tag's success criteria*. Convert it to a Gitea issue (or sub-issue of #513) with: option A description, option B description, decision owner, decision deadline (must be before tag), and the trigger condition for each option. As written, this risks falling between the cracks during the deploy. 2. **Acceptance criterion missing in Given-When-Then form.** The bug is described prose-style. The implicit AC is: > Given an operator sets `APP_ADMIN_USERNAME=admin@archiv.raddatz.cloud` in the prod env-file, > When the backend boots for the first time on a fresh database, > Then the seeded admin user's email is `admin@archiv.raddatz.cloud`, not the application.yaml default. The unit test covers half of this (the property binds) but not the integration half (the seeded `AppUser` actually carries that email). Recommend adding that AC explicitly to issue #513 and tracking whether the post-deploy log-check (`Default Admin erstellt: Email='...'`) satisfies it. 3. **PR scope/description mismatch.** The diff touches `docker-compose.prod.yml` (restart policy + dependency on `create-buckets`). The PR body does not mention this. Either update the PR description to include the compose changes (and reference #510 alongside #513), or split. As a requirements artifact, the PR's "Summary" should match the diff. ### Strengths to preserve - HIGH severity rationale is concrete and measurable (locked-in password, ADR + DEPLOYMENT.md citations, a real env-var example). - Discovery context (found while debugging #514) is noted — useful for the post-mortem / lessons-learned register. - Test plan distinguishes "passes" (✓) from "to verify" (☐) — keeps the unresolved work visible. ### Suggestion When you cut v1.0.0, add a one-line entry to a "production readiness checklist" issue (or DEPLOYMENT.md §3.5): "Verified that `APP_ADMIN_USERNAME` is read into the seeded admin's email field via post-deploy log inspection." Closes the loop between the fix and the production-deploy gate.

marcel commented

2026-05-11 16:57:17 +02:00

Nora "NullX" Steiner — Application Security Engineer

Verdict: ⚠️ Approved with concerns

This is a security-relevant bugfix masquerading as a config rename. Per ADR-011 + DEPLOYMENT.md §3.5, the first-deploy admin password is permanently locked. Before this fix, an operator following the deployment runbook to set a strong admin email/password would end up with admin@familyarchive.local / admin123 seeded into production — and that seed is irrevocable on a real first deploy without a DB wipe. CWE-1188 (Insecure Default Initialization of Resource) / CWE-1392 (Use of Default Credentials), severity HIGH, exposure window = until v1.0.0 ships.

The fix itself is correct. The test pins the contract.

Blockers

The hardcoded dev defaults (admin@familyarchive.local / admin123) are still in application.yaml and still fall back silently if APP_ADMIN_USERNAME / APP_ADMIN_PASSWORD are unset. This PR closes the naming hole but not the fail-open default hole. For a production profile, the application should refuse to start when these env vars are unset — fail closed, not fall back to admin123. Today, if an operator forgets to add the env-var to the prod env-file, you still get the dev default in production. That's the same outcome #513 was trying to prevent, just via a different failure mode.

Concrete fix (post-merge or in a follow-up issue is acceptable, but it must exist before v1.0.0):
```
@Value("${app.admin.email}")  // no default — Spring fails fast if missing
private String adminEmail;
```
Or scope it to the prod profile: keep dev defaults in application-dev.yaml, strip them from the base application.yaml, and let application-prod.yaml require explicit values.

This is the standard "no hardcoded fallback credentials" rule. If you want, I'll write the failing test myself.

Concerns

Test coverage gap. AdminSeedPropertyKeyTest verifies the yaml exposes the key. It does not verify that UserDataInitializer actually uses that key — i.e., a future refactor that renames the @Value placeholder back to app.admin.username would make this test still pass while reintroducing the bug. A second integration test that boots UserDataInitializer with app.admin.email=foo@bar.com and asserts the seeded AppUser.email is foo@bar.com would close the loop. Felix can write this in 15 minutes with MockEnvironment + a thin context.
No log redaction check. If the seed log line literally prints Default Admin erstellt: Email='admin@archiv.raddatz.cloud', that's fine — emails are not secrets. But please confirm no codepath logs the password alongside it. Quick grep on UserDataInitializer for any log.*password patterns.

Notes

Inline yaml comment is exactly the kind of threat-model documentation I want to see. "Key must be email, not username — UserDataInitializer reads ${app.admin.email:...}. See #513." Future auditor doesn't have to git-blame.
Out-of-scope docker-compose.prod.yml changes (restart policy, create-buckets dependency) are not security-impacting from what I can see, but as a hygiene matter I support DevOps's split request — coupled commits make security reverts harder.

Suggestion

When you tag v1.0.0, open an issue: "Production admin seed must fail-closed on missing env-vars (CWE-1188 follow-up to #513)." Track it as P1, target patch release.

## Nora "NullX" Steiner — Application Security Engineer **Verdict: ⚠️ Approved with concerns** This is a security-relevant bugfix masquerading as a config rename. Per ADR-011 + DEPLOYMENT.md §3.5, the first-deploy admin password is permanently locked. Before this fix, an operator following the deployment runbook to set a strong admin email/password would end up with `admin@familyarchive.local` / `admin123` seeded into production — and that seed is irrevocable on a real first deploy without a DB wipe. CWE-1188 (Insecure Default Initialization of Resource) / CWE-1392 (Use of Default Credentials), severity HIGH, exposure window = until v1.0.0 ships. The fix itself is correct. The test pins the contract. ### Blockers 1. **The hardcoded dev defaults (`admin@familyarchive.local` / `admin123`) are still in `application.yaml` and still fall back silently if `APP_ADMIN_USERNAME` / `APP_ADMIN_PASSWORD` are unset.** This PR closes the *naming* hole but not the *fail-open default* hole. For a production profile, the application should refuse to start when these env vars are unset — fail closed, not fall back to `admin123`. Today, if an operator forgets to add the env-var to the prod env-file, you still get the dev default in production. That's the same outcome #513 was trying to prevent, just via a different failure mode. **Concrete fix** (post-merge or in a follow-up issue is acceptable, but it must exist before v1.0.0): ```java @Value("${app.admin.email}") // no default — Spring fails fast if missing private String adminEmail; ``` Or scope it to the prod profile: keep dev defaults in `application-dev.yaml`, strip them from the base `application.yaml`, and let `application-prod.yaml` require explicit values. This is the standard "no hardcoded fallback credentials" rule. If you want, I'll write the failing test myself. ### Concerns 2. **Test coverage gap.** `AdminSeedPropertyKeyTest` verifies the yaml exposes the key. It does *not* verify that `UserDataInitializer` actually uses that key — i.e., a future refactor that renames the `@Value` placeholder back to `app.admin.username` would make this test still pass while reintroducing the bug. A second integration test that boots `UserDataInitializer` with `app.admin.email=foo@bar.com` and asserts the seeded `AppUser.email` is `foo@bar.com` would close the loop. Felix can write this in 15 minutes with `MockEnvironment` + a thin context. 3. **No log redaction check.** If the seed log line literally prints `Default Admin erstellt: Email='admin@archiv.raddatz.cloud'`, that's fine — emails are not secrets. But please confirm no codepath logs the *password* alongside it. Quick grep on `UserDataInitializer` for any `log.*password` patterns. ### Notes - Inline yaml comment is exactly the kind of threat-model documentation I want to see. "Key must be `email`, not `username` — UserDataInitializer reads `${app.admin.email:...}`. See #513." Future auditor doesn't have to git-blame. - Out-of-scope `docker-compose.prod.yml` changes (restart policy, create-buckets dependency) are not security-impacting from what I can see, but as a hygiene matter I support DevOps's split request — coupled commits make security reverts harder. ### Suggestion When you tag v1.0.0, open an issue: "Production admin seed must fail-closed on missing env-vars (CWE-1188 follow-up to #513)." Track it as P1, target patch release.

marcel commented

2026-05-11 16:57:32 +02:00

Sara Holt — Senior QA Engineer

Verdict: ⚠️ Approved with concerns

This is exactly the kind of bug a contract test should catch — config-vs-code drift that compiles, boots, and silently produces wrong behavior in production. Adding AdminSeedPropertyKeyTest is the right move. Discipline: red without fix, green with fix, confirmed in the PR description. ✓

What's good

Descriptive test names that read as sentences: admin_email_key_binds_from_yaml, admin_password_key_binds_from_yaml. No testYaml1() nonsense.
One logical assertion per test. Email and password are separate tests with separate orElseThrow failure modes — when one breaks in CI, you know exactly which key drifted.
Failure messages that aid debugging. The orElseThrow message explains why the test exists and what a future "fix" should not do. That's the kind of test message that saves a CI investigation.
No Spring context. Binder-pattern Java config test — runs in ~50ms, no Testcontainers, no @SpringBootTest. Cheap and fast. Models the ForwardHeadersConfigurationTest precedent, which is good consistency.
AssertJ with .as(...) describing the business meaning of the assertion. Per my own readability rules, exemplary.

Concerns

Contract is asserted at the yaml layer only; not at the consumer. The test verifies app.admin.email resolves from the yaml. It does not verify that UserDataInitializer actually reads app.admin.email. A future refactor that renames the @Value placeholder would make this test pass while reintroducing the production bug. Recommend a complementary test (integration, slow, separate file): boot UserDataInitializer with app.admin.email=foo@bar, assert seeded AppUser.email == "foo@bar". That closes the round-trip contract.
@Disabled / @Tag markers are missing. Not blocker. But if you want this to run on every PR (it should — it's a 50ms safeguard against a HIGH-severity prod bug), confirm it lives in the default Maven test phase and not behind a profile.
PR scope leak. docker-compose.prod.yml changes are not covered by any test in this PR. The depends_on + service_completed_successfully change has its own failure mode (backend hangs forever if create-buckets errors on a re-deploy). That should have its own test or a documented smoke-test step. Splitting the PR (per DevOps's request) would let me push the compose changes through their own validation path.

Suggestions

Consider parameterizing the two tests as a single @ParameterizedTest over Stream.of("app.admin.email", "app.admin.password"). Slightly more compact, same coverage, single failure mode per key. Not blocking — two separate methods read fine and are easier to find in logs.
Add a related coverage check: a @SpringBootTest-level smoke that asserts AppUser table is populated with the expected email from the test config after Flyway+seed runs. Per pyramid rule that's an integration test, lives separately, runs slower — but it's what would actually have caught #513 in CI before staging deploy.

Quality gate

Passes the "tests precede the fix" rule. Passes the "test name explains failure" rule. Passes the "no Spring context unless needed" rule. The only gate I'd add is the round-trip test in concern #1 — without it, this test is necessary but not sufficient.

## Sara Holt — Senior QA Engineer **Verdict: ⚠️ Approved with concerns** This is exactly the kind of bug a contract test should catch — config-vs-code drift that compiles, boots, and silently produces wrong behavior in production. Adding `AdminSeedPropertyKeyTest` is the right move. Discipline: red without fix, green with fix, confirmed in the PR description. ✓ ### What's good - **Descriptive test names** that read as sentences: `admin_email_key_binds_from_yaml`, `admin_password_key_binds_from_yaml`. No `testYaml1()` nonsense. - **One logical assertion per test.** Email and password are separate tests with separate `orElseThrow` failure modes — when one breaks in CI, you know exactly which key drifted. - **Failure messages that aid debugging.** The `orElseThrow` message explains *why* the test exists and *what* a future "fix" should not do. That's the kind of test message that saves a CI investigation. - **No Spring context.** Binder-pattern Java config test — runs in ~50ms, no Testcontainers, no `@SpringBootTest`. Cheap and fast. Models the `ForwardHeadersConfigurationTest` precedent, which is good consistency. - **AssertJ with `.as(...)`** describing the business meaning of the assertion. Per my own readability rules, exemplary. ### Concerns 1. **Contract is asserted at the yaml layer only; not at the consumer.** The test verifies `app.admin.email` resolves *from the yaml*. It does not verify that `UserDataInitializer` actually reads `app.admin.email`. A future refactor that renames the `@Value` placeholder would make this test pass while reintroducing the production bug. Recommend a complementary test (integration, slow, separate file): boot `UserDataInitializer` with `app.admin.email=foo@bar`, assert seeded `AppUser.email == "foo@bar"`. That closes the round-trip contract. 2. **`@Disabled` / `@Tag` markers are missing.** Not blocker. But if you want this to run on every PR (it should — it's a 50ms safeguard against a HIGH-severity prod bug), confirm it lives in the default Maven test phase and not behind a profile. 3. **PR scope leak.** `docker-compose.prod.yml` changes are not covered by any test in this PR. The depends_on + `service_completed_successfully` change has its own failure mode (backend hangs forever if `create-buckets` errors on a re-deploy). That should have its own test or a documented smoke-test step. Splitting the PR (per DevOps's request) would let me push the compose changes through their own validation path. ### Suggestions 1. Consider parameterizing the two tests as a single `@ParameterizedTest` over `Stream.of("app.admin.email", "app.admin.password")`. Slightly more compact, same coverage, single failure mode per key. Not blocking — two separate methods read fine and are easier to find in logs. 2. Add a related coverage check: a `@SpringBootTest`-level smoke that asserts `AppUser` table is populated with the *expected* email from the test config after Flyway+seed runs. Per pyramid rule that's an integration test, lives separately, runs slower — but it's what would actually have caught #513 in CI before staging deploy. ### Quality gate Passes the "tests precede the fix" rule. Passes the "test name explains failure" rule. Passes the "no Spring context unless needed" rule. The only gate I'd add is the round-trip test in concern #1 — without it, this test is necessary but not sufficient.

marcel commented

2026-05-11 16:57:38 +02:00

Leonie Voss — UI/UX Design Lead

Verdict: ✅ Approved

No UI surface touched. This is a backend configuration fix (application.yaml), a backend unit test (AdminSeedPropertyKeyTest.java), and a docker-compose orchestration tweak (docker-compose.prod.yml). Zero frontend, zero Svelte, zero design tokens, zero copy, zero a11y impact.

What I checked

No changes under frontend/
No new error codes that would need a getErrorMessage() i18n mapping in frontend/src/lib/shared/errors.ts or new keys in messages/{de,en,es}.json
No login-screen, admin-onboarding, or password-prompt UX implications visible in the diff
The admin seed email default flip (admin → admin@familienarchiv.local) is internal — not a user-facing string and not rendered anywhere on the frontend that I can see from this diff

One downstream note for whoever cuts v1.0.0

If the operator's chosen admin email is going to be displayed anywhere in the UI on first login (welcome screen, profile dropdown, audit log), now's the moment to confirm that surface is i18n-safe and handles a long email gracefully at 320px. Not this PR's scope — just flagging since #513 changes the value that ends up in AppUser.email.

LGTM from a design/UX perspective.

## Leonie Voss — UI/UX Design Lead **Verdict: ✅ Approved** No UI surface touched. This is a backend configuration fix (`application.yaml`), a backend unit test (`AdminSeedPropertyKeyTest.java`), and a docker-compose orchestration tweak (`docker-compose.prod.yml`). Zero frontend, zero Svelte, zero design tokens, zero copy, zero a11y impact. ### What I checked - No changes under `frontend/` - No new error codes that would need a `getErrorMessage()` i18n mapping in `frontend/src/lib/shared/errors.ts` or new keys in `messages/{de,en,es}.json` - No login-screen, admin-onboarding, or password-prompt UX implications visible in the diff - The admin seed email default flip (`admin` → `admin@familienarchiv.local`) is internal — not a user-facing string and not rendered anywhere on the frontend that I can see from this diff ### One downstream note for whoever cuts v1.0.0 If the operator's chosen admin email is going to be displayed anywhere in the UI on first login (welcome screen, profile dropdown, audit log), now's the moment to confirm that surface is i18n-safe and handles a long email gracefully at 320px. Not this PR's scope — just flagging since #513 changes the value that ends up in `AppUser.email`. LGTM from a design/UX perspective.

marcel added 2 commits 2026-05-11 17:10:16 +02:00

fix(user): rename yaml key username→email so admin seed reads APP_ADMIN_USERNAME 228f39c0d8

Closes #513.

UserDataInitializer reads `@Value("${app.admin.email:...}")` but
application.yaml mapped APP_ADMIN_USERNAME to `app.admin.username`.
The keys never connected — env vars APP_ADMIN_USERNAME and
APP_ADMIN_PASSWORD were silently ignored and the admin user got
seeded with the hardcoded defaults admin@familyarchive.local /
admin123.

For production this is HIGH severity: DEPLOYMENT.md §3.5 documents
the admin password as permanently locked on first deploy. The
bug locked the lock-in to dev defaults, not to whatever an operator
set in PROD_APP_ADMIN_PASSWORD.

Rename yaml key from `username:` to `email:` so the Spring property
`app.admin.email` actually exists. Keep env-var name
APP_ADMIN_USERNAME (matches the already-set Gitea secrets and
DEPLOYMENT.md §3.3). Default value updated to an email-shape.

Added AdminSeedPropertyKeyTest (Binder pattern, no Spring context):
verifies both `app.admin.email` and `app.admin.password` resolve
from the yaml. Confirmed red without the fix, green with it.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

fix(user): fail-closed when admin seed would use dev defaults outside dev/test/e2e

CI / Unit & Component Tests (pull_request) Has been cancelled

Details

CI / OCR Service Tests (pull_request) Has been cancelled

Details

CI / Backend Unit Tests (pull_request) Has been cancelled

Details

CI / fail2ban Regex (pull_request) Has been cancelled

Details

CI / Compose Bucket Idempotency (pull_request) Has been cancelled

Details

CI / Unit & Component Tests (push) Has been cancelled

Details

CI / OCR Service Tests (push) Has been cancelled

Details

CI / Backend Unit Tests (push) Has been cancelled

Details

CI / fail2ban Regex (push) Has been cancelled

Details

CI / Compose Bucket Idempotency (push) Has been cancelled

Details

e974a520c3

Addresses Nora's review concern on #513/#516.

The previous fix only made env-vars take effect — it did NOT close the
fail-open default path. If an operator forgets APP_ADMIN_USERNAME /
APP_ADMIN_PASSWORD on first prod boot, the seeded admin is the
well-known `admin@familienarchiv.local` / `admin123` and is permanently
locked (UserDataInitializer only seeds when the row is missing).

Refuse to seed outside dev/test/e2e profiles when either credential
matches the documented default. The startup fails fast with a clear
message pointing at the env-var names and the permanence trap.

Also adds Markus/Felix/Sara's "pin the Java side" coverage: a
reflection test on the @Value placeholder catches a future rename
of `${app.admin.email:...}` back to `${app.admin.username:...}`,
which would otherwise pass the yaml-side test but silently break
the binding.

Tests:
- AdminSeedFailClosedTest pins fail-closed for non-local profiles
  and verifies the dev/test/e2e bypass.
- AdminSeedPropertyKeyTest now also asserts the @Value placeholder
  string on UserDataInitializer.adminEmail/adminPassword.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

marcel force-pushed fix/issue-513-admin-seed-property-key from 9ebcf59af9 to e974a520c3

2026-05-11 17:10:16 +02:00

Compare

marcel merged commit ea0b3050e4 into main

2026-05-11 17:12:37 +02:00

marcel deleted branch fix/issue-513-admin-seed-property-key

2026-05-11 17:12:37 +02:00

marcel referenced this issue from a commit

2026-05-11 17:42:10 +02:00

fix(security): promote auth_token cookie to Authorization header for browser /api/* calls

marcel referenced this pull request

2026-05-11 17:42:25 +02:00