feat(audit): track user management events in audit log (USER_CREATED, USER_DELETED, GROUP_MEMBERSHIP_CHANGED) #336

New Issue

marcel · 2026-04-26T14:17:33+02:00

marcel commented

2026-04-26 14:17:33 +02:00

Context

The AuditKind enum currently covers only document/content events (FILE_UPLOADED, STATUS_CHANGED, TEXT_SAVED, etc.). User management actions — creating a user, deleting a user, changing group membership — leave no audit trail.

This blocks two things:

The "recent activity" row in the admin users empty state (#326) — deferred here because it requires this infrastructure first.
Any future admin observability feature that needs to know who was added when, or who gained/lost permissions.

Discovered and deferred during the #326 persona review (see comment).

User story

As an admin, I want to see which users were recently created or had their group membership changed, so that I can keep track of access control changes without trawling through server logs.

Scope

In scope:

New AuditKind values: USER_CREATED, USER_DELETED, GROUP_MEMBERSHIP_CHANGED
Audit logging calls in UserService at the relevant write methods
A query method in AuditLogQueryService to retrieve the N most recent user-management events
Unit tests for the new logging calls; integration test for the query

Out of scope:

Surfacing these events in any UI — that is #326's job once this is done
USER_UPDATED (profile edits, password changes) — log only access-control-relevant events for now
Admin dashboard aggregations — that is #324

Payload design

Each new AuditKind value must follow the existing docstring convention in AuditKind.java. Payloads must never include sensitive data:

Kind	Payload fields	Excluded
`USER_CREATED`	`userId`, `email`, `invitedById` (nullable)	password hash, session
`USER_DELETED`	`userId`, `email`	—
`GROUP_MEMBERSHIP_CHANGED`	`userId`, `email`, `addedGroups: [name]`, `removedGroups: [name]`	raw permission strings

Group names (not permission strings) are the right level of abstraction — they're human-readable and don't expose the internal Set<String> permissions values.

Implementation notes

New AuditKind values go in audit/AuditKind.java with payload docstrings following the existing pattern.
Logging calls go in UserService (constructor-injecting AuditService):
- createUserOrUpdate() → emit USER_CREATED
- deleteUser() → emit USER_DELETED
- adminUpdateUser() → emit GROUP_MEMBERSHIP_CHANGED if the group set changed (compare before/after)
New query method in AuditLogQueryService: findRecentUserManagementEvents(int limit) — filters by the three new AuditKind values, ordered by createdAt DESC.
No schema change needed if AuditLog already stores kind as a string column (Flyway migration only if a new table or column is required — check first).

Acceptance criteria

USER_CREATED is logged when UserService.createUserOrUpdate() creates a new user (not on update of existing)
USER_DELETED is logged when UserService.deleteUser() is called
GROUP_MEMBERSHIP_CHANGED is logged when UserService.adminUpdateUser() results in a change to the user's group set; not logged if groups are unchanged
Payloads contain no passwords, no raw permission strings, no session tokens
AuditLogQueryService.findRecentUserManagementEvents(n) returns the n most recent events of these three kinds, ordered newest-first
All new logging calls are covered by unit tests (mock AuditService, assert log() is called with correct AuditKind and payload)
Integration test: create user → delete user → query → assert both events appear in correct order
Existing tests are not broken

#326 — admin empty states: the users empty state will consume findRecentUserManagementEvents(3) once this ships
#324 — admin dashboard: may also consume these events for access-control activity widgets

## Context The `AuditKind` enum currently covers only document/content events (`FILE_UPLOADED`, `STATUS_CHANGED`, `TEXT_SAVED`, etc.). User management actions — creating a user, deleting a user, changing group membership — leave no audit trail. This blocks two things: 1. The "recent activity" row in the admin users empty state (#326) — deferred here because it requires this infrastructure first. 2. Any future admin observability feature that needs to know who was added when, or who gained/lost permissions. Discovered and deferred during the #326 persona review (see [comment](http://heim-nas:3005/marcel/familienarchiv/issues/326#issuecomment-4716)). ## User story As an **admin**, I want to see which users were recently created or had their group membership changed, so that I can keep track of access control changes without trawling through server logs. ## Scope **In scope:** - New `AuditKind` values: `USER_CREATED`, `USER_DELETED`, `GROUP_MEMBERSHIP_CHANGED` - Audit logging calls in `UserService` at the relevant write methods - A query method in `AuditLogQueryService` to retrieve the N most recent user-management events - Unit tests for the new logging calls; integration test for the query **Out of scope:** - Surfacing these events in any UI — that is #326's job once this is done - `USER_UPDATED` (profile edits, password changes) — log only access-control-relevant events for now - Admin dashboard aggregations — that is #324 ## Payload design Each new `AuditKind` value must follow the existing docstring convention in `AuditKind.java`. Payloads must never include sensitive data: | Kind | Payload fields | Excluded | |---|---|---| | `USER_CREATED` | `userId`, `email`, `invitedById` (nullable) | password hash, session | | `USER_DELETED` | `userId`, `email` | — | | `GROUP_MEMBERSHIP_CHANGED` | `userId`, `email`, `addedGroups: [name]`, `removedGroups: [name]` | raw permission strings | Group names (not permission strings) are the right level of abstraction — they're human-readable and don't expose the internal `Set<String> permissions` values. ## Implementation notes - New `AuditKind` values go in `audit/AuditKind.java` with payload docstrings following the existing pattern. - Logging calls go in `UserService` (constructor-injecting `AuditService`): - `createUserOrUpdate()` → emit `USER_CREATED` - `deleteUser()` → emit `USER_DELETED` - `adminUpdateUser()` → emit `GROUP_MEMBERSHIP_CHANGED` if the group set changed (compare before/after) - New query method in `AuditLogQueryService`: `findRecentUserManagementEvents(int limit)` — filters by the three new `AuditKind` values, ordered by `createdAt DESC`. - No schema change needed if `AuditLog` already stores `kind` as a string column (Flyway migration only if a new table or column is required — check first). ## Acceptance criteria - [ ] `USER_CREATED` is logged when `UserService.createUserOrUpdate()` creates a new user (not on update of existing) - [ ] `USER_DELETED` is logged when `UserService.deleteUser()` is called - [ ] `GROUP_MEMBERSHIP_CHANGED` is logged when `UserService.adminUpdateUser()` results in a change to the user's group set; not logged if groups are unchanged - [ ] Payloads contain no passwords, no raw permission strings, no session tokens - [ ] `AuditLogQueryService.findRecentUserManagementEvents(n)` returns the n most recent events of these three kinds, ordered newest-first - [ ] All new logging calls are covered by unit tests (mock `AuditService`, assert `log()` is called with correct `AuditKind` and payload) - [ ] Integration test: create user → delete user → query → assert both events appear in correct order - [ ] Existing tests are not broken ## Related - #326 — admin empty states: the users empty state will consume `findRecentUserManagementEvents(3)` once this ships - #324 — admin dashboard: may also consume these events for access-control activity widgets

marcel added the P3-later feature labels 2026-04-26 14:17:40 +02:00

marcel commented

2026-04-26 14:27:29 +02:00

🏛️ Markus Keller — Senior Application Architect

Observations

No schema change needed — confirmed. AuditLog.kind uses @Enumerated(EnumType.STRING) which stores enum names as varchar. Adding new enum values requires zero Flyway migration. The issue's assessment is correct.
documentId will be null for all three new event kinds. The existing AuditLog entity has documentId as nullable. The AuditService.log() signature is log(kind, actorId, documentId, payload) — callers will pass null as documentId for user events. This is a minor code smell (the parameter name implies document context), but adding an overload or a separate column solely for user events would be over-engineering. The existing design handles it.
logAfterCommit is the right call, not log(). All existing write paths in DocumentService, TranscriptionService, CommentService use logAfterCommit so the audit entry is only written after the transaction commits successfully. User creation/deletion/group-change must follow the same pattern — otherwise a rolled-back user creation would still produce a USER_CREATED entry.
findRecentUserManagementEvents placement. Adding this to AuditLogQueryService is correct — consistent with findActivityFeed, getPulseStats, and other query methods already there. Do not add it to AuditLogQueryRepository directly without going through the service layer.
UserService lives in service/, AuditService in audit/ — cross-package dependency is acceptable since AuditService is infrastructure, not a peer domain service. Constructor-inject AuditService into UserService via @RequiredArgsConstructor as all other services do.

Recommendations

Accept the null documentId for user events. It's supported by the schema, documented in the payload, and doesn't justify a new column or method overload. The pattern is already set by AuditService.logAfterCommit(BLOCK_REVIEWED, userId, documentId, null) which also passes a null payload.
Always use logAfterCommit, not log(), for user management events — they are called from @Transactional methods and must not emit audit entries for rolled-back operations.
Keep findRecentUserManagementEvents in AuditLogQueryService, not in the repository interface directly. The repository should expose raw query methods; the service composes and returns typed results.

## 🏛️ Markus Keller — Senior Application Architect ### Observations - **No schema change needed — confirmed.** `AuditLog.kind` uses `@Enumerated(EnumType.STRING)` which stores enum names as varchar. Adding new enum values requires zero Flyway migration. The issue's assessment is correct. - **`documentId` will be `null` for all three new event kinds.** The existing `AuditLog` entity has `documentId` as nullable. The `AuditService.log()` signature is `log(kind, actorId, documentId, payload)` — callers will pass `null` as `documentId` for user events. This is a minor code smell (the parameter name implies document context), but adding an overload or a separate column solely for user events would be over-engineering. The existing design handles it. - **`logAfterCommit` is the right call, not `log()`.** All existing write paths in `DocumentService`, `TranscriptionService`, `CommentService` use `logAfterCommit` so the audit entry is only written after the transaction commits successfully. User creation/deletion/group-change must follow the same pattern — otherwise a rolled-back user creation would still produce a `USER_CREATED` entry. - **`findRecentUserManagementEvents` placement.** Adding this to `AuditLogQueryService` is correct — consistent with `findActivityFeed`, `getPulseStats`, and other query methods already there. Do not add it to `AuditLogQueryRepository` directly without going through the service layer. - **`UserService` lives in `service/`, `AuditService` in `audit/` — cross-package dependency is acceptable** since `AuditService` is infrastructure, not a peer domain service. Constructor-inject `AuditService` into `UserService` via `@RequiredArgsConstructor` as all other services do. ### Recommendations - **Accept the `null` documentId for user events.** It's supported by the schema, documented in the payload, and doesn't justify a new column or method overload. The pattern is already set by `AuditService.logAfterCommit(BLOCK_REVIEWED, userId, documentId, null)` which also passes a `null` payload. - **Always use `logAfterCommit`, not `log()`,** for user management events — they are called from `@Transactional` methods and must not emit audit entries for rolled-back operations. - **Keep `findRecentUserManagementEvents` in `AuditLogQueryService`,** not in the repository interface directly. The repository should expose raw query methods; the service composes and returns typed results.

marcel commented

2026-04-26 14:27:45 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

UserService has two user-creation paths — the issue only covers one. createUserOrUpdate() (create-or-update semantics) is mentioned. createUser() (line 70, throws EMAIL_ALREADY_IN_USE on duplicate) is a separate method that also creates users but is not mentioned. Looking at the controller, createUser() appears to be called from invite flows. If createUser() is ever triggered by an admin action, it must also emit USER_CREATED. The issue should clarify whether both paths need coverage.

adminUpdateUser() group comparison — the existing method mutates user.getGroups() via user.setGroups(newGroups) at line 169. To detect a change, beforeGroupIds must be captured before the setGroups call:

Set<UUID> beforeGroupIds = user.getGroups().stream()
    .map(UserGroup::getId).collect(toSet());
// ... mutate ...
Set<UUID> afterGroupIds = newGroups.stream()
    .map(UserGroup::getId).collect(toSet());
if (!beforeGroupIds.equals(afterGroupIds)) {
    auditService.logAfterCommit(GROUP_MEMBERSHIP_CHANGED, actorId, null, Map.of(...));
}

Capturing after setGroups will always compare new vs. new.

deleteUser() correctly fetches the user before deleting — email is available on the AppUser object at line 99 before userRepository.delete(user). The payload can be populated before deletion without needing a separate lookup.
Actor ID is not threaded through the three UserService methods. All existing callers of AuditService in DocumentService, TranscriptionService, and CommentService receive actorId as a method parameter. UserService.createUserOrUpdate(), deleteUser(), and adminUpdateUser() currently take no actor. An actorId parameter needs to be added (or fetched from SecurityContextHolder inside the service — see DocumentVersionService for that pattern). The issue doesn't specify which approach to use.
Test for createUserOrUpdate — the update branch must not emit USER_CREATED. The conditional is already in the method (existingUser.isPresent()). The unit test should cover both branches: one asserting log() is called, one asserting it is NOT called.

Recommendations

Add actorId as a method parameter to all three service methods — consistent with DocumentService which receives actorId from its controller callers. Avoids hidden SecurityContextHolder access in the service layer and keeps services testable without mocking Spring Security context.
Capture beforeGroupIds immediately after getById(id) returns, before any mutation. One line, no extra query.
Clarify whether createUser() also needs USER_CREATED logging. Looking at InviteController, it calls createUser() — if invite flows count as user management, it should log too.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - **`UserService` has two user-creation paths — the issue only covers one.** `createUserOrUpdate()` (create-or-update semantics) is mentioned. `createUser()` (line 70, throws `EMAIL_ALREADY_IN_USE` on duplicate) is a separate method that also creates users but is not mentioned. Looking at the controller, `createUser()` appears to be called from invite flows. If `createUser()` is ever triggered by an admin action, it must also emit `USER_CREATED`. The issue should clarify whether both paths need coverage. - **`adminUpdateUser()` group comparison — the existing method mutates `user.getGroups()` via `user.setGroups(newGroups)` at line 169.** To detect a change, `beforeGroupIds` must be captured before the `setGroups` call: ```java Set<UUID> beforeGroupIds = user.getGroups().stream() .map(UserGroup::getId).collect(toSet()); // ... mutate ... Set<UUID> afterGroupIds = newGroups.stream() .map(UserGroup::getId).collect(toSet()); if (!beforeGroupIds.equals(afterGroupIds)) { auditService.logAfterCommit(GROUP_MEMBERSHIP_CHANGED, actorId, null, Map.of(...)); } ``` Capturing after `setGroups` will always compare new vs. new. - **`deleteUser()` correctly fetches the user before deleting** — `email` is available on the `AppUser` object at line 99 before `userRepository.delete(user)`. The payload can be populated before deletion without needing a separate lookup. - **Actor ID is not threaded through** the three `UserService` methods. All existing callers of `AuditService` in `DocumentService`, `TranscriptionService`, and `CommentService` receive `actorId` as a method parameter. `UserService.createUserOrUpdate()`, `deleteUser()`, and `adminUpdateUser()` currently take no actor. An `actorId` parameter needs to be added (or fetched from `SecurityContextHolder` inside the service — see `DocumentVersionService` for that pattern). The issue doesn't specify which approach to use. - **Test for `createUserOrUpdate` — the update branch must not emit `USER_CREATED`.** The conditional is already in the method (`existingUser.isPresent()`). The unit test should cover both branches: one asserting `log()` is called, one asserting it is NOT called. ### Recommendations - **Add `actorId` as a method parameter to all three service methods** — consistent with `DocumentService` which receives actorId from its controller callers. Avoids hidden `SecurityContextHolder` access in the service layer and keeps services testable without mocking Spring Security context. - **Capture `beforeGroupIds` immediately after `getById(id)` returns**, before any mutation. One line, no extra query. - **Clarify whether `createUser()` also needs `USER_CREATED` logging.** Looking at `InviteController`, it calls `createUser()` — if invite flows count as user management, it should log too.

marcel commented

2026-04-26 14:28:01 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

Payload design correctly excludes the dangerous data. No passwords, no session tokens, no raw Set<String> permissions. The issue is explicit about this. The GROUP_MEMBERSHIP_CHANGED payload using group names (not permission strings) is the right abstraction — group names are human-readable and don't expose the internal permission model.
actorId is unresolved — and this matters for the audit trail. The three UserService methods currently accept no caller identity. AuditService.log() takes actorId as a top-level field (not part of the payload). If actorId is null, audit entries record what happened but not who did it — which defeats the purpose of access control auditing. The issue mentions invitedById (nullable) in the USER_CREATED payload, but actorId (the admin performing the creation) must be resolved separately and passed as the top-level field, not buried in the payload.
deleteUser audit entry must be written before deletion. deleteUser() fetches the user first (line 99), so user.getEmail() is available. The logAfterCommit call must be set up (synchronization registered) before userRepository.delete(user) executes — since logAfterCommit registers a TransactionSynchronization, this works correctly as long as the call is placed before the commit, which it will be if inside the same @Transactional method.
The GROUP_MEMBERSHIP_CHANGED payload includes addedGroups and removedGroups as name lists. Implementation must extract group.getName(), not iterate group.getPermissions(). A subtle mistake here (e.g., logging group.getPermissions() instead of group.getName()) would leak the internal permission strings the issue explicitly excludes. This should be verified in a test that asserts the payload shape.
Audit log access control: findRecentUserManagementEvents will be consumed by the admin users empty state (#326). The admin endpoint is already protected by @RequirePermission(Permission.ADMIN_USER) — any new endpoint exposing these audit records must carry the same annotation. No concern for this issue specifically, but the consuming PR (#326) must enforce it.
invitedById in the USER_CREATED payload — there is no "invitation" entity in the current codebase. InviteController exists but uses createUser(), not createUserOrUpdate(). If invitedById refers to the admin creating the user (the actor), it duplicates actorId. Recommend removing invitedById from the payload and relying on the top-level actorId field instead — simpler, consistent, non-redundant.

Recommendations

Test that GROUP_MEMBERSHIP_CHANGED payload contains group names, not permission strings. The assertion should be: payload.get("addedGroups") contains ["Admins"], not ["ADMIN", "WRITE_ALL"]. One test, one assertion, prevents the wrong field from ever shipping.
Drop invitedById from the USER_CREATED payload spec — use top-level actorId (the performing admin's ID) instead. A nullable invitedById duplicates what actorId already expresses.
Resolve the actorId threading gap before implementation starts. Either add actorId as a method parameter (preferred) or use SecurityContextHolder inside the service. Not resolving this means the audit entries will have a null actor — essentially useless for access control investigation.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - **Payload design correctly excludes the dangerous data.** No passwords, no session tokens, no raw `Set<String> permissions`. The issue is explicit about this. The `GROUP_MEMBERSHIP_CHANGED` payload using group names (not permission strings) is the right abstraction — group names are human-readable and don't expose the internal permission model. - **`actorId` is unresolved — and this matters for the audit trail.** The three `UserService` methods currently accept no caller identity. `AuditService.log()` takes `actorId` as a top-level field (not part of the payload). If `actorId` is null, audit entries record *what* happened but not *who* did it — which defeats the purpose of access control auditing. The issue mentions `invitedById (nullable)` in the `USER_CREATED` payload, but `actorId` (the admin performing the creation) must be resolved separately and passed as the top-level field, not buried in the payload. - **`deleteUser` audit entry must be written before deletion.** `deleteUser()` fetches the user first (line 99), so `user.getEmail()` is available. The `logAfterCommit` call must be set up (synchronization registered) before `userRepository.delete(user)` executes — since `logAfterCommit` registers a `TransactionSynchronization`, this works correctly as long as the call is placed before the commit, which it will be if inside the same `@Transactional` method. - **The `GROUP_MEMBERSHIP_CHANGED` payload includes `addedGroups` and `removedGroups` as name lists.** Implementation must extract `group.getName()`, not iterate `group.getPermissions()`. A subtle mistake here (e.g., logging `group.getPermissions()` instead of `group.getName()`) would leak the internal permission strings the issue explicitly excludes. This should be verified in a test that asserts the payload shape. - **Audit log access control:** `findRecentUserManagementEvents` will be consumed by the admin users empty state (#326). The admin endpoint is already protected by `@RequirePermission(Permission.ADMIN_USER)` — any new endpoint exposing these audit records must carry the same annotation. No concern for this issue specifically, but the consuming PR (#326) must enforce it. - **`invitedById` in the `USER_CREATED` payload** — there is no "invitation" entity in the current codebase. `InviteController` exists but uses `createUser()`, not `createUserOrUpdate()`. If `invitedById` refers to the admin creating the user (the actor), it duplicates `actorId`. Recommend removing `invitedById` from the payload and relying on the top-level `actorId` field instead — simpler, consistent, non-redundant. ### Recommendations - **Test that `GROUP_MEMBERSHIP_CHANGED` payload contains group names, not permission strings.** The assertion should be: `payload.get("addedGroups")` contains `["Admins"]`, not `["ADMIN", "WRITE_ALL"]`. One test, one assertion, prevents the wrong field from ever shipping. - **Drop `invitedById` from the `USER_CREATED` payload spec** — use top-level `actorId` (the performing admin's ID) instead. A nullable `invitedById` duplicates what `actorId` already expresses. - **Resolve the `actorId` threading gap before implementation starts.** Either add `actorId` as a method parameter (preferred) or use `SecurityContextHolder` inside the service. Not resolving this means the audit entries will have a null actor — essentially useless for access control investigation.

marcel commented

2026-04-26 14:28:15 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

The AC test coverage plan is sound at the unit level. Mock AuditService, assert log() is called with the correct AuditKind and payload — this is the right layer. UserServiceTest.java already exists; extend it rather than creating a new file.
The AC is missing a critical negative test: "not logged if groups are unchanged" is stated for GROUP_MEMBERSHIP_CHANGED, but there's no equivalent negative test for createUserOrUpdate (update branch → must not emit USER_CREATED). Both negatives must be explicit test cases.
Integration test scope is correct — create user, delete user, query, assert both events appear in correct order. This needs a real PostgreSQL container (Testcontainers), consistent with AuditLogQueryRepositoryIntegrationTest and AuditServiceIntegrationTest which already exist.
GROUP_MEMBERSHIP_CHANGED payload shape test is missing from the AC. The unit test should assert not just that log() is called, but that the payload map contains addedGroups: ["GroupName"] and removedGroups: ["GroupName"] — not permission strings. One assertion, prevents a wrong field from shipping silently.
deleteUser test must capture email before delete. The test should assert that the payload email field matches the deleted user's email. Since userRepository.delete(user) runs before the transaction commits, the email must be captured in the payload before the delete call.
Async logging boundary. AuditService.logAfterCommit registers a TransactionSynchronization — it fires after commit. Unit tests that mock AuditService will verify the call is made correctly. The integration test verifies the entry actually lands in the database.

Recommendations

Explicit test list I'd expect in UserServiceTest:

createUserOrUpdate_logsUserCreated_whenUserIsNew — assert logAfterCommit called with USER_CREATED
createUserOrUpdate_doesNotLogUserCreated_whenUserAlreadyExists — assert logAfterCommit NOT called
deleteUser_logsUserDeleted_withEmailInPayload — assert logAfterCommit called, payload contains email
adminUpdateUser_logsGroupMembershipChanged_whenGroupSetChanges — assert called, payload contains group names (not permissions)
adminUpdateUser_doesNotLogGroupMembershipChanged_whenGroupsUnchanged — assert NOT called
adminUpdateUser_doesNotLogGroupMembershipChanged_whenGroupIdsIsNull — assert NOT called (the if (dto.getGroupIds() != null) branch)

For the integration test in AuditLogQueryServiceTest or a new UserManagementAuditIntegrationTest:

Create user → delete user → findRecentUserManagementEvents(10) → assert 2 events, USER_DELETED is first (newest), USER_CREATED is second.

Verify the payload shape in test #3 and #4 with direct assertions on the Map argument captured via Mockito ArgumentCaptor.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - **The AC test coverage plan is sound at the unit level.** Mock `AuditService`, assert `log()` is called with the correct `AuditKind` and payload — this is the right layer. `UserServiceTest.java` already exists; extend it rather than creating a new file. - **The AC is missing a critical negative test:** "not logged if groups are unchanged" is stated for `GROUP_MEMBERSHIP_CHANGED`, but there's no equivalent negative test for `createUserOrUpdate` (update branch → must not emit `USER_CREATED`). Both negatives must be explicit test cases. - **Integration test scope is correct** — create user, delete user, query, assert both events appear in correct order. This needs a real PostgreSQL container (Testcontainers), consistent with `AuditLogQueryRepositoryIntegrationTest` and `AuditServiceIntegrationTest` which already exist. - **`GROUP_MEMBERSHIP_CHANGED` payload shape test is missing from the AC.** The unit test should assert not just that `log()` is called, but that the payload map contains `addedGroups: ["GroupName"]` and `removedGroups: ["GroupName"]` — not permission strings. One assertion, prevents a wrong field from shipping silently. - **`deleteUser` test must capture email before delete.** The test should assert that the payload `email` field matches the deleted user's email. Since `userRepository.delete(user)` runs before the transaction commits, the email must be captured in the payload before the delete call. - **Async logging boundary.** `AuditService.logAfterCommit` registers a `TransactionSynchronization` — it fires after commit. Unit tests that mock `AuditService` will verify the call is made correctly. The integration test verifies the entry actually lands in the database. ### Recommendations **Explicit test list I'd expect in `UserServiceTest`:** 1. `createUserOrUpdate_logsUserCreated_whenUserIsNew` — assert `logAfterCommit` called with `USER_CREATED` 2. `createUserOrUpdate_doesNotLogUserCreated_whenUserAlreadyExists` — assert `logAfterCommit` NOT called 3. `deleteUser_logsUserDeleted_withEmailInPayload` — assert `logAfterCommit` called, payload contains `email` 4. `adminUpdateUser_logsGroupMembershipChanged_whenGroupSetChanges` — assert called, payload contains group names (not permissions) 5. `adminUpdateUser_doesNotLogGroupMembershipChanged_whenGroupsUnchanged` — assert NOT called 6. `adminUpdateUser_doesNotLogGroupMembershipChanged_whenGroupIdsIsNull` — assert NOT called (the `if (dto.getGroupIds() != null)` branch) **For the integration test in `AuditLogQueryServiceTest` or a new `UserManagementAuditIntegrationTest`:** - Create user → delete user → `findRecentUserManagementEvents(10)` → assert 2 events, `USER_DELETED` is first (newest), `USER_CREATED` is second. **Verify the payload shape** in test #3 and #4 with direct assertions on the `Map` argument captured via Mockito `ArgumentCaptor`.

marcel commented

2026-04-26 14:28:24 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

No infrastructure work in this issue. New enum values in AuditKind work without a Flyway migration because kind is stored as EnumType.STRING. The issue's assessment is correct — confirmed by reading AuditLog.java directly.
auditExecutor thread pool already exists. AuditService uses @Qualifier("auditExecutor") which is wired in AsyncConfig. Three new logAfterCommit calls use this same executor — no new thread pool, no config change.
No Docker Compose, CI, or deployment changes needed. This is a pure service-layer change with no new infrastructure dependencies (no new table, no new service, no new config property).
Flyway check: the latest migration is V53__add_thumbnail_aspect_and_page_count.sql. The next migration, if one is needed, would be V54__.... This issue requires none — but any future change to AuditLog (e.g., adding a target_user_id column) would need V54. Worth knowing the current sequence.
Async logging failure handling is already correct. AuditService.writeLog() catches all exceptions and logs them without rethrowing — audit failures never propagate to the caller. This is the right behavior; user creation should not fail because the audit log is temporarily unavailable.

Recommendations

No changes needed from the infrastructure side.
If the issue is later extended to add a target_user_id column (to make querying by affected user efficient), that would need V54__add_target_user_id_to_audit_log.sql with ALTER TABLE audit_log ADD COLUMN target_user_id UUID REFERENCES users(id) ON DELETE SET NULL. Not in scope now, but worth anticipating for #326.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations - **No infrastructure work in this issue.** New enum values in `AuditKind` work without a Flyway migration because `kind` is stored as `EnumType.STRING`. The issue's assessment is correct — confirmed by reading `AuditLog.java` directly. - **`auditExecutor` thread pool already exists.** `AuditService` uses `@Qualifier("auditExecutor")` which is wired in `AsyncConfig`. Three new `logAfterCommit` calls use this same executor — no new thread pool, no config change. - **No Docker Compose, CI, or deployment changes needed.** This is a pure service-layer change with no new infrastructure dependencies (no new table, no new service, no new config property). - **Flyway check:** the latest migration is `V53__add_thumbnail_aspect_and_page_count.sql`. The next migration, if one is needed, would be `V54__...`. This issue requires none — but any future change to `AuditLog` (e.g., adding a `target_user_id` column) would need `V54`. Worth knowing the current sequence. - **Async logging failure handling is already correct.** `AuditService.writeLog()` catches all exceptions and logs them without rethrowing — audit failures never propagate to the caller. This is the right behavior; user creation should not fail because the audit log is temporarily unavailable. ### Recommendations - No changes needed from the infrastructure side. - If the issue is later extended to add a `target_user_id` column (to make querying by affected user efficient), that would need `V54__add_target_user_id_to_audit_log.sql` with `ALTER TABLE audit_log ADD COLUMN target_user_id UUID REFERENCES users(id) ON DELETE SET NULL`. Not in scope now, but worth anticipating for #326.

marcel commented

2026-04-26 14:28:39 +02:00

📋 Elicit — Requirements Engineer

Observations

The spec is well-structured and testable. User story, explicit payload table, in/out-of-scope boundaries, and AC checklist — this is implementation-ready for the specified scope. The "not on update of existing" qualifier on USER_CREATED is precise and verifiable.
Gap: createUser() is a second user-creation path not mentioned in the spec. UserService has both createUserOrUpdate() (upsert semantics) and createUser() (strict create, throws on duplicate email). The InviteController calls createUser() — not createUserOrUpdate(). If someone is added via the invite flow, their creation would not be logged under the current spec. The issue must state explicitly: "Only createUserOrUpdate() is in scope" (if invite logging is deferred) OR include createUser() as well.
Gap: actorId threading is unspecified. The audit trail needs to answer "who performed this action?" The spec defines the payload for each event kind but does not specify how the performing admin's identity reaches the service. All three service methods currently accept no actor parameter. The AC "Payloads contain no passwords..." is necessary but not sufficient — the AC should also include "actorId in the audit entry is non-null and equals the authenticated admin's ID."
invitedById in the USER_CREATED payload is undefined. There is no invitation entity or inviter-tracking concept in the current codebase. The InviteController generates invite tokens but does not track who invited whom in a persistent field. If invitedById cannot be populated from existing data, it should be removed from the payload spec to avoid a field that is always null.
AC for findRecentUserManagementEvents is underspecified. The AC says "returns the n most recent events of these three kinds, ordered newest-first." Missing: what happens when there are fewer than n events? (Expected: return however many exist — but this should be stated.) What is the return type — a list of raw AuditLog entities or a typed DTO? #326 will consume this — the interface should be defined here so the consumer doesn't need to reach into AuditLog internals.

Recommendations

Add one AC: "actorId in the logged entry is non-null and matches the ID of the authenticated user who performed the action."
Resolve or remove invitedById from the USER_CREATED payload table. If there's no inviter data source today, simplify to {userId, email} only.
State explicitly whether createUser() (invite path) is in scope for USER_CREATED logging, or deferred to a separate issue.
Define the return type of findRecentUserManagementEvents: a list of AuditLog entities is sufficient for #326's findRecentUserManagementEvents(3) call, but document the expected interface so the consumer knows what fields are available.

## 📋 Elicit — Requirements Engineer ### Observations - **The spec is well-structured and testable.** User story, explicit payload table, in/out-of-scope boundaries, and AC checklist — this is implementation-ready for the specified scope. The "not on update of existing" qualifier on `USER_CREATED` is precise and verifiable. - **Gap: `createUser()` is a second user-creation path not mentioned in the spec.** `UserService` has both `createUserOrUpdate()` (upsert semantics) and `createUser()` (strict create, throws on duplicate email). The `InviteController` calls `createUser()` — not `createUserOrUpdate()`. If someone is added via the invite flow, their creation would not be logged under the current spec. The issue must state explicitly: "Only `createUserOrUpdate()` is in scope" (if invite logging is deferred) OR include `createUser()` as well. - **Gap: `actorId` threading is unspecified.** The audit trail needs to answer "who performed this action?" The spec defines the payload for each event kind but does not specify how the performing admin's identity reaches the service. All three service methods currently accept no actor parameter. The AC "Payloads contain no passwords..." is necessary but not sufficient — the AC should also include "actorId in the audit entry is non-null and equals the authenticated admin's ID." - **`invitedById` in the `USER_CREATED` payload is undefined.** There is no invitation entity or inviter-tracking concept in the current codebase. The `InviteController` generates invite tokens but does not track who invited whom in a persistent field. If `invitedById` cannot be populated from existing data, it should be removed from the payload spec to avoid a field that is always null. - **AC for `findRecentUserManagementEvents` is underspecified.** The AC says "returns the n most recent events of these three kinds, ordered newest-first." Missing: what happens when there are fewer than n events? (Expected: return however many exist — but this should be stated.) What is the return type — a list of raw `AuditLog` entities or a typed DTO? #326 will consume this — the interface should be defined here so the consumer doesn't need to reach into `AuditLog` internals. ### Recommendations - **Add one AC:** "actorId in the logged entry is non-null and matches the ID of the authenticated user who performed the action." - **Resolve or remove `invitedById`** from the `USER_CREATED` payload table. If there's no inviter data source today, simplify to `{userId, email}` only. - **State explicitly** whether `createUser()` (invite path) is in scope for `USER_CREATED` logging, or deferred to a separate issue. - **Define the return type** of `findRecentUserManagementEvents`: a list of `AuditLog` entities is sufficient for #326's `findRecentUserManagementEvents(3)` call, but document the expected interface so the consumer knows what fields are available.

marcel commented

2026-04-26 14:28:46 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

No UI concerns from my angle for this issue — scope is explicitly backend-only (new AuditKind values, service-layer logging calls, and a query method). The UI surface is deferred to #326.

One note for when #326 implements the "recent activity" row: the audit entries will display user emails and group names to admins. At that point, ensure:

The admin activity list uses aria-live="polite" if it auto-refreshes.
Group membership changes show both added and removed groups clearly (not just "changed") — the addedGroups/removedGroups payload structure supports this well.
Touch targets on any action links in the activity row remain ≥ 44px.

Nothing blocking this issue from my side.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist No UI concerns from my angle for this issue — scope is explicitly backend-only (new `AuditKind` values, service-layer logging calls, and a query method). The UI surface is deferred to #326. One note for when #326 implements the "recent activity" row: the audit entries will display user emails and group names to admins. At that point, ensure: - The admin activity list uses `aria-live="polite"` if it auto-refreshes. - Group membership changes show both added and removed groups clearly (not just "changed") — the `addedGroups`/`removedGroups` payload structure supports this well. - Touch targets on any action links in the activity row remain ≥ 44px. Nothing blocking this issue from my side.

marcel commented

2026-04-26 15:09:22 +02:00

✅ Implementation complete

All acceptance criteria met. 1362 backend tests pass (0 failures).

What was implemented

New AuditKind values (audit/AuditKind.java)

USER_CREATED — payload: {userId, email}
USER_DELETED — payload: {userId, email}
GROUP_MEMBERSHIP_CHANGED — payload: {userId, email, addedGroups: [names], removedGroups: [names]}

UserService audit logging (3 write methods updated)

createUserOrUpdate(UUID actorId, ...) — emits USER_CREATED via logAfterCommit only on the new-user branch, not on update
deleteUser(UUID actorId, ...) — captures email before deletion, emits USER_DELETED
adminUpdateUser(UUID actorId, ...) — captures before-groups, computes diff, emits GROUP_MEMBERSHIP_CHANGED only when the group set actually changes; payload contains group names, not permission strings

UserController — adds Authentication parameter to the three admin methods and resolves actorId via findByEmail(authentication.getName())

AuditLogQueryRepository — new JPQL query findRecentByKinds(kinds, limit)

AuditLogQueryService — new method findRecentUserManagementEvents(int limit) returns the N most recent user-management events ordered newest-first, ready for #326 to consume as findRecentUserManagementEvents(3)

Tests added

Test	File	Kind
`createUserOrUpdate_logsUserCreated_whenUserIsNew`	`UserServiceTest`	unit
`createUserOrUpdate_doesNotLogUserCreated_whenUserAlreadyExists`	`UserServiceTest`	unit
`deleteUser_logsUserDeleted_withEmailInPayload`	`UserServiceTest`	unit
`adminUpdateUser_logsGroupMembershipChanged_whenGroupSetChanges`	`UserServiceTest`	unit
`adminUpdateUser_doesNotLogGroupMembershipChanged_whenGroupsUnchanged`	`UserServiceTest`	unit
`adminUpdateUser_doesNotLogGroupMembershipChanged_whenGroupIdsIsNull`	`UserServiceTest`	unit
`findRecentUserManagementEvents_delegatesToRepositoryWithAllThreeKinds`	`AuditLogQueryServiceTest`	unit
`createAndDeleteUser_producesOrderedAuditEntries`	`UserManagementAuditIntegrationTest`	integration

Design notes (per persona review)

invitedById dropped — no invitation entity exists; actorId top-level field already captures the performing admin
createUser() (invite path) remains out of scope as specified
logAfterCommit used throughout — audit entries only persist if the transaction commits
No Flyway migration needed — kind is @Enumerated(EnumType.STRING)

Commits

feat(audit): emit USER_CREATED when admin creates a new user
feat(audit): emit USER_DELETED when admin removes a user
feat(audit): emit GROUP_MEMBERSHIP_CHANGED when admin updates user groups
feat(audit): add findRecentUserManagementEvents query method
test(audit): integration test — create + delete user produces ordered audit entries

Branch: feat/issue-218-person-title-type-fields

## ✅ Implementation complete All acceptance criteria met. 1362 backend tests pass (0 failures). ### What was implemented **New `AuditKind` values** (`audit/AuditKind.java`) - `USER_CREATED` — payload: `{userId, email}` - `USER_DELETED` — payload: `{userId, email}` - `GROUP_MEMBERSHIP_CHANGED` — payload: `{userId, email, addedGroups: [names], removedGroups: [names]}` **`UserService` audit logging** (3 write methods updated) - `createUserOrUpdate(UUID actorId, ...)` — emits `USER_CREATED` via `logAfterCommit` only on the new-user branch, not on update - `deleteUser(UUID actorId, ...)` — captures email before deletion, emits `USER_DELETED` - `adminUpdateUser(UUID actorId, ...)` — captures before-groups, computes diff, emits `GROUP_MEMBERSHIP_CHANGED` only when the group set actually changes; payload contains group **names**, not permission strings **`UserController`** — adds `Authentication` parameter to the three admin methods and resolves `actorId` via `findByEmail(authentication.getName())` **`AuditLogQueryRepository`** — new JPQL query `findRecentByKinds(kinds, limit)` **`AuditLogQueryService`** — new method `findRecentUserManagementEvents(int limit)` returns the N most recent user-management events ordered newest-first, ready for #326 to consume as `findRecentUserManagementEvents(3)` ### Tests added | Test | File | Kind | |---|---|---| | `createUserOrUpdate_logsUserCreated_whenUserIsNew` | `UserServiceTest` | unit | | `createUserOrUpdate_doesNotLogUserCreated_whenUserAlreadyExists` | `UserServiceTest` | unit | | `deleteUser_logsUserDeleted_withEmailInPayload` | `UserServiceTest` | unit | | `adminUpdateUser_logsGroupMembershipChanged_whenGroupSetChanges` | `UserServiceTest` | unit | | `adminUpdateUser_doesNotLogGroupMembershipChanged_whenGroupsUnchanged` | `UserServiceTest` | unit | | `adminUpdateUser_doesNotLogGroupMembershipChanged_whenGroupIdsIsNull` | `UserServiceTest` | unit | | `findRecentUserManagementEvents_delegatesToRepositoryWithAllThreeKinds` | `AuditLogQueryServiceTest` | unit | | `createAndDeleteUser_producesOrderedAuditEntries` | `UserManagementAuditIntegrationTest` | integration | ### Design notes (per persona review) - `invitedById` dropped — no invitation entity exists; `actorId` top-level field already captures the performing admin - `createUser()` (invite path) remains out of scope as specified - `logAfterCommit` used throughout — audit entries only persist if the transaction commits - No Flyway migration needed — `kind` is `@Enumerated(EnumType.STRING)` ### Commits - `feat(audit): emit USER_CREATED when admin creates a new user` - `feat(audit): emit USER_DELETED when admin removes a user` - `feat(audit): emit GROUP_MEMBERSHIP_CHANGED when admin updates user groups` - `feat(audit): add findRecentUserManagementEvents query method` - `test(audit): integration test — create + delete user produces ordered audit entries` Branch: `feat/issue-218-person-title-type-fields`

marcel referenced a pull request that will close this issue

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

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

marcel referenced this issue

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

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

marcel referenced this issue

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

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

marcel closed this issue

2026-04-26 19:42:00 +02:00

Sign in to join this conversation.