-
Notifications
You must be signed in to change notification settings - Fork 1
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
Changes from 9 commits
06b0c46
9eb55a8
aabf53a
b2cb1b8
1e4523e
9cb5ee5
7ec460c
d1a9ab9
f1523af
28132f4
a679464
d559e0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
---|---|---|
@@ -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 | ||
) | ||
@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 + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the rest are already specified in the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The functionality is handled by a new StoragePath.Builder class. Here is the Javadoc for reference:
|
||
@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,13 @@ | ||
package org.radarbase.appserver.exception; | ||
|
||
public class FileStorageException extends RuntimeException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you map these to HTTP codes using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added! Can you check this please. I have never used this mechanism. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,13 @@ | ||
package org.radarbase.appserver.exception; | ||
|
||
public class InvalidFileDetailsException extends IllegalArgumentException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you map these to HTTP codes using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added! Can you check this please. I have never used this mechanism. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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,13 @@ | ||
package org.radarbase.appserver.exception; | ||
|
||
public class InvalidPathDetailsException extends IllegalArgumentException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you map these to HTTP codes using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added! Can you check this please. I have never used this mechanism. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 |
---|---|---|
@@ -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("${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; | ||
|
||
@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; | ||
} | ||
|
||
} |
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()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the check for blank values is being done in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
|
||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...