Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impl. file upload endpoint #470

Merged
merged 12 commits into from
Jul 30, 2024
2 changes: 2 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ ext {
radarSpringAuthVersion = '1.2.1'
springSecurityVersion = '6.0.2'
hibernateValidatorVersion = '8.0.0.Final'
minioVersion = '8.5.10'
}

sourceSets {
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@

@Configuration
@EnableJpaAuditing
@EnableConfigurationProperties({FcmServerConfig.class})
@EnableConfigurationProperties({FcmServerConfig.class, S3StorageProperties.class})
@EnableTransactionManagement
@EnableAsync
@EnableScheduling
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
*
* * 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("radar.storage.s3")
public class S3StorageProperties {
private String url;
private String accessKey;
private String secretKey;
private String bucketName;
private Path path = new Path();

@Data
public static class Path {
private String prefix;
private boolean collectPerDay;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,14 @@ 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";
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";
Expand Down
Original file line number Diff line number Diff line change
@@ -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 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.RequestMethod;
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 = "radar.file-upload.enabled", havingValue = "true")
@Slf4j
public class UploadController {

@Autowired
private transient StorageService storageService;

@Authorized(
permission = AuthPermissions.CREATE,
entity = AuthEntities.MEASUREMENT,
permissionOn = PermissionOn.SUBJECT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to use CREATE MEASUREMENT permissions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops you are right! Updated...

)
@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 +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add "files" to the path too like .../files/topics to denote it is for file uploads

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

"/upload")
@CrossOrigin(
origins = "*",
allowedHeaders = "*",
exposedHeaders = "Location", // needed to get the URI of the uploaded file in aRMT
methods = { RequestMethod.POST }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the rest are already specified in the MultiHttpSecurityConfig, should we only use exposedHeaders here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we tried this and it dit not work. Leaving out origins caused the annotation to not work. It is almost as if the MultiHttpSecurityConfig does not apply to the endpoints. We also tried adding .exposedHeaders(...) to MultiHttpSecurityConfig but this also did not work. The annotation in its current form above the upload method was needed.

)
public ResponseEntity<?> subjectFileUpload(
@RequestParam("file") MultipartFile file,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not accept a filename parameter too, which by default can be the random name you generate below.

Copy link
Contributor Author

@pvannierop pvannierop Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a security risk. The uploading party better not determine the filename if not needed. See OWASP Unrestricted file upload. I decided to assign server-side randomized filename for simplicity so that I did not have to do filename sanitization.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, good point. Can you then add the timestamp to the generated file name when like {timestamp}_{randomUUID}.{ext}? And can also go in a folder per day like 022024)

Copy link
Contributor Author

@pvannierop pvannierop Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this feature. A timestamp is always added in front of the random filename as per your suggestion. You can opt-in for collection per day via a new property storage.s3.path.collect-per-day.

The functionality is handled by a new StoragePath.Builder class. Here is the Javadoc for reference:

 * Represents path on Object Storage for uploaded files.
 * <p>
 * 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.
 * </p>
 *
 * <p>
 * Usage example:
 * </p>
 * <pre>
 * 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'
 * </pre>

@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();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you map these to HTTP codes using @ResponseStatus(HttpStatus.XYZ)? Perhaps http code 500 in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added! Can you check this please. I have never used this mechanism.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes looks good, thanks

private static final long serialVersionUID = -793674245766939L;

public FileStorageException(String message) {
super(message);
}

public FileStorageException(String message, Object object) {
super(message + " " + object.toString());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you map these to HTTP codes using @ResponseStatus(HttpStatus.XYZ)? Perhaps Expectation failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added! Can you check this please. I have never used this mechanism.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes looks good, thanks

private static final long serialVersionUID = -793674245766939L;

public InvalidFileDetailsException(String message) {
super(message);
}

public InvalidFileDetailsException(String message, Object object) {
super(message + " " + object.toString());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you map these to HTTP codes using @ResponseStatus(HttpStatus.XYZ)? Perhaps Expectation failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added! Can you check this please. I have never used this mechanism.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the code being added, can you double check and add @ResponseStatus(HttpStatus.EXPECTATION_FAILED) here. Thanks.

private static final long serialVersionUID = -793674245766939L;

public InvalidPathDetailsException(String message) {
super(message);
}

public InvalidPathDetailsException(String message, Object object) {
super(message + " " + object.toString());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,21 @@ public final ResponseEntity<ErrorDetails> handleInvalidUserDetailsException(Exce
return handleEntityWithCause(ex, request);
}

@ExceptionHandler(InvalidFileDetailsException.class)
public final ResponseEntity<ErrorDetails> handleInvalidFileDetailsException(Exception ex, WebRequest request) {
return handleEntityWithCause(ex, request);
}

@ExceptionHandler(InvalidPathDetailsException.class)
public final ResponseEntity<ErrorDetails> handleInvalidPathDetailsException(Exception ex, WebRequest request) {
return handleEntityWithCause(ex, request);
}

@ExceptionHandler(FileStorageException.class)
public final ResponseEntity<ErrorDetails> handleFileStorageException(Exception ex, WebRequest request) {
return handleEntityWithCause(ex, request);
}

public ResponseEntity<ErrorDetails> handleEntityWithCause(Exception ex, WebRequest request) {
String cause = ex.getCause() != null ? ex.getCause().getMessage() : null;
HttpStatus status = ex.getClass().getAnnotation(ResponseStatus.class).value();
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

@Component
@ConditionalOnExpression("${radar.file-upload.enabled:false} and 's3' == '${radar.storage.type:}'")
public class MinioClientInitializer {

private transient MinioClient minioClient;
private transient String bucketName;

@Autowired
private transient S3StorageProperties s3StorageProperties;

private void initClient() {
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() {
if (minioClient == null) {
initClient();
}
return minioClient;
}

public String getBucketName() {
return bucketName;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
*
* * 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.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;
import org.springframework.util.Assert;
import org.springframework.web.multipart.MultipartFile;


@Slf4j
@Service
@ConditionalOnExpression("${radar.file-upload.enabled:false} and 's3' == '${radar.storage.type:}'")
public class S3StorageService implements StorageService {

@Autowired
private transient S3StorageProperties s3StorageProperties;

@Autowired
private transient MinioClientInitializer bucketClient;

public String store(MultipartFile file, String projectId, String subjectId, String topicId) {
if (
file == null || projectId == null || subjectId == null || topicId == null
|| file.isEmpty()|| projectId.isEmpty() || subjectId.isEmpty() || topicId.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the check for blank values is being done in StoragePath, maybe we don't need the check for empty here anymore?

Copy link
Contributor Author

@pvannierop pvannierop Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that every component should enforce its own contract/API. But this is my opinion.

throw new InvalidFileDetailsException("File, project, subject and topic IDs must not be empty");
}

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());

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 FileStorageException("There is a problem storing the file on the object storage", e);
}
}

}
Loading
Loading