fix(user): findOrCreate Administrators group instead of blind-INSERT (#518) #519

Merged

marcel merged 1 commits from fix/issue-518-userdatainitializer-findorcreate-group into main 2026-05-11 18:19:50 +02:00

Conversation 7 Commits 1 Files Changed 2 +63 -7

marcel commented 2026-05-11 17:30:17 +02:00

Owner

Copy Link

Summary

Closes #518. Blind INSERT of the Administrators UserGroup hit user_groups_name_key on the second boot of the staging stack today after we DELETEd just the orphan admin user to retry the seed with the corrected APP_ADMIN_USERNAME.

Fix

Match the existing findByName(...).orElseGet(...) pattern that initE2EData already uses for the "Leser" group:

UserGroup adminGroup = groupRepository.findByName("Administrators")
        .orElseGet(() -> groupRepository.save(UserGroup.builder()
                .name("Administrators")
                .permissions(Set.of("ADMIN", "READ_ALL", "WRITE_ALL", ...))
                .build()));

Tests

Two new cases in AdminSeedFailClosedTest:

• reuses_existing_Administrators_group_when_seeding_a_new_admin — group pre-exists, new admin should attach to it; groupRepository.save MUST NOT be called.
• creates_Administrators_group_when_seeding_admin_on_a_fresh_database — group absent, both inserts happen.

Plus the existing tests updated to stub groupRepository.save since the happy path now flows through it.

All 7 tests in AdminSeedFailClosedTest + 4 in AdminSeedPropertyKeyTest pass.

🤖 Generated with Claude Code

## Summary Closes #518. Blind INSERT of the `Administrators` UserGroup hit `user_groups_name_key` on the second boot of the staging stack today after we DELETEd just the orphan admin user to retry the seed with the corrected `APP_ADMIN_USERNAME`. ## Fix Match the existing `findByName(...).orElseGet(...)` pattern that `initE2EData` already uses for the "Leser" group: ```java UserGroup adminGroup = groupRepository.findByName("Administrators") .orElseGet(() -> groupRepository.save(UserGroup.builder() .name("Administrators") .permissions(Set.of("ADMIN", "READ_ALL", "WRITE_ALL", ...)) .build())); ``` ## Tests Two new cases in `AdminSeedFailClosedTest`: - `reuses_existing_Administrators_group_when_seeding_a_new_admin` — group pre-exists, new admin should attach to it; `groupRepository.save` MUST NOT be called. - `creates_Administrators_group_when_seeding_admin_on_a_fresh_database` — group absent, both inserts happen. Plus the existing tests updated to stub `groupRepository.save` since the happy path now flows through it. All 7 tests in `AdminSeedFailClosedTest` + 4 in `AdminSeedPropertyKeyTest` pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 1 commit 2026-05-11 17:30:18 +02:00

fix(user): findOrCreate Administrators group instead of blind-INSERT (#518) ... ad3b571bba

Closes #518.

UserDataInitializer.initAdminUser was doing groupRepository.save(adminGroup)
unconditionally. If a previous boot had seeded the group but failed
before creating the admin user (or if the operator deleted just the
admin row to retry with a corrected APP_ADMIN_USERNAME), the next
seed attempt violated user_groups_name_key and aborted the context.

Switch to the same findByName(...).orElseGet(...) pattern initE2EData
already uses for the "Leser" group.

Tests in AdminSeedFailClosedTest:
- reuses_existing_Administrators_group_when_seeding_a_new_admin
- creates_Administrators_group_when_seeding_admin_on_a_fresh_database
Plus updated existing tests to stub groupRepository.save now that the
seed path also exercises it.

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

marcel commented 2026-05-11 17:43:46 +02:00

Author

Owner

Copy Link

Markus Keller — Senior Application Architect

Verdict: Approved with concerns

What this fix actually does

You replaced an application-level "uniqueness assumption" with findOrCreate. That's fine and matches the initE2EData precedent for Leser. The PR is small, focused, and reverts a prod-blocker. Ship it.

But let me name what this incident really exposed, because the patch treats the symptom:

Architectural concerns

1. The real issue is that seeding is not idempotent. A boot-time seeder that creates two rows in two repository calls, with no transaction boundary spanning them, will leave torn state when anything in between throws — env-var validation, password encoding, even a LazyInitializationException on a future refactor. You just patched the one row pair we hit on staging. The next half-failure (e.g. a future "create Reader group + reader user" in initAdminUser, or a WRITE_ALL permission audit) reproduces the exact same class of bug.

   Concrete fix: wrap the seed body in @Transactional (on a dedicated seeder service method, not on the @Configuration class) so either both rows land or neither does. Then findOrCreate becomes a belt-and-suspenders defense, not the only defense.

2. Push uniqueness enforcement to the database — which you already do, then catch the violation. user_groups_name_key is doing its job: it blocked a duplicate insert. The application response to that constraint should not be "crash the context" — it should be "the constraint told us the row exists, look it up." Either findOrCreate (this PR) or try { save } catch (DataIntegrityViolationException) { findByName.orElseThrow }. Both work. findOrCreate has a TOCTOU race window if two contexts boot simultaneously, but for a CommandLineRunner on a single backend that's not realistic.

3. initE2EData has the same pattern duplicated three times (admin block, "Leser" for reader, "Leser" for reset — all calling findByName("Leser").orElseGet(save(...))). Extract a private UserGroup findOrCreateGroup(String name, Set<String> permissions) helper. Not a blocker for this PR, but file a follow-up.

Documentation

No diagrams or docs/adr/ entry needed — this is bugfix scope. The comment block in initAdminUser (lines 70–74) is good: it explains the why (a previous boot, an operator retry, the unique constraint), which is exactly what future readers need. That's the right kind of comment.

Suggestions, not blockers

• Add @Transactional around the seed block (separate PR, but file it before the next seed-related bug)
• Extract findOrCreateGroup helper in a follow-up cleanup PR
• Consider whether the seeder belongs in a @Service under user/ rather than a @Configuration — it's data initialization, not configuration

The fix is correct for what it claims to fix. My concerns are about the larger pattern, not this patch.

## Markus Keller — Senior Application Architect **Verdict: Approved with concerns** ### What this fix actually does You replaced an application-level "uniqueness assumption" with `findOrCreate`. That's fine and matches the `initE2EData` precedent for `Leser`. The PR is small, focused, and reverts a prod-blocker. Ship it. But let me name what this incident really exposed, because the patch treats the symptom: ### Architectural concerns 1. **The real issue is that seeding is not idempotent.** A boot-time seeder that creates two rows in two repository calls, with no transaction boundary spanning them, **will** leave torn state when anything in between throws — env-var validation, password encoding, even a `LazyInitializationException` on a future refactor. You just patched the one row pair we hit on staging. The next half-failure (e.g. a future "create Reader group + reader user" in `initAdminUser`, or a `WRITE_ALL` permission audit) reproduces the exact same class of bug. Concrete fix: wrap the seed body in `@Transactional` (on a dedicated seeder service method, not on the `@Configuration` class) so either both rows land or neither does. Then `findOrCreate` becomes a belt-and-suspenders defense, not the only defense. 2. **Push uniqueness enforcement to the database — which you already do, then catch the violation.** `user_groups_name_key` is doing its job: it blocked a duplicate insert. The application response to that constraint should not be "crash the context" — it should be "the constraint told us the row exists, look it up." Either `findOrCreate` (this PR) or `try { save } catch (DataIntegrityViolationException) { findByName.orElseThrow }`. Both work. `findOrCreate` has a TOCTOU race window if two contexts boot simultaneously, but for a `CommandLineRunner` on a single backend that's not realistic. 3. **`initE2EData` has the same pattern duplicated three times** (admin block, "Leser" for reader, "Leser" for reset — all calling `findByName("Leser").orElseGet(save(...))`). Extract a `private UserGroup findOrCreateGroup(String name, Set<String> permissions)` helper. Not a blocker for this PR, but file a follow-up. ### Documentation No diagrams or `docs/adr/` entry needed — this is bugfix scope. The comment block in `initAdminUser` (lines 70–74) is good: it explains the **why** (a previous boot, an operator retry, the unique constraint), which is exactly what future readers need. That's the right kind of comment. ### Suggestions, not blockers - Add `@Transactional` around the seed block (separate PR, but file it before the next seed-related bug) - Extract `findOrCreateGroup` helper in a follow-up cleanup PR - Consider whether the seeder belongs in a `@Service` under `user/` rather than a `@Configuration` — it's data initialization, not configuration The fix is correct for what it claims to fix. My concerns are about the larger pattern, not this patch.

marcel commented 2026-05-11 17:43:57 +02:00

Author

Owner

Copy Link

Felix Brandt — Senior Fullstack Developer

Verdict: Approved

TDD evidence — good

Two new tests, both named as sentences describing behavior:

• reuses_existing_Administrators_group_when_seeding_a_new_admin
• creates_Administrators_group_when_seeding_admin_on_a_fresh_database

Both follow Arrange-Act-Assert. The "reuse" test uses verify(groupRepository, never()).save(...) — that's the load-bearing assertion, and it's precise. The "fresh DB" test verifies both inserts happen exactly once. Together they pin both branches of the orElseGet.

You also updated the two existing happy-path tests to stub groupRepository.findByName + save — necessary because the production path now flows through both. Good discipline; nothing left passing by coincidence.

Code quality

The fix itself is six lines of meaningful diff. The Lombok builder reads cleanly inside the orElseGet lambda:

UserGroup adminGroup = groupRepository.findByName("Administrators")
    .orElseGet(() -> groupRepository.save(UserGroup.builder()
            .name("Administrators")
            .permissions(Set.of("ADMIN", "READ_ALL", ...))
            .build()));

This is the same shape initE2EData already uses for Leser. Consistency across both seed paths is the right move.

The comment above the block explains the why (previous half-seed, operator retry, unique constraint), not the what. That's the rule. Good.

Minor suggestions (not blockers)

1. The permissions string list is duplicated nowhere else, but it's a magic literal. If you ever introduce a WRITE_ALL rename or a new admin permission, this will be the spot you forget. Consider a private static final Set<String> ADMIN_PERMISSIONS = Set.of(...) constant — costs nothing, makes the intent explicit.

2. initE2EData has the same findByName("Leser").orElseGet(save(...)) block twice (for the reader and reset users). After this PR lands, extract a findOrCreateGroup(String name, Set<String> permissions) helper and have all three call sites use it. KISS says extract on the third duplication — we're there. File a small follow-up.

Neither blocks merge. The patch does exactly what the failing test demanded, and nothing more. Red → green achieved cleanly.

## Felix Brandt — Senior Fullstack Developer **Verdict: Approved** ### TDD evidence — good Two new tests, both named as sentences describing behavior: - `reuses_existing_Administrators_group_when_seeding_a_new_admin` - `creates_Administrators_group_when_seeding_admin_on_a_fresh_database` Both follow Arrange-Act-Assert. The "reuse" test uses `verify(groupRepository, never()).save(...)` — that's the load-bearing assertion, and it's precise. The "fresh DB" test verifies both inserts happen exactly once. Together they pin both branches of the `orElseGet`. You also updated the two existing happy-path tests to stub `groupRepository.findByName` + `save` — necessary because the production path now flows through both. Good discipline; nothing left passing by coincidence. ### Code quality The fix itself is six lines of meaningful diff. The Lombok builder reads cleanly inside the `orElseGet` lambda: ```java UserGroup adminGroup = groupRepository.findByName("Administrators") .orElseGet(() -> groupRepository.save(UserGroup.builder() .name("Administrators") .permissions(Set.of("ADMIN", "READ_ALL", ...)) .build())); ``` This is the same shape `initE2EData` already uses for `Leser`. Consistency across both seed paths is the right move. The comment above the block explains the **why** (previous half-seed, operator retry, unique constraint), not the **what**. That's the rule. Good. ### Minor suggestions (not blockers) 1. The permissions string list is duplicated nowhere else, but it's a magic literal. If you ever introduce a `WRITE_ALL` rename or a new admin permission, this will be the spot you forget. Consider a `private static final Set<String> ADMIN_PERMISSIONS = Set.of(...)` constant — costs nothing, makes the intent explicit. 2. `initE2EData` has the same `findByName("Leser").orElseGet(save(...))` block twice (for the reader and reset users). After this PR lands, extract a `findOrCreateGroup(String name, Set<String> permissions)` helper and have all three call sites use it. KISS says extract on the third duplication — we're there. File a small follow-up. Neither blocks merge. The patch does exactly what the failing test demanded, and nothing more. Red → green achieved cleanly.

marcel commented 2026-05-11 17:44:09 +02:00

Author

Owner

Copy Link

Tobias Wendt — DevOps & Platform Engineer

Verdict: Approved

This is a deployment reliability bug, full stop. Worth saying out loud what almost happened on staging today:

1. First boot of the new stack: admin seed fired with the wrong APP_ADMIN_USERNAME, created the Administrators group, then created an admin user with the wrong email.
2. Operator fixed APP_ADMIN_USERNAME and DELETEd the orphan user row to retry.
3. Second boot: userRepository.findByEmail(correct@email) returned empty → seed path re-entered → groupRepository.save(adminGroup) → DataIntegrityViolationException on user_groups_name_key → Spring context aborts → backend will not start.

The Compose stack health check would have correctly reported the backend as unhealthy. We would have had no admin user, no UI, and a stuck deployment. That's a P0 incident at 10pm on a Friday for any operator who isn't me. Fixing this is the right call.

Operational observations

1. The fix matches initE2EData. That's good — same idiom in both seed paths means an operator debugging "why didn't my seed work" has one mental model, not two. Consistency across boot paths matters more than elegance.

2. The seed is still not transactional across both rows. If passwordEncoder.encode(adminPassword) ever throws (BCrypt OOM, missing bean), or userRepository.save(admin) hits a constraint, the group lands and the user doesn't — same starting state that bit us today, just at a different step. This PR fixes the specific row pair we hit. The class of bug still exists. Worth a follow-up ticket: wrap the seed body in @Transactional.

3. Retest the actual recovery scenario on staging. Before declaring done: deploy this branch, run the exact recovery flow (DELETE the user row, restart backend), confirm second boot succeeds without manual SQL. The unit tests prove the Java logic; staging proves the deployment story.

4. Runbook entry. Add a one-paragraph note to docs/infrastructure/ (or wherever ops notes live) describing the recovery procedure: "If admin seed fails partway, you can DELETE the admin user row and restart — the group will be reused." This is the documentation the next on-call needs.

Nothing infrastructure-shaped is broken

• No Compose changes
• No CI changes
• No image, env var, or secret changes
• No new failure mode for the prod stack

Ship it. File the follow-ups.

## Tobias Wendt — DevOps & Platform Engineer **Verdict: Approved** This is a deployment reliability bug, full stop. Worth saying out loud what almost happened on staging today: 1. First boot of the new stack: admin seed fired with the wrong `APP_ADMIN_USERNAME`, created the `Administrators` group, then created an admin user with the wrong email. 2. Operator fixed `APP_ADMIN_USERNAME` and `DELETE`d the orphan user row to retry. 3. Second boot: `userRepository.findByEmail(correct@email)` returned empty → seed path re-entered → `groupRepository.save(adminGroup)` → `DataIntegrityViolationException` on `user_groups_name_key` → Spring context aborts → **backend will not start**. The Compose stack health check would have correctly reported the backend as unhealthy. We would have had no admin user, no UI, and a stuck deployment. That's a P0 incident at 10pm on a Friday for any operator who isn't me. Fixing this is the right call. ### Operational observations 1. **The fix matches `initE2EData`.** That's good — same idiom in both seed paths means an operator debugging "why didn't my seed work" has one mental model, not two. Consistency across boot paths matters more than elegance. 2. **The seed is still not transactional across both rows.** If `passwordEncoder.encode(adminPassword)` ever throws (BCrypt OOM, missing bean), or `userRepository.save(admin)` hits a constraint, the group lands and the user doesn't — same starting state that bit us today, just at a different step. This PR fixes the specific row pair we hit. The class of bug still exists. Worth a follow-up ticket: wrap the seed body in `@Transactional`. 3. **Retest the actual recovery scenario on staging.** Before declaring done: deploy this branch, run the exact recovery flow (DELETE the user row, restart backend), confirm second boot succeeds without manual SQL. The unit tests prove the Java logic; staging proves the deployment story. 4. **Runbook entry.** Add a one-paragraph note to `docs/infrastructure/` (or wherever ops notes live) describing the recovery procedure: "If admin seed fails partway, you can DELETE the admin user row and restart — the group will be reused." This is the documentation the next on-call needs. ### Nothing infrastructure-shaped is broken - No Compose changes - No CI changes - No image, env var, or secret changes - No new failure mode for the prod stack Ship it. File the follow-ups.

marcel commented 2026-05-11 17:44:24 +02:00

Author

Owner

Copy Link

Elicit — Requirements Engineer

Verdict: Approved

This PR is in scope as a bug fix against a confirmed prod-blocker. The requirements story is clean: issue #518 names the failure, the PR description names the fix, the staging scenario is documented in the commit context. Traceability holds — incident → issue → PR → tests.

Definition of Ready (retrospective check on #518)

Criterion	Status
Clear, descriptive title	Pass — "fix(user): findOrCreate Administrators group..."
Context explaining the problem	Pass — staging incident with concrete reproduction
Acceptance criteria	Implied by failing scenario, codified in tests
Labeled	bug / devops / HIGH per the incident severity
Sized	XS — single-file logic change, one new test class extended
Dependencies identified	None — purely backend
Milestone	Should attach to whichever release this hot-fixes

Definition of Done (this PR)

• Acceptance criteria met (second boot no longer crashes when group exists and admin user does not)
• Tests cover both branches (group exists → reuse; group absent → create)
• Existing tests updated, not deleted, when the contract shifted
• Commit references the issue
• Verified against staging (recommend before close — Tobias raised this too)

Requirements observations

1. The original requirement was under-specified. Issue #513 introduced the fail-closed default-credentials check. Issue #518 reveals that "seed an admin" was treated as a one-shot operation, never as an operation that might be retried after partial failure. The implicit assumption — "the database is empty on first boot, and stays consistent thereafter" — was a hidden assumption that production proved wrong.

   Suggestion: when the seeder is touched next, add to its design notes an explicit non-functional requirement:

   NFR-RELIABILITY-001: The seed routine shall be idempotent under partial-failure recovery. Re-running after any subset of rows already exists shall converge to the intended end state without manual intervention.

2. No gap analysis needed for this PR. It's a targeted fix, not a feature.

3. Open question (TBD): Are there other startup seeders in the codebase (Tag defaults? permission defaults? OCR model registration?) with the same blind-INSERT pattern? If yes, those are latent #518s. Worth a 30-minute backlog audit. File as a chore: "Audit boot-time seeders for idempotency."

Nothing blocking

The PR is INVEST-compliant against issue #518. Acceptance criteria are testable and tested. Approved from the requirements side. The follow-ups above are backlog items, not gates on this merge.

## Elicit — Requirements Engineer **Verdict: Approved** This PR is in scope as a bug fix against a confirmed prod-blocker. The requirements story is clean: issue #518 names the failure, the PR description names the fix, the staging scenario is documented in the commit context. Traceability holds — incident → issue → PR → tests. ### Definition of Ready (retrospective check on #518) | Criterion | Status | |---|---| | Clear, descriptive title | Pass — "fix(user): findOrCreate Administrators group..." | | Context explaining the problem | Pass — staging incident with concrete reproduction | | Acceptance criteria | Implied by failing scenario, codified in tests | | Labeled | bug / devops / HIGH per the incident severity | | Sized | XS — single-file logic change, one new test class extended | | Dependencies identified | None — purely backend | | Milestone | Should attach to whichever release this hot-fixes | ### Definition of Done (this PR) - [x] Acceptance criteria met (second boot no longer crashes when group exists and admin user does not) - [x] Tests cover both branches (group exists → reuse; group absent → create) - [x] Existing tests updated, not deleted, when the contract shifted - [x] Commit references the issue - [ ] Verified against staging (recommend before close — Tobias raised this too) ### Requirements observations 1. **The original requirement was under-specified.** Issue #513 introduced the fail-closed default-credentials check. Issue #518 reveals that "seed an admin" was treated as a one-shot operation, never as an operation that might be retried after partial failure. The implicit assumption — "the database is empty on first boot, and stays consistent thereafter" — was a hidden assumption that production proved wrong. Suggestion: when the seeder is touched next, add to its design notes an explicit non-functional requirement: > **NFR-RELIABILITY-001**: The seed routine shall be idempotent under partial-failure recovery. Re-running after any subset of rows already exists shall converge to the intended end state without manual intervention. 2. **No gap analysis needed for this PR.** It's a targeted fix, not a feature. 3. **Open question (TBD):** Are there other startup seeders in the codebase (Tag defaults? permission defaults? OCR model registration?) with the same blind-INSERT pattern? If yes, those are latent #518s. Worth a 30-minute backlog audit. File as a chore: "Audit boot-time seeders for idempotency." ### Nothing blocking The PR is INVEST-compliant against issue #518. Acceptance criteria are testable and tested. Approved from the requirements side. The follow-ups above are backlog items, not gates on this merge.

marcel commented 2026-05-11 17:44:40 +02:00

Author

Owner

Copy Link

Nora "NullX" Steiner — Application Security Engineer

Verdict: Approved

I came in expecting to flag something around the admin-seed path — it's a high-value target, the comment in the file (lines 67–76) says "fail-closed in production," and any change touching it deserves scrutiny. After reading the diff, the existing default-credential fail-closed check in initAdminUser is untouched, so the security posture established in #513 is preserved.

What I checked

1. No change to the credential validation block. IllegalStateException still fires when (!isLocalProfile && (DEFAULT_ADMIN_EMAIL.equals(adminEmail) || DEFAULT_ADMIN_PASSWORD.equals(adminPassword))). Good — that gate is the load-bearing security control here, and the PR does not weaken it.

2. The new findOrCreate does not create an unauthenticated bypass. A pre-existing Administrators group with the canonical name will be reused — but the group is just a permission bag; reusing it does not grant any admin privilege to anyone. The new admin user is still gated by the findByEmail(adminEmail).isEmpty() check above it, and the fail-closed credential check still runs first. There is no path here where a seed runs without a strong password.

3. No injection vector introduced. "Administrators" is a literal string passed to a derived JPA query (findByName). Named parameter under the hood. Clean.

4. Permissions set is unchanged. Still ADMIN, READ_ALL, WRITE_ALL, ANNOTATE_ALL, ADMIN_USER, ADMIN_TAG, ADMIN_PERMISSION. No silent permission grant or removal.

5. Tests assert the security-relevant behavior. verify(groupRepository, never()).save(...) proves the existing group is reused, not replaced — important, because a replacement would mean a future admin seeded against a tampered group with weaker permissions could inherit those. The test pins the "reuse exact existing row" contract.

One observation — not a blocker

The "reuse existing group" path means: if an attacker with DB write access ever modified the Administrators group's permissions row (e.g. dropped ADMIN_PERMISSION), the next seed of a fresh admin user would attach to the tampered group, not re-create the canonical permission set. The blind-INSERT version of the code had the same property in practice (the tampered group would have stayed, and the new seed would have crashed exactly as #518 describes — slightly more visible). This is a low-severity defense-in-depth observation, not a finding against this PR: an attacker with UPDATE user_groups access has already won. Don't change anything here. Just noting it for the threat model.

Threat-model artifact for the file

The inline comment block at lines 70–74 explains why findOrCreate is safe (recoverable seed retry). Consider adding a one-liner sibling comment that names the assumption — something like:

// Note: this trusts the existing Administrators group's permission set. Tampering with
// user_groups directly is out of scope for application-layer defense (requires DB-level access).

Documenting the threat-model boundary is more valuable than any other comment in this file. Not a merge blocker.

Approved.

## Nora "NullX" Steiner — Application Security Engineer **Verdict: Approved** I came in expecting to flag something around the admin-seed path — it's a high-value target, the comment in the file (lines 67–76) says "fail-closed in production," and any change touching it deserves scrutiny. After reading the diff, the existing default-credential fail-closed check in `initAdminUser` is untouched, so the security posture established in #513 is preserved. ### What I checked 1. **No change to the credential validation block.** `IllegalStateException` still fires when `(!isLocalProfile && (DEFAULT_ADMIN_EMAIL.equals(adminEmail) || DEFAULT_ADMIN_PASSWORD.equals(adminPassword)))`. Good — that gate is the load-bearing security control here, and the PR does not weaken it. 2. **The new `findOrCreate` does not create an unauthenticated bypass.** A pre-existing `Administrators` group with the canonical name will be reused — but the group is just a permission bag; reusing it does not grant any admin privilege to anyone. The new admin user is still gated by the `findByEmail(adminEmail).isEmpty()` check above it, and the fail-closed credential check still runs first. There is no path here where a seed runs without a strong password. 3. **No injection vector introduced.** `"Administrators"` is a literal string passed to a derived JPA query (`findByName`). Named parameter under the hood. Clean. 4. **Permissions set is unchanged.** Still `ADMIN, READ_ALL, WRITE_ALL, ANNOTATE_ALL, ADMIN_USER, ADMIN_TAG, ADMIN_PERMISSION`. No silent permission grant or removal. 5. **Tests assert the security-relevant behavior.** `verify(groupRepository, never()).save(...)` proves the existing group is reused, not replaced — important, because a replacement would mean a future admin seeded against a tampered group with weaker permissions could inherit those. The test pins the "reuse exact existing row" contract. ### One observation — not a blocker The "reuse existing group" path means: **if an attacker with DB write access ever modified the `Administrators` group's permissions row** (e.g. dropped `ADMIN_PERMISSION`), the next seed of a fresh admin user would attach to the **tampered** group, not re-create the canonical permission set. The blind-INSERT version of the code had the same property in practice (the tampered group would have stayed, and the new seed would have crashed exactly as #518 describes — slightly more visible). This is a low-severity defense-in-depth observation, not a finding against this PR: an attacker with `UPDATE user_groups` access has already won. Don't change anything here. Just noting it for the threat model. ### Threat-model artifact for the file The inline comment block at lines 70–74 explains **why** `findOrCreate` is safe (recoverable seed retry). Consider adding a one-liner sibling comment that names the assumption — something like: ```java // Note: this trusts the existing Administrators group's permission set. Tampering with // user_groups directly is out of scope for application-layer defense (requires DB-level access). ``` Documenting the threat-model boundary is more valuable than any other comment in this file. Not a merge blocker. Approved.

marcel commented 2026-05-11 17:44:56 +02:00

Author

Owner

Copy Link

Sara Holt — Senior QA Engineer

Verdict: Approved with concerns

The tests are well-shaped. The concerns are about what they don't cover, not about what they do.

What's good

1. Test names read as sentences.

   • reuses_existing_Administrators_group_when_seeding_a_new_admin
   • creates_Administrators_group_when_seeding_admin_on_a_fresh_database

   When either fails in CI, the developer knows what broke without opening the file. This is the standard.

2. One logical assertion per test branch. The "reuse" test pins two things — verify(groupRepository, never()).save(...) AND the captor on the saved AppUser confirming getGroups() contains the pre-existing group. That's not assertion sprawl; it's verifying the same behavior end-to-end (the group is reused AND it's actually attached to the new admin). Acceptable.

3. Existing tests updated, not bypassed. The two pre-existing happy-path tests (allows_seed_when_both_values_are_set_and_profile_is_not_dev and allows_seed_with_defaults_when_profile_is_dev) were updated to stub groupRepository.findByName and groupRepository.save. That's the right move — when the production path changes, the tests must reflect it, not paper over it. No tests were @Disabled to make this PR go green.

4. Arrange-Act-Assert structure is clean in both new tests. Factory-style construction via UserGroup.builder() directly inline. Acceptable for two cases; if a fifth case appears, extract a makeAdminGroup() helper.

Concerns

1. Unit-test only. No integration test against real Postgres. This is the file's pre-existing state, not a regression, but worth naming: the bug we shipped lived in the interaction between two writes against a real UNIQUE constraint on user_groups.name. The Mockito tests cannot catch a future regression where, say, the constraint is renamed in a migration but the seed code keeps the old name. An AdminSeedIntegrationTest with @SpringBootTest + Testcontainers (postgres:16-alpine) that exercises the actual CommandLineRunner against a real DB — and a real partial-state setup (insert group, no user, then run the seeder) — is the test that would have caught #518 before staging. Strongly recommend filing as a follow-up.

2. No test for "group exists with different permissions." If a future migration changes the Administrators permission set, the orElseGet branch will silently keep the old, stale permissions on the existing group. That's a real risk for a permission system. A test asserting "permissions on the reused group are not silently mutated by the seeder, and a stale permission set is detectable" would be valuable.

3. The two-line stub of groupRepository.save in the two happy-path tests uses inv -> inv.getArgument(0). Correct, but if anyone copies this pattern they should know it returns the un-persisted entity (no generated ID). For these tests it doesn't matter because we don't assert on the group's ID downstream. Worth a one-line comment in the test, or a _savedGroup_returns_input helper.

Quality gates

• All 7 AdminSeedFailClosedTest cases pass (per PR description)
• All 4 AdminSeedPropertyKeyTest cases pass (per PR description)
• No integration test for the real DB-constraint behavior (follow-up)
• No staging verification yet (Tobias raised this — agree)

Approved on the unit-test changes. The integration-test gap is a follow-up, not a blocker for this specific patch.

## Sara Holt — Senior QA Engineer **Verdict: Approved with concerns** The tests are well-shaped. The concerns are about what they don't cover, not about what they do. ### What's good 1. **Test names read as sentences.** - `reuses_existing_Administrators_group_when_seeding_a_new_admin` - `creates_Administrators_group_when_seeding_admin_on_a_fresh_database` When either fails in CI, the developer knows what broke without opening the file. This is the standard. 2. **One logical assertion per test branch.** The "reuse" test pins two things — `verify(groupRepository, never()).save(...)` AND the captor on the saved `AppUser` confirming `getGroups()` contains the pre-existing group. That's not assertion sprawl; it's verifying the same behavior end-to-end (the group is reused AND it's actually attached to the new admin). Acceptable. 3. **Existing tests updated, not bypassed.** The two pre-existing happy-path tests (`allows_seed_when_both_values_are_set_and_profile_is_not_dev` and `allows_seed_with_defaults_when_profile_is_dev`) were updated to stub `groupRepository.findByName` and `groupRepository.save`. That's the right move — when the production path changes, the tests must reflect it, not paper over it. No tests were `@Disabled` to make this PR go green. 4. **Arrange-Act-Assert structure** is clean in both new tests. Factory-style construction via `UserGroup.builder()` directly inline. Acceptable for two cases; if a fifth case appears, extract a `makeAdminGroup()` helper. ### Concerns 1. **Unit-test only. No integration test against real Postgres.** This is the file's pre-existing state, not a regression, but worth naming: the bug we shipped lived in the interaction between two writes against a real `UNIQUE` constraint on `user_groups.name`. The Mockito tests cannot catch a future regression where, say, the constraint is renamed in a migration but the seed code keeps the old name. An `AdminSeedIntegrationTest` with `@SpringBootTest + Testcontainers (postgres:16-alpine)` that exercises the actual `CommandLineRunner` against a real DB — and a real partial-state setup (insert group, no user, then run the seeder) — is the test that would have caught #518 before staging. **Strongly recommend filing as a follow-up.** 2. **No test for "group exists with different permissions."** If a future migration changes the `Administrators` permission set, the `orElseGet` branch will silently keep the old, stale permissions on the existing group. That's a real risk for a permission system. A test asserting "permissions on the reused group are not silently mutated by the seeder, and a stale permission set is detectable" would be valuable. 3. **The two-line stub of `groupRepository.save` in the two happy-path tests** uses `inv -> inv.getArgument(0)`. Correct, but if anyone copies this pattern they should know it returns the un-persisted entity (no generated ID). For these tests it doesn't matter because we don't assert on the group's ID downstream. Worth a one-line comment in the test, or a `_savedGroup_returns_input` helper. ### Quality gates - [x] All 7 `AdminSeedFailClosedTest` cases pass (per PR description) - [x] All 4 `AdminSeedPropertyKeyTest` cases pass (per PR description) - [ ] No integration test for the real DB-constraint behavior (follow-up) - [ ] No staging verification yet (Tobias raised this — agree) Approved on the unit-test changes. The integration-test gap is a follow-up, not a blocker for this specific patch.

marcel commented 2026-05-11 17:45:04 +02:00

Author

Owner

Copy Link

Leonie Voss — UX & Accessibility

Verdict: Approved — LGTM, no UX surface

Checked the diff: two files, both backend Java (UserDataInitializer.java and AdminSeedFailClosedTest.java). No Svelte components, no routes, no Tailwind classes, no copy strings, no error-message surfaces visible to users.

What I checked anyway (because the failure mode is user-facing)

When this bug fires in production, the user experience is "the application will not load at all" — the Spring context aborts and the backend serves nothing. From a UX standpoint that's the worst-possible degraded state: no error page, no support hint, no path forward for a non-technical operator. So fixing it improves the UX outcome by removing a hard failure mode, even though the fix itself touches no UI.

One small thing the team might consider for a future PR — and explicitly not blocking this one:

• If the seed ever does fail (for a different reason: BCrypt failure, password validation throw, whatever), is there a documented operator-facing message? Right now the failure is a stack trace in the application log. A non-technical operator (and we have one in scope per req_engineer review) cannot act on that. A clearly worded log.error("Admin seed failed: ...") with a one-line "what to do" — pointing to a runbook — would close that gap. Tobias mentioned runbook in his review; this lives in the same neighborhood.

Nothing for this PR. Approved.

## Leonie Voss — UX & Accessibility **Verdict: Approved — LGTM, no UX surface** Checked the diff: two files, both backend Java (`UserDataInitializer.java` and `AdminSeedFailClosedTest.java`). No Svelte components, no routes, no Tailwind classes, no copy strings, no error-message surfaces visible to users. ### What I checked anyway (because the failure mode is user-facing) When this bug fires in production, the user experience is **"the application will not load at all"** — the Spring context aborts and the backend serves nothing. From a UX standpoint that's the worst-possible degraded state: no error page, no support hint, no path forward for a non-technical operator. So fixing it improves the UX outcome by removing a hard failure mode, even though the fix itself touches no UI. One small thing the team might consider for a future PR — and explicitly **not blocking this one**: - If the seed ever does fail (for a different reason: BCrypt failure, password validation throw, whatever), is there a documented operator-facing message? Right now the failure is a stack trace in the application log. A non-technical operator (and we have one in scope per `req_engineer` review) cannot act on that. A clearly worded `log.error("Admin seed failed: ...")` with a one-line "what to do" — pointing to a runbook — would close that gap. Tobias mentioned runbook in his review; this lives in the same neighborhood. Nothing for this PR. Approved.

marcel merged commit ad3b571bba into main 2026-05-11 18:19:50 +02:00

marcel deleted branch fix/issue-518-userdatainitializer-findorcreate-group 2026-05-11 18:19:50 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#519