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.

Sign up for GitHub

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

Jump to bottom

Impl. file upload endpoint #470

Merged
merged 12 commits into from
Jul 30, 2024
Merged

Impl. file upload endpoint #470

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
06b0c46
Impl. file upload endpoint
pvannierop May 31, 2024
9eb55a8
Add '/files' to the REST path for file upload
pvannierop Jul 2, 2024
aabf53a
Change to CREATE MEASUREMENT permission for upload endpoint
pvannierop Jul 2, 2024
b2cb1b8
Implement per-day folder and timestamp as part of filename
pvannierop Jul 2, 2024
1e4523e
Fix PMD tests
pvannierop Jul 3, 2024
9cb5ee5
Return path to bucket root in HTTP response
pvannierop Jul 3, 2024
7ec460c
Throw custom exceptions handled by ResponseEntityExceptionHandler
pvannierop Jul 3, 2024
d1a9ab9
Add 'radar.' prefix for storage properties
pvannierop Jul 8, 2024
f1523af
Add Location to exposed headers in CORS
pvannierop Jul 9, 2024
28132f4
Refactor StoragePath
pvannierop Jul 9, 2024
a679464
Add @ResponseStatus on file storage exceptions
pvannierop Jul 16, 2024
d559e0f
Add lazy init of S3 client
pvannierop Jul 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/main/java/org/radarbase/appserver/controller/UploadController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -62,6 +63,12 @@ public class UploadController {
"/" + 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 •
edited
Loading

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 •
edited
Loading

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,
Expand Down
Loading