marcel/familienarchiv
1
0
Fork 0
Code Issues 111 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(audit): address review cycle 1 feedback
Some checks failed
CI / Unit & Component Tests (pull_request) Failing after 2m34s
Details
CI / OCR Service Tests (pull_request) Successful in 34s
Details
CI / Unit & Component Tests (push) Failing after 2m35s
Details
CI / OCR Service Tests (push) Successful in 33s
Details
CI / Backend Unit Tests (push) Failing after 2m50s
Details
CI / Backend Unit Tests (pull_request) Failing after 2m46s
Details

Browse Source 
- Extract logAfterCommit() from AnnotationService and TranscriptionService
  into AuditService, eliminating duplicate boilerplate (Markus)
- Remove UserService from DocumentService; add actorId param to
  storeDocument(), attachFile(), updateDocument() instead — resolves
  SecurityContextHolder coupling concern (Markus)
- Update DocumentController to inject UserService and resolve actorId
  from Authentication, passing it through to service methods
- Add logAfterCommit() tests to AuditServiceTest with MockedStatic
- Update all test verify() calls to use logAfterCommit() (not log())

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Marcel
2026-04-19 14:07:20 +02:00
parent 2deaaf167e
commit 5a3b5ff3c7
10 changed files with 159 additions and 127 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
backend/src/main/java/org/raddatz/familienarchiv/audit/AuditService.java
View File

