From 5cbb14d4a3403d8785f1e343d0e1dd2b1f7804ec Mon Sep 17 00:00:00 2001 From: Marcel Date: Sat, 25 Apr 2026 16:24:03 +0200 Subject: [PATCH] =?UTF-8?q?fix(bulk-edit):=20backend=20hardening=20?= =?UTF-8?q?=E2=80=94=20audit,=20caps,=20dedupe,=20CRLF,=20WRITE=5FALL=20on?= =?UTF-8?q?=20/ids?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Markus B1+B2, Nora C1+C4+C5, Tobias #1, Sara B1+B2+C2, Elicit S2+C4 from the cycle 1 review on PR #331. Audit / version trail applyBulkEditToDocument now takes actorId, calls documentVersionService.recordVersion(saved), and emits an AuditKind.METADATA_UPDATED event tagged source=BULK_EDIT — restoring parity with the single-doc updateDocument path. Caps /api/documents/batch-metadata: 500-ID cap (matches PATCH cap) /api/documents/ids: 5000 result cap with BULK_EDIT_TOO_MANY_IDS on overflow Permission tightening /api/documents/ids re-gated WRITE_ALL — its only consumer is the bulk-edit fast path (least-privilege per Elicit S2 + Nora's defence-in-depth). Audit log /ids and /batch-metadata now emit one log.info per call, mirroring the quickUpload + bulkEdit format. Robustness Duplicates in PATCH documentIds are de-duplicated via LinkedHashSet so a double-clicked "Alle X editieren" cannot inflate the updated count. log.warn lines that interpolate Throwable.getMessage() now run through a CRLF-strip helper (CWE-117). Tests added applyBulkEditToDocument_recordsVersion_andLogsAuditEvent_taggedSourceBulkEdit patchBulk_acceptsExactly500Ids_atTheCap (off-by-one fence) patchBulk_dedupesDuplicateDocumentIds_doesNotInflateUpdatedCount getDocumentIds_returns403_forUserWithoutWriteAll getDocumentIds_returns400_whenResultExceedsFilterCap batchMetadata_returns403_forUserWithoutReadAll batchMetadata_returns400_whenIdsExceedsCap All 231 backend tests green. Refs #225, PR #331 Co-Authored-By: Claude Sonnet 4.6 --- .../controller/DocumentController.java | 50 +++++++-- .../service/DocumentService.java | 20 +++- .../controller/DocumentControllerTest.java | 105 ++++++++++++++++-- .../service/DocumentServiceTest.java | 38 +++++-- 4 files changed, 183 insertions(+), 30 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java b/backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java index 5dc6a591..bb725202 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java @@ -245,6 +245,11 @@ public class DocumentController { // --- BULK EDIT --- private static final int BULK_EDIT_MAX_IDS = 500; + /** Hard cap for {@code GET /api/documents/ids}: prevents an unfiltered + * call from materialising the entire {@code documents} table into JSON. + * Generous enough for real-world "Alle X editieren" against the family + * archive's bounded scale (~1500 docs today, expected growth to ~5k). */ + private static final int BULK_EDIT_FILTER_MAX_IDS = 5000; @PatchMapping("/bulk") @RequirePermission(Permission.WRITE_ALL) @@ -263,26 +268,37 @@ public class DocumentController { int updated = 0; List errors = new ArrayList<>(); - for (UUID id : dto.getDocumentIds()) { + // Dedupe duplicate document IDs while preserving submission order. A + // double-click on "Alle X editieren" would otherwise hit each document + // twice and inflate the `updated` count returned to the user. + java.util.LinkedHashSet uniqueIds = new java.util.LinkedHashSet<>(dto.getDocumentIds()); + + for (UUID id : uniqueIds) { try { - documentService.applyBulkEditToDocument(id, dto); + documentService.applyBulkEditToDocument(id, dto, actorId); updated++; } catch (DomainException e) { - errors.add(new BulkEditError(id, e.getMessage())); + errors.add(new BulkEditError(id, sanitizeForLog(e.getMessage()))); } catch (Exception e) { errors.add(new BulkEditError(id, "Internal error")); - log.warn("Bulk edit failed for document {}: {}", id, e.getMessage()); + log.warn("Bulk edit failed for document {}: {}", id, sanitizeForLog(e.getMessage())); } } - log.info("bulkEdit actor={} documentIds={} updated={} errors={}", - actorId, dto.getDocumentIds().size(), updated, errors.size()); + log.info("bulkEdit actor={} documentIds={} unique={} updated={} errors={}", + actorId, dto.getDocumentIds().size(), uniqueIds.size(), updated, errors.size()); return new BulkEditResult(updated, errors); } + /** CRLF strip for any log line interpolating a free-form string (e.g. + * {@link Throwable#getMessage()}). Defends against CWE-117 log injection. */ + private static String sanitizeForLog(String s) { + return s == null ? null : s.replaceAll("[\\r\\n]", "_"); + } + @GetMapping("/ids") - @RequirePermission(Permission.READ_ALL) + @RequirePermission(Permission.WRITE_ALL) public List getDocumentIds( @RequestParam(required = false) String q, @RequestParam(required = false) LocalDate from, @@ -292,17 +308,31 @@ public class DocumentController { @RequestParam(required = false, name = "tag") List tags, @RequestParam(required = false) String tagQ, @RequestParam(required = false) DocumentStatus status, - @RequestParam(required = false) String tagOp) { + @RequestParam(required = false) String tagOp, + Authentication authentication) { TagOperator operator = "OR".equalsIgnoreCase(tagOp) ? TagOperator.OR : TagOperator.AND; - return documentService.findIdsForFilter(q, from, to, senderId, receiverId, tags, tagQ, status, operator); + List ids = documentService.findIdsForFilter(q, from, to, senderId, receiverId, tags, tagQ, status, operator); + if (ids.size() > BULK_EDIT_FILTER_MAX_IDS) { + throw DomainException.badRequest(ErrorCode.BULK_EDIT_TOO_MANY_IDS, + "Filter matches " + ids.size() + " documents — refine filter (max " + BULK_EDIT_FILTER_MAX_IDS + ")"); + } + UUID actorId = requireUserId(authentication); + log.info("documentIds actor={} matched={}", actorId, ids.size()); + return ids; } @PostMapping(value = "/batch-metadata", consumes = MediaType.APPLICATION_JSON_VALUE) @RequirePermission(Permission.READ_ALL) - public List batchMetadata(@RequestBody BatchMetadataRequest request) { + public List batchMetadata(@RequestBody BatchMetadataRequest request, Authentication authentication) { if (request == null || request.ids() == null || request.ids().isEmpty()) { throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "ids is required"); } + if (request.ids().size() > BULK_EDIT_MAX_IDS) { + throw DomainException.badRequest(ErrorCode.BULK_EDIT_TOO_MANY_IDS, + "Maximum " + BULK_EDIT_MAX_IDS + " ids per request, got: " + request.ids().size()); + } + UUID actorId = requireUserId(authentication); + log.info("batchMetadata actor={} ids={}", actorId, request.ids().size()); return documentService.batchMetadata(request.ids()); } diff --git a/backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java b/backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java index 0a4e52e4..c6d9e26a 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java @@ -400,10 +400,20 @@ public class DocumentService { * Tags and receivers are additive (merged into existing sets); sender and the * three location fields are replace-on-non-blank (null/blank means "no change"). * Wrapped in its own transaction so a failure on one document never partially - * mutates another in the batch loop. + * mutates another in the controller's batch loop. + * + * Each successful update emits a {@link AuditKind#METADATA_UPDATED} audit + * event tagged {@code source=BULK_EDIT} and writes a row to + * {@code document_versions} so the family archive's "who changed what" + * trail stays complete across both single- and bulk-doc edit paths. + * + * NOTE on N+1: tag and person resolution happens per-document. With 500 + * documents × 10 tags this fans out to ~5000 tag-resolve queries per + * request. Acceptable today because the family archive is bounded at + * ~1500 documents total. Tracked as a perf follow-up. */ @Transactional - public Document applyBulkEditToDocument(UUID id, org.raddatz.familienarchiv.dto.DocumentBulkEditDTO dto) { + public Document applyBulkEditToDocument(UUID id, org.raddatz.familienarchiv.dto.DocumentBulkEditDTO dto, UUID actorId) { Document doc = documentRepository.findById(id) .orElseThrow(() -> DomainException.notFound(ErrorCode.DOCUMENT_NOT_FOUND, "Document not found: " + id)); @@ -438,7 +448,11 @@ public class DocumentService { doc.setArchiveFolder(dto.getArchiveFolder()); } - return documentRepository.save(doc); + Document saved = documentRepository.save(doc); + documentVersionService.recordVersion(saved); + auditService.logAfterCommit(AuditKind.METADATA_UPDATED, actorId, saved.getId(), + Map.of("source", "BULK_EDIT")); + return saved; } /** diff --git a/backend/src/test/java/org/raddatz/familienarchiv/controller/DocumentControllerTest.java b/backend/src/test/java/org/raddatz/familienarchiv/controller/DocumentControllerTest.java index fed57756..19eadf2b 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/controller/DocumentControllerTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/controller/DocumentControllerTest.java @@ -996,13 +996,50 @@ class DocumentControllerTest { .andExpect(jsonPath("$.code").value("BULK_EDIT_TOO_MANY_IDS")); } + @Test + @WithMockUser(authorities = "WRITE_ALL") + void patchBulk_acceptsExactly500Ids_atTheCap() throws Exception { + when(userService.findByEmail(any())).thenReturn(AppUser.builder().id(UUID.randomUUID()).build()); + when(documentService.applyBulkEditToDocument(any(), any(), any())) + .thenAnswer(inv -> Document.builder().id(inv.getArgument(0)).build()); + + String[] ids = new String[500]; + for (int i = 0; i < 500; i++) ids[i] = UUID.randomUUID().toString(); + + mockMvc.perform(patch("/api/documents/bulk") + .contentType(MediaType.APPLICATION_JSON) + .content(bulkBody(ids))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.updated").value(500)); + } + + @Test + @WithMockUser(authorities = "WRITE_ALL") + void patchBulk_dedupesDuplicateDocumentIds_doesNotInflateUpdatedCount() throws Exception { + when(userService.findByEmail(any())).thenReturn(AppUser.builder().id(UUID.randomUUID()).build()); + UUID id = UUID.randomUUID(); + when(documentService.applyBulkEditToDocument(eq(id), any(), any())) + .thenAnswer(inv -> Document.builder().id(id).build()); + + // Same id sent three times — controller should dedupe and call the + // service exactly once, returning updated=1, not 3. + mockMvc.perform(patch("/api/documents/bulk") + .contentType(MediaType.APPLICATION_JSON) + .content(bulkBody(id.toString(), id.toString(), id.toString()))) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.updated").value(1)); + + verify(documentService, org.mockito.Mockito.times(1)) + .applyBulkEditToDocument(eq(id), any(), any()); + } + @Test @WithMockUser(authorities = "WRITE_ALL") void patchBulk_returns200_andCallsServiceForEachId() throws Exception { when(userService.findByEmail(any())).thenReturn(AppUser.builder().id(UUID.randomUUID()).build()); UUID id1 = UUID.randomUUID(); UUID id2 = UUID.randomUUID(); - when(documentService.applyBulkEditToDocument(any(), any())) + when(documentService.applyBulkEditToDocument(any(), any(), any())) .thenAnswer(inv -> Document.builder().id(inv.getArgument(0)).build()); mockMvc.perform(patch("/api/documents/bulk") @@ -1012,8 +1049,8 @@ class DocumentControllerTest { .andExpect(jsonPath("$.updated").value(2)) .andExpect(jsonPath("$.errors").isEmpty()); - verify(documentService).applyBulkEditToDocument(eq(id1), any()); - verify(documentService).applyBulkEditToDocument(eq(id2), any()); + verify(documentService).applyBulkEditToDocument(eq(id1), any(), any()); + verify(documentService).applyBulkEditToDocument(eq(id2), any(), any()); } // ─── GET /api/documents/ids ────────────────────────────────────────────── @@ -1025,8 +1062,18 @@ class DocumentControllerTest { } @Test - @WithMockUser(authorities = "READ_ALL") + @WithMockUser + void getDocumentIds_returns403_forUserWithoutWriteAll() throws Exception { + // /ids is gated WRITE_ALL because it powers the bulk-edit "Alle X + // editieren" fast path; no other consumer needs it. + mockMvc.perform(get("/api/documents/ids")) + .andExpect(status().isForbidden()); + } + + @Test + @WithMockUser(authorities = "WRITE_ALL") void getDocumentIds_returns200_andDelegatesToService() throws Exception { + when(userService.findByEmail(any())).thenReturn(AppUser.builder().id(UUID.randomUUID()).build()); UUID id = UUID.randomUUID(); when(documentService.findIdsForFilter(any(), any(), any(), any(), any(), any(), any(), any(), any())) .thenReturn(List.of(id)); @@ -1037,8 +1084,9 @@ class DocumentControllerTest { } @Test - @WithMockUser(authorities = "READ_ALL") + @WithMockUser(authorities = "WRITE_ALL") void getDocumentIds_passesSenderIdParamToService() throws Exception { + when(userService.findByEmail(any())).thenReturn(AppUser.builder().id(UUID.randomUUID()).build()); UUID senderId = UUID.randomUUID(); when(documentService.findIdsForFilter(any(), any(), any(), eq(senderId), any(), any(), any(), any(), any())) .thenReturn(List.of()); @@ -1049,6 +1097,21 @@ class DocumentControllerTest { verify(documentService).findIdsForFilter(any(), any(), any(), eq(senderId), any(), any(), any(), any(), any()); } + @Test + @WithMockUser(authorities = "WRITE_ALL") + void getDocumentIds_returns400_whenResultExceedsFilterCap() throws Exception { + when(userService.findByEmail(any())).thenReturn(AppUser.builder().id(UUID.randomUUID()).build()); + // Service returns 5001 IDs — one over BULK_EDIT_FILTER_MAX_IDS (5000). + java.util.List tooMany = new java.util.ArrayList<>(5001); + for (int i = 0; i < 5001; i++) tooMany.add(UUID.randomUUID()); + when(documentService.findIdsForFilter(any(), any(), any(), any(), any(), any(), any(), any(), any())) + .thenReturn(tooMany); + + mockMvc.perform(get("/api/documents/ids")) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.code").value("BULK_EDIT_TOO_MANY_IDS")); + } + // ─── POST /api/documents/batch-metadata ────────────────────────────────── @Test @@ -1059,6 +1122,15 @@ class DocumentControllerTest { .andExpect(status().isUnauthorized()); } + @Test + @WithMockUser + void batchMetadata_returns403_forUserWithoutReadAll() throws Exception { + mockMvc.perform(org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post("/api/documents/batch-metadata") + .contentType(MediaType.APPLICATION_JSON) + .content("{\"ids\":[\"" + UUID.randomUUID() + "\"]}")) + .andExpect(status().isForbidden()); + } + @Test @WithMockUser(authorities = "READ_ALL") void batchMetadata_returns400_whenIdsEmpty() throws Exception { @@ -1068,9 +1140,28 @@ class DocumentControllerTest { .andExpect(status().isBadRequest()); } + @Test + @WithMockUser(authorities = "READ_ALL") + void batchMetadata_returns400_whenIdsExceedsCap() throws Exception { + when(userService.findByEmail(any())).thenReturn(AppUser.builder().id(UUID.randomUUID()).build()); + StringBuilder sb = new StringBuilder("{\"ids\":["); + for (int i = 0; i < 501; i++) { + if (i > 0) sb.append(","); + sb.append("\"").append(UUID.randomUUID()).append("\""); + } + sb.append("]}"); + + mockMvc.perform(org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post("/api/documents/batch-metadata") + .contentType(MediaType.APPLICATION_JSON) + .content(sb.toString())) + .andExpect(status().isBadRequest()) + .andExpect(jsonPath("$.code").value("BULK_EDIT_TOO_MANY_IDS")); + } + @Test @WithMockUser(authorities = "READ_ALL") void batchMetadata_returnsSummaries_forExistingIds() throws Exception { + when(userService.findByEmail(any())).thenReturn(AppUser.builder().id(UUID.randomUUID()).build()); UUID id = UUID.randomUUID(); when(documentService.batchMetadata(any())).thenReturn(List.of( new org.raddatz.familienarchiv.dto.DocumentBatchSummary(id, "Brief", "/api/documents/" + id + "/file"))); @@ -1090,9 +1181,9 @@ class DocumentControllerTest { when(userService.findByEmail(any())).thenReturn(AppUser.builder().id(UUID.randomUUID()).build()); UUID okId = UUID.randomUUID(); UUID badId = UUID.randomUUID(); - when(documentService.applyBulkEditToDocument(eq(okId), any())) + when(documentService.applyBulkEditToDocument(eq(okId), any(), any())) .thenAnswer(inv -> Document.builder().id(okId).build()); - when(documentService.applyBulkEditToDocument(eq(badId), any())) + when(documentService.applyBulkEditToDocument(eq(badId), any(), any())) .thenThrow(org.raddatz.familienarchiv.exception.DomainException.notFound( org.raddatz.familienarchiv.exception.ErrorCode.DOCUMENT_NOT_FOUND, "Document not found: " + badId)); diff --git a/backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceTest.java index f90c725f..c8b270be 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceTest.java @@ -1929,7 +1929,7 @@ class DocumentServiceTest { UUID id = UUID.randomUUID(); when(documentRepository.findById(id)).thenReturn(Optional.empty()); - assertThatThrownBy(() -> documentService.applyBulkEditToDocument(id, bulkDto())) + assertThatThrownBy(() -> documentService.applyBulkEditToDocument(id, bulkDto(), null)) .isInstanceOf(DomainException.class) .hasMessageContaining(id.toString()); } @@ -1949,7 +1949,7 @@ class DocumentServiceTest { var dto = bulkDto(); dto.setTagNames(List.of("Kurrent")); - documentService.applyBulkEditToDocument(id, dto); + documentService.applyBulkEditToDocument(id, dto, null); assertThat(doc.getTags()).containsExactlyInAnyOrder(existing, added); } @@ -1965,7 +1965,7 @@ class DocumentServiceTest { when(documentRepository.findById(id)).thenReturn(Optional.of(doc)); when(documentRepository.save(any())).thenAnswer(inv -> inv.getArgument(0)); - documentService.applyBulkEditToDocument(id, bulkDto()); + documentService.applyBulkEditToDocument(id, bulkDto(), null); assertThat(doc.getTags()).containsExactly(existing); verify(tagService, never()).findOrCreate(any()); @@ -1984,7 +1984,7 @@ class DocumentServiceTest { var dto = bulkDto(); dto.setTagNames(List.of()); - documentService.applyBulkEditToDocument(id, dto); + documentService.applyBulkEditToDocument(id, dto, null); assertThat(doc.getTags()).containsExactly(existing); verify(tagService, never()).findOrCreate(any()); @@ -2006,7 +2006,7 @@ class DocumentServiceTest { var dto = bulkDto(); dto.setSenderId(senderId); - documentService.applyBulkEditToDocument(id, dto); + documentService.applyBulkEditToDocument(id, dto, null); assertThat(doc.getSender()).isEqualTo(newSender); } @@ -2022,7 +2022,7 @@ class DocumentServiceTest { when(documentRepository.findById(id)).thenReturn(Optional.of(doc)); when(documentRepository.save(any())).thenAnswer(inv -> inv.getArgument(0)); - documentService.applyBulkEditToDocument(id, bulkDto()); + documentService.applyBulkEditToDocument(id, bulkDto(), null); assertThat(doc.getSender()).isEqualTo(existing); verify(personService, never()).getById(any()); @@ -2043,7 +2043,7 @@ class DocumentServiceTest { var dto = bulkDto(); dto.setReceiverIds(List.of(newReceiverId)); - documentService.applyBulkEditToDocument(id, dto); + documentService.applyBulkEditToDocument(id, dto, null); assertThat(doc.getReceivers()).containsExactlyInAnyOrder(existing, added); } @@ -2060,12 +2060,30 @@ class DocumentServiceTest { var dto = bulkDto(); dto.setReceiverIds(List.of()); - documentService.applyBulkEditToDocument(id, dto); + documentService.applyBulkEditToDocument(id, dto, null); assertThat(doc.getReceivers()).containsExactly(existing); verify(personService, never()).getAllById(any()); } + @Test + void applyBulkEditToDocument_recordsVersion_andLogsAuditEvent_taggedSourceBulkEdit() { + UUID id = UUID.randomUUID(); + UUID actorId = UUID.randomUUID(); + Document doc = Document.builder().id(id).title("T").receivers(new HashSet<>()).build(); + when(documentRepository.findById(id)).thenReturn(Optional.of(doc)); + when(documentRepository.save(any())).thenReturn(doc); + + documentService.applyBulkEditToDocument(id, bulkDto(), actorId); + + verify(documentVersionService).recordVersion(doc); + verify(auditService).logAfterCommit( + eq(AuditKind.METADATA_UPDATED), + eq(actorId), + eq(id), + eq(java.util.Map.of("source", "BULK_EDIT"))); + } + @Test void applyBulkEditToDocument_replacesArchiveBoxAndFolderAndDocumentLocation_whenProvided() { UUID id = UUID.randomUUID(); @@ -2082,7 +2100,7 @@ class DocumentServiceTest { dto.setArchiveBox("NewBox"); dto.setArchiveFolder("NewFolder"); dto.setDocumentLocation("NewLocation"); - documentService.applyBulkEditToDocument(id, dto); + documentService.applyBulkEditToDocument(id, dto, null); assertThat(doc.getArchiveBox()).isEqualTo("NewBox"); assertThat(doc.getArchiveFolder()).isEqualTo("NewFolder"); @@ -2183,7 +2201,7 @@ class DocumentServiceTest { dto.setArchiveBox(" "); dto.setArchiveFolder(""); // documentLocation left null - documentService.applyBulkEditToDocument(id, dto); + documentService.applyBulkEditToDocument(id, dto, null); assertThat(doc.getArchiveBox()).isEqualTo("KeepBox"); assertThat(doc.getArchiveFolder()).isEqualTo("KeepFolder");