feat(audit): track user management events in audit log (#336) #337

Merged

marcel merged 14 commits from feat/issue-336-audit-user-management into main 2026-04-26 19:42:00 +02:00

Conversation 15 Commits 14 Files Changed 10 +480 -32

marcel commented 2026-04-26 15:13:09 +02:00

Owner

Copy Link

Summary

• Adds USER_CREATED, USER_DELETED, GROUP_MEMBERSHIP_CHANGED to AuditKind (no schema migration needed — stored as EnumType.STRING)
• Injects AuditService into UserService; emits logAfterCommit in createUserOrUpdate, deleteUser, adminUpdateUser — audit entries only persist if the transaction commits
• GROUP_MEMBERSHIP_CHANGED payload contains group names, not permission strings; only emitted when the group set actually changes
• Adds findRecentUserManagementEvents(int limit) to AuditLogQueryService — ready for #326 to consume as findRecentUserManagementEvents(3)

Test plan

• UserServiceTest — 6 new unit tests covering all branches (log / don't-log) for the three service methods
• AuditLogQueryServiceTest — 1 unit test verifying findRecentByKinds is called with all three new kinds
• UserManagementAuditIntegrationTest — integration test: create user → delete user → query → assert USER_DELETED newest, USER_CREATED second
• Full suite: ./mvnw test → 1362 tests, 0 failures

Closes #336

## Summary - Adds `USER_CREATED`, `USER_DELETED`, `GROUP_MEMBERSHIP_CHANGED` to `AuditKind` (no schema migration needed — stored as `EnumType.STRING`) - Injects `AuditService` into `UserService`; emits `logAfterCommit` in `createUserOrUpdate`, `deleteUser`, `adminUpdateUser` — audit entries only persist if the transaction commits - `GROUP_MEMBERSHIP_CHANGED` payload contains group **names**, not permission strings; only emitted when the group set actually changes - Adds `findRecentUserManagementEvents(int limit)` to `AuditLogQueryService` — ready for #326 to consume as `findRecentUserManagementEvents(3)` ## Test plan - [ ] `UserServiceTest` — 6 new unit tests covering all branches (log / don't-log) for the three service methods - [ ] `AuditLogQueryServiceTest` — 1 unit test verifying `findRecentByKinds` is called with all three new kinds - [ ] `UserManagementAuditIntegrationTest` — integration test: create user → delete user → query → assert `USER_DELETED` newest, `USER_CREATED` second - [ ] Full suite: `./mvnw test` → 1362 tests, 0 failures Closes #336

marcel added 5 commits 2026-04-26 15:16:34 +02:00

feat(audit): emit USER_CREATED when admin creates a new user ... e7c7f801c9

Adds USER_CREATED, USER_DELETED, GROUP_MEMBERSHIP_CHANGED to AuditKind.
Injects AuditService into UserService; changes createUserOrUpdate to
accept actorId and emits logAfterCommit(USER_CREATED) only on the
new-user branch. Updates UserController to resolve and pass actorId.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(audit): emit USER_DELETED when admin removes a user ... a736b7399a

Adds actorId param to deleteUser(), captures email before deletion,
emits logAfterCommit(USER_DELETED) with userId+email in payload.
Updates UserController to resolve and pass actorId.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(audit): emit GROUP_MEMBERSHIP_CHANGED when admin updates user groups ... eb8f9d4dc4

Adds actorId param to adminUpdateUser(), captures beforeGroups before
mutation, computes added/removed group names, emits logAfterCommit only
when the group set actually changes. Payload contains group names, not
permission strings.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(audit): add findRecentUserManagementEvents query method ... 36529f7e11

Adds findRecentByKinds JPQL query to AuditLogQueryRepository and
findRecentUserManagementEvents(int limit) to AuditLogQueryService,
returning the N most recent USER_CREATED/USER_DELETED/GROUP_MEMBERSHIP_CHANGED
events ordered newest-first.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(audit): integration test — create + delete user produces ordered audit entries ... 77affcfb4f

Creates a real actor user first (needed for audit_log FK constraint),
then creates and deletes a target user, asserts USER_DELETED is newest
and USER_CREATED is second via findRecentUserManagementEvents.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel force-pushed feat/issue-336-audit-user-management from dd1bd837ad to 77affcfb4f 2026-04-26 15:16:34 +02:00 Compare

marcel commented 2026-04-26 15:18:46 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

TDD evidence is strong — the 7 new unit tests and the integration test clearly cover every branch. Test names are descriptive and read as sentences. Awaitility instead of Thread.sleep is the right call. Good stuff overall. Two things I'd push back on.

---

Blockers

adminUpdateUser violates SRP (UserService.java, lines 158–202)

The method now does three things: updates scalar fields, computes a group membership diff, and emits an audit event. The group diff block is 18 lines of beforeIds / newGroups / afterIds / added / removed — complex enough to have its own name and test.

Extract it:

private Optional<Map<String, Object>> groupChangePayload(
        Set<UserGroup> before, Set<UserGroup> after, UUID userId, String email) {
    Set<UUID> beforeIds = before.stream().map(UserGroup::getId).collect(toSet());
    Set<UUID> afterIds = after.stream().map(UserGroup::getId).collect(toSet());
    if (beforeIds.equals(afterIds)) return Optional.empty();
    List<String> added = after.stream()
            .filter(g -> !beforeIds.contains(g.getId())).map(UserGroup::getName).toList();
    List<String> removed = before.stream()
            .filter(g -> !afterIds.contains(g.getId())).map(UserGroup::getName).toList();
    return Optional.of(Map.of(
            "userId", userId.toString(), "email", email,
            "addedGroups", added, "removedGroups", removed));
}

Then adminUpdateUser just calls groupChangePayload(...).ifPresent(payload -> auditService.logAfterCommit(...)). The helper is unit-testable in isolation and adminUpdateUser reads in one pass.

---

Suggestions

DRY violation in controller (UserController.java, createUser, adminUpdateUser, deleteUser)

All three methods open with the same two lines:

AppUser actor = userService.findByEmail(authentication.getName());

Extract to a private helper:

private UUID actorId(Authentication auth) {
    return userService.findByEmail(auth.getName()).getId();
}

One call site per method, zero repetition. Not a blocker since it's three methods, but the pattern will spread.

isNew flag is readable but the control flow could be tighter (UserService.java, lines 52–83)

The isNew boolean set inside an if/else and checked 12 lines later is fine, but the createUserOrUpdate method is already ~40 lines with the audit addition. If adminUpdateUser gets its private helper, consider splitting createUserOrUpdate into createUser and updateExistingUser — the "or update" semantics are already awkward when only the create path audits.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** TDD evidence is strong — the 7 new unit tests and the integration test clearly cover every branch. Test names are descriptive and read as sentences. Awaitility instead of `Thread.sleep` is the right call. Good stuff overall. Two things I'd push back on. --- ### Blockers **`adminUpdateUser` violates SRP** (`UserService.java`, lines 158–202) The method now does three things: updates scalar fields, computes a group membership diff, and emits an audit event. The group diff block is 18 lines of `beforeIds` / `newGroups` / `afterIds` / `added` / `removed` — complex enough to have its own name and test. Extract it: ```java private Optional<Map<String, Object>> groupChangePayload( Set<UserGroup> before, Set<UserGroup> after, UUID userId, String email) { Set<UUID> beforeIds = before.stream().map(UserGroup::getId).collect(toSet()); Set<UUID> afterIds = after.stream().map(UserGroup::getId).collect(toSet()); if (beforeIds.equals(afterIds)) return Optional.empty(); List<String> added = after.stream() .filter(g -> !beforeIds.contains(g.getId())).map(UserGroup::getName).toList(); List<String> removed = before.stream() .filter(g -> !afterIds.contains(g.getId())).map(UserGroup::getName).toList(); return Optional.of(Map.of( "userId", userId.toString(), "email", email, "addedGroups", added, "removedGroups", removed)); } ``` Then `adminUpdateUser` just calls `groupChangePayload(...).ifPresent(payload -> auditService.logAfterCommit(...))`. The helper is unit-testable in isolation and `adminUpdateUser` reads in one pass. --- ### Suggestions **DRY violation in controller** (`UserController.java`, `createUser`, `adminUpdateUser`, `deleteUser`) All three methods open with the same two lines: ```java AppUser actor = userService.findByEmail(authentication.getName()); ``` Extract to a private helper: ```java private UUID actorId(Authentication auth) { return userService.findByEmail(auth.getName()).getId(); } ``` One call site per method, zero repetition. Not a blocker since it's three methods, but the pattern will spread. **`isNew` flag is readable but the control flow could be tighter** (`UserService.java`, lines 52–83) The `isNew` boolean set inside an `if/else` and checked 12 lines later is fine, but the `createUserOrUpdate` method is already ~40 lines with the audit addition. If `adminUpdateUser` gets its private helper, consider splitting `createUserOrUpdate` into `createUser` and `updateExistingUser` — the "or update" semantics are already awkward when only the create path audits.

marcel commented 2026-04-26 15:18:53 +02:00

Author

Owner

Copy Link

🏗️ Markus Keller — Application Architect

Verdict: ✅ Approved

The structural choices here are sound. AuditService is injected into UserService via constructor — service calls service, no repository leakage. The new AuditKind values live in the audit feature package. findRecentUserManagementEvents provides a clean API boundary for #326 to consume without coupling it to the audit internals. Module boundaries are respected throughout.

---

Notes (non-blocking)

LIMIT :limit in JPQL is a Hibernate HQL extension, not portable JPQL (AuditLogQueryRepository.java)

@Query("SELECT a FROM AuditLog a WHERE a.kind IN :kinds ORDER BY a.happenedAt DESC LIMIT :limit")
List<AuditLog> findRecentByKinds(...);

This works because Spring Boot 4 uses Hibernate 6, which accepts LIMIT in HQL. But it's a Hibernate-specific extension and the :limit binding is non-standard JPQL. The more idiomatic Spring Data approaches are:

// Option A — Spring Data method name limit
List<AuditLog> findTop10ByKindInOrderByHappenedAtDesc(Collection<AuditKind> kinds);

// Option B — Pageable (most flexible for future #326 pagination)
Page<AuditLog> findByKindIn(Collection<AuditKind> kinds, Pageable pageable);
// called as: findByKindIn(kinds, PageRequest.of(0, limit, Sort.by("happenedAt").descending()))

Option A is the simplest drop-in. Option B gives #326 pagination for free. Either avoids the HQL-specific LIMIT :limit binding. Not a blocker — Hibernate 6 supports it — but worth normalising.

Extra DB lookup per admin operation (UserController.java)

Each of the three modified endpoints now does userService.findByEmail(authentication.getName()) before the main operation. For an admin-only path this is low frequency, so not a performance concern. Just worth being aware that if the actor lookup fails (user deleted between login and request), the endpoint throws USER_NOT_FOUND rather than serving the admin action. This is the correct fail-closed behaviour, just document it if it surprises someone during an incident.

null actorId accepted by logAfterCommit

The integration test deliberately passes null actorId for the bootstrap admin creation. The production createUserOrUpdate also passes null as actorId when creating the initial admin (it receives null from the test). In production, the controller always provides a real actorId, so this only surfaces during seeding or tests. Acceptable, but a @Nullable annotation on the actorId parameter in logAfterCommit's signature would make the contract explicit.

## 🏗️ Markus Keller — Application Architect **Verdict: ✅ Approved** The structural choices here are sound. `AuditService` is injected into `UserService` via constructor — service calls service, no repository leakage. The new `AuditKind` values live in the `audit` feature package. `findRecentUserManagementEvents` provides a clean API boundary for #326 to consume without coupling it to the audit internals. Module boundaries are respected throughout. --- ### Notes (non-blocking) **`LIMIT :limit` in JPQL is a Hibernate HQL extension, not portable JPQL** (`AuditLogQueryRepository.java`) ```java @Query("SELECT a FROM AuditLog a WHERE a.kind IN :kinds ORDER BY a.happenedAt DESC LIMIT :limit") List<AuditLog> findRecentByKinds(...); ``` This works because Spring Boot 4 uses Hibernate 6, which accepts `LIMIT` in HQL. But it's a Hibernate-specific extension and the `:limit` binding is non-standard JPQL. The more idiomatic Spring Data approaches are: ```java // Option A — Spring Data method name limit List<AuditLog> findTop10ByKindInOrderByHappenedAtDesc(Collection<AuditKind> kinds); // Option B — Pageable (most flexible for future #326 pagination) Page<AuditLog> findByKindIn(Collection<AuditKind> kinds, Pageable pageable); // called as: findByKindIn(kinds, PageRequest.of(0, limit, Sort.by("happenedAt").descending())) ``` Option A is the simplest drop-in. Option B gives #326 pagination for free. Either avoids the HQL-specific `LIMIT :limit` binding. Not a blocker — Hibernate 6 supports it — but worth normalising. **Extra DB lookup per admin operation** (`UserController.java`) Each of the three modified endpoints now does `userService.findByEmail(authentication.getName())` before the main operation. For an admin-only path this is low frequency, so not a performance concern. Just worth being aware that if the actor lookup fails (user deleted between login and request), the endpoint throws `USER_NOT_FOUND` rather than serving the admin action. This is the correct fail-closed behaviour, just document it if it surprises someone during an incident. **`null` actorId accepted by `logAfterCommit`** The integration test deliberately passes `null` actorId for the bootstrap admin creation. The production `createUserOrUpdate` also passes `null` as actorId when creating the initial admin (it receives null from the test). In production, the controller always provides a real actorId, so this only surfaces during seeding or tests. Acceptable, but a `@Nullable` annotation on the `actorId` parameter in `logAfterCommit`'s signature would make the contract explicit.

marcel commented 2026-04-26 15:19:11 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ⚠️ Approved with concerns

No new injection vectors. Parameterized JPQL. @RequirePermission(ADMIN_USER) is already on all three modified endpoints. The actor identity is resolved server-side from the Authentication principal — the client cannot supply a fake actor. These are all correct.

Two concerns, one of which I'd call a blocker for an audit trail.

---

Blockers

No permission-enforcement test for the modified endpoints

The PR adds actor-resolution logic to createUser, adminUpdateUser, and deleteUser, but there is no @WebMvcTest test verifying that a caller without ADMIN_USER gets a 403. Existing tests mock the service and never exercise the @RequirePermission AOP path. A missing @RequirePermission annotation is silent — it compiles fine and returns 200 to everyone.

Minimum regression test for each endpoint:

@Test
void createUser_returns403_when_caller_lacks_ADMIN_USER() throws Exception {
    mockMvc.perform(post("/api/admin/users")
            .contentType(MediaType.APPLICATION_JSON)
            .content("{\"email\":\"x@x.com\",\"initialPassword\":\"pw\"}")
            .with(user("viewer").authorities(new SimpleGrantedAuthority("READ_ALL"))))
        .andExpect(status().isForbidden());
}

Without this, removing the annotation by mistake would go undetected until manual testing.

---

Suggestions

PII in audit payload — document the data retention decision

The USER_CREATED, USER_DELETED, and GROUP_MEMBERSHIP_CHANGED payloads all include email. This is correct for a useful audit trail, but it means the audit_log table now stores personally identifiable data that must be covered by the GDPR retention policy. Since this is a family archive with European users, this matters.

Suggested: add a comment in AuditKind.java or the migration for the audit_log table noting that email-containing events are subject to the data retention policy. Not a code fix — a documentation marker so future-you doesn't have to rediscover this.

null actorId weakens audit trail attribution

logAfterCommit(AuditKind.USER_CREATED, null, null, payload) is called when the bootstrap admin creates itself (createUserOrUpdate(null, adminReq) in the integration test). In production the controller always provides a real actorId, so this is low-risk. But if someone calls createUserOrUpdate directly from a migration or data-seeding script without an actorId, the audit entry is unattributable. Consider a distinct AuditKind.USER_CREATED_SYSTEM (no actorId required by design) versus USER_CREATED (actorId required, throw if null). This makes the contract unambiguous rather than silently nullable.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ⚠️ Approved with concerns** No new injection vectors. Parameterized JPQL. `@RequirePermission(ADMIN_USER)` is already on all three modified endpoints. The actor identity is resolved server-side from the `Authentication` principal — the client cannot supply a fake actor. These are all correct. Two concerns, one of which I'd call a blocker for an audit trail. --- ### Blockers **No permission-enforcement test for the modified endpoints** The PR adds actor-resolution logic to `createUser`, `adminUpdateUser`, and `deleteUser`, but there is no `@WebMvcTest` test verifying that a caller without `ADMIN_USER` gets a 403. Existing tests mock the service and never exercise the `@RequirePermission` AOP path. A missing `@RequirePermission` annotation is silent — it compiles fine and returns 200 to everyone. Minimum regression test for each endpoint: ```java @Test void createUser_returns403_when_caller_lacks_ADMIN_USER() throws Exception { mockMvc.perform(post("/api/admin/users") .contentType(MediaType.APPLICATION_JSON) .content("{\"email\":\"x@x.com\",\"initialPassword\":\"pw\"}") .with(user("viewer").authorities(new SimpleGrantedAuthority("READ_ALL")))) .andExpect(status().isForbidden()); } ``` Without this, removing the annotation by mistake would go undetected until manual testing. --- ### Suggestions **PII in audit payload — document the data retention decision** The `USER_CREATED`, `USER_DELETED`, and `GROUP_MEMBERSHIP_CHANGED` payloads all include `email`. This is correct for a useful audit trail, but it means the `audit_log` table now stores personally identifiable data that must be covered by the GDPR retention policy. Since this is a family archive with European users, this matters. Suggested: add a comment in `AuditKind.java` or the migration for the `audit_log` table noting that email-containing events are subject to the data retention policy. Not a code fix — a documentation marker so future-you doesn't have to rediscover this. **`null` actorId weakens audit trail attribution** `logAfterCommit(AuditKind.USER_CREATED, null, null, payload)` is called when the bootstrap admin creates itself (`createUserOrUpdate(null, adminReq)` in the integration test). In production the controller always provides a real actorId, so this is low-risk. But if someone calls `createUserOrUpdate` directly from a migration or data-seeding script without an actorId, the audit entry is unattributable. Consider a distinct `AuditKind.USER_CREATED_SYSTEM` (no actorId required by design) versus `USER_CREATED` (actorId required, throw if null). This makes the contract unambiguous rather than silently nullable.

marcel commented 2026-04-26 15:19:23 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

The test coverage is good at the unit layer — 7 new unit tests with clear names, proper Arrange-Act-Assert, ArgumentCaptor used correctly. The integration test uses Awaitility (not Thread.sleep), hits a real Postgres container, and verifies ordering. All strong positives.

Two gaps I'd flag before this ships.

---

Blockers

No integration test for GROUP_MEMBERSHIP_CHANGED

UserManagementAuditIntegrationTest covers the create→delete sequence. But GROUP_MEMBERSHIP_CHANGED is the most complex event: it fires only when the group set actually changes, it computes added/removed lists from set differences, and the payload is a Map<String, Object> with heterogeneous value types (String and List<String>). The unit test (adminUpdateUser_logsGroupMembershipChanged_whenGroupSetChanges) verifies the service logic against a mock, but it does not verify that the payload serializes correctly through the full stack (service → AuditService → async write → audit_log row → query).

Suggested addition to the integration test:

@Test
void updateUserGroups_producesGroupMembershipChangedEvent() {
    // create actor, create target user, call adminUpdateUser to change groups,
    // await event, verify kind + payload.addedGroups/removedGroups in the stored JSON
}

---

Suggestions

@DirtiesContext(AFTER_EACH_TEST_METHOD) will hurt CI as the class grows

Right now UserManagementAuditIntegrationTest has one test method, so the cost is hidden. AFTER_EACH_TEST_METHOD tears down and rebuilds the entire Spring application context after each test — the Postgres Testcontainer is shared (static), but the Spring context rebuild adds 5–10 seconds per test on this project.

The integration test clears audit log rows manually with auditLogRepository.deleteAll() mid-test. The same pattern works as a @BeforeEach:

@BeforeEach
void clearAuditLog() {
    transactionTemplate.execute(status -> { auditLogRepository.deleteAll(); return null; });
}

With that, @DirtiesContext can be dropped entirely. Test isolation becomes explicit without the context rebuild penalty.

Wait condition relies on absolute count()

await().atMost(5, SECONDS).until(() -> auditLogRepository.count() > 0);

After the deleteAll() mid-test, this is correct. But if a background process or another async operation writes to audit_log, a stray row could satisfy the count before the expected event arrives. Prefer a condition that checks for the specific expected event:

await().atMost(5, SECONDS)
    .until(() -> auditLogRepository.existsByKind(AuditKind.USER_CREATED));

This makes the wait semantically precise and immune to noise from other test-generated audit entries.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** The test coverage is good at the unit layer — 7 new unit tests with clear names, proper Arrange-Act-Assert, ArgumentCaptor used correctly. The integration test uses Awaitility (not `Thread.sleep`), hits a real Postgres container, and verifies ordering. All strong positives. Two gaps I'd flag before this ships. --- ### Blockers **No integration test for `GROUP_MEMBERSHIP_CHANGED`** `UserManagementAuditIntegrationTest` covers the create→delete sequence. But `GROUP_MEMBERSHIP_CHANGED` is the most complex event: it fires only when the group *set* actually changes, it computes added/removed lists from set differences, and the payload is a `Map<String, Object>` with heterogeneous value types (`String` and `List<String>`). The unit test (`adminUpdateUser_logsGroupMembershipChanged_whenGroupSetChanges`) verifies the service logic against a mock, but it does not verify that the payload serializes correctly through the full stack (service → AuditService → async write → `audit_log` row → query). Suggested addition to the integration test: ```java @Test void updateUserGroups_producesGroupMembershipChangedEvent() { // create actor, create target user, call adminUpdateUser to change groups, // await event, verify kind + payload.addedGroups/removedGroups in the stored JSON } ``` --- ### Suggestions **`@DirtiesContext(AFTER_EACH_TEST_METHOD)` will hurt CI as the class grows** Right now `UserManagementAuditIntegrationTest` has one test method, so the cost is hidden. `AFTER_EACH_TEST_METHOD` tears down and rebuilds the entire Spring application context after each test — the Postgres Testcontainer is shared (static), but the Spring context rebuild adds 5–10 seconds per test on this project. The integration test clears audit log rows manually with `auditLogRepository.deleteAll()` mid-test. The same pattern works as a `@BeforeEach`: ```java @BeforeEach void clearAuditLog() { transactionTemplate.execute(status -> { auditLogRepository.deleteAll(); return null; }); } ``` With that, `@DirtiesContext` can be dropped entirely. Test isolation becomes explicit without the context rebuild penalty. **Wait condition relies on absolute `count()`** ```java await().atMost(5, SECONDS).until(() -> auditLogRepository.count() > 0); ``` After the `deleteAll()` mid-test, this is correct. But if a background process or another async operation writes to `audit_log`, a stray row could satisfy the count before the expected event arrives. Prefer a condition that checks for the specific expected event: ```java await().atMost(5, SECONDS) .until(() -> auditLogRepository.existsByKind(AuditKind.USER_CREATED)); ``` This makes the wait semantically precise and immune to noise from other test-generated audit entries.

marcel commented 2026-04-26 15:19:31 +02:00

Author

Owner

Copy Link

🚀 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

No infrastructure changes in this PR — no Compose changes, no new CI steps, no Dockerfile modifications, no new environment variables, no secrets handling. From an operations perspective, the feature piggybacks entirely on the existing audit_log table and the existing async infrastructure, so there is nothing new to operate.

One observation for CI maintainability:

@DirtiesContext(AFTER_EACH_TEST_METHOD) adds rebuild cost to CI

UserManagementAuditIntegrationTest uses DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD. Each Spring context rebuild on this project takes ~8–12 seconds on the CI runner. With one test method now it is invisible, but the class will grow. Sara's note in the QA review covers the fix — a @BeforeEach cleanup of the audit log rows removes the need for it.

Everything else here is routine backend Java. The new test class uses PostgresContainerConfig (shared Testcontainer), the @MockitoBean S3Client correctly stubs out the MinIO client, and the @ActiveProfiles("test") is consistent with every other integration test. CI should pick this up cleanly.

LGTM on the infra side.

## 🚀 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** No infrastructure changes in this PR — no Compose changes, no new CI steps, no Dockerfile modifications, no new environment variables, no secrets handling. From an operations perspective, the feature piggybacks entirely on the existing `audit_log` table and the existing async infrastructure, so there is nothing new to operate. One observation for CI maintainability: **`@DirtiesContext(AFTER_EACH_TEST_METHOD)` adds rebuild cost to CI** `UserManagementAuditIntegrationTest` uses `DirtiesContext.ClassMode.AFTER_EACH_TEST_METHOD`. Each Spring context rebuild on this project takes ~8–12 seconds on the CI runner. With one test method now it is invisible, but the class will grow. Sara's note in the QA review covers the fix — a `@BeforeEach` cleanup of the audit log rows removes the need for it. Everything else here is routine backend Java. The new test class uses `PostgresContainerConfig` (shared Testcontainer), the `@MockitoBean S3Client` correctly stubs out the MinIO client, and the `@ActiveProfiles("test")` is consistent with every other integration test. CI should pick this up cleanly. LGTM on the infra side.

marcel commented 2026-04-26 15:19:39 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

This PR is entirely backend — new AuditKind values, service-layer audit emission, a new query method, and tests. There are no .svelte files, no Tailwind classes, no HTML markup, and no user-facing strings changed. Nothing to review from a UI, accessibility, or brand-compliance perspective.

One forward-looking note for when #326 surfaces these events in the admin panel:

The GROUP_MEMBERSHIP_CHANGED payload includes addedGroups and removedGroups as arrays of group names. When this reaches the UI, the display pattern matters for the senior audience (60+): a plain ["Admin", "Editors"] list dumped as text is illegible. I'd expect a diff-style representation:

• +Admin in brand-mint (with a + prefix or aria-label="hinzugefügt" — never color alone)
• −Viewers in a muted red-adjacent tone (again, icon + color, not color only)

Flag this when #326 is spec'd — happy to provide the exact Tailwind classes and WCAG contrast values at that point.

LGTM on this PR.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** This PR is entirely backend — new `AuditKind` values, service-layer audit emission, a new query method, and tests. There are no `.svelte` files, no Tailwind classes, no HTML markup, and no user-facing strings changed. Nothing to review from a UI, accessibility, or brand-compliance perspective. One forward-looking note for when #326 surfaces these events in the admin panel: **The `GROUP_MEMBERSHIP_CHANGED` payload includes `addedGroups` and `removedGroups` as arrays of group names.** When this reaches the UI, the display pattern matters for the senior audience (60+): a plain `["Admin", "Editors"]` list dumped as text is illegible. I'd expect a diff-style representation: - `+Admin` in brand-mint (with a `+` prefix or `aria-label="hinzugefügt"` — never color alone) - `−Viewers` in a muted red-adjacent tone (again, icon + color, not color only) Flag this when #326 is spec'd — happy to provide the exact Tailwind classes and WCAG contrast values at that point. LGTM on this PR.

marcel added 6 commits 2026-04-26 15:53:57 +02:00

refactor(audit): extract groupChangePayload() from adminUpdateUser ... 1d3a3b3338

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(audit): add 403 permission tests for createUser, adminUpdateUser, deleteUser ... f4a4436eda

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(audit): extract actorId() helper in UserController ... f6bcc4f72a

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(audit): replace LIMIT :limit JPQL with Pageable in audit query ... 11d93919b2

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(audit): drop @DirtiesContext, add @BeforeEach, use existsByKind in wait conditions ... 23cff1cdd7

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(audit): add GROUP_MEMBERSHIP_CHANGED integration test with payload assertions ... 1dd6e054fc

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-04-26 15:55:31 +02:00

Author

Owner

Copy Link

Review concerns addressed

All blockers and actionable suggestions from the six-persona review have been resolved. Summary of changes pushed to this branch:

---

Blockers resolved

Felix — SRP violation in adminUpdateUser ✅
Extracted the 18-line group diff block into a private groupChangePayload(Set<UserGroup> before, Set<UserGroup> after, UUID userId, String email) → Optional<Map<String, Object>> helper. adminUpdateUser now calls .ifPresent(...) — single responsibility restored.
→ commit refactor(audit): extract groupChangePayload() from adminUpdateUser

Nora — No permission enforcement tests ✅
Added three @WebMvcTest tests to UserControllerTest verifying that createUser, adminUpdateUser, and deleteUser each return 403 when the caller lacks ADMIN_USER. Follows the existing getUser_returns403_* pattern.
→ commit test(audit): add 403 permission tests for createUser, adminUpdateUser, deleteUser

Sara — No integration test for GROUP_MEMBERSHIP_CHANGED ✅
Added updateUserGroups_producesGroupMembershipChangedEvent() to UserManagementAuditIntegrationTest. Creates two groups and a target user pre-assigned to group A, changes to group B via adminUpdateUser, awaits the event, then asserts kind, email, addedGroups, and removedGroups from the DB-round-tripped JSONB payload.
→ commit test(audit): add GROUP_MEMBERSHIP_CHANGED integration test with payload assertions

---

Suggestions implemented

Felix — DRY in controller ✅
Extracted actorId(Authentication) private helper in UserController — three methods now each call actorId(authentication) instead of repeating userService.findByEmail(auth.getName()).getId().
→ commit refactor(audit): extract actorId() helper in UserController

Markus — LIMIT :limit Hibernate HQL ✅
Replaced findRecentByKinds(@Param("limit") int limit) with Page<AuditLog> findByKindIn(Collection<AuditKind>, Pageable). findRecentUserManagementEvents now uses PageRequest.of(0, limit, Sort.by("happenedAt").descending()). Standard Spring Data, no Hibernate extension.
→ commit refactor(audit): replace LIMIT :limit JPQL with Pageable in audit query

Sara / Tobias — @DirtiesContext(AFTER_EACH_TEST_METHOD) ✅
Dropped @DirtiesContext. Added @BeforeEach void clearAuditLog() for between-test isolation without context rebuild.
→ commit refactor(audit): drop @DirtiesContext, add @BeforeEach, use existsByKind in wait conditions

Sara — Fragile count()-based wait conditions ✅
Added existsByKind(AuditKind) to AuditLogRepository. All three await().until(() -> count() > 0) / count() >= 2 conditions replaced with semantically precise existsByKind(USER_CREATED) / existsByKind(USER_DELETED) / existsByKind(GROUP_MEMBERSHIP_CHANGED) checks.
→ same commit as above

---

Test suite

1366 tests, 0 failures (+4 net: 3 × 403 controller tests + 1 group membership integration test).

## Review concerns addressed All blockers and actionable suggestions from the six-persona review have been resolved. Summary of changes pushed to this branch: --- ### Blockers resolved **Felix — SRP violation in `adminUpdateUser`** ✅ Extracted the 18-line group diff block into a private `groupChangePayload(Set<UserGroup> before, Set<UserGroup> after, UUID userId, String email) → Optional<Map<String, Object>>` helper. `adminUpdateUser` now calls `.ifPresent(...)` — single responsibility restored. → commit `refactor(audit): extract groupChangePayload() from adminUpdateUser` **Nora — No permission enforcement tests** ✅ Added three `@WebMvcTest` tests to `UserControllerTest` verifying that `createUser`, `adminUpdateUser`, and `deleteUser` each return 403 when the caller lacks `ADMIN_USER`. Follows the existing `getUser_returns403_*` pattern. → commit `test(audit): add 403 permission tests for createUser, adminUpdateUser, deleteUser` **Sara — No integration test for `GROUP_MEMBERSHIP_CHANGED`** ✅ Added `updateUserGroups_producesGroupMembershipChangedEvent()` to `UserManagementAuditIntegrationTest`. Creates two groups and a target user pre-assigned to group A, changes to group B via `adminUpdateUser`, awaits the event, then asserts kind, email, `addedGroups`, and `removedGroups` from the DB-round-tripped JSONB payload. → commit `test(audit): add GROUP_MEMBERSHIP_CHANGED integration test with payload assertions` --- ### Suggestions implemented **Felix — DRY in controller** ✅ Extracted `actorId(Authentication)` private helper in `UserController` — three methods now each call `actorId(authentication)` instead of repeating `userService.findByEmail(auth.getName()).getId()`. → commit `refactor(audit): extract actorId() helper in UserController` **Markus — `LIMIT :limit` Hibernate HQL** ✅ Replaced `findRecentByKinds(@Param("limit") int limit)` with `Page<AuditLog> findByKindIn(Collection<AuditKind>, Pageable)`. `findRecentUserManagementEvents` now uses `PageRequest.of(0, limit, Sort.by("happenedAt").descending())`. Standard Spring Data, no Hibernate extension. → commit `refactor(audit): replace LIMIT :limit JPQL with Pageable in audit query` **Sara / Tobias — `@DirtiesContext(AFTER_EACH_TEST_METHOD)`** ✅ Dropped `@DirtiesContext`. Added `@BeforeEach void clearAuditLog()` for between-test isolation without context rebuild. → commit `refactor(audit): drop @DirtiesContext, add @BeforeEach, use existsByKind in wait conditions` **Sara — Fragile `count()`-based wait conditions** ✅ Added `existsByKind(AuditKind)` to `AuditLogRepository`. All three `await().until(() -> count() > 0)` / `count() >= 2` conditions replaced with semantically precise `existsByKind(USER_CREATED)` / `existsByKind(USER_DELETED)` / `existsByKind(GROUP_MEMBERSHIP_CHANGED)` checks. → same commit as above --- ### Test suite 1366 tests, 0 failures (+4 net: 3 × 403 controller tests + 1 group membership integration test).

marcel commented 2026-04-26 15:57:14 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Overall this is clean, disciplined backend work. The implementation is focused, the helpers do one thing, and the test coverage matches the surface area.

Suggestions

actorId() does a DB lookup on every admin call
UserController.actorId() calls userService.findByEmail(auth.getName()) which hits the database on every createUser, adminUpdateUser, and deleteUser request — just to resolve the acting user's UUID. The Spring Security Authentication principal is already an AppUser object (loaded by UserDetailsService); you could cast it directly instead:

private UUID actorId(Authentication auth) {
    return ((AppUser) auth.getPrincipal()).getId();
}

This avoids a redundant SELECT per admin operation. Minor, but worth fixing since it scales with every protected endpoint.

isNew boolean flag in createUserOrUpdate is fine as a local variable
It's a local flag, not a boolean parameter — this doesn't violate the "no boolean args" rule. The branching is clear. No action needed.

groupChangePayload() is a well-extracted private helper
Clean: compares UUID sets (not entity equality), builds the payload only when a real change exists, returns Optional.empty() otherwise. This is exactly the right shape for this logic.

logAfterCommit with null actorId (bootstrap case)
The integration test passes null as actorId for the bootstrap admin creation. The production code path permits this. Whether AuditService.logAfterCommit handles null gracefully isn't visible in this diff — worth verifying if there's a null check or DB NOT NULL constraint on actor_id. Not a blocker if the service is already null-safe, but worth a glance.

Test coverage
Six unit tests in UserServiceTest, one in AuditLogQueryServiceTest, one full integration test with two scenarios, three 403 permission tests in UserControllerTest. This matches the implementation surface area precisely.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** Overall this is clean, disciplined backend work. The implementation is focused, the helpers do one thing, and the test coverage matches the surface area. ### Suggestions **`actorId()` does a DB lookup on every admin call** `UserController.actorId()` calls `userService.findByEmail(auth.getName())` which hits the database on every `createUser`, `adminUpdateUser`, and `deleteUser` request — just to resolve the acting user's UUID. The Spring Security `Authentication` principal is already an `AppUser` object (loaded by `UserDetailsService`); you could cast it directly instead: ```java private UUID actorId(Authentication auth) { return ((AppUser) auth.getPrincipal()).getId(); } ``` This avoids a redundant SELECT per admin operation. Minor, but worth fixing since it scales with every protected endpoint. **`isNew` boolean flag in `createUserOrUpdate` is fine as a local variable** It's a local flag, not a boolean parameter — this doesn't violate the "no boolean args" rule. The branching is clear. No action needed. **`groupChangePayload()` is a well-extracted private helper** Clean: compares UUID sets (not entity equality), builds the payload only when a real change exists, returns `Optional.empty()` otherwise. This is exactly the right shape for this logic. **`logAfterCommit` with `null` actorId (bootstrap case)** The integration test passes `null` as `actorId` for the bootstrap admin creation. The production code path permits this. Whether `AuditService.logAfterCommit` handles null gracefully isn't visible in this diff — worth verifying if there's a null check or DB NOT NULL constraint on `actor_id`. Not a blocker if the service is already null-safe, but worth a glance. **Test coverage** Six unit tests in `UserServiceTest`, one in `AuditLogQueryServiceTest`, one full integration test with two scenarios, three 403 permission tests in `UserControllerTest`. This matches the implementation surface area precisely.

marcel commented 2026-04-26 15:57:32 +02:00

Author

Owner

Copy Link

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: ⚠️ Approved with concerns

The permission model is correct, the audit trail is well-structured, and the 403 tests are a welcome addition. I have two findings that need attention.

---

Blockers

Finding 1: Null actorId accepted in production code path

UserService.createUserOrUpdate(UUID actorId, CreateUserRequest request) accepts a null actorId and the integration test deliberately passes null:

AppUser actor = transactionTemplate.execute(status ->
        userService.createUserOrUpdate(null, adminReq));

The test comments this as "bypasses audit logging so no FK issue." This means production code silently accepts a null actor, relying on AuditService.logAfterCommit to handle it gracefully.

The risk: If AuditService or the audit_log table has a NOT NULL constraint on actor_id, bootstrap calls will fail at runtime. If it doesn't, any caller that forgets to pass the actorId (passes null) will silently log an un-attributable event with no error. Un-attributable audit events are a compliance gap — the whole point of an audit trail is knowing who did what.

Recommendation: Either make actorId @NonNull and provide a dedicated createUserForBootstrap(CreateUserRequest) without audit, or document that null actorId is the explicit "system-initiated" actor with a sentinel UUID (e.g., SYSTEM_ACTOR_ID = UUID(0, 0)). The current ambiguity is a security smell.

---

Finding 2: actorId() in UserController — extra DB round-trip + potential NPE

private UUID actorId(Authentication auth) {
    return userService.findByEmail(auth.getName()).getId();
}

If findByEmail returns a user that doesn't exist (e.g., account deleted between auth and request), it will throw DomainException.notFound. That's fail-closed and acceptable. However, this is a hidden assumption: the authenticated principal is always in the database. If the AppUser entity is the UserDetails implementation (which is typical with Spring Security), its getId() is already available via (AppUser) auth.getPrincipal() without a DB call. This also eliminates the window between auth and actor resolution.

---

Suggestions

Permission tests cover 403 (authenticated, wrong role) — good. Consider also adding a test for 401 (unauthenticated) for each of the three endpoints. These are different security failures: one is "not logged in," the other is "logged in but not authorized." Both should be tested per the defense-in-depth principle.

@Test
void createUser_returns401_whenUnauthenticated() throws Exception {
    mockMvc.perform(post("/api/users")
                    .contentType(APPLICATION_JSON)
                    .content("{\"email\":\"x@x.com\",\"initialPassword\":\"secret123\"}"))
            .andExpect(status().isUnauthorized());
}

Audit payload contains email — this is the right choice for audit trails. Group names (not permission strings) in the payload is also the right call: human-readable, less sensitive than internal IDs, and stable for display in an admin UI.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: ⚠️ Approved with concerns** The permission model is correct, the audit trail is well-structured, and the 403 tests are a welcome addition. I have two findings that need attention. --- ### Blockers **Finding 1: Null `actorId` accepted in production code path** `UserService.createUserOrUpdate(UUID actorId, CreateUserRequest request)` accepts a `null` actorId and the integration test deliberately passes `null`: ```java AppUser actor = transactionTemplate.execute(status -> userService.createUserOrUpdate(null, adminReq)); ``` The test comments this as "bypasses audit logging so no FK issue." This means production code silently accepts a null actor, relying on `AuditService.logAfterCommit` to handle it gracefully. **The risk:** If `AuditService` or the `audit_log` table has a NOT NULL constraint on `actor_id`, bootstrap calls will fail at runtime. If it doesn't, any caller that forgets to pass the actorId (passes null) will silently log an un-attributable event with no error. Un-attributable audit events are a compliance gap — the whole point of an audit trail is knowing *who* did *what*. **Recommendation:** Either make `actorId` `@NonNull` and provide a dedicated `createUserForBootstrap(CreateUserRequest)` without audit, or document that `null` actorId is the explicit "system-initiated" actor with a sentinel UUID (e.g., `SYSTEM_ACTOR_ID = UUID(0, 0)`). The current ambiguity is a security smell. --- **Finding 2: `actorId()` in `UserController` — extra DB round-trip + potential NPE** ```java private UUID actorId(Authentication auth) { return userService.findByEmail(auth.getName()).getId(); } ``` If `findByEmail` returns a user that doesn't exist (e.g., account deleted between auth and request), it will throw `DomainException.notFound`. That's fail-closed and acceptable. However, this is a hidden assumption: the authenticated principal is always in the database. If the `AppUser` entity is the `UserDetails` implementation (which is typical with Spring Security), its `getId()` is already available via `(AppUser) auth.getPrincipal()` without a DB call. This also eliminates the window between auth and actor resolution. --- ### Suggestions **Permission tests cover 403 (authenticated, wrong role) — good.** Consider also adding a test for 401 (unauthenticated) for each of the three endpoints. These are different security failures: one is "not logged in," the other is "logged in but not authorized." Both should be tested per the defense-in-depth principle. ```java @Test void createUser_returns401_whenUnauthenticated() throws Exception { mockMvc.perform(post("/api/users") .contentType(APPLICATION_JSON) .content("{\"email\":\"x@x.com\",\"initialPassword\":\"secret123\"}")) .andExpect(status().isUnauthorized()); } ``` **Audit payload contains email** — this is the right choice for audit trails. Group names (not permission strings) in the payload is also the right call: human-readable, less sensitive than internal IDs, and stable for display in an admin UI.

marcel commented 2026-04-26 15:57:47 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

Verdict: ✅ Approved

This is a solid test addition. The right tools are used at each layer, the async pattern is handled correctly, and coverage matches the implementation. A few observations:

---

What's done well

Awaitility, not Thread.sleep — the integration test uses await().atMost(5, SECONDS).until(...) throughout. This is exactly right for async post-commit audit events. No sleep loops.

No @Transactional on integration test methods — correct. Rolling back the transaction would prevent the logAfterCommit async listener from ever firing. Manual transactionTemplate.execute() is the right pattern here.

@BeforeEach clears the audit log — clean slate before every test, no cross-test contamination.

Unit tests cover all branches: log / don't-log for all three methods. The adminUpdateUser_doesNotLogGroupMembershipChanged_whenGroupsUnchanged and whenGroupIdsIsNull tests are especially important — they verify the no-op path explicitly.

---

Suggestions

Integration test setup is complex and sequential — the test updateUserGroups_producesGroupMembershipChangedEvent has five transactionTemplate.execute() calls with interleaved await() checkpoints to clear state between phases. This is technically correct but fragile under CI load: if the async event fires slowly (GC pause, CI contention), the 5-second timeout will occasionally trip.

Consider using existsByKind with a specific kind and payload constraint to be more precise, or increase the timeout to 10 seconds for the integration tests. The pattern is correct; the timeout budget is tight.

@SuppressWarnings("unchecked") on payload casts — these are unavoidable since Map<String, Object> stores the list as Object. They're correctly scoped to the local variable. No action needed; just noting this is expected.

No test for createUserOrUpdate update path in integration — the integration test only covers the create path (new user → audit entry) and the delete path. There's no integration test for the update-existing-user path. The unit test createUserOrUpdate_doesNotLogUserCreated_whenUserAlreadyExists covers the logic, which is sufficient since the behavior is "emit nothing." LGTM.

existsByKind on AuditLogRepository — used only in integration tests as a wait condition. This is fine; it's a simple derived query. Not a concern.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ✅ Approved** This is a solid test addition. The right tools are used at each layer, the async pattern is handled correctly, and coverage matches the implementation. A few observations: --- ### What's done well **Awaitility, not `Thread.sleep`** — the integration test uses `await().atMost(5, SECONDS).until(...)` throughout. This is exactly right for async post-commit audit events. No sleep loops. **No `@Transactional` on integration test methods** — correct. Rolling back the transaction would prevent the `logAfterCommit` async listener from ever firing. Manual `transactionTemplate.execute()` is the right pattern here. **`@BeforeEach` clears the audit log** — clean slate before every test, no cross-test contamination. **Unit tests cover all branches:** log / don't-log for all three methods. The `adminUpdateUser_doesNotLogGroupMembershipChanged_whenGroupsUnchanged` and `whenGroupIdsIsNull` tests are especially important — they verify the no-op path explicitly. --- ### Suggestions **Integration test setup is complex and sequential** — the test `updateUserGroups_producesGroupMembershipChangedEvent` has five `transactionTemplate.execute()` calls with interleaved `await()` checkpoints to clear state between phases. This is technically correct but fragile under CI load: if the async event fires slowly (GC pause, CI contention), the 5-second timeout will occasionally trip. Consider using `existsByKind` with a specific kind *and* payload constraint to be more precise, or increase the timeout to 10 seconds for the integration tests. The pattern is correct; the timeout budget is tight. **`@SuppressWarnings("unchecked")` on payload casts** — these are unavoidable since `Map<String, Object>` stores the list as `Object`. They're correctly scoped to the local variable. No action needed; just noting this is expected. **No test for `createUserOrUpdate` update path in integration** — the integration test only covers the create path (new user → audit entry) and the delete path. There's no integration test for the update-existing-user path. The unit test `createUserOrUpdate_doesNotLogUserCreated_whenUserAlreadyExists` covers the logic, which is sufficient since the behavior is "emit nothing." LGTM. **`existsByKind` on `AuditLogRepository`** — used only in integration tests as a wait condition. This is fine; it's a simple derived query. Not a concern.

marcel commented 2026-04-26 15:57:59 +02:00

Author

Owner

Copy Link

🏗️ Markus Keller — Application Architect

Verdict: ✅ Approved

The structural choices are sound. I want to flag one dependency direction question and one minor design note.

---

Observations

UserService → AuditService dependency direction

UserService (in service/) now injects AuditService (in audit/). The CLAUDE.md architecture defines the layering as Controller → Service → Repository. The audit/ package sits at the same level as service/, so this is a lateral dependency rather than a layer violation.

For a small codebase this is acceptable — audit is a cross-cutting concern and not a domain boundary in the same sense as PersonService vs DocumentService. If audit grows (audit queries, audit dashboards), consider making AuditService an explicit infrastructure/cross-cutting package with a clearly defined inbound interface, rather than letting every domain service inject it freely. Not a blocker for this PR.

existsByKind on AuditLogRepository is test-only

AuditLogRepository.existsByKind(AuditKind kind) is derived from Spring Data and used exclusively in integration test wait conditions. Adding production repository methods for test-only use is a minor smell. It's also trivially harmless — Spring Data generates it at startup, it's not a security issue, and it's a simple query. I mention it only because it sets a precedent; if it stays test-only, it should eventually move to a test helper or be expressed differently in the await() condition. Not a blocker.

findRecentUserManagementEvents(int limit) is correctly designed

The limit is passed from the caller rather than hardcoded in the service, which is the right call. The query sorts by happenedAt descending. The PR description mentions findRecentUserManagementEvents(3) will be the call site from #326 — the signature is ready.

groupChangePayload compares UUID sets, not entity sets

Set<UUID> beforeIds = before.stream().map(UserGroup::getId).collect(toSet());
Set<UUID> afterIds = after.stream().map(UserGroup::getId).collect(toSet());
if (beforeIds.equals(afterIds)) return Optional.empty();

Correct. Entity equality on Hibernate-managed objects can behave unexpectedly depending on equals/hashCode implementation and proxy state. UUID comparison is deterministic and correct. Good call.

## 🏗️ Markus Keller — Application Architect **Verdict: ✅ Approved** The structural choices are sound. I want to flag one dependency direction question and one minor design note. --- ### Observations **`UserService` → `AuditService` dependency direction** `UserService` (in `service/`) now injects `AuditService` (in `audit/`). The CLAUDE.md architecture defines the layering as `Controller → Service → Repository`. The `audit/` package sits at the same level as `service/`, so this is a lateral dependency rather than a layer violation. For a small codebase this is acceptable — audit is a cross-cutting concern and not a domain boundary in the same sense as `PersonService` vs `DocumentService`. If audit grows (audit queries, audit dashboards), consider making `AuditService` an explicit infrastructure/cross-cutting package with a clearly defined inbound interface, rather than letting every domain service inject it freely. Not a blocker for this PR. **`existsByKind` on `AuditLogRepository` is test-only** `AuditLogRepository.existsByKind(AuditKind kind)` is derived from Spring Data and used exclusively in integration test wait conditions. Adding production repository methods for test-only use is a minor smell. It's also trivially harmless — Spring Data generates it at startup, it's not a security issue, and it's a simple query. I mention it only because it sets a precedent; if it stays test-only, it should eventually move to a test helper or be expressed differently in the `await()` condition. Not a blocker. **`findRecentUserManagementEvents(int limit)` is correctly designed** The `limit` is passed from the caller rather than hardcoded in the service, which is the right call. The query sorts by `happenedAt` descending. The PR description mentions `findRecentUserManagementEvents(3)` will be the call site from #326 — the signature is ready. **`groupChangePayload` compares UUID sets, not entity sets** ```java Set<UUID> beforeIds = before.stream().map(UserGroup::getId).collect(toSet()); Set<UUID> afterIds = after.stream().map(UserGroup::getId).collect(toSet()); if (beforeIds.equals(afterIds)) return Optional.empty(); ``` Correct. Entity equality on Hibernate-managed objects can behave unexpectedly depending on `equals`/`hashCode` implementation and proxy state. UUID comparison is deterministic and correct. Good call.

marcel commented 2026-04-26 15:58:07 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

This is a backend-only PR with no infrastructure, Compose, CI, or migration changes. From a platform perspective, everything is in order.

---

What I checked

No Flyway migration required — the PR description confirms AuditKind uses EnumType.STRING, so the three new enum values (USER_CREATED, USER_DELETED, GROUP_MEMBERSHIP_CHANGED) are stored as plain strings. PostgreSQL doesn't care about new enum values in this case — no DDL change, no migration file needed. Correct.

Integration tests use PostgresContainerConfig — real Postgres via Testcontainers, not H2. The async audit assertions use Awaitility. Both are CI-friendly patterns that won't introduce flakiness from database engine differences.

No new infrastructure dependencies — AuditService was already in the codebase. No new services, no new ports, no Compose changes. The PR stays self-contained.

No secrets or credentials in test fixtures — integration test creates test users with inline passwords ("admin-secret", "secret"). These are isolated to the Testcontainers database and don't persist anywhere. Acceptable for test data.

Nothing to flag from the infrastructure side. Ship it when the code review concerns are addressed.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** This is a backend-only PR with no infrastructure, Compose, CI, or migration changes. From a platform perspective, everything is in order. --- ### What I checked **No Flyway migration required** — the PR description confirms `AuditKind` uses `EnumType.STRING`, so the three new enum values (`USER_CREATED`, `USER_DELETED`, `GROUP_MEMBERSHIP_CHANGED`) are stored as plain strings. PostgreSQL doesn't care about new enum values in this case — no DDL change, no migration file needed. Correct. **Integration tests use `PostgresContainerConfig`** — real Postgres via Testcontainers, not H2. The async audit assertions use Awaitility. Both are CI-friendly patterns that won't introduce flakiness from database engine differences. **No new infrastructure dependencies** — `AuditService` was already in the codebase. No new services, no new ports, no Compose changes. The PR stays self-contained. **No secrets or credentials in test fixtures** — integration test creates test users with inline passwords (`"admin-secret"`, `"secret"`). These are isolated to the Testcontainers database and don't persist anywhere. Acceptable for test data. Nothing to flag from the infrastructure side. Ship it when the code review concerns are addressed.

marcel commented 2026-04-26 15:58:10 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Design Lead

Verdict: ✅ Approved

This PR contains no frontend or UI changes — it is entirely backend: new audit event kinds, service wiring, and tests.

Nothing to flag from a UI, accessibility, or brand perspective. When #326 consumes findRecentUserManagementEvents(3) and exposes these events in the admin dashboard, I'll review the display layer then (audit event labels, timestamp formatting, empty states, truncation on mobile).

LGTM.

## 🎨 Leonie Voss — UX Design Lead **Verdict: ✅ Approved** This PR contains no frontend or UI changes — it is entirely backend: new audit event kinds, service wiring, and tests. Nothing to flag from a UI, accessibility, or brand perspective. When #326 consumes `findRecentUserManagementEvents(3)` and exposes these events in the admin dashboard, I'll review the display layer then (audit event labels, timestamp formatting, empty states, truncation on mobile). LGTM.

marcel commented 2026-04-26 15:58:20 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ⚠️ Approved with concerns

The implemented scope aligns well with what the PR description claims. My job is to flag what the PR doesn't cover that users might reasonably expect from "audit user management events."

---

Requirement Coverage Assessment

Covered:

• ✅ USER_CREATED — only for new users (not upsert updates) — intentional and documented in PR description
• ✅ USER_DELETED — with email captured before deletion
• ✅ GROUP_MEMBERSHIP_CHANGED — with added/removed group names, only when group set actually changes
• ✅ Actor attribution (who performed the action)
• ✅ Audit entries only persist if transaction commits (logAfterCommit)
• ✅ Query API ready for #326 (findRecentUserManagementEvents)

---

Gap: No audit event for non-group admin updates

adminUpdateUser emits GROUP_MEMBERSHIP_CHANGED only when group membership changes. When an admin changes a user's email, password, display name, or contact info, no audit event is emitted.

From an admin's perspective: if a user's email is changed by another admin, there is no audit trail for that action. This is a meaningful compliance gap for a system with role-based access control.

This may be intentional scope for issue #336 — the PR description doesn't explicitly address non-group user updates. If so, I'd recommend opening a follow-up issue for USER_PROFILE_UPDATED (or similar) before this feature reaches users who rely on the audit trail for accountability.

Open question: Does the acceptance criteria for #336 explicitly exclude non-group admin updates from audit scope? If yes, this is correctly deferred. If the criteria are silent on it, this is a gap.

---

Requirement Traceability Note

The PR description mentions findRecentUserManagementEvents(3) is "ready for #326 to consume." This forward dependency should be explicitly tracked: #326 is blocked on this PR. Ensure #326 has a "depends on #337" link in Gitea so it doesn't get scheduled out of order.

## 📋 Elicit — Requirements Engineer **Verdict: ⚠️ Approved with concerns** The implemented scope aligns well with what the PR description claims. My job is to flag what the PR *doesn't* cover that users might reasonably expect from "audit user management events." --- ### Requirement Coverage Assessment **Covered:** - ✅ `USER_CREATED` — only for new users (not upsert updates) — intentional and documented in PR description - ✅ `USER_DELETED` — with email captured before deletion - ✅ `GROUP_MEMBERSHIP_CHANGED` — with added/removed group names, only when group set actually changes - ✅ Actor attribution (who performed the action) - ✅ Audit entries only persist if transaction commits (`logAfterCommit`) - ✅ Query API ready for #326 (`findRecentUserManagementEvents`) --- ### Gap: No audit event for non-group admin updates `adminUpdateUser` emits `GROUP_MEMBERSHIP_CHANGED` only when group membership changes. When an admin changes a user's **email, password, display name, or contact info**, no audit event is emitted. From an admin's perspective: if a user's email is changed by another admin, there is no audit trail for that action. This is a meaningful compliance gap for a system with role-based access control. This may be intentional scope for issue #336 — the PR description doesn't explicitly address non-group user updates. If so, I'd recommend opening a follow-up issue for `USER_PROFILE_UPDATED` (or similar) before this feature reaches users who rely on the audit trail for accountability. **Open question:** Does the acceptance criteria for #336 explicitly exclude non-group admin updates from audit scope? If yes, this is correctly deferred. If the criteria are silent on it, this is a gap. --- ### Requirement Traceability Note The PR description mentions `findRecentUserManagementEvents(3)` is "ready for #326 to consume." This forward dependency should be explicitly tracked: #326 is blocked on this PR. Ensure #326 has a "depends on #337" link in Gitea so it doesn't get scheduled out of order.

marcel added 3 commits 2026-04-26 17:47:51 +02:00

refactor(audit): extract createUserForBootstrap() to make null actorId contract explicit ... 6d9910b805

createUserOrUpdate(UUID actorId, ...) is always called from the controller with
a real authenticated actor. createUserForBootstrap() handles seeding/test setup
without emitting an audit event, making the two contracts unambiguous.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(audit): replace null-actorId bootstrap calls with createUserForBootstrap(), increase timeouts to 10s ... a6c8af0971

Removes the wait+clear cycles that existed only to drain the audit events
emitted by createUserOrUpdate(null, ...). Timeouts increased 5 → 10 s to
reduce CI flakiness under load.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(audit): add 401 unauthenticated tests for createUser, adminUpdateUser, deleteUser ... ce41e96a45

Regression guards verifying that Spring Security returns 401 (not 200) when
no credentials are provided, complementing the existing 403 permission tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-04-26 17:48:13 +02:00

Author

Owner

Copy Link

Second-round review concerns addressed

All open blockers and actionable suggestions from the second review round have been resolved.

---

Blockers resolved

Nora — Finding 1: null actorId accepted in production code path ✅
Extracted createUserForBootstrap(CreateUserRequest) — no actorId parameter, no audit event — for use in tests and seeding scenarios. createUserOrUpdate(UUID actorId, ...) remains for controller use and always receives a real authenticated actor. The two contracts are now unambiguous.
Unit test added: createUserForBootstrap_createsUserWithoutAuditEvent verifies auditService.logAfterCommit() is never called.
→ commit refactor(audit): extract createUserForBootstrap() to make null actorId contract explicit

Nora — Finding 2: actorId() DB lookup — cast-principal suggestion 🚫 Not applicable
CustomUserDetailsService returns a Spring Security User object, not AppUser. Casting (AppUser) auth.getPrincipal() would throw ClassCastException. The current DB lookup in actorId(Authentication) is the correct approach for this architecture. No change made.

---

Suggestions implemented

Nora — 401 tests for unauthenticated access ✅
Added createUser_returns401_whenUnauthenticated, adminUpdateUser_returns401_whenUnauthenticated, and deleteUser_returns401_whenUnauthenticated to UserControllerTest. These complement the existing 403 permission tests and guard against any future regression that opens the endpoints to unauthenticated callers.
→ commit test(audit): add 401 unauthenticated tests for createUser, adminUpdateUser, deleteUser

Sara — Integration test setup simplification + timeout increase ✅
With createUserForBootstrap() no longer emitting an audit event, the two await(...existsByKind(USER_CREATED)) + deleteAll() cycles that existed only to drain bootstrap noise are removed. All remaining Awaitility waits increased from 5 → 10 seconds.
→ commit test(audit): replace null-actorId bootstrap calls with createUserForBootstrap(), increase timeouts to 10s

---

Deferred (out of scope per #336)

Elicit — No audit event for non-group admin updates
Issue #336 explicitly excludes USER_UPDATED: "log only access-control-relevant events for now." A follow-up issue should be opened for USER_PROFILE_UPDATED if audit coverage of email/password/profile changes is needed before the audit dashboard ships.

---

Test suite

1371 tests, 0 failures (+5 net: 1 × createUserForBootstrap unit test + 3 × 401 controller tests + integration test simplified but count unchanged).

## Second-round review concerns addressed All open blockers and actionable suggestions from the second review round have been resolved. --- ### Blockers resolved **Nora — Finding 1: null `actorId` accepted in production code path** ✅ Extracted `createUserForBootstrap(CreateUserRequest)` — no actorId parameter, no audit event — for use in tests and seeding scenarios. `createUserOrUpdate(UUID actorId, ...)` remains for controller use and always receives a real authenticated actor. The two contracts are now unambiguous. Unit test added: `createUserForBootstrap_createsUserWithoutAuditEvent` verifies `auditService.logAfterCommit()` is never called. → commit `refactor(audit): extract createUserForBootstrap() to make null actorId contract explicit` **Nora — Finding 2: `actorId()` DB lookup — cast-principal suggestion** 🚫 Not applicable `CustomUserDetailsService` returns a Spring Security `User` object, not `AppUser`. Casting `(AppUser) auth.getPrincipal()` would throw `ClassCastException`. The current DB lookup in `actorId(Authentication)` is the correct approach for this architecture. No change made. --- ### Suggestions implemented **Nora — 401 tests for unauthenticated access** ✅ Added `createUser_returns401_whenUnauthenticated`, `adminUpdateUser_returns401_whenUnauthenticated`, and `deleteUser_returns401_whenUnauthenticated` to `UserControllerTest`. These complement the existing 403 permission tests and guard against any future regression that opens the endpoints to unauthenticated callers. → commit `test(audit): add 401 unauthenticated tests for createUser, adminUpdateUser, deleteUser` **Sara — Integration test setup simplification + timeout increase** ✅ With `createUserForBootstrap()` no longer emitting an audit event, the two `await(...existsByKind(USER_CREATED)) + deleteAll()` cycles that existed only to drain bootstrap noise are removed. All remaining Awaitility waits increased from 5 → 10 seconds. → commit `test(audit): replace null-actorId bootstrap calls with createUserForBootstrap(), increase timeouts to 10s` --- ### Deferred (out of scope per #336) **Elicit — No audit event for non-group admin updates** Issue #336 explicitly excludes `USER_UPDATED`: "log only access-control-relevant events for now." A follow-up issue should be opened for `USER_PROFILE_UPDATED` if audit coverage of email/password/profile changes is needed before the audit dashboard ships. --- ### Test suite 1371 tests, 0 failures (+5 net: 1 × `createUserForBootstrap` unit test + 3 × 401 controller tests + integration test simplified but count unchanged).

marcel merged commit ce41e96a45 into main 2026-04-26 19:42:00 +02:00

marcel deleted branch feat/issue-336-audit-user-management 2026-04-26 19:42:01 +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#337