@@ -4,6 +4,8 @@ import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.scheduling.annotation.Async;
import org.springframework.stereotype.Service;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;
import java.util.Map;
import java.util.UUID;
@@ -17,6 +19,23 @@ public class AuditService {
@Async("auditExecutor")
public void log(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) {
writeLog(kind, actorId, documentId, payload);
}
public void logAfterCommit(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) {
if (TransactionSynchronizationManager.isActualTransactionActive()) {
TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
@Override
public void afterCommit() {
writeLog(kind, actorId, documentId, payload);
}
});
} else {
writeLog(kind, actorId, documentId, payload);
}
}
private void writeLog(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) {
try {
auditLogRepository.save(AuditLog.builder()
.kind(kind)

31
backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java
View File

@@ -25,12 +25,15 @@ import org.raddatz.familienarchiv.dto.DocumentSort;
import org.raddatz.familienarchiv.model.DocumentStatus;
import org.raddatz.familienarchiv.model.TrainingLabel;
import org.raddatz.familienarchiv.model.DocumentVersion;
import org.raddatz.familienarchiv.model.AppUser;
import org.raddatz.familienarchiv.security.Permission;
import org.raddatz.familienarchiv.security.RequirePermission;
import org.raddatz.familienarchiv.service.DocumentService;
import org.raddatz.familienarchiv.service.DocumentVersionService;
import org.raddatz.familienarchiv.service.FileService;
import org.raddatz.familienarchiv.service.UserService;
import org.springframework.data.domain.Sort;
import org.springframework.security.core.Authentication;
import org.springframework.http.HttpHeaders;
import org.springframework.http.MediaType;
import org.springframework.http.ResponseEntity;
@@ -63,6 +66,7 @@ public class DocumentController {
private final DocumentService documentService;
private final DocumentVersionService documentVersionService;
private final FileService fileService;
private final UserService userService;
// --- DOWNLOAD ---
@GetMapping("/{id}/file")
@@ -112,9 +116,10 @@ public class DocumentController {
public Document updateDocument(
@PathVariable UUID id,
@ModelAttribute DocumentUpdateDTO dto,
@RequestPart(value = "file", required = false) MultipartFile file) {
@RequestPart(value = "file", required = false) MultipartFile file,
Authentication authentication) {
try {
return documentService.updateDocument(id, dto, file);
return documentService.updateDocument(id, dto, file, requireUserId(authentication));
} catch (IOException e) {
throw DomainException.internal(ErrorCode.FILE_UPLOAD_FAILED, "Failed to upload file: " + e.getMessage());
}
@@ -138,12 +143,13 @@ public class DocumentController {
@RequirePermission(Permission.WRITE_ALL)
public Document attachFile(
@PathVariable UUID id,
@RequestPart("file") MultipartFile file) {
@RequestPart("file") MultipartFile file,
Authentication authentication) {
String contentType = file.getContentType();
if (contentType == null || !ALLOWED_CONTENT_TYPES.contains(contentType)) {
throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Unsupported file type: " + contentType);
}
return documentService.attachFile(id, file);
return documentService.attachFile(id, file, requireUserId(authentication));
}
// --- QUICK UPLOAD ---
@@ -154,7 +160,8 @@ public class DocumentController {
@PostMapping(value = "/quick-upload", consumes = MediaType.MULTIPART_FORM_DATA_VALUE)
@RequirePermission(Permission.WRITE_ALL)
public QuickUploadResult quickUpload(
@RequestPart(value = "files", required = false) List<MultipartFile> files) {
@RequestPart(value = "files", required = false) List<MultipartFile> files,
Authentication authentication) {
List<Document> created = new ArrayList<>();
List<Document> updated = new ArrayList<>();
List<UploadError> errors = new ArrayList<>();
@@ -163,13 +170,14 @@ public class DocumentController {
return new QuickUploadResult(created, updated, errors);
}
UUID actorId = requireUserId(authentication);
for (MultipartFile file : files) {
if (!ALLOWED_CONTENT_TYPES.contains(file.getContentType())) {
errors.add(new UploadError(file.getOriginalFilename(), "UNSUPPORTED_FILE_TYPE"));
continue;
}
try {
DocumentService.StoreResult result = documentService.storeDocument(file);
DocumentService.StoreResult result = documentService.storeDocument(file, actorId);
if (result.isNew()) {
created.add(result.document());
} else {
@@ -276,4 +284,15 @@ public class DocumentController {
Sort sort = Sort.by(Sort.Direction.fromString(dir.toUpperCase()), "documentDate");
return documentService.getConversationFiltered(senderId, receiverId, from, to, sort);
}
private UUID requireUserId(Authentication authentication) {
if (authentication == null || !authentication.isAuthenticated()) {
throw DomainException.unauthorized("Authentication required");
}
AppUser user = userService.findByEmail(authentication.getName());
if (user == null) {
throw DomainException.unauthorized("User not found");
}
return user.getId();
}
}

17
backend/src/main/java/org/raddatz/familienarchiv/service/AnnotationService.java
View File

@@ -14,8 +14,6 @@ import org.raddatz.familienarchiv.repository.TranscriptionBlockRepository;
import org.springframework.dao.DataIntegrityViolationException;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;
import java.util.List;
import java.util.Map;
@@ -49,7 +47,7 @@ public class AnnotationService {
.build();
DocumentAnnotation saved = annotationRepository.save(annotation);
logAfterCommit(AuditKind.ANNOTATION_CREATED, userId, saved.getDocumentId(),
auditService.logAfterCommit(AuditKind.ANNOTATION_CREATED, userId, saved.getDocumentId(),
Map.of("pageNumber", saved.getPageNumber()));
return saved;
}
@@ -117,17 +115,4 @@ public class AnnotationService {
});
}
private void logAfterCommit(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) {
if (TransactionSynchronizationManager.isActualTransactionActive()) {
TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
@Override
public void afterCommit() {
auditService.log(kind, actorId, documentId, payload);
}
});
} else {
auditService.log(kind, actorId, documentId, payload);
}
}
}

44
backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java
View File

@@ -12,7 +12,6 @@ import org.raddatz.familienarchiv.dto.IncompleteDocumentDTO;
import org.raddatz.familienarchiv.dto.MatchOffset;
import org.raddatz.familienarchiv.dto.SearchMatchData;
import org.raddatz.familienarchiv.dto.TagOperator;
import org.raddatz.familienarchiv.model.AppUser;
import org.raddatz.familienarchiv.model.Document;
import org.raddatz.familienarchiv.model.DocumentStatus;
import org.raddatz.familienarchiv.model.ScriptType;
@@ -23,10 +22,6 @@ import org.raddatz.familienarchiv.repository.DocumentRepository;
import org.springframework.data.domain.PageRequest;
import org.springframework.data.domain.Sort;
import org.springframework.data.jpa.domain.Specification;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;
import org.raddatz.familienarchiv.exception.DomainException;
import org.raddatz.familienarchiv.exception.ErrorCode;
import org.springframework.stereotype.Service;
@@ -64,7 +59,6 @@ public class DocumentService {
private final DocumentVersionService documentVersionService;
private final AnnotationService annotationService;
private final AuditService auditService;
private final UserService userService;
public record StoreResult(Document document, boolean isNew) {}
@@ -84,7 +78,7 @@ public class DocumentService {
* - Wenn NEIN: Erstellt neuen Eintrag — isNew = true.
*/
@Transactional
public StoreResult storeDocument(MultipartFile file) throws IOException {
public StoreResult storeDocument(MultipartFile file, UUID actorId) throws IOException {
String originalFilename = file.getOriginalFilename();
// 1. Check for existing record (findFirst to survive duplicate filenames in the DB)
@@ -124,8 +118,7 @@ public class DocumentService {
Document saved = documentRepository.save(document);
if (wasPlaceholder) {
UUID actorId = resolveCurrentUserId();
logAfterCommit(AuditKind.FILE_UPLOADED, actorId, saved.getId(), null);
auditService.logAfterCommit(AuditKind.FILE_UPLOADED, actorId, saved.getId(), null);
}
return new StoreResult(saved, isNew);
}
@@ -203,7 +196,7 @@ public class DocumentService {
}
@Transactional
public Document updateDocument(UUID id, DocumentUpdateDTO dto, MultipartFile newFile) throws IOException {
public Document updateDocument(UUID id, DocumentUpdateDTO dto, MultipartFile newFile, UUID actorId) throws IOException {
Document doc = documentRepository.findById(id)
.orElseThrow(() -> DomainException.notFound(ErrorCode.DOCUMENT_NOT_FOUND, "Document not found: " + id));
@@ -263,12 +256,11 @@ public class DocumentService {
Document saved = documentRepository.save(doc);
documentVersionService.recordVersion(saved);
UUID actorId = resolveCurrentUserId();
if (saved.getStatus() != statusBefore) {
logAfterCommit(AuditKind.STATUS_CHANGED, actorId, saved.getId(),
auditService.logAfterCommit(AuditKind.STATUS_CHANGED, actorId, saved.getId(),
Map.of("oldStatus", statusBefore.name(), "newStatus", saved.getStatus().name()));
} else {
logAfterCommit(AuditKind.METADATA_UPDATED, actorId, saved.getId(), null);
auditService.logAfterCommit(AuditKind.METADATA_UPDATED, actorId, saved.getId(), null);
}
return saved;
@@ -313,7 +305,7 @@ public class DocumentService {
}
@Transactional
public Document attachFile(UUID id, MultipartFile file) {
public Document attachFile(UUID id, MultipartFile file, UUID actorId) {
Document doc = documentRepository.findById(id)
.orElseThrow(() -> DomainException.notFound(ErrorCode.DOCUMENT_NOT_FOUND, "Document not found: " + id));
FileService.UploadResult upload;
@@ -333,8 +325,7 @@ public class DocumentService {
Document saved = documentRepository.save(doc);
documentVersionService.recordVersion(saved);
if (wasPlaceholder) {
UUID actorId = resolveCurrentUserId();
logAfterCommit(AuditKind.FILE_UPLOADED, actorId, saved.getId(), null);
auditService.logAfterCommit(AuditKind.FILE_UPLOADED, actorId, saved.getId(), null);
}
return saved;
}
@@ -757,25 +748,4 @@ public class DocumentService {
}
}
private UUID resolveCurrentUserId() {
Authentication auth = SecurityContextHolder.getContext().getAuthentication();
if (auth == null || !auth.isAuthenticated()) return null;
String email = auth.getName();
if (email == null) return null;
AppUser user = userService.findByEmail(email);
return user != null ? user.getId() : null;
}
private void logAfterCommit(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) {
if (TransactionSynchronizationManager.isActualTransactionActive()) {
TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
@Override
public void afterCommit() {
auditService.log(kind, actorId, documentId, payload);
}
});
} else {
auditService.log(kind, actorId, documentId, payload);
}
}
}

18
backend/src/main/java/org/raddatz/familienarchiv/service/TranscriptionService.java
View File

@@ -21,8 +21,6 @@ import org.raddatz.familienarchiv.repository.TranscriptionBlockRepository;
import org.raddatz.familienarchiv.repository.TranscriptionBlockVersionRepository;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.transaction.support.TransactionSynchronization;
import org.springframework.transaction.support.TransactionSynchronizationManager;
import java.util.List;
import java.util.Map;
@@ -144,7 +142,7 @@ public class TranscriptionService {
if (!text.equals(previousText)) {
Optional<DocumentAnnotation> annotation = annotationRepository.findById(block.getAnnotationId());
int pageNumber = annotation.map(DocumentAnnotation::getPageNumber).orElse(0);
logAfterCommit(AuditKind.TEXT_SAVED, userId, documentId, Map.of("pageNumber", pageNumber));
auditService.logAfterCommit(AuditKind.TEXT_SAVED, userId, documentId, Map.of("pageNumber", pageNumber));
}
Document doc = documentService.getDocumentById(documentId);
@@ -201,7 +199,7 @@ public class TranscriptionService {
block.setReviewed(!wasReviewed);
TranscriptionBlock saved = blockRepository.save(block);
if (!wasReviewed && saved.isReviewed()) {
logAfterCommit(AuditKind.BLOCK_REVIEWED, userId, documentId, null);
auditService.logAfterCommit(AuditKind.BLOCK_REVIEWED, userId, documentId, null);
}
return saved;
}
@@ -228,16 +226,4 @@ public class TranscriptionService {
return text;
}
private void logAfterCommit(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) {
if (TransactionSynchronizationManager.isActualTransactionActive()) {
TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
@Override
public void afterCommit() {
auditService.log(kind, actorId, documentId, payload);
}
});
} else {
auditService.log(kind, actorId, documentId, payload);
}
}
}