diff --git a/src/main/java/org/broadinstitute/consent/http/db/FileStorageObjectDAO.java b/src/main/java/org/broadinstitute/consent/http/db/FileStorageObjectDAO.java index fa1607d59..44ded06f9 100644 --- a/src/main/java/org/broadinstitute/consent/http/db/FileStorageObjectDAO.java +++ b/src/main/java/org/broadinstitute/consent/http/db/FileStorageObjectDAO.java @@ -7,6 +7,8 @@ import org.broadinstitute.consent.http.models.FileStorageObject; import org.jdbi.v3.sqlobject.config.RegisterRowMapper; import org.jdbi.v3.sqlobject.customizer.Bind; +import org.jdbi.v3.sqlobject.customizer.BindList; +import org.jdbi.v3.sqlobject.customizer.BindList.EmptyHandling; import org.jdbi.v3.sqlobject.statement.GetGeneratedKeys; import org.jdbi.v3.sqlobject.statement.SqlQuery; import org.jdbi.v3.sqlobject.statement.SqlUpdate; @@ -146,6 +148,34 @@ default FileStorageObject findById(Integer fileStorageObjectId) { FileStorageObject findActiveFileByIdAndEntityId( @Bind("entityId") String entityId, @Bind("fileStorageObjectId") Integer fileStorageObjectId); + /** Returns the file only when it belongs to the entity and one of the allowed categories. */ + @RegisterRowMapper(FileStorageObjectMapperWithFSOPrefix.class) + @SqlQuery( + """ + SELECT + fso.file_storage_object_id AS fso_file_storage_object_id, + fso.entity_id AS fso_entity_id, + fso.file_name AS fso_file_name, + fso.category AS fso_category, + fso.gcs_file_uri AS fso_gcs_file_uri, + fso.media_type AS fso_media_type, + fso.create_date AS fso_create_date, + fso.create_user_id AS fso_create_user_id, + fso.update_date AS fso_update_date, + fso.update_user_id AS fso_update_user_id, + fso.deleted AS fso_deleted, + fso.delete_user_id AS fso_delete_user_id + FROM file_storage_object fso + WHERE fso.entity_id = :entityId + AND fso.file_storage_object_id = :fileStorageObjectId + AND (fso.deleted = false OR fso.deleted IS NULL) + AND fso.category IN () + """) + FileStorageObject findActiveFileByIdAndEntityIdAndCategories( + @Bind("entityId") String entityId, + @Bind("fileStorageObjectId") Integer fileStorageObjectId, + @BindList(value = "categories", onEmpty = EmptyHandling.NULL_STRING) List categories); + /** * Returns ALL file records for the entity, including soft-deleted ones. Callers are responsible * for filtering on {@code deleted} if needed. @@ -193,4 +223,29 @@ List findFilesByEntityIdAndCategory( ORDER BY fso.create_date DESC """) List findFileMetadataByEntityId(@Bind("entityId") String entityId); + + @UseRowMapper(FileStorageObjectMapperWithFSOPrefix.class) + @SqlQuery( + """ + SELECT + fso.file_storage_object_id AS fso_file_storage_object_id, + fso.entity_id AS fso_entity_id, + fso.file_name AS fso_file_name, + fso.category AS fso_category, + fso.gcs_file_uri AS fso_gcs_file_uri, + fso.media_type AS fso_media_type, + fso.create_date AS fso_create_date, + fso.create_user_id AS fso_create_user_id, + fso.update_date AS fso_update_date, + fso.update_user_id AS fso_update_user_id, + fso.deleted AS fso_deleted, + fso.delete_user_id AS fso_delete_user_id + FROM file_storage_object fso + WHERE fso.entity_id = :entityId + AND fso.category IN () + ORDER BY fso.create_date DESC + """) + List findFileMetadataByEntityIdAndCategories( + @Bind("entityId") String entityId, + @BindList(value = "categories", onEmpty = EmptyHandling.NULL_STRING) List categories); } diff --git a/src/main/java/org/broadinstitute/consent/http/service/FileStorageObjectService.java b/src/main/java/org/broadinstitute/consent/http/service/FileStorageObjectService.java index f0f7d1ec4..7811227d3 100644 --- a/src/main/java/org/broadinstitute/consent/http/service/FileStorageObjectService.java +++ b/src/main/java/org/broadinstitute/consent/http/service/FileStorageObjectService.java @@ -117,7 +117,7 @@ public FileStorageObject getDocument( checkAccess(user, entity, entityId, null, OperationType.READ); String resolvedEntityId = resolveEntityId(user, documentEntity, entityId); - return fetchMetadataByEntityIdAndId(resolvedEntityId, fileStorageObjectId); + return fetchMetadataByEntityIdAndId(resolvedEntityId, fileStorageObjectId, documentEntity); } /** @@ -134,7 +134,8 @@ public FileStorageObject getDocumentFile( User user, String entity, String entityId, Integer fileStorageObjectId) { DocumentEntity documentEntity = requireDocumentEntity(entity); String resolvedEntityId = resolveEntityId(user, documentEntity, entityId); - FileStorageObject fso = fetchMetadataByEntityIdAndId(resolvedEntityId, fileStorageObjectId); + FileStorageObject fso = + fetchMetadataByEntityIdAndId(resolvedEntityId, fileStorageObjectId, documentEntity); checkAccess(user, entity, entityId, fso.getCategory(), OperationType.READ); @@ -167,7 +168,7 @@ public List listDocuments(User user, String entity, String en checkAccess(user, entity, entityId, null, OperationType.READ); String resolvedEntityId = resolveEntityId(user, documentEntity, entityId); - return fetchAllMetadataByEntityId(resolvedEntityId); + return fetchAllMetadataByEntityId(resolvedEntityId, documentEntity); } /** @@ -188,7 +189,7 @@ public FileStorageObject updateDocumentCategory( String resolvedEntityId = resolveEntityId(user, documentEntity, entityId); FileStorageObject fileStorageObject = - fetchMetadataByEntityIdAndId(resolvedEntityId, fileStorageObjectId); + fetchMetadataByEntityIdAndId(resolvedEntityId, fileStorageObjectId, documentEntity); Instant updateDate = Instant.now(); fileStorageObjectDAO.updateCategory(fileStorageObjectId, category.getValue(), user.getUserId()); @@ -219,7 +220,7 @@ public FileStorageObject deleteDocument( DocumentEntity documentEntity = requireDocumentEntity(entity); String resolvedEntityId = resolveEntityId(user, documentEntity, entityId); FileStorageObject fileStorageObject = - fetchMetadataByEntityIdAndId(resolvedEntityId, fileStorageObjectId); + fetchMetadataByEntityIdAndId(resolvedEntityId, fileStorageObjectId, documentEntity); checkAccess(user, entity, entityId, fileStorageObject.getCategory(), OperationType.WRITE); @@ -503,6 +504,22 @@ public FileStorageObject fetchMetadataByEntityIdAndId( return fileStorageObject; } + /** + * Fetches metadata for an active file constrained to categories valid for the requested entity. + * This prevents records for another document domain from being returned when IDs overlap. + */ + public FileStorageObject fetchMetadataByEntityIdAndId( + String entityId, Integer fileStorageObjectId, DocumentEntity entity) + throws NotFoundException { + FileStorageObject fileStorageObject = + fileStorageObjectDAO.findActiveFileByIdAndEntityIdAndCategories( + entityId, fileStorageObjectId, allowedCategoryValuesForEntity(entity)); + if (fileStorageObject == null) { + throw new NotFoundException("File not found"); + } + return fileStorageObject; + } + /** * Returns ALL file metadata for the entity, including soft-deleted records. Callers should * inspect {@link FileStorageObject#getDeleted()} and filter as appropriate for their use case. @@ -511,6 +528,13 @@ public List fetchAllMetadataByEntityId(String entityId) { return fileStorageObjectDAO.findFileMetadataByEntityId(entityId); } + /** Returns file metadata scoped to categories valid for the requested entity. */ + public List fetchAllMetadataByEntityId( + String entityId, DocumentEntity entity) { + return fileStorageObjectDAO.findFileMetadataByEntityIdAndCategories( + entityId, allowedCategoryValuesForEntity(entity)); + } + /** * Returns ALL files for the entity (active + deleted), populated with GCS content. Callers should * inspect {@link FileStorageObject#getDeleted()} and filter as appropriate. @@ -597,6 +621,18 @@ private boolean isCategoryAllowedForEntity(DocumentEntity entity, FileCategory c }; } + private List allowedCategoryValuesForEntity(DocumentEntity entity) { + return switch (entity) { + case DAR -> + List.of( + FileCategory.IRB_COLLABORATION_LETTER.getValue(), + FileCategory.DATA_USE_LETTER.getValue()); + case DAC -> List.of(FileCategory.DATA_ACCESS_AGREEMENT.getValue()); + case DATASET -> List.of(FileCategory.NIH_INSTITUTIONAL_CERTIFICATION.getValue()); + case STUDY -> List.of(FileCategory.ALTERNATIVE_DATA_SHARING_PLAN.getValue()); + }; + } + private Integer parseNumericEntityId(String entityId) { try { return Integer.valueOf(entityId); diff --git a/src/test/java/org/broadinstitute/consent/http/db/FileStorageObjectDAOTest.java b/src/test/java/org/broadinstitute/consent/http/db/FileStorageObjectDAOTest.java index 914b71c16..350f786a0 100644 --- a/src/test/java/org/broadinstitute/consent/http/db/FileStorageObjectDAOTest.java +++ b/src/test/java/org/broadinstitute/consent/http/db/FileStorageObjectDAOTest.java @@ -271,6 +271,30 @@ void testFindFileMetadataByEntityId() { filesFound.forEach(file -> assertEquals(entityId, file.getEntityId())); } + @Test + void testFindFileMetadataByEntityIdAndCategoriesFiltersCategory() { + String entityId = randomAlphabetic(10); + + FileStorageObject matching1 = + createFileStorageObject(entityId, FileCategory.IRB_COLLABORATION_LETTER); + FileStorageObject matching2 = createFileStorageObject(entityId, FileCategory.DATA_USE_LETTER); + createFileStorageObject(entityId, FileCategory.DATA_ACCESS_AGREEMENT); + createFileStorageObject(randomAlphabetic(8), FileCategory.IRB_COLLABORATION_LETTER); + + List filesFound = + fileStorageObjectDAO.findFileMetadataByEntityIdAndCategories( + entityId, + List.of( + FileCategory.IRB_COLLABORATION_LETTER.getValue(), + FileCategory.DATA_USE_LETTER.getValue())); + + assertEquals(2, filesFound.size()); + List foundIds = + filesFound.stream().map(FileStorageObject::getFileStorageObjectId).toList(); + assertTrue(foundIds.contains(matching1.getFileStorageObjectId())); + assertTrue(foundIds.contains(matching2.getFileStorageObjectId())); + } + @Test void testFindActiveFileByIdAndEntityId() { String entityId = randomAlphabetic(10); @@ -309,6 +333,20 @@ void testFindActiveFileByIdAndEntityIdWrongEntityReturnsNull() { assertNull(found); } + @Test + void testFindActiveFileByIdAndEntityIdAndCategoriesWrongCategoryReturnsNull() { + String entityId = randomAlphabetic(10); + FileStorageObject file = createFileStorageObject(entityId, FileCategory.DATA_ACCESS_AGREEMENT); + + FileStorageObject found = + fileStorageObjectDAO.findActiveFileByIdAndEntityIdAndCategories( + entityId, + file.getFileStorageObjectId(), + List.of(FileCategory.IRB_COLLABORATION_LETTER.getValue())); + + assertNull(found); + } + @Test void testCreateFile() { String fileName = randomAlphabetic(10); diff --git a/src/test/java/org/broadinstitute/consent/http/service/FileStorageObjectServiceTest.java b/src/test/java/org/broadinstitute/consent/http/service/FileStorageObjectServiceTest.java index aba9cec1e..f5f27d4c0 100644 --- a/src/test/java/org/broadinstitute/consent/http/service/FileStorageObjectServiceTest.java +++ b/src/test/java/org/broadinstitute/consent/http/service/FileStorageObjectServiceTest.java @@ -299,7 +299,10 @@ void testFetchMetadataByEntityAndEntityIdForReadDataset() { FileStorageObject fileStorageObject = new FileStorageObject(); when(datasetService.findDatasetByIdForRead(user, datasetId)).thenReturn(new Dataset()); - when(fileStorageObjectDAO.findActiveFileByIdAndEntityId(datasetId.toString(), fileId)) + when(fileStorageObjectDAO.findActiveFileByIdAndEntityIdAndCategories( + datasetId.toString(), + fileId, + List.of(FileCategory.NIH_INSTITUTIONAL_CERTIFICATION.getValue()))) .thenReturn(fileStorageObject); initService(); @@ -319,7 +322,9 @@ void testAllFetchMetadataByEntityAndEntityIdForReadStudy() { List fileStorageObjects = List.of(new FileStorageObject()); when(datasetService.findStudyByIdForRead(user, studyId)).thenReturn(study); - when(fileStorageObjectDAO.findFileMetadataByEntityId(study.getUuid().toString())) + when(fileStorageObjectDAO.findFileMetadataByEntityIdAndCategories( + study.getUuid().toString(), + List.of(FileCategory.ALTERNATIVE_DATA_SHARING_PLAN.getValue()))) .thenReturn(fileStorageObjects); initService(); @@ -352,14 +357,19 @@ void testListDocumentsForStudyAccessByRole(String role, boolean allowed) { } when(datasetService.findStudyByIdForRead(user, studyId)).thenReturn(study); - when(fileStorageObjectDAO.findFileMetadataByEntityId(study.getUuid().toString())) + when(fileStorageObjectDAO.findFileMetadataByEntityIdAndCategories( + study.getUuid().toString(), + List.of(FileCategory.ALTERNATIVE_DATA_SHARING_PLAN.getValue()))) .thenReturn(fileStorageObjects); List returnedFiles = service.listDocuments(user, "study", studyId.toString()); assertEquals(fileStorageObjects, returnedFiles); - verify(fileStorageObjectDAO).findFileMetadataByEntityId(study.getUuid().toString()); + verify(fileStorageObjectDAO) + .findFileMetadataByEntityIdAndCategories( + study.getUuid().toString(), + List.of(FileCategory.ALTERNATIVE_DATA_SHARING_PLAN.getValue())); } private void applyRole(User user, String role) { @@ -378,7 +388,8 @@ void testFetchAllMetadataByEntityAndEntityIdForReadDac() { List fileStorageObjects = List.of(new FileStorageObject()); when(dacService.findById(dacId)).thenReturn(new Dac()); - when(fileStorageObjectDAO.findFileMetadataByEntityId(dacId.toString())) + when(fileStorageObjectDAO.findFileMetadataByEntityIdAndCategories( + dacId.toString(), List.of(FileCategory.DATA_ACCESS_AGREEMENT.getValue()))) .thenReturn(fileStorageObjects); initService(); @@ -386,7 +397,9 @@ void testFetchAllMetadataByEntityAndEntityIdForReadDac() { List returnedFiles = service.listDocuments(user, "dac", dacId.toString()); assertEquals(fileStorageObjects, returnedFiles); - verify(fileStorageObjectDAO).findFileMetadataByEntityId(dacId.toString()); + verify(fileStorageObjectDAO) + .findFileMetadataByEntityIdAndCategories( + dacId.toString(), List.of(FileCategory.DATA_ACCESS_AGREEMENT.getValue())); } @Test @@ -399,7 +412,11 @@ void testFetchAllMetadataByEntityAndEntityIdForReadDar() { dar.setUserId(user.getUserId()); when(dataAccessRequestService.findByReferenceId(darReferenceId)).thenReturn(dar); - when(fileStorageObjectDAO.findFileMetadataByEntityId(darReferenceId)) + when(fileStorageObjectDAO.findFileMetadataByEntityIdAndCategories( + darReferenceId, + List.of( + FileCategory.IRB_COLLABORATION_LETTER.getValue(), + FileCategory.DATA_USE_LETTER.getValue()))) .thenReturn(fileStorageObjects); initService(); @@ -407,7 +424,12 @@ void testFetchAllMetadataByEntityAndEntityIdForReadDar() { List returnedFiles = service.listDocuments(user, "dar", darReferenceId); assertEquals(fileStorageObjects, returnedFiles); - verify(fileStorageObjectDAO).findFileMetadataByEntityId(darReferenceId); + verify(fileStorageObjectDAO) + .findFileMetadataByEntityIdAndCategories( + darReferenceId, + List.of( + FileCategory.IRB_COLLABORATION_LETTER.getValue(), + FileCategory.DATA_USE_LETTER.getValue())); } @Test @@ -421,7 +443,10 @@ void testFetchMetadataByEntityAndEntityIdForReadStudy() { FileStorageObject fileStorageObject = new FileStorageObject(); when(datasetService.findStudyByIdForRead(user, studyId)).thenReturn(study); - when(fileStorageObjectDAO.findActiveFileByIdAndEntityId(study.getUuid().toString(), fileId)) + when(fileStorageObjectDAO.findActiveFileByIdAndEntityIdAndCategories( + study.getUuid().toString(), + fileId, + List.of(FileCategory.ALTERNATIVE_DATA_SHARING_PLAN.getValue()))) .thenReturn(fileStorageObject); initService(); @@ -511,7 +536,10 @@ void testGetDocumentFileByEntityAndEntityIdForReadDataset() throws Exception { byte[] content = "streamed-file-content".getBytes(); when(datasetService.findDatasetByIdForRead(user, datasetId)).thenReturn(new Dataset()); - when(fileStorageObjectDAO.findActiveFileByIdAndEntityId(datasetId.toString(), fileId)) + when(fileStorageObjectDAO.findActiveFileByIdAndEntityIdAndCategories( + datasetId.toString(), + fileId, + List.of(FileCategory.NIH_INSTITUTIONAL_CERTIFICATION.getValue()))) .thenReturn(fileStorageObject); when(gcsService.getDocument(fileStorageObject.getBlobId())) .thenReturn(new ByteArrayInputStream(content)); @@ -523,7 +551,11 @@ void testGetDocumentFileByEntityAndEntityIdForReadDataset() throws Exception { assertEquals(fileStorageObject, returned); assertArrayEquals(content, returned.getUploadedFile().readAllBytes()); - verify(fileStorageObjectDAO).findActiveFileByIdAndEntityId(datasetId.toString(), fileId); + verify(fileStorageObjectDAO) + .findActiveFileByIdAndEntityIdAndCategories( + datasetId.toString(), + fileId, + List.of(FileCategory.NIH_INSTITUTIONAL_CERTIFICATION.getValue())); verify(gcsService).getDocument(fileStorageObject.getBlobId()); } @@ -540,7 +572,8 @@ void testGetDocumentFileThrowsBadGatewayWhenGcsFails() { fileStorageObject.setCategory(FileCategory.NIH_INSTITUTIONAL_CERTIFICATION); when(datasetService.findDatasetByIdForRead(user, datasetId)).thenReturn(new Dataset()); - when(fileStorageObjectDAO.findActiveFileByIdAndEntityId(entityId, fileId)) + when(fileStorageObjectDAO.findActiveFileByIdAndEntityIdAndCategories( + entityId, fileId, List.of(FileCategory.NIH_INSTITUTIONAL_CERTIFICATION.getValue()))) .thenReturn(fileStorageObject); when(gcsService.getDocument(fileStorageObject.getBlobId())) .thenThrow(new RuntimeException("GCS unavailable")); @@ -563,7 +596,9 @@ void testGetDocumentFileThrowsNotFoundWhenMetadataLookupFails() { String entityId = datasetId.toString(); when(datasetService.findDatasetByIdForRead(user, datasetId)).thenReturn(new Dataset()); - when(fileStorageObjectDAO.findActiveFileByIdAndEntityId(entityId, fileId)).thenReturn(null); + when(fileStorageObjectDAO.findActiveFileByIdAndEntityIdAndCategories( + entityId, fileId, List.of(FileCategory.NIH_INSTITUTIONAL_CERTIFICATION.getValue()))) + .thenReturn(null); initService(); @@ -595,7 +630,9 @@ void testDeleteDocumentSetsDeletedFieldsAndCallsDao() { deleted.setDeleteDate(java.time.Instant.now()); when(datasetService.findDatasetByIdForRead(user, datasetId)).thenReturn(new Dataset()); - when(fileStorageObjectDAO.findActiveFileByIdAndEntityId(entityId, fileId)).thenReturn(active); + when(fileStorageObjectDAO.findActiveFileByIdAndEntityIdAndCategories( + entityId, fileId, List.of(FileCategory.NIH_INSTITUTIONAL_CERTIFICATION.getValue()))) + .thenReturn(active); when(fileStorageObjectDAO.findById(fileId)).thenReturn(deleted); initService(); @@ -617,7 +654,9 @@ void testDeleteDocumentThrowsNotFoundWhenFileAlreadyDeleted() { String entityId = datasetId.toString(); when(datasetService.findDatasetByIdForRead(user, datasetId)).thenReturn(new Dataset()); - when(fileStorageObjectDAO.findActiveFileByIdAndEntityId(entityId, fileId)).thenReturn(null); + when(fileStorageObjectDAO.findActiveFileByIdAndEntityIdAndCategories( + entityId, fileId, List.of(FileCategory.NIH_INSTITUTIONAL_CERTIFICATION.getValue()))) + .thenReturn(null); initService(); @@ -654,7 +693,9 @@ void testUpdateDocumentCategoryUpdatesCategoryAndAuditFields() { updated.setUpdateDate(java.time.Instant.now()); when(datasetService.findDatasetByIdForRead(user, datasetId)).thenReturn(new Dataset()); - when(fileStorageObjectDAO.findActiveFileByIdAndEntityId(entityId, fileId)).thenReturn(active); + when(fileStorageObjectDAO.findActiveFileByIdAndEntityIdAndCategories( + entityId, fileId, List.of(FileCategory.NIH_INSTITUTIONAL_CERTIFICATION.getValue()))) + .thenReturn(active); when(fileStorageObjectDAO.findById(fileId)).thenReturn(updated); initService(); @@ -696,7 +737,9 @@ void testUpdateDocumentCategoryThrowsNotFoundWhenFileDeletedOrMissing() { String entityId = datasetId.toString(); when(datasetService.findDatasetByIdForRead(user, datasetId)).thenReturn(new Dataset()); - when(fileStorageObjectDAO.findActiveFileByIdAndEntityId(entityId, fileId)).thenReturn(null); + when(fileStorageObjectDAO.findActiveFileByIdAndEntityIdAndCategories( + entityId, fileId, List.of(FileCategory.NIH_INSTITUTIONAL_CERTIFICATION.getValue()))) + .thenReturn(null); initService();