From 3c3680b1e66ed0bb7010bfa67b088c0263a1075f Mon Sep 17 00:00:00 2001 From: Marcel Date: Sat, 18 Apr 2026 14:27:22 +0200 Subject: [PATCH] fix(backend): move IOException into service, add content-type whitelist to attachFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - DocumentService.attachFile() now catches IOException internally and re-throws as DomainException.internal — the IOException no longer leaks through the service boundary - DocumentController.attachFile() is now a plain delegate (no try/catch) - ALLOWED_CONTENT_TYPES whitelist (PDF/JPEG/PNG/TIFF) is now enforced on the attachFile endpoint, matching the existing quick-upload validation - Added 5 DocumentService unit tests for attachFile (notFound, status transition PLACEHOLDER→UPLOADED, no-change when already UPLOADED, field assignment from upload result, IOException→DomainException) - Added controller tests: 400 on disallowed content type, 404 on missing doc Co-Authored-By: Claude Sonnet 4.6 --- .../controller/DocumentController.java | 14 ++-- .../service/DocumentService.java | 9 ++- .../controller/DocumentControllerTest.java | 29 ++++++++ .../service/DocumentServiceTest.java | 71 +++++++++++++++++++ 4 files changed, 114 insertions(+), 9 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 cc8f6707..ba34092b 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java @@ -131,23 +131,23 @@ public class DocumentController { // --- ATTACH FILE --- + private static final Set ALLOWED_CONTENT_TYPES = Set.of( + "application/pdf", "image/jpeg", "image/png", "image/tiff"); + @PostMapping(value = "/{id}/file", consumes = MediaType.MULTIPART_FORM_DATA_VALUE) @RequirePermission(Permission.WRITE_ALL) public Document attachFile( @PathVariable UUID id, @RequestPart("file") MultipartFile file) { - try { - return documentService.attachFile(id, file); - } catch (IOException e) { - throw DomainException.internal(ErrorCode.FILE_UPLOAD_FAILED, "Failed to upload file: " + e.getMessage()); + 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); } // --- QUICK UPLOAD --- - private static final Set ALLOWED_CONTENT_TYPES = Set.of( - "application/pdf", "image/jpeg", "image/png", "image/tiff"); - public record UploadError(String filename, String code) {} public record QuickUploadResult(List created, List updated, List errors) {} 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 fecf7117..b2d14d87 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java @@ -287,10 +287,15 @@ public class DocumentService { } @Transactional - public Document attachFile(UUID id, MultipartFile file) throws IOException { + public Document attachFile(UUID id, MultipartFile file) { Document doc = documentRepository.findById(id) .orElseThrow(() -> DomainException.notFound(ErrorCode.DOCUMENT_NOT_FOUND, "Document not found: " + id)); - FileService.UploadResult upload = fileService.uploadFile(file, file.getOriginalFilename()); + FileService.UploadResult upload; + try { + upload = fileService.uploadFile(file, file.getOriginalFilename()); + } catch (IOException e) { + throw DomainException.internal(ErrorCode.FILE_UPLOAD_FAILED, "Failed to upload file: " + e.getMessage()); + } doc.setFilePath(upload.s3Key()); doc.setFileHash(upload.fileHash()); doc.setOriginalFilename(file.getOriginalFilename()); 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 4153556e..8bb86ed0 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/controller/DocumentControllerTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/controller/DocumentControllerTest.java @@ -4,6 +4,8 @@ import org.junit.jupiter.api.Test; import org.raddatz.familienarchiv.dto.DocumentSearchResult; import org.raddatz.familienarchiv.dto.DocumentVersionSummary; import org.raddatz.familienarchiv.dto.IncompleteDocumentDTO; +import org.raddatz.familienarchiv.exception.DomainException; +import org.raddatz.familienarchiv.exception.ErrorCode; import org.raddatz.familienarchiv.model.Document; import org.raddatz.familienarchiv.model.DocumentStatus; import org.raddatz.familienarchiv.model.DocumentVersion; @@ -593,6 +595,33 @@ class DocumentControllerTest { .andExpect(jsonPath("$.status").value("UPLOADED")); } + @Test + @WithMockUser(authorities = "WRITE_ALL") + void attachFile_returns400_whenContentTypeIsNotWhitelisted() throws Exception { + org.springframework.mock.web.MockMultipartFile htmlFile = + new org.springframework.mock.web.MockMultipartFile( + "file", "evil.html", "text/html", "".getBytes()); + + mockMvc.perform(multipart("/api/documents/" + UUID.randomUUID() + "/file").file(htmlFile)) + .andExpect(status().isBadRequest()); + } + + @Test + @WithMockUser(authorities = "WRITE_ALL") + void attachFile_returns404_whenDocumentDoesNotExist() throws Exception { + UUID id = UUID.randomUUID(); + when(documentService.attachFile(eq(id), any())) + .thenThrow(DomainException.notFound( + ErrorCode.DOCUMENT_NOT_FOUND, + "Document not found: " + id)); + + org.springframework.mock.web.MockMultipartFile file = + new org.springframework.mock.web.MockMultipartFile("file", "brief.pdf", "application/pdf", new byte[]{1}); + + mockMvc.perform(multipart("/api/documents/" + id + "/file").file(file)) + .andExpect(status().isNotFound()); + } + // ─── GET /api/documents/{id}/versions/{versionId} ──────────────────────── @Test 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 2070dd46..69e127e2 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceTest.java @@ -1428,4 +1428,75 @@ class DocumentServiceTest { new MatchOffset(10, 4) // "Welt" ); } + + // ─── attachFile ─────────────────────────────────────────────────────────── + + @Test + void attachFile_throwsNotFound_whenDocumentDoesNotExist() { + UUID id = UUID.randomUUID(); + when(documentRepository.findById(id)).thenReturn(Optional.empty()); + + MockMultipartFile file = new MockMultipartFile("file", "brief.pdf", "application/pdf", new byte[]{1}); + assertThatThrownBy(() -> documentService.attachFile(id, file)) + .isInstanceOf(DomainException.class) + .hasMessageContaining(id.toString()); + } + + @Test + void attachFile_setsStatusToUploaded_whenDocumentIsPlaceholder() throws Exception { + UUID id = UUID.randomUUID(); + Document placeholder = Document.builder().id(id).status(DocumentStatus.PLACEHOLDER).build(); + when(documentRepository.findById(id)).thenReturn(Optional.of(placeholder)); + when(fileService.uploadFile(any(), any())).thenReturn(new FileService.UploadResult("s3/key.pdf", "abc123")); + when(documentRepository.save(any())).thenAnswer(inv -> inv.getArgument(0)); + + MockMultipartFile file = new MockMultipartFile("file", "brief.pdf", "application/pdf", new byte[]{1}); + Document result = documentService.attachFile(id, file); + + assertThat(result.getStatus()).isEqualTo(DocumentStatus.UPLOADED); + } + + @Test + void attachFile_doesNotChangeStatus_whenAlreadyUploaded() throws Exception { + UUID id = UUID.randomUUID(); + Document uploaded = Document.builder().id(id).status(DocumentStatus.UPLOADED).build(); + when(documentRepository.findById(id)).thenReturn(Optional.of(uploaded)); + when(fileService.uploadFile(any(), any())).thenReturn(new FileService.UploadResult("s3/key.pdf", "abc123")); + when(documentRepository.save(any())).thenAnswer(inv -> inv.getArgument(0)); + + MockMultipartFile file = new MockMultipartFile("file", "brief.pdf", "application/pdf", new byte[]{1}); + Document result = documentService.attachFile(id, file); + + assertThat(result.getStatus()).isEqualTo(DocumentStatus.UPLOADED); + } + + @Test + void attachFile_setsFilePathAndContentType_fromUploadResult() throws Exception { + UUID id = UUID.randomUUID(); + Document doc = Document.builder().id(id).status(DocumentStatus.PLACEHOLDER).build(); + when(documentRepository.findById(id)).thenReturn(Optional.of(doc)); + when(fileService.uploadFile(any(), any())) + .thenReturn(new FileService.UploadResult("s3/brief.pdf", "deadbeef")); + when(documentRepository.save(any())).thenAnswer(inv -> inv.getArgument(0)); + + MockMultipartFile file = new MockMultipartFile("file", "brief.pdf", "application/pdf", new byte[]{1}); + Document result = documentService.attachFile(id, file); + + assertThat(result.getFilePath()).isEqualTo("s3/brief.pdf"); + assertThat(result.getFileHash()).isEqualTo("deadbeef"); + assertThat(result.getContentType()).isEqualTo("application/pdf"); + assertThat(result.getOriginalFilename()).isEqualTo("brief.pdf"); + } + + @Test + void attachFile_throwsDomainException_whenFileUploadFails() throws Exception { + UUID id = UUID.randomUUID(); + Document doc = Document.builder().id(id).status(DocumentStatus.PLACEHOLDER).build(); + when(documentRepository.findById(id)).thenReturn(Optional.of(doc)); + when(fileService.uploadFile(any(), any())).thenThrow(new java.io.IOException("storage unavailable")); + + MockMultipartFile file = new MockMultipartFile("file", "brief.pdf", "application/pdf", new byte[]{1}); + assertThatThrownBy(() -> documentService.attachFile(id, file)) + .isInstanceOf(DomainException.class); + } }