From 06b0c46b1a16d27e237a43afa41ec1110927d435 Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Fri, 31 May 2024 15:56:04 +0200 Subject: [PATCH 01/12] Impl. file upload endpoint Currently only implemented for S3 storage type. --- build.gradle | 2 + .../appserver/config/ApplicationConfig.java | 2 +- .../appserver/config/S3StorageProperties.java | 32 +++++ .../appserver/controller/PathsUtil.java | 2 + .../controller/UploadController.java | 76 ++++++++++++ .../storage/MinioClientInitializer.java | 67 +++++++++++ .../RandomUuidFilenameStorageService.java | 41 +++++++ .../service/storage/S3StorageService.java | 73 ++++++++++++ .../service/storage/StorageService.java | 7 ++ .../resources/application-prod.properties | 8 ++ .../controller/UploadControllerTest.java | 84 +++++++++++++ .../service/storage/S3StorageServiceTest.java | 110 ++++++++++++++++++ 12 files changed, 503 insertions(+), 1 deletion(-) create mode 100644 src/main/java/org/radarbase/appserver/config/S3StorageProperties.java create mode 100644 src/main/java/org/radarbase/appserver/controller/UploadController.java create mode 100644 src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java create mode 100644 src/main/java/org/radarbase/appserver/service/storage/RandomUuidFilenameStorageService.java create mode 100644 src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java create mode 100644 src/main/java/org/radarbase/appserver/service/storage/StorageService.java create mode 100644 src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java create mode 100644 src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java diff --git a/build.gradle b/build.gradle index 7a4abdcb..696e705c 100644 --- a/build.gradle +++ b/build.gradle @@ -41,6 +41,7 @@ ext { radarSpringAuthVersion = '1.2.1' springSecurityVersion = '6.0.2' hibernateValidatorVersion = '8.0.0.Final' + minioVersion = '8.5.10' } sourceSets { @@ -65,6 +66,7 @@ dependencies { implementation('org.springframework.security.oauth.boot:spring-security-oauth2-autoconfigure:' + springBootVersion) implementation('org.springframework.security.oauth:spring-security-oauth2:' + springOauth2Version) runtimeOnly("org.hibernate.validator:hibernate-validator:$hibernateValidatorVersion") + implementation("io.minio:minio:$minioVersion") // Open API spec implementation(group: 'org.springdoc', name: 'springdoc-openapi-starter-webmvc-ui', version: springDocVersion) diff --git a/src/main/java/org/radarbase/appserver/config/ApplicationConfig.java b/src/main/java/org/radarbase/appserver/config/ApplicationConfig.java index 33fb511f..a68e95f8 100644 --- a/src/main/java/org/radarbase/appserver/config/ApplicationConfig.java +++ b/src/main/java/org/radarbase/appserver/config/ApplicationConfig.java @@ -31,7 +31,7 @@ @Configuration @EnableJpaAuditing -@EnableConfigurationProperties({FcmServerConfig.class}) +@EnableConfigurationProperties({FcmServerConfig.class, S3StorageProperties.class}) @EnableTransactionManagement @EnableAsync @EnableScheduling diff --git a/src/main/java/org/radarbase/appserver/config/S3StorageProperties.java b/src/main/java/org/radarbase/appserver/config/S3StorageProperties.java new file mode 100644 index 00000000..439c9239 --- /dev/null +++ b/src/main/java/org/radarbase/appserver/config/S3StorageProperties.java @@ -0,0 +1,32 @@ +/* + * + * * Copyright 2024 The Hyve + * * + * * Licensed under the Apache License, Version 2.0 (the "License"); + * * you may not use this file except in compliance with the License. + * * You may obtain a copy of the License at + * * + * * http://www.apache.org/licenses/LICENSE-2.0 + * * + * * Unless required by applicable law or agreed to in writing, software + * * distributed under the License is distributed on an "AS IS" BASIS, + * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * * See the License for the specific language governing permissions and + * * limitations under the License. + * + */ + +package org.radarbase.appserver.config; + +import lombok.Data; +import org.springframework.boot.context.properties.ConfigurationProperties; + +@Data +@ConfigurationProperties("storage.s3") +public class S3StorageProperties { + private String url; + private String accessKey; + private String secretKey; + private String bucketName; + private String subPath; +} diff --git a/src/main/java/org/radarbase/appserver/controller/PathsUtil.java b/src/main/java/org/radarbase/appserver/controller/PathsUtil.java index edfeb430..0157f7c4 100644 --- a/src/main/java/org/radarbase/appserver/controller/PathsUtil.java +++ b/src/main/java/org/radarbase/appserver/controller/PathsUtil.java @@ -30,11 +30,13 @@ public class PathsUtil { public static final String USER_PATH = "users"; public static final String PROJECT_PATH = "projects"; + public static final String TOPIC_PATH = "topics"; public static final String MESSAGING_NOTIFICATION_PATH = "messaging/notifications"; public static final String MESSAGING_DATA_PATH = "messaging/data"; public static final String PROTOCOL_PATH = "protocols"; public static final String PROJECT_ID_CONSTANT = "{projectId}"; public static final String SUBJECT_ID_CONSTANT = "{subjectId}"; + public static final String TOPIC_ID_CONSTANT = "{topicId}"; public static final String NOTIFICATION_ID_CONSTANT = "{notificationId}"; public static final String NOTIFICATION_STATE_EVENTS_PATH = "state_events"; public static final String QUESTIONNAIRE_SCHEDULE_PATH = "questionnaire/schedule"; diff --git a/src/main/java/org/radarbase/appserver/controller/UploadController.java b/src/main/java/org/radarbase/appserver/controller/UploadController.java new file mode 100644 index 00000000..e0611771 --- /dev/null +++ b/src/main/java/org/radarbase/appserver/controller/UploadController.java @@ -0,0 +1,76 @@ +/* + * + * * Copyright 2024 The Hyve + * * + * * Licensed under the Apache License, Version 2.0 (the "License"); + * * you may not use this file except in compliance with the License. + * * You may obtain a copy of the License at + * * + * * http://www.apache.org/licenses/LICENSE-2.0 + * * + * * Unless required by applicable law or agreed to in writing, software + * * distributed under the License is distributed on an "AS IS" BASIS, + * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * * See the License for the specific language governing permissions and + * * limitations under the License. + * + */ + +package org.radarbase.appserver.controller; + +import lombok.extern.slf4j.Slf4j; +import org.radarbase.appserver.config.AuthConfig.AuthEntities; +import org.radarbase.appserver.config.AuthConfig.AuthPermissions; +import org.radarbase.appserver.service.storage.StorageService; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.CrossOrigin; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.multipart.MultipartFile; +import radar.spring.auth.common.Authorized; +import radar.spring.auth.common.PermissionOn; + +import java.net.URI; +import java.net.URISyntaxException; + +/** + * Resource Endpoint for uploading assets to a data store. + * + * @author Pim van Nierop + */ +@CrossOrigin +@RestController +@ConditionalOnProperty(value = "file-upload.enabled", havingValue = "true") +@Slf4j +public class UploadController { + + @Autowired + private StorageService storageService; + + @Authorized( + permission = AuthPermissions.UPDATE, + entity = AuthEntities.SUBJECT, + permissionOn = PermissionOn.SUBJECT + ) + @PostMapping( + "/" + PathsUtil.PROJECT_PATH + "/" + PathsUtil.PROJECT_ID_CONSTANT + + "/" + PathsUtil.USER_PATH + "/" + PathsUtil.SUBJECT_ID_CONSTANT + + "/" + PathsUtil.TOPIC_PATH + "/" + PathsUtil.TOPIC_ID_CONSTANT + + "/upload") + public ResponseEntity subjectFileUpload( + @RequestParam("file") MultipartFile file, + @PathVariable String projectId, + @PathVariable String subjectId, + @PathVariable String topicId) throws URISyntaxException { + + log.info("Storing file for project: {}, subject: {}, topic: {}", projectId, subjectId, topicId); + + String filePath = storageService.store(file, projectId, subjectId, topicId); + return ResponseEntity.created(new URI(filePath)).build(); + } + +} diff --git a/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java b/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java new file mode 100644 index 00000000..5ee6a2b6 --- /dev/null +++ b/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java @@ -0,0 +1,67 @@ +/* + * + * * Copyright 2024 The Hyve + * * + * * Licensed under the Apache License, Version 2.0 (the "License"); + * * you may not use this file except in compliance with the License. + * * You may obtain a copy of the License at + * * + * * http://www.apache.org/licenses/LICENSE-2.0 + * * + * * Unless required by applicable law or agreed to in writing, software + * * distributed under the License is distributed on an "AS IS" BASIS, + * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * * See the License for the specific language governing permissions and + * * limitations under the License. + * + */ + +package org.radarbase.appserver.service.storage; + +import io.minio.BucketExistsArgs; +import io.minio.MinioClient; +import org.radarbase.appserver.config.S3StorageProperties; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression; +import org.springframework.stereotype.Component; + +import javax.annotation.PostConstruct; + +@Component +@ConditionalOnExpression("${file-upload.enabled:false} and 's3' == '${storage.type:}'") +public class MinioClientInitializer { + + private MinioClient minioClient; + private String bucketName; + + @Autowired + private S3StorageProperties s3StorageProperties; + + @PostConstruct + public void init() { + try { + minioClient = + MinioClient.builder() + .endpoint(s3StorageProperties.getUrl()) + .credentials(s3StorageProperties.getAccessKey(), s3StorageProperties.getSecretKey()) + .build(); + bucketName = s3StorageProperties.getBucketName(); + boolean found = + minioClient.bucketExists(BucketExistsArgs.builder().bucket(bucketName).build()); + if (!found) { + throw new RuntimeException(String.format("S3 bucket '%s' does not exist", bucketName)); + } + } catch (Exception e) { + throw new RuntimeException("Could not connect to S3", e); + } + } + + public MinioClient getClient() { + return minioClient; + } + + public String getBucketName() { + return bucketName; + } + +} diff --git a/src/main/java/org/radarbase/appserver/service/storage/RandomUuidFilenameStorageService.java b/src/main/java/org/radarbase/appserver/service/storage/RandomUuidFilenameStorageService.java new file mode 100644 index 00000000..f4de12dd --- /dev/null +++ b/src/main/java/org/radarbase/appserver/service/storage/RandomUuidFilenameStorageService.java @@ -0,0 +1,41 @@ +/* + * + * * Copyright 2024 The Hyve + * * + * * Licensed under the Apache License, Version 2.0 (the "License"); + * * you may not use this file except in compliance with the License. + * * You may obtain a copy of the License at + * * + * * http://www.apache.org/licenses/LICENSE-2.0 + * * + * * Unless required by applicable law or agreed to in writing, software + * * distributed under the License is distributed on an "AS IS" BASIS, + * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * * See the License for the specific language governing permissions and + * * limitations under the License. + * + */ + +package org.radarbase.appserver.service.storage; + +import java.util.UUID; + +public abstract class RandomUuidFilenameStorageService { + + // Storing files under their original filename is a security risk, as it can be used to + // overwrite existing files. This method generates a random filename to mitigate this risk. + // See https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload + String generateRandomFilename(String originalFilename) { + return UUID.randomUUID() + getFileExtension(originalFilename); + } + + private String getFileExtension(String originalFilename) { + int lastDot = originalFilename.lastIndexOf('.'); + if (lastDot < 0) { + return ""; + } else { + return originalFilename.substring(lastDot); + } + } + +} diff --git a/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java new file mode 100644 index 00000000..1f413e77 --- /dev/null +++ b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java @@ -0,0 +1,73 @@ +/* + * + * * Copyright 2024 The Hyve + * * + * * Licensed under the Apache License, Version 2.0 (the "License"); + * * you may not use this file except in compliance with the License. + * * You may obtain a copy of the License at + * * + * * http://www.apache.org/licenses/LICENSE-2.0 + * * + * * Unless required by applicable law or agreed to in writing, software + * * distributed under the License is distributed on an "AS IS" BASIS, + * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * * See the License for the specific language governing permissions and + * * limitations under the License. + * + */ + +package org.radarbase.appserver.service.storage; + +import io.minio.PutObjectArgs; +import lombok.extern.slf4j.Slf4j; +import org.radarbase.appserver.config.S3StorageProperties; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression; +import org.springframework.stereotype.Service; +import org.springframework.util.Assert; +import org.springframework.web.multipart.MultipartFile; + +import javax.annotation.PostConstruct; + + +@Slf4j +@Service +@ConditionalOnExpression("${file-upload.enabled:false} and 's3' == '${storage.type:}'") +public class S3StorageService extends RandomUuidFilenameStorageService implements StorageService { + + @Autowired + private S3StorageProperties s3StorageProperties; + + @Autowired + private MinioClientInitializer bucketClient; + private String subPath = ""; + + @PostConstruct + public void init() { + if (s3StorageProperties.getSubPath() != null) { + subPath = s3StorageProperties.getSubPath().replaceAll("^/|/$", ""); + if (!subPath.isEmpty()) { + subPath = subPath + "/"; + } + } + } + + public String store(MultipartFile file, String projectId, String subjectId, String topicId) { + Assert.notNull(file, "File must not be null"); + Assert.notEmpty(new String[]{projectId, subjectId, topicId}, "Project, subject and topic IDs must not be empty"); + String filePath = String.format("%s%s/%s/%s/%s", subPath, projectId, subjectId, topicId, generateRandomFilename(file.getOriginalFilename())); + log.debug("Storing file at path: {}", filePath); + try { + bucketClient.getClient().putObject(PutObjectArgs + .builder() + .bucket(bucketClient.getBucketName()) + .object(filePath) + .stream(file.getInputStream(), file.getSize(), -1) + .build()); + } catch (Exception e) { + throw new RuntimeException("Could not store file", e); + } + return filePath; + } + +} diff --git a/src/main/java/org/radarbase/appserver/service/storage/StorageService.java b/src/main/java/org/radarbase/appserver/service/storage/StorageService.java new file mode 100644 index 00000000..97a5aaf1 --- /dev/null +++ b/src/main/java/org/radarbase/appserver/service/storage/StorageService.java @@ -0,0 +1,7 @@ +package org.radarbase.appserver.service.storage; + +import org.springframework.web.multipart.MultipartFile; + +public interface StorageService { + String store(MultipartFile file, String projectId, String subjectId, String topicId); +} diff --git a/src/main/resources/application-prod.properties b/src/main/resources/application-prod.properties index 54ec9503..9580719b 100644 --- a/src/main/resources/application-prod.properties +++ b/src/main/resources/application-prod.properties @@ -88,3 +88,11 @@ security.github.client.maxContentLength=1000000 security.github.cache.size=10000 security.github.cache.duration=3600 security.github.cache.retryDuration=60 + +# DATA UPLOAD +file-upload.enabled=true +storage.type=s3 # can be 's3' +storage.s3.url=http://localhost:9000 +storage.s3.access-key=access-key +storage.s3.secret-key=secret-key +storage.s3.bucket-name=radar \ No newline at end of file diff --git a/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java b/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java new file mode 100644 index 00000000..0ab534e2 --- /dev/null +++ b/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java @@ -0,0 +1,84 @@ +/* + * + * * Copyright 2024 The Hyve + * * + * * Licensed under the Apache License, Version 2.0 (the "License"); + * * you may not use this file except in compliance with the License. + * * You may obtain a copy of the License at + * * + * * http://www.apache.org/licenses/LICENSE-2.0 + * * + * * Unless required by applicable law or agreed to in writing, software + * * distributed under the License is distributed on an "AS IS" BASIS, + * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * * See the License for the specific language governing permissions and + * * limitations under the License. + * + */ + +package org.radarbase.appserver.controller; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.radarbase.appserver.service.storage.StorageService; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.mock.web.MockMultipartFile; +import org.springframework.test.context.TestPropertySource; +import org.springframework.test.context.junit.jupiter.SpringExtension; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; + +import static org.hamcrest.Matchers.is; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.BDDMockito.given; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +@ExtendWith(SpringExtension.class) +@WebMvcTest(UploadController.class) +@AutoConfigureMockMvc(addFilters = false) +@TestPropertySource(properties = { + "file-upload.enabled=true" +}) +class UploadControllerTest { + + @Autowired private transient MockMvc mockMvc; + + @MockBean private transient StorageService storageService; + + private static final String PROJECT_ID = "my-project"; + private static final String SUBJECT_ID = "my-subject"; + private static final String TOPIC_ID = "my-topic"; + private static final byte[] file = "my-file-content".getBytes(); + private static final String FILE_PATH = "my-file-path/UUID.txt"; + + @BeforeEach + void setUp() { + given(storageService.store(any(), eq(PROJECT_ID), eq(SUBJECT_ID), eq(TOPIC_ID))) + .willReturn(FILE_PATH); + } + + MockMultipartFile multipartFile = new MockMultipartFile( + "file", "my-file.txt", "text/plain", file + ); + + @Test + void testUploadFile() throws Exception { + String uri = String.format( + "/projects/%s/users/%s/topics/%s/upload", PROJECT_ID, SUBJECT_ID, TOPIC_ID); + mockMvc + .perform( + MockMvcRequestBuilders + .multipart(uri) + .file(multipartFile)) + .andExpect(status().isCreated()) + .andExpect(header().exists("Location")) + .andExpect(header().string("Location", is(FILE_PATH))); + } + +} diff --git a/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java b/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java new file mode 100644 index 00000000..8c596690 --- /dev/null +++ b/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java @@ -0,0 +1,110 @@ +/* + * + * * Copyright 2024 The Hyve + * * + * * Licensed under the Apache License, Version 2.0 (the "License"); + * * you may not use this file except in compliance with the License. + * * You may obtain a copy of the License at + * * + * * http://www.apache.org/licenses/LICENSE-2.0 + * * + * * Unless required by applicable law or agreed to in writing, software + * * distributed under the License is distributed on an "AS IS" BASIS, + * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * * See the License for the specific language governing permissions and + * * limitations under the License. + * + */ + +package org.radarbase.appserver.service.storage; + +import io.minio.MinioClient; +import io.minio.errors.ErrorResponseException; +import io.minio.errors.InsufficientDataException; +import io.minio.errors.InternalException; +import io.minio.errors.InvalidResponseException; +import io.minio.errors.ServerException; +import io.minio.errors.XmlParserException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.radarbase.appserver.config.S3StorageProperties; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.mock.web.MockMultipartFile; +import org.springframework.test.context.junit.jupiter.SpringExtension; +import org.springframework.web.multipart.MultipartFile; + +import java.io.IOException; +import java.security.InvalidKeyException; +import java.security.NoSuchAlgorithmException; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.*; +import static org.mockito.Mockito.mock; + +@ExtendWith(SpringExtension.class) +@SpringBootTest( + classes = {S3StorageService.class}, + properties = { + "file-upload.enabled=true", + "storage.type=s3", + "storage.s3.bucket-name=my-bucket", + "storage.s3.sub-path=my-sub-path", + } +) +@EnableConfigurationProperties({S3StorageProperties.class}) +class S3StorageServiceTest { + + @Autowired + private S3StorageService s3StorageService; + + @MockBean + private MinioClientInitializer minioInit; + + @Mock + private MinioClient minioClient; + + MockMultipartFile multipartFile = new MockMultipartFile( + "file", "my-file.txt", "text/plain", "my-file-content".getBytes() + ); + + @BeforeEach + public void setUp() throws Exception { + given(minioInit.getBucketName()).willReturn("my-bucket"); + given(minioInit.getClient()).willReturn(minioClient); + given(minioClient.putObject(any())).willReturn(null); + } + + @Test + void testInvalidArguments() { + assertThrows(Exception.class, () -> s3StorageService.store(null, "projectId", "subjectId", "topicId")); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), null, "subjectId", "topicId")); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), "projectId", null, "topicId")); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), "projectId", "subjectId", null)); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), "", "subjectId", "topicId")); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), "projectId", "", "topicId")); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), "projectId", "subjectId", "")); + } + + @Test + void testStore() throws Exception { + String path = s3StorageService.store(multipartFile, "projectId", "subjectId", "topicId"); + // Make sure that MinioClient.putObject is called with the correct arguments. + verify(minioClient).putObject(argThat(args -> args.bucket().equals("my-bucket") + && args.object().startsWith("my-sub-path/projectId/subjectId/topicId/") + && args.object().endsWith(".txt")) + ); + + assertTrue(path.startsWith("my-sub-path/projectId/subjectId/topicId/")); + } + +} From 9eb55a87d61b56acdf44905e6d2f7104c8d7adfb Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Tue, 2 Jul 2024 14:28:14 +0200 Subject: [PATCH 02/12] Add '/files' to the REST path for file upload --- src/main/java/org/radarbase/appserver/controller/PathsUtil.java | 1 + .../org/radarbase/appserver/controller/UploadController.java | 1 + 2 files changed, 2 insertions(+) diff --git a/src/main/java/org/radarbase/appserver/controller/PathsUtil.java b/src/main/java/org/radarbase/appserver/controller/PathsUtil.java index 0157f7c4..032493dc 100644 --- a/src/main/java/org/radarbase/appserver/controller/PathsUtil.java +++ b/src/main/java/org/radarbase/appserver/controller/PathsUtil.java @@ -31,6 +31,7 @@ public class PathsUtil { public static final String USER_PATH = "users"; public static final String PROJECT_PATH = "projects"; public static final String TOPIC_PATH = "topics"; + public static final String FILE_PATH = "files"; public static final String MESSAGING_NOTIFICATION_PATH = "messaging/notifications"; public static final String MESSAGING_DATA_PATH = "messaging/data"; public static final String PROTOCOL_PATH = "protocols"; diff --git a/src/main/java/org/radarbase/appserver/controller/UploadController.java b/src/main/java/org/radarbase/appserver/controller/UploadController.java index e0611771..d87936a7 100644 --- a/src/main/java/org/radarbase/appserver/controller/UploadController.java +++ b/src/main/java/org/radarbase/appserver/controller/UploadController.java @@ -59,6 +59,7 @@ public class UploadController { @PostMapping( "/" + PathsUtil.PROJECT_PATH + "/" + PathsUtil.PROJECT_ID_CONSTANT + "/" + PathsUtil.USER_PATH + "/" + PathsUtil.SUBJECT_ID_CONSTANT + + "/" + PathsUtil.FILE_PATH + "/" + PathsUtil.TOPIC_PATH + "/" + PathsUtil.TOPIC_ID_CONSTANT + "/upload") public ResponseEntity subjectFileUpload( From aabf53ac026ba529427a9542e05d8dfc061183d9 Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Tue, 2 Jul 2024 16:46:31 +0200 Subject: [PATCH 03/12] Change to CREATE MEASUREMENT permission for upload endpoint --- .../org/radarbase/appserver/controller/UploadController.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/radarbase/appserver/controller/UploadController.java b/src/main/java/org/radarbase/appserver/controller/UploadController.java index d87936a7..75feb728 100644 --- a/src/main/java/org/radarbase/appserver/controller/UploadController.java +++ b/src/main/java/org/radarbase/appserver/controller/UploadController.java @@ -52,8 +52,8 @@ public class UploadController { private StorageService storageService; @Authorized( - permission = AuthPermissions.UPDATE, - entity = AuthEntities.SUBJECT, + permission = AuthPermissions.CREATE, + entity = AuthEntities.MEASUREMENT, permissionOn = PermissionOn.SUBJECT ) @PostMapping( From b2cb1b845b612c07082b317b3b3093a36f94df9b Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Tue, 2 Jul 2024 17:29:43 +0200 Subject: [PATCH 04/12] Implement per-day folder and timestamp as part of filename --- .../appserver/config/S3StorageProperties.java | 8 +- .../RandomUuidFilenameStorageService.java | 41 ----- .../service/storage/S3StorageService.java | 33 ++-- .../service/storage/StoragePath.java | 168 ++++++++++++++++++ .../resources/application-prod.properties | 9 +- .../controller/UploadControllerTest.java | 5 +- .../service/storage/S3StorageServiceTest.java | 28 +-- .../service/storage/StoragePathTest.java | 113 ++++++++++++ 8 files changed, 319 insertions(+), 86 deletions(-) delete mode 100644 src/main/java/org/radarbase/appserver/service/storage/RandomUuidFilenameStorageService.java create mode 100644 src/main/java/org/radarbase/appserver/service/storage/StoragePath.java create mode 100644 src/test/java/org/radarbase/appserver/service/storage/StoragePathTest.java diff --git a/src/main/java/org/radarbase/appserver/config/S3StorageProperties.java b/src/main/java/org/radarbase/appserver/config/S3StorageProperties.java index 439c9239..6f790c31 100644 --- a/src/main/java/org/radarbase/appserver/config/S3StorageProperties.java +++ b/src/main/java/org/radarbase/appserver/config/S3StorageProperties.java @@ -28,5 +28,11 @@ public class S3StorageProperties { private String accessKey; private String secretKey; private String bucketName; - private String subPath; + private Path path = new Path(); + + @Data + public static class Path { + private String prefix; + private boolean collectPerDay; + } } diff --git a/src/main/java/org/radarbase/appserver/service/storage/RandomUuidFilenameStorageService.java b/src/main/java/org/radarbase/appserver/service/storage/RandomUuidFilenameStorageService.java deleted file mode 100644 index f4de12dd..00000000 --- a/src/main/java/org/radarbase/appserver/service/storage/RandomUuidFilenameStorageService.java +++ /dev/null @@ -1,41 +0,0 @@ -/* - * - * * Copyright 2024 The Hyve - * * - * * Licensed under the Apache License, Version 2.0 (the "License"); - * * you may not use this file except in compliance with the License. - * * You may obtain a copy of the License at - * * - * * http://www.apache.org/licenses/LICENSE-2.0 - * * - * * Unless required by applicable law or agreed to in writing, software - * * distributed under the License is distributed on an "AS IS" BASIS, - * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * * See the License for the specific language governing permissions and - * * limitations under the License. - * - */ - -package org.radarbase.appserver.service.storage; - -import java.util.UUID; - -public abstract class RandomUuidFilenameStorageService { - - // Storing files under their original filename is a security risk, as it can be used to - // overwrite existing files. This method generates a random filename to mitigate this risk. - // See https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload - String generateRandomFilename(String originalFilename) { - return UUID.randomUUID() + getFileExtension(originalFilename); - } - - private String getFileExtension(String originalFilename) { - int lastDot = originalFilename.lastIndexOf('.'); - if (lastDot < 0) { - return ""; - } else { - return originalFilename.substring(lastDot); - } - } - -} diff --git a/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java index 1f413e77..00a8b7ee 100644 --- a/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java +++ b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java @@ -27,47 +27,44 @@ import org.springframework.util.Assert; import org.springframework.web.multipart.MultipartFile; -import javax.annotation.PostConstruct; - @Slf4j @Service @ConditionalOnExpression("${file-upload.enabled:false} and 's3' == '${storage.type:}'") -public class S3StorageService extends RandomUuidFilenameStorageService implements StorageService { +public class S3StorageService implements StorageService { @Autowired private S3StorageProperties s3StorageProperties; @Autowired private MinioClientInitializer bucketClient; - private String subPath = ""; - - @PostConstruct - public void init() { - if (s3StorageProperties.getSubPath() != null) { - subPath = s3StorageProperties.getSubPath().replaceAll("^/|/$", ""); - if (!subPath.isEmpty()) { - subPath = subPath + "/"; - } - } - } public String store(MultipartFile file, String projectId, String subjectId, String topicId) { Assert.notNull(file, "File must not be null"); Assert.notEmpty(new String[]{projectId, subjectId, topicId}, "Project, subject and topic IDs must not be empty"); - String filePath = String.format("%s%s/%s/%s/%s", subPath, projectId, subjectId, topicId, generateRandomFilename(file.getOriginalFilename())); - log.debug("Storing file at path: {}", filePath); + + StoragePath filePath = StoragePath.builder() + .prefix(s3StorageProperties.getPath().getPrefix()) + .projectId(projectId) + .subjectId(subjectId) + .topicId(topicId) + .collectPerDay(s3StorageProperties.getPath().isCollectPerDay()) + .filename(file.getOriginalFilename()) + .build(); + + log.debug("Attempt storing file at path: {}", filePath.getFullPath()); + try { bucketClient.getClient().putObject(PutObjectArgs .builder() .bucket(bucketClient.getBucketName()) - .object(filePath) + .object(filePath.getFullPath()) .stream(file.getInputStream(), file.getSize(), -1) .build()); } catch (Exception e) { throw new RuntimeException("Could not store file", e); } - return filePath; + return filePath.getPathInTopicDir(); } } diff --git a/src/main/java/org/radarbase/appserver/service/storage/StoragePath.java b/src/main/java/org/radarbase/appserver/service/storage/StoragePath.java new file mode 100644 index 00000000..55c5399b --- /dev/null +++ b/src/main/java/org/radarbase/appserver/service/storage/StoragePath.java @@ -0,0 +1,168 @@ +package org.radarbase.appserver.service.storage; + +import org.springframework.util.Assert; + +import java.time.LocalDate; +import java.time.LocalDateTime; +import java.time.format.DateTimeFormatter; +import java.util.UUID; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +/** + * Represents path on Object Storage for uploaded files. + *

+ * The storage path is constructed to include required arguments (projectId, subjectId, topicId and filename) + * and optional arguments (prefix, collectPerDay, folderTimestampPattern, fileTimestampPattern). The path will follow + * the format: prefix/projectId/subjectId/topicId/[day folder]/timestamp_filename.extension. + * The day folder is included if collectPerDay is set to true. File extensions are converted to lowercase. + *

+ * + *

+ * Usage example: + *

+ * 
+ * StoragePath path = StoragePath.builder()
+ *     .prefix("uploads")
+ *     .projectId("project1")
+ *     .subjectId("subjectA")
+ *     .topicId("topicX")
+ *     .collectPerDay(true)
+ *     .filename("example.txt")
+ *     .build();
+ *
+ * System.out.println(path.getFullPath();
+ * 'uploads/project1/subjectA/topicX/20220101/20220101_example.txt'
+ *
+ * System.out.println(path.getPathInTopicDir()
+ * '20220101/20220101_example.txt'
+ *
+ */ +public class StoragePath { + + private String pathInBucket; + private String pathInTopicDirectory; + + public StoragePath (String pathInBucket, String pathInTopicDirectory) { + this.pathInBucket = pathInBucket; + this.pathInTopicDirectory = pathInTopicDirectory; + } + + public String getFullPath() { + return pathInBucket; + } + + public String getPathInTopicDir() { + return pathInTopicDirectory; + } + + public static Builder builder() { + return new Builder(); + } + + public static class Builder { + + private String prefix = ""; + private String filename = ""; + private boolean collectPerDay = false; + private String projectId = ""; + private String subjectId = ""; + private String topicId = ""; + private String folderTimestampPattern = "yyyyMMdd"; + private String fileTimestampPattern = "yyyyMMddHHmmss"; + private String dirSep = "/"; + + public Builder filename(String filename) { + this.filename = filename; + return this; + } + + public Builder prefix(String prefix) { + Assert.notNull(prefix, "Prefix must not be null"); + this.prefix = prefix; + return this; + } + + public Builder collectPerDay(boolean collectPerDay) { + this.collectPerDay = collectPerDay; + return this; + } + + public Builder projectId(String projectId) { + Assert.notNull(projectId, "Project Id must not be null"); + this.projectId = projectId; + return this; + } + + public Builder subjectId(String subjectId) { + Assert.notNull(subjectId, "Subject Id must not be null"); + this.subjectId = subjectId; + return this; + } + + public Builder topicId(String topicId) { + Assert.notNull(topicId, "Topic Id must not be null"); + this.topicId = topicId; + return this; + } + + public Builder dayFolderPattern(String dayFolderPattern) { + Assert.notNull(dayFolderPattern, "Day folder pattern must not be null"); + this.folderTimestampPattern = dayFolderPattern; + return this; + } + + public Builder fileTimestampPattern(String fileTimestampPattern) { + Assert.notNull(fileTimestampPattern, "File timestamp pattern must not be null"); + this.fileTimestampPattern = fileTimestampPattern; + return this; + } + + public StoragePath build() { + Assert.isTrue(!filename.isBlank(), "Filename must be set."); + Assert.isTrue(!projectId.isBlank(), "Project Id must be set."); + Assert.isTrue(!subjectId.isBlank(), "Subject Id must be set."); + Assert.isTrue(!topicId.isBlank(), "Topic Id must be set."); + + String pathInTopicDir = Stream.of( + this.collectPerDay ? getDayFolder() : "", + // Storing files under their original filename is a security risk, as it can be used to + // overwrite existing files. We generate a random filename server-side to mitigate this risk. + // See https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload + generateRandomFilename(this.filename) + ).filter(s -> !s.isBlank()) + .collect(Collectors.joining(this.dirSep)); + + String fullPath = Stream.of( + this.prefix, + projectId, + subjectId, + topicId, + pathInTopicDir + ).filter(s -> !s.isBlank()) + .collect(Collectors.joining(this.dirSep)); + + return new StoragePath(fullPath, pathInTopicDir); + } + + private String generateRandomFilename(String originalFilename) { + String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern(this.fileTimestampPattern)); + return timestamp + "_" + UUID.randomUUID() + getFileExtension(originalFilename); + } + + private String getDayFolder() { + return LocalDate.now().format(DateTimeFormatter.ofPattern(this.folderTimestampPattern)); + } + + private String getFileExtension(String originalFilename) { + int lastDot = originalFilename.lastIndexOf('.'); + if (lastDot < 0) { + return ""; + } else { + return originalFilename.substring(lastDot).toLowerCase(); + } + } + + } + +} diff --git a/src/main/resources/application-prod.properties b/src/main/resources/application-prod.properties index 9580719b..3884b42c 100644 --- a/src/main/resources/application-prod.properties +++ b/src/main/resources/application-prod.properties @@ -91,8 +91,13 @@ security.github.cache.retryDuration=60 # DATA UPLOAD file-upload.enabled=true -storage.type=s3 # can be 's3' +# Can be only 's3' at the moment. +storage.type=s3 storage.s3.url=http://localhost:9000 storage.s3.access-key=access-key storage.s3.secret-key=secret-key -storage.s3.bucket-name=radar \ No newline at end of file +storage.s3.bucket-name=radar +# Fixed prefix path in the output bucket (e.g., https://s3.amazonaws.com/bucket-name//subject-id/topic-id/...) +storage.s3.path.prefix= +# Collect uploaded files per day in a separate folder (e.g., https://s3.amazonaws.com/bucket-name/subject-id/topic-id/<20240220>/...) +storage.s3.path.collect-per-day=true \ No newline at end of file diff --git a/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java b/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java index 0ab534e2..535c0003 100644 --- a/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java +++ b/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java @@ -43,7 +43,8 @@ @WebMvcTest(UploadController.class) @AutoConfigureMockMvc(addFilters = false) @TestPropertySource(properties = { - "file-upload.enabled=true" + "file-upload.enabled=true", + "security.radar.managementportal.enabled=false" }) class UploadControllerTest { @@ -70,7 +71,7 @@ void setUp() { @Test void testUploadFile() throws Exception { String uri = String.format( - "/projects/%s/users/%s/topics/%s/upload", PROJECT_ID, SUBJECT_ID, TOPIC_ID); + "/projects/%s/users/%s/files/topics/%s/upload", PROJECT_ID, SUBJECT_ID, TOPIC_ID); mockMvc .perform( MockMvcRequestBuilders diff --git a/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java b/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java index 8c596690..85c42716 100644 --- a/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java +++ b/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java @@ -19,17 +19,11 @@ package org.radarbase.appserver.service.storage; import io.minio.MinioClient; -import io.minio.errors.ErrorResponseException; -import io.minio.errors.InsufficientDataException; -import io.minio.errors.InternalException; -import io.minio.errors.InvalidResponseException; -import io.minio.errors.ServerException; -import io.minio.errors.XmlParserException; +import io.minio.PutObjectArgs; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; -import org.mockito.Mockito; import org.radarbase.appserver.config.S3StorageProperties; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.context.properties.EnableConfigurationProperties; @@ -39,17 +33,12 @@ import org.springframework.test.context.junit.jupiter.SpringExtension; import org.springframework.web.multipart.MultipartFile; -import java.io.IOException; -import java.security.InvalidKeyException; -import java.security.NoSuchAlgorithmException; - import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.BDDMockito.given; -import static org.mockito.Mockito.*; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; @ExtendWith(SpringExtension.class) @SpringBootTest( @@ -58,7 +47,7 @@ "file-upload.enabled=true", "storage.type=s3", "storage.s3.bucket-name=my-bucket", - "storage.s3.sub-path=my-sub-path", + "storage.s3.path.prefix=my-sub-path", } ) @EnableConfigurationProperties({S3StorageProperties.class}) @@ -81,7 +70,7 @@ class S3StorageServiceTest { public void setUp() throws Exception { given(minioInit.getBucketName()).willReturn("my-bucket"); given(minioInit.getClient()).willReturn(minioClient); - given(minioClient.putObject(any())).willReturn(null); + given(minioClient.putObject(any(PutObjectArgs.class))).willReturn(null); } @Test @@ -98,13 +87,8 @@ void testInvalidArguments() { @Test void testStore() throws Exception { String path = s3StorageService.store(multipartFile, "projectId", "subjectId", "topicId"); - // Make sure that MinioClient.putObject is called with the correct arguments. - verify(minioClient).putObject(argThat(args -> args.bucket().equals("my-bucket") - && args.object().startsWith("my-sub-path/projectId/subjectId/topicId/") - && args.object().endsWith(".txt")) - ); - - assertTrue(path.startsWith("my-sub-path/projectId/subjectId/topicId/")); + verify(minioClient).putObject(any()); + assertTrue(path.matches("[0-9]+_[a-z0-9-]+\\.txt")); } } diff --git a/src/test/java/org/radarbase/appserver/service/storage/StoragePathTest.java b/src/test/java/org/radarbase/appserver/service/storage/StoragePathTest.java new file mode 100644 index 00000000..ee4495dd --- /dev/null +++ b/src/test/java/org/radarbase/appserver/service/storage/StoragePathTest.java @@ -0,0 +1,113 @@ +package org.radarbase.appserver.service.storage; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.springframework.test.context.junit.jupiter.SpringExtension; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +@ExtendWith(SpringExtension.class) +class StoragePathTest { + + @Test + void minimalValidPath() { + StoragePath path = StoragePath.builder() + .filename("example.txt") + .projectId("project1") + .subjectId("subjectA") + .topicId("topicX") + .build(); + assertTrue(path.getFullPath().matches("project1/subjectA/topicX/[0-9]+_[a-z0-9-]+\\.txt")); + assertTrue(path.getPathInTopicDir().matches("[0-9]+_[a-z0-9-]+\\.txt")); + } + + @Test + void includeDayFolder() { + StoragePath path = StoragePath.builder() + .filename("example.txt") + .projectId("project1") + .subjectId("subjectA") + .topicId("topicX") + .collectPerDay(true) + .build(); + assertTrue(path.getFullPath().matches("project1/subjectA/topicX/[0-9]+/[0-9]+_[a-z0-9-]+\\.txt")); + assertTrue(path.getPathInTopicDir().matches("[0-9]+/[0-9]+_[a-z0-9-]+\\.txt")); + } + + @Test + void includePrefix() { + StoragePath path = StoragePath.builder() + .prefix("prefix") + .filename("example.txt") + .projectId("project1") + .subjectId("subjectA") + .topicId("topicX") + .build(); + assertTrue(path.getFullPath().matches("prefix/project1/subjectA/topicX/[0-9]+_[a-z0-9-]+\\.txt")); + assertTrue(path.getPathInTopicDir().matches("[0-9]+_[a-z0-9-]+\\.txt")); + } + + @Test + void testLowercaseExtension() { + StoragePath path = StoragePath.builder() + .filename("example.TXT") + .projectId("project1") + .subjectId("subjectA") + .topicId("topicX") + .build(); + assertTrue(path.getFullPath().matches("project1/subjectA/topicX/[0-9]+_[a-z0-9-]+\\.txt")); + assertTrue(path.getPathInTopicDir().matches("[0-9]+_[a-z0-9-]+\\.txt")); + } + + @Test + void testAllCombined() { + StoragePath path = StoragePath.builder() + .prefix("prefix") + .filename("example.TXT") + .projectId("project1") + .subjectId("subjectA") + .topicId("topicX") + .collectPerDay(true) + .build(); + assertTrue(path.getFullPath().matches("prefix/project1/subjectA/topicX/[0-9]+/[0-9]+_[a-z0-9-]+\\.txt")); + assertTrue(path.getPathInTopicDir().matches("[0-9]+/[0-9]+_[a-z0-9-]+\\.txt")); + } + + @Test + void testDotsInFilename() { + StoragePath path = StoragePath.builder() + .filename("example.com.txt") + .projectId("project1") + .subjectId("subjectA") + .topicId("topicX") + .build(); + assertTrue(path.getFullPath().matches("project1/subjectA/topicX/[0-9]+_[a-z0-9-]+\\.txt")); + assertTrue(path.getPathInTopicDir().matches("[0-9]+_[a-z0-9-]+\\.txt")); + } + + @Test + void testThrowsIllegalArguments() { + assertThrows(IllegalArgumentException.class, () -> StoragePath.builder() + .projectId("project1") + .subjectId("subjectA") + .topicId("topicX") + .build()); + assertThrows(IllegalArgumentException.class, () -> StoragePath.builder() + .filename("example.txt") + .subjectId("subjectA") + .topicId("topicX") + .build()); + assertThrows(IllegalArgumentException.class, () -> StoragePath.builder() + .filename("example.txt") + .projectId("project1") + .topicId("topicX") + .build()); + assertThrows(IllegalArgumentException.class, () -> StoragePath.builder() + .filename("example.txt") + .projectId("project1") + .subjectId("subjectA") + .build()); + } + +} \ No newline at end of file From 1e4523e18dfbc27400d64102a24cb8df3afdaff2 Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Wed, 3 Jul 2024 11:05:05 +0200 Subject: [PATCH 05/12] Fix PMD tests --- .../controller/UploadController.java | 2 +- .../storage/MinioClientInitializer.java | 6 +- .../service/storage/S3StorageService.java | 4 +- .../service/storage/StoragePath.java | 65 +++++++------- .../controller/UploadControllerTest.java | 2 +- .../service/storage/S3StorageServiceTest.java | 28 +++--- .../service/storage/StoragePathTest.java | 86 ++++++++++--------- 7 files changed, 103 insertions(+), 90 deletions(-) diff --git a/src/main/java/org/radarbase/appserver/controller/UploadController.java b/src/main/java/org/radarbase/appserver/controller/UploadController.java index 75feb728..2802edde 100644 --- a/src/main/java/org/radarbase/appserver/controller/UploadController.java +++ b/src/main/java/org/radarbase/appserver/controller/UploadController.java @@ -49,7 +49,7 @@ public class UploadController { @Autowired - private StorageService storageService; + private transient StorageService storageService; @Authorized( permission = AuthPermissions.CREATE, diff --git a/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java b/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java index 5ee6a2b6..08cb0912 100644 --- a/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java +++ b/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java @@ -31,11 +31,11 @@ @ConditionalOnExpression("${file-upload.enabled:false} and 's3' == '${storage.type:}'") public class MinioClientInitializer { - private MinioClient minioClient; - private String bucketName; + private transient MinioClient minioClient; + private transient String bucketName; @Autowired - private S3StorageProperties s3StorageProperties; + private transient S3StorageProperties s3StorageProperties; @PostConstruct public void init() { diff --git a/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java index 00a8b7ee..fa30b16b 100644 --- a/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java +++ b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java @@ -34,10 +34,10 @@ public class S3StorageService implements StorageService { @Autowired - private S3StorageProperties s3StorageProperties; + private transient S3StorageProperties s3StorageProperties; @Autowired - private MinioClientInitializer bucketClient; + private transient MinioClientInitializer bucketClient; public String store(MultipartFile file, String projectId, String subjectId, String topicId) { Assert.notNull(file, "File must not be null"); diff --git a/src/main/java/org/radarbase/appserver/service/storage/StoragePath.java b/src/main/java/org/radarbase/appserver/service/storage/StoragePath.java index 55c5399b..3648888f 100644 --- a/src/main/java/org/radarbase/appserver/service/storage/StoragePath.java +++ b/src/main/java/org/radarbase/appserver/service/storage/StoragePath.java @@ -5,6 +5,7 @@ import java.time.LocalDate; import java.time.LocalDateTime; import java.time.format.DateTimeFormatter; +import java.util.Locale; import java.util.UUID; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -40,8 +41,8 @@ */ public class StoragePath { - private String pathInBucket; - private String pathInTopicDirectory; + private transient String pathInBucket; + private transient String pathInTopicDirectory; public StoragePath (String pathInBucket, String pathInTopicDirectory) { this.pathInBucket = pathInBucket; @@ -62,82 +63,82 @@ public static Builder builder() { public static class Builder { - private String prefix = ""; - private String filename = ""; - private boolean collectPerDay = false; - private String projectId = ""; - private String subjectId = ""; - private String topicId = ""; - private String folderTimestampPattern = "yyyyMMdd"; - private String fileTimestampPattern = "yyyyMMddHHmmss"; - private String dirSep = "/"; + private transient String pathPrefix = ""; + private transient String file = ""; + private transient boolean doCollectPerDay = false; + private transient String project = ""; + private transient String subject = ""; + private transient String topic = ""; + private transient String folderPattern = "yyyyMMdd"; + private transient String filePattern = "yyyyMMddHHmmss"; + private transient String dirSep = "/"; public Builder filename(String filename) { - this.filename = filename; + this.file = filename; return this; } public Builder prefix(String prefix) { Assert.notNull(prefix, "Prefix must not be null"); - this.prefix = prefix; + this.pathPrefix = prefix; return this; } public Builder collectPerDay(boolean collectPerDay) { - this.collectPerDay = collectPerDay; + this.doCollectPerDay = collectPerDay; return this; } public Builder projectId(String projectId) { Assert.notNull(projectId, "Project Id must not be null"); - this.projectId = projectId; + this.project = projectId; return this; } public Builder subjectId(String subjectId) { Assert.notNull(subjectId, "Subject Id must not be null"); - this.subjectId = subjectId; + this.subject = subjectId; return this; } public Builder topicId(String topicId) { Assert.notNull(topicId, "Topic Id must not be null"); - this.topicId = topicId; + this.topic = topicId; return this; } public Builder dayFolderPattern(String dayFolderPattern) { Assert.notNull(dayFolderPattern, "Day folder pattern must not be null"); - this.folderTimestampPattern = dayFolderPattern; + this.folderPattern = dayFolderPattern; return this; } public Builder fileTimestampPattern(String fileTimestampPattern) { Assert.notNull(fileTimestampPattern, "File timestamp pattern must not be null"); - this.fileTimestampPattern = fileTimestampPattern; + this.filePattern = fileTimestampPattern; return this; } public StoragePath build() { - Assert.isTrue(!filename.isBlank(), "Filename must be set."); - Assert.isTrue(!projectId.isBlank(), "Project Id must be set."); - Assert.isTrue(!subjectId.isBlank(), "Subject Id must be set."); - Assert.isTrue(!topicId.isBlank(), "Topic Id must be set."); + Assert.isTrue(!file.isBlank(), "Filename must be set."); + Assert.isTrue(!project.isBlank(), "Project Id must be set."); + Assert.isTrue(!subject.isBlank(), "Subject Id must be set."); + Assert.isTrue(!topic.isBlank(), "Topic Id must be set."); String pathInTopicDir = Stream.of( - this.collectPerDay ? getDayFolder() : "", + this.doCollectPerDay ? getDayFolder() : "", // Storing files under their original filename is a security risk, as it can be used to // overwrite existing files. We generate a random filename server-side to mitigate this risk. // See https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload - generateRandomFilename(this.filename) + generateRandomFilename(this.file) ).filter(s -> !s.isBlank()) .collect(Collectors.joining(this.dirSep)); String fullPath = Stream.of( - this.prefix, - projectId, - subjectId, - topicId, + this.pathPrefix, + project, + subject, + topic, pathInTopicDir ).filter(s -> !s.isBlank()) .collect(Collectors.joining(this.dirSep)); @@ -146,12 +147,12 @@ public StoragePath build() { } private String generateRandomFilename(String originalFilename) { - String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern(this.fileTimestampPattern)); + String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern(this.filePattern)); return timestamp + "_" + UUID.randomUUID() + getFileExtension(originalFilename); } private String getDayFolder() { - return LocalDate.now().format(DateTimeFormatter.ofPattern(this.folderTimestampPattern)); + return LocalDate.now().format(DateTimeFormatter.ofPattern(this.folderPattern)); } private String getFileExtension(String originalFilename) { @@ -159,7 +160,7 @@ private String getFileExtension(String originalFilename) { if (lastDot < 0) { return ""; } else { - return originalFilename.substring(lastDot).toLowerCase(); + return originalFilename.substring(lastDot).toLowerCase(Locale.ENGLISH); } } diff --git a/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java b/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java index 535c0003..d60ffc5a 100644 --- a/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java +++ b/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java @@ -64,7 +64,7 @@ void setUp() { .willReturn(FILE_PATH); } - MockMultipartFile multipartFile = new MockMultipartFile( + private transient MockMultipartFile multipartFile = new MockMultipartFile( "file", "my-file.txt", "text/plain", file ); diff --git a/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java b/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java index 85c42716..c95a0a0b 100644 --- a/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java +++ b/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java @@ -54,18 +54,22 @@ class S3StorageServiceTest { @Autowired - private S3StorageService s3StorageService; + private transient S3StorageService s3StorageService; @MockBean - private MinioClientInitializer minioInit; + private transient MinioClientInitializer minioInit; @Mock - private MinioClient minioClient; + private transient MinioClient minioClient; - MockMultipartFile multipartFile = new MockMultipartFile( + private transient MockMultipartFile multipartFile = new MockMultipartFile( "file", "my-file.txt", "text/plain", "my-file-content".getBytes() ); + private static final String PROJECT_ID = "projectId"; + private static final String SUBJECT_ID = "subjectId"; + private static final String TOPIC_ID = "topicId"; + @BeforeEach public void setUp() throws Exception { given(minioInit.getBucketName()).willReturn("my-bucket"); @@ -75,18 +79,18 @@ public void setUp() throws Exception { @Test void testInvalidArguments() { - assertThrows(Exception.class, () -> s3StorageService.store(null, "projectId", "subjectId", "topicId")); - assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), null, "subjectId", "topicId")); - assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), "projectId", null, "topicId")); - assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), "projectId", "subjectId", null)); - assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), "", "subjectId", "topicId")); - assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), "projectId", "", "topicId")); - assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), "projectId", "subjectId", "")); + assertThrows(Exception.class, () -> s3StorageService.store(null, PROJECT_ID, SUBJECT_ID, TOPIC_ID)); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), null, SUBJECT_ID, TOPIC_ID)); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), PROJECT_ID, null, TOPIC_ID)); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), PROJECT_ID, SUBJECT_ID, null)); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), "", SUBJECT_ID, TOPIC_ID)); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), PROJECT_ID, "", TOPIC_ID)); + assertThrows(Exception.class, () -> s3StorageService.store(mock(MultipartFile.class), PROJECT_ID, SUBJECT_ID, "")); } @Test void testStore() throws Exception { - String path = s3StorageService.store(multipartFile, "projectId", "subjectId", "topicId"); + String path = s3StorageService.store(multipartFile, PROJECT_ID, SUBJECT_ID, TOPIC_ID); verify(minioClient).putObject(any()); assertTrue(path.matches("[0-9]+_[a-z0-9-]+\\.txt")); } diff --git a/src/test/java/org/radarbase/appserver/service/storage/StoragePathTest.java b/src/test/java/org/radarbase/appserver/service/storage/StoragePathTest.java index ee4495dd..56359969 100644 --- a/src/test/java/org/radarbase/appserver/service/storage/StoragePathTest.java +++ b/src/test/java/org/radarbase/appserver/service/storage/StoragePathTest.java @@ -10,25 +10,33 @@ @ExtendWith(SpringExtension.class) class StoragePathTest { + private static final String PREFIX = "prefix"; + private static final String FILENAME = "example.txt"; + private static final String PROJECT_ID = "project1"; + private static final String SUBJECT_ID = "subjectA"; + private static final String TOPIC_ID = "topicX"; + + private static final String SIMPLE_LOCALFILE_PATTERN = "[0-9]+_[a-z0-9-]+\\.txt"; + @Test void minimalValidPath() { StoragePath path = StoragePath.builder() - .filename("example.txt") - .projectId("project1") - .subjectId("subjectA") - .topicId("topicX") + .filename(FILENAME) + .projectId(PROJECT_ID) + .subjectId(SUBJECT_ID) + .topicId(TOPIC_ID) .build(); assertTrue(path.getFullPath().matches("project1/subjectA/topicX/[0-9]+_[a-z0-9-]+\\.txt")); - assertTrue(path.getPathInTopicDir().matches("[0-9]+_[a-z0-9-]+\\.txt")); + assertTrue(path.getPathInTopicDir().matches(SIMPLE_LOCALFILE_PATTERN)); } @Test void includeDayFolder() { StoragePath path = StoragePath.builder() - .filename("example.txt") - .projectId("project1") - .subjectId("subjectA") - .topicId("topicX") + .filename(FILENAME) + .projectId(PROJECT_ID) + .subjectId(SUBJECT_ID) + .topicId(TOPIC_ID) .collectPerDay(true) .build(); assertTrue(path.getFullPath().matches("project1/subjectA/topicX/[0-9]+/[0-9]+_[a-z0-9-]+\\.txt")); @@ -38,36 +46,36 @@ void includeDayFolder() { @Test void includePrefix() { StoragePath path = StoragePath.builder() - .prefix("prefix") - .filename("example.txt") - .projectId("project1") - .subjectId("subjectA") - .topicId("topicX") + .prefix(PREFIX) + .filename(FILENAME) + .projectId(PROJECT_ID) + .subjectId(SUBJECT_ID) + .topicId(TOPIC_ID) .build(); assertTrue(path.getFullPath().matches("prefix/project1/subjectA/topicX/[0-9]+_[a-z0-9-]+\\.txt")); - assertTrue(path.getPathInTopicDir().matches("[0-9]+_[a-z0-9-]+\\.txt")); + assertTrue(path.getPathInTopicDir().matches(SIMPLE_LOCALFILE_PATTERN)); } @Test void testLowercaseExtension() { StoragePath path = StoragePath.builder() .filename("example.TXT") - .projectId("project1") - .subjectId("subjectA") - .topicId("topicX") + .projectId(PROJECT_ID) + .subjectId(SUBJECT_ID) + .topicId(TOPIC_ID) .build(); assertTrue(path.getFullPath().matches("project1/subjectA/topicX/[0-9]+_[a-z0-9-]+\\.txt")); - assertTrue(path.getPathInTopicDir().matches("[0-9]+_[a-z0-9-]+\\.txt")); + assertTrue(path.getPathInTopicDir().matches(SIMPLE_LOCALFILE_PATTERN)); } @Test void testAllCombined() { StoragePath path = StoragePath.builder() - .prefix("prefix") + .prefix(PREFIX) .filename("example.TXT") - .projectId("project1") - .subjectId("subjectA") - .topicId("topicX") + .projectId(PROJECT_ID) + .subjectId(SUBJECT_ID) + .topicId(TOPIC_ID) .collectPerDay(true) .build(); assertTrue(path.getFullPath().matches("prefix/project1/subjectA/topicX/[0-9]+/[0-9]+_[a-z0-9-]+\\.txt")); @@ -78,35 +86,35 @@ void testAllCombined() { void testDotsInFilename() { StoragePath path = StoragePath.builder() .filename("example.com.txt") - .projectId("project1") - .subjectId("subjectA") - .topicId("topicX") + .projectId(PROJECT_ID) + .subjectId(SUBJECT_ID) + .topicId(TOPIC_ID) .build(); assertTrue(path.getFullPath().matches("project1/subjectA/topicX/[0-9]+_[a-z0-9-]+\\.txt")); - assertTrue(path.getPathInTopicDir().matches("[0-9]+_[a-z0-9-]+\\.txt")); + assertTrue(path.getPathInTopicDir().matches(SIMPLE_LOCALFILE_PATTERN)); } @Test void testThrowsIllegalArguments() { assertThrows(IllegalArgumentException.class, () -> StoragePath.builder() - .projectId("project1") - .subjectId("subjectA") - .topicId("topicX") + .projectId(PROJECT_ID) + .subjectId(SUBJECT_ID) + .topicId(TOPIC_ID) .build()); assertThrows(IllegalArgumentException.class, () -> StoragePath.builder() - .filename("example.txt") - .subjectId("subjectA") - .topicId("topicX") + .filename(FILENAME) + .subjectId(SUBJECT_ID) + .topicId(TOPIC_ID) .build()); assertThrows(IllegalArgumentException.class, () -> StoragePath.builder() - .filename("example.txt") - .projectId("project1") - .topicId("topicX") + .filename(FILENAME) + .projectId(PROJECT_ID) + .topicId(TOPIC_ID) .build()); assertThrows(IllegalArgumentException.class, () -> StoragePath.builder() - .filename("example.txt") - .projectId("project1") - .subjectId("subjectA") + .filename(FILENAME) + .projectId(PROJECT_ID) + .subjectId(SUBJECT_ID) .build()); } From 9cb5ee5cb03196885137d0b6d9d576501fbd210a Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Wed, 3 Jul 2024 12:18:33 +0200 Subject: [PATCH 06/12] Return path to bucket root in HTTP response --- .../radarbase/appserver/service/storage/S3StorageService.java | 2 +- .../appserver/service/storage/S3StorageServiceTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java index fa30b16b..c9aaec0c 100644 --- a/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java +++ b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java @@ -64,7 +64,7 @@ public String store(MultipartFile file, String projectId, String subjectId, Stri } catch (Exception e) { throw new RuntimeException("Could not store file", e); } - return filePath.getPathInTopicDir(); + return filePath.getFullPath(); } } diff --git a/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java b/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java index c95a0a0b..8df9b1f9 100644 --- a/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java +++ b/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java @@ -92,7 +92,7 @@ void testInvalidArguments() { void testStore() throws Exception { String path = s3StorageService.store(multipartFile, PROJECT_ID, SUBJECT_ID, TOPIC_ID); verify(minioClient).putObject(any()); - assertTrue(path.matches("[0-9]+_[a-z0-9-]+\\.txt")); + assertTrue(path.matches("my-sub-path/"+PROJECT_ID+"/"+SUBJECT_ID+"/"+TOPIC_ID+"/[0-9]+_[a-z0-9-]+\\.txt")); } } From 7ec460cc1d259931b40365c74c15fc81c4ead224 Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Wed, 3 Jul 2024 14:06:40 +0200 Subject: [PATCH 07/12] Throw custom exceptions handled by ResponseEntityExceptionHandler --- .../exception/FileStorageException.java | 13 +++++++ .../InvalidFileDetailsException.java | 13 +++++++ .../InvalidPathDetailsException.java | 13 +++++++ .../ResponseEntityExceptionHandler.java | 15 ++++++++ .../service/storage/S3StorageService.java | 37 ++++++++++++------- 5 files changed, 77 insertions(+), 14 deletions(-) create mode 100644 src/main/java/org/radarbase/appserver/exception/FileStorageException.java create mode 100644 src/main/java/org/radarbase/appserver/exception/InvalidFileDetailsException.java create mode 100644 src/main/java/org/radarbase/appserver/exception/InvalidPathDetailsException.java diff --git a/src/main/java/org/radarbase/appserver/exception/FileStorageException.java b/src/main/java/org/radarbase/appserver/exception/FileStorageException.java new file mode 100644 index 00000000..b7c98145 --- /dev/null +++ b/src/main/java/org/radarbase/appserver/exception/FileStorageException.java @@ -0,0 +1,13 @@ +package org.radarbase.appserver.exception; + +public class FileStorageException extends RuntimeException { + private static final long serialVersionUID = -793674245766939L; + + public FileStorageException(String message) { + super(message); + } + + public FileStorageException(String message, Object object) { + super(message + " " + object.toString()); + } +} diff --git a/src/main/java/org/radarbase/appserver/exception/InvalidFileDetailsException.java b/src/main/java/org/radarbase/appserver/exception/InvalidFileDetailsException.java new file mode 100644 index 00000000..f969facc --- /dev/null +++ b/src/main/java/org/radarbase/appserver/exception/InvalidFileDetailsException.java @@ -0,0 +1,13 @@ +package org.radarbase.appserver.exception; + +public class InvalidFileDetailsException extends IllegalArgumentException { + private static final long serialVersionUID = -793674245766939L; + + public InvalidFileDetailsException(String message) { + super(message); + } + + public InvalidFileDetailsException(String message, Object object) { + super(message + " " + object.toString()); + } +} diff --git a/src/main/java/org/radarbase/appserver/exception/InvalidPathDetailsException.java b/src/main/java/org/radarbase/appserver/exception/InvalidPathDetailsException.java new file mode 100644 index 00000000..49be4c0d --- /dev/null +++ b/src/main/java/org/radarbase/appserver/exception/InvalidPathDetailsException.java @@ -0,0 +1,13 @@ +package org.radarbase.appserver.exception; + +public class InvalidPathDetailsException extends IllegalArgumentException { + private static final long serialVersionUID = -793674245766939L; + + public InvalidPathDetailsException(String message) { + super(message); + } + + public InvalidPathDetailsException(String message, Object object) { + super(message + " " + object.toString()); + } +} diff --git a/src/main/java/org/radarbase/appserver/exception/handler/ResponseEntityExceptionHandler.java b/src/main/java/org/radarbase/appserver/exception/handler/ResponseEntityExceptionHandler.java index d111119b..5a7ef43d 100644 --- a/src/main/java/org/radarbase/appserver/exception/handler/ResponseEntityExceptionHandler.java +++ b/src/main/java/org/radarbase/appserver/exception/handler/ResponseEntityExceptionHandler.java @@ -64,6 +64,21 @@ public final ResponseEntity handleInvalidUserDetailsException(Exce return handleEntityWithCause(ex, request); } + @ExceptionHandler(InvalidFileDetailsException.class) + public final ResponseEntity handleInvalidFileDetailsException(Exception ex, WebRequest request) { + return handleEntityWithCause(ex, request); + } + + @ExceptionHandler(InvalidPathDetailsException.class) + public final ResponseEntity handleInvalidPathDetailsException(Exception ex, WebRequest request) { + return handleEntityWithCause(ex, request); + } + + @ExceptionHandler(FileStorageException.class) + public final ResponseEntity handleFileStorageException(Exception ex, WebRequest request) { + return handleEntityWithCause(ex, request); + } + public ResponseEntity handleEntityWithCause(Exception ex, WebRequest request) { String cause = ex.getCause() != null ? ex.getCause().getMessage() : null; HttpStatus status = ex.getClass().getAnnotation(ResponseStatus.class).value(); diff --git a/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java index c9aaec0c..24db2b24 100644 --- a/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java +++ b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java @@ -21,6 +21,9 @@ import io.minio.PutObjectArgs; import lombok.extern.slf4j.Slf4j; import org.radarbase.appserver.config.S3StorageProperties; +import org.radarbase.appserver.exception.FileStorageException; +import org.radarbase.appserver.exception.InvalidFileDetailsException; +import org.radarbase.appserver.exception.InvalidPathDetailsException; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression; import org.springframework.stereotype.Service; @@ -40,31 +43,37 @@ public class S3StorageService implements StorageService { private transient MinioClientInitializer bucketClient; public String store(MultipartFile file, String projectId, String subjectId, String topicId) { - Assert.notNull(file, "File must not be null"); - Assert.notEmpty(new String[]{projectId, subjectId, topicId}, "Project, subject and topic IDs must not be empty"); + if ( + file == null || projectId == null || subjectId == null || topicId == null + || file.isEmpty()|| projectId.isEmpty() || subjectId.isEmpty() || topicId.isEmpty()) { + throw new InvalidFileDetailsException("File, project, subject and topic IDs must not be empty"); + } - StoragePath filePath = StoragePath.builder() - .prefix(s3StorageProperties.getPath().getPrefix()) - .projectId(projectId) - .subjectId(subjectId) - .topicId(topicId) - .collectPerDay(s3StorageProperties.getPath().isCollectPerDay()) - .filename(file.getOriginalFilename()) - .build(); + try { + StoragePath filePath = StoragePath.builder() + .prefix(s3StorageProperties.getPath().getPrefix()) + .projectId(projectId) + .subjectId(subjectId) + .topicId(topicId) + .collectPerDay(s3StorageProperties.getPath().isCollectPerDay()) + .filename(file.getOriginalFilename()) + .build(); - log.debug("Attempt storing file at path: {}", filePath.getFullPath()); + log.debug("Attempt storing file at path: {}", filePath.getFullPath()); - try { bucketClient.getClient().putObject(PutObjectArgs .builder() .bucket(bucketClient.getBucketName()) .object(filePath.getFullPath()) .stream(file.getInputStream(), file.getSize(), -1) .build()); + + return filePath.getFullPath(); + } catch (IllegalArgumentException e) { + throw new InvalidPathDetailsException("There is a problem resolving the path on the object storage", e); } catch (Exception e) { - throw new RuntimeException("Could not store file", e); + throw new FileStorageException("There is a problem storing the file on the object storage", e); } - return filePath.getFullPath(); } } From d1a9ab9988e9836d55de5d9557d83218a76cc0d8 Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Mon, 8 Jul 2024 10:53:05 +0200 Subject: [PATCH 08/12] Add 'radar.' prefix for storage properties --- .../appserver/config/S3StorageProperties.java | 2 +- .../appserver/controller/UploadController.java | 2 +- .../service/storage/MinioClientInitializer.java | 2 +- .../service/storage/S3StorageService.java | 2 +- src/main/resources/application-prod.properties | 16 ++++++++-------- .../controller/UploadControllerTest.java | 2 +- .../service/storage/S3StorageServiceTest.java | 8 ++++---- 7 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/radarbase/appserver/config/S3StorageProperties.java b/src/main/java/org/radarbase/appserver/config/S3StorageProperties.java index 6f790c31..7fe432b3 100644 --- a/src/main/java/org/radarbase/appserver/config/S3StorageProperties.java +++ b/src/main/java/org/radarbase/appserver/config/S3StorageProperties.java @@ -22,7 +22,7 @@ import org.springframework.boot.context.properties.ConfigurationProperties; @Data -@ConfigurationProperties("storage.s3") +@ConfigurationProperties("radar.storage.s3") public class S3StorageProperties { private String url; private String accessKey; diff --git a/src/main/java/org/radarbase/appserver/controller/UploadController.java b/src/main/java/org/radarbase/appserver/controller/UploadController.java index 2802edde..cb4eba4f 100644 --- a/src/main/java/org/radarbase/appserver/controller/UploadController.java +++ b/src/main/java/org/radarbase/appserver/controller/UploadController.java @@ -44,7 +44,7 @@ */ @CrossOrigin @RestController -@ConditionalOnProperty(value = "file-upload.enabled", havingValue = "true") +@ConditionalOnProperty(value = "radar.file-upload.enabled", havingValue = "true") @Slf4j public class UploadController { diff --git a/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java b/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java index 08cb0912..cac9450b 100644 --- a/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java +++ b/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java @@ -28,7 +28,7 @@ import javax.annotation.PostConstruct; @Component -@ConditionalOnExpression("${file-upload.enabled:false} and 's3' == '${storage.type:}'") +@ConditionalOnExpression("${radar.file-upload.enabled:false} and 's3' == '${radar.storage.type:}'") public class MinioClientInitializer { private transient MinioClient minioClient; diff --git a/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java index 24db2b24..e01ef5a6 100644 --- a/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java +++ b/src/main/java/org/radarbase/appserver/service/storage/S3StorageService.java @@ -33,7 +33,7 @@ @Slf4j @Service -@ConditionalOnExpression("${file-upload.enabled:false} and 's3' == '${storage.type:}'") +@ConditionalOnExpression("${radar.file-upload.enabled:false} and 's3' == '${radar.storage.type:}'") public class S3StorageService implements StorageService { @Autowired diff --git a/src/main/resources/application-prod.properties b/src/main/resources/application-prod.properties index 3884b42c..38e44407 100644 --- a/src/main/resources/application-prod.properties +++ b/src/main/resources/application-prod.properties @@ -90,14 +90,14 @@ security.github.cache.duration=3600 security.github.cache.retryDuration=60 # DATA UPLOAD -file-upload.enabled=true +radar.file-upload.enabled=true # Can be only 's3' at the moment. -storage.type=s3 -storage.s3.url=http://localhost:9000 -storage.s3.access-key=access-key -storage.s3.secret-key=secret-key -storage.s3.bucket-name=radar +radar.storage.type=s3 +radar.storage.s3.url=http://localhost:9000 +radar.storage.s3.access-key=access-key +radar.storage.s3.secret-key=secret-key +radar.storage.s3.bucket-name=radar # Fixed prefix path in the output bucket (e.g., https://s3.amazonaws.com/bucket-name//subject-id/topic-id/...) -storage.s3.path.prefix= +radar.storage.s3.path.prefix= # Collect uploaded files per day in a separate folder (e.g., https://s3.amazonaws.com/bucket-name/subject-id/topic-id/<20240220>/...) -storage.s3.path.collect-per-day=true \ No newline at end of file +radar.storage.s3.path.collect-per-day=true \ No newline at end of file diff --git a/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java b/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java index d60ffc5a..6e21ad29 100644 --- a/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java +++ b/src/test/java/org/radarbase/appserver/controller/UploadControllerTest.java @@ -43,7 +43,7 @@ @WebMvcTest(UploadController.class) @AutoConfigureMockMvc(addFilters = false) @TestPropertySource(properties = { - "file-upload.enabled=true", + "radar.file-upload.enabled=true", "security.radar.managementportal.enabled=false" }) class UploadControllerTest { diff --git a/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java b/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java index 8df9b1f9..1fdc4364 100644 --- a/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java +++ b/src/test/java/org/radarbase/appserver/service/storage/S3StorageServiceTest.java @@ -44,10 +44,10 @@ @SpringBootTest( classes = {S3StorageService.class}, properties = { - "file-upload.enabled=true", - "storage.type=s3", - "storage.s3.bucket-name=my-bucket", - "storage.s3.path.prefix=my-sub-path", + "radar.file-upload.enabled=true", + "radar.storage.type=s3", + "radar.storage.s3.bucket-name=my-bucket", + "radar.storage.s3.path.prefix=my-sub-path", } ) @EnableConfigurationProperties({S3StorageProperties.class}) From f1523afe81682a86ab9197849f7a5f3d38f5f21b Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Tue, 9 Jul 2024 16:27:12 +0200 Subject: [PATCH 09/12] Add Location to exposed headers in CORS --- .../radarbase/appserver/controller/UploadController.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/main/java/org/radarbase/appserver/controller/UploadController.java b/src/main/java/org/radarbase/appserver/controller/UploadController.java index cb4eba4f..81acba88 100644 --- a/src/main/java/org/radarbase/appserver/controller/UploadController.java +++ b/src/main/java/org/radarbase/appserver/controller/UploadController.java @@ -28,6 +28,7 @@ import org.springframework.web.bind.annotation.CrossOrigin; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.multipart.MultipartFile; @@ -62,6 +63,12 @@ public class UploadController { "/" + PathsUtil.FILE_PATH + "/" + PathsUtil.TOPIC_PATH + "/" + PathsUtil.TOPIC_ID_CONSTANT + "/upload") + @CrossOrigin( + origins = "*", + allowedHeaders = "*", + exposedHeaders = "Location", // needed to get the URI of the uploaded file in aRMT + methods = { RequestMethod.POST } + ) public ResponseEntity subjectFileUpload( @RequestParam("file") MultipartFile file, @PathVariable String projectId, From 28132f41ec4ca71199638f1807c373d68ced8e1d Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Tue, 9 Jul 2024 16:27:12 +0200 Subject: [PATCH 10/12] Refactor StoragePath --- .../service/storage/StoragePath.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/radarbase/appserver/service/storage/StoragePath.java b/src/main/java/org/radarbase/appserver/service/storage/StoragePath.java index 3648888f..28ff7505 100644 --- a/src/main/java/org/radarbase/appserver/service/storage/StoragePath.java +++ b/src/main/java/org/radarbase/appserver/service/storage/StoragePath.java @@ -125,14 +125,7 @@ public StoragePath build() { Assert.isTrue(!subject.isBlank(), "Subject Id must be set."); Assert.isTrue(!topic.isBlank(), "Topic Id must be set."); - String pathInTopicDir = Stream.of( - this.doCollectPerDay ? getDayFolder() : "", - // Storing files under their original filename is a security risk, as it can be used to - // overwrite existing files. We generate a random filename server-side to mitigate this risk. - // See https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload - generateRandomFilename(this.file) - ).filter(s -> !s.isBlank()) - .collect(Collectors.joining(this.dirSep)); + String pathInTopicDir = buildPathInTopicDir(); String fullPath = Stream.of( this.pathPrefix, @@ -146,6 +139,17 @@ public StoragePath build() { return new StoragePath(fullPath, pathInTopicDir); } + private String buildPathInTopicDir() { + return Stream.of( + this.doCollectPerDay ? getDayFolder() : "", + // Storing files under their original filename is a security risk, as it can be used to + // overwrite existing files. We generate a random filename server-side to mitigate this risk. + // See https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload + generateRandomFilename(this.file) + ).filter(s -> !s.isBlank()) + .collect(Collectors.joining(this.dirSep)); + } + private String generateRandomFilename(String originalFilename) { String timestamp = LocalDateTime.now().format(DateTimeFormatter.ofPattern(this.filePattern)); return timestamp + "_" + UUID.randomUUID() + getFileExtension(originalFilename); From a679464461b35f18853559b72fd434eb68c62a15 Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Tue, 16 Jul 2024 07:36:52 +0200 Subject: [PATCH 11/12] Add @ResponseStatus on file storage exceptions --- .../radarbase/appserver/exception/FileStorageException.java | 4 ++++ .../appserver/exception/InvalidFileDetailsException.java | 4 ++++ .../appserver/exception/InvalidPathDetailsException.java | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/src/main/java/org/radarbase/appserver/exception/FileStorageException.java b/src/main/java/org/radarbase/appserver/exception/FileStorageException.java index b7c98145..d78590f2 100644 --- a/src/main/java/org/radarbase/appserver/exception/FileStorageException.java +++ b/src/main/java/org/radarbase/appserver/exception/FileStorageException.java @@ -1,5 +1,9 @@ package org.radarbase.appserver.exception; +import org.springframework.http.HttpStatus; +import org.springframework.web.bind.annotation.ResponseStatus; + +@ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR) public class FileStorageException extends RuntimeException { private static final long serialVersionUID = -793674245766939L; diff --git a/src/main/java/org/radarbase/appserver/exception/InvalidFileDetailsException.java b/src/main/java/org/radarbase/appserver/exception/InvalidFileDetailsException.java index f969facc..c31e64ed 100644 --- a/src/main/java/org/radarbase/appserver/exception/InvalidFileDetailsException.java +++ b/src/main/java/org/radarbase/appserver/exception/InvalidFileDetailsException.java @@ -1,5 +1,9 @@ package org.radarbase.appserver.exception; +import org.springframework.http.HttpStatus; +import org.springframework.web.bind.annotation.ResponseStatus; + +@ResponseStatus(HttpStatus.EXPECTATION_FAILED) public class InvalidFileDetailsException extends IllegalArgumentException { private static final long serialVersionUID = -793674245766939L; diff --git a/src/main/java/org/radarbase/appserver/exception/InvalidPathDetailsException.java b/src/main/java/org/radarbase/appserver/exception/InvalidPathDetailsException.java index 49be4c0d..bf719ebe 100644 --- a/src/main/java/org/radarbase/appserver/exception/InvalidPathDetailsException.java +++ b/src/main/java/org/radarbase/appserver/exception/InvalidPathDetailsException.java @@ -1,5 +1,9 @@ package org.radarbase.appserver.exception; +import org.springframework.http.HttpStatus; +import org.springframework.web.bind.annotation.ResponseStatus; + +@ResponseStatus(HttpStatus.EXPECTATION_FAILED) public class InvalidPathDetailsException extends IllegalArgumentException { private static final long serialVersionUID = -793674245766939L; From d559e0fe436268bbc8a9b8387ee5b640c7121b07 Mon Sep 17 00:00:00 2001 From: Pim van Nierop Date: Fri, 26 Jul 2024 12:04:37 +0200 Subject: [PATCH 12/12] Add lazy init of S3 client Init at application start interfered with the deployment of RADAR-base where S3 services are installed after Appserver. --- .../appserver/service/storage/MinioClientInitializer.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java b/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java index cac9450b..da559354 100644 --- a/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java +++ b/src/main/java/org/radarbase/appserver/service/storage/MinioClientInitializer.java @@ -25,8 +25,6 @@ import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression; import org.springframework.stereotype.Component; -import javax.annotation.PostConstruct; - @Component @ConditionalOnExpression("${radar.file-upload.enabled:false} and 's3' == '${radar.storage.type:}'") public class MinioClientInitializer { @@ -37,8 +35,7 @@ public class MinioClientInitializer { @Autowired private transient S3StorageProperties s3StorageProperties; - @PostConstruct - public void init() { + private void initClient() { try { minioClient = MinioClient.builder() @@ -57,6 +54,9 @@ public void init() { } public MinioClient getClient() { + if (minioClient == null) { + initClient(); + } return minioClient; }