Upload files using Rest #4300

zdeveloper · 2022-09-20T18:59:00Z

BACKEND PULL REQUEST

Related Issue

resolves Move file uploads to use Rest instead of Graphql #4267

Changes Proposed

new graphql library doesn't natively support multipart file uploads, we got around this issue by adding manual Multipart support based on this Add multipart request support spring-projects/spring-graphql#430, just so we can lower the number of changes to the Spring upgrade PR.
This will remove this manual support and switch file uploads to REST, related convo

Additional Information

decided to not split changes into two PRs to guarantee graphql backward compatibility, for two reasons
- The app front end reloads when it detects that a new version is out
- We have a very low number of file upload users

Testing

Checklist for Primary Reviewer

Any large-scale changes have been deployed to test, dev, or pentest and smoke tested

zdeveloper · 2022-09-22T14:45:15Z

backend/src/main/java/gov/cdc/usds/simplereport/db/model/TestResultUpload.java

@@ -34,6 +35,7 @@ public class TestResultUpload extends AuditedEntity {

  @ManyToOne(optional = false, fetch = FetchType.LAZY)
  @JoinColumn(name = "org_id")
+  @JsonIgnore
  private Organization organization;


Json Ignoring this since we don't want to return it as part of the response object when uploading results

zdeveloper · 2022-09-22T15:34:55Z

backend/src/main/java/gov/cdc/usds/simplereport/api/model/errors/CsvProcessingException.java


 /** An error thrown when CSV uploads fail for reasons other than validation */
+@ResponseStatus(HttpStatus.BAD_REQUEST)


Since allows us to set the response status to 400 when we throw this error inside the upload controller

emmastephenson · 2022-09-22T18:10:17Z

backend/src/main/java/gov/cdc/usds/simplereport/api/uploads/FileUploadController.java

+  private final TestResultUploadService testResultUploadService;
+
+  @PostMapping(PATIENT_UPLOAD)
+  public String handlePatientsUpload(@RequestParam("file") MultipartFile file) {


Is there a way to put some stricter typing/validation on these multipart files? Such that we can reject if it's not a CSV? In particular, I'm worried that since this won't be applying graphQL's argument limits anymore, theoretically people could try to upload massive or incorrectly formatted files - it's a bit of a security concern, even though this endpoint is only open to authenticated users.

~~Looks like it's a Spring configuration we can enable: spring.servlet.multipart.max-file-size~~
Great minds think alike - you fixed this in the same minute I commented 🤣

I am also adding an assert on the type of the file, even though that does nothing to the security since file extensions are a syntactical sugar on files

have we thought about using something like clamav for file type and virus scanning?

(we were using clamav for file upload scanning on a defense project and it worked pretty well, though not sure it would work for this specific use case)

I dont think I ever heard of clamav, it could be something we can look into, doesnt seem like a quick thing to integrate with spring though

no I wasn't suggesting it for this PR 😄 maybe just a future thing if we want to bump up our security

also from what I remember it was not too bad to integrate with java/spring but not a quick add

zdeveloper · 2022-09-22T18:51:58Z

backend/src/main/resources/application.yaml

+      max-file-size: 50MB
+server:
+  error:
+    include-stacktrace: never


This removes the trace from any exceptions returned to the frontend

zdeveloper · 2022-09-22T18:59:01Z

backend/src/main/resources/application.yaml

@@ -49,6 +49,12 @@ spring:
    jdbc:
      initialize-schema: never
      table-name: ${spring.jpa.properties.hibernate.default_schema}.spring_session
+  servlet:
+    multipart:
+      max-file-size: 50MB


This is set to the same max file limit enforced by the frontend for result upload

zdeveloper · 2022-09-22T18:59:33Z

backend/src/test/java/gov/cdc/usds/simplereport/api/patient/PatientMutationResolverTest.java

-        assertThrows(IllegalGraphqlArgumentException.class, () -> sut.uploadPatients(input));
-    assertThat(caught).hasMessage("PANIC");
-  }
-


These tests have been replaced by FileUploadControllerTest

sonarqubecloud · 2022-09-22T19:02:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

94.6% Coverage
0.0% Duplication

georgehager

thanks Zedd! I had a question about how we scan files for upload and like 2 tiny things, but nothing blocking 😄

backend/src/main/java/gov/cdc/usds/simplereport/api/patient/PatientMutationResolver.java

georgehager · 2022-09-22T19:00:05Z

backend/src/main/java/gov/cdc/usds/simplereport/api/uploads/FileUploadController.java

+  private final TestResultUploadService testResultUploadService;
+
+  @PostMapping(PATIENT_UPLOAD)
+  public String handlePatientsUpload(@RequestParam("file") MultipartFile file) {


have we thought about using something like clamav for file type and virus scanning?

georgehager · 2022-09-22T19:02:14Z

backend/src/main/java/gov/cdc/usds/simplereport/api/uploads/FileUploadController.java

+  private final TestResultUploadService testResultUploadService;
+
+  @PostMapping(PATIENT_UPLOAD)
+  public String handlePatientsUpload(@RequestParam("file") MultipartFile file) {


(we were using clamav for file upload scanning on a defense project and it worked pretty well, though not sure it would work for this specific use case)

backend/src/main/java/gov/cdc/usds/simplereport/service/TestResultUploadService.java

backend/src/test/java/gov/cdc/usds/simplereport/api/patient/PatientMutationResolverTest.java

emmastephenson

LGTM! Thanks for getting to these Spring cleanup tickets so quickly 😄

* upload files via rest * update frontend tests * add file upload controller test and remove unused resolvers * remove unused code * add tests and enable cors * add Patient Upload tests * adding sonar feedback * add access-token to FileUploadService * update patient upload error alert title * update uploads error checking * limit upload file size to 50MB in the backend to match frontend * only accept csv files and dont send back exception traces

zdeveloper temporarily deployed to Dev2 September 21, 2022 20:48 Inactive

zdeveloper temporarily deployed to Dev2 September 21, 2022 21:17 Inactive

zdeveloper commented Sep 22, 2022

View reviewed changes

zdeveloper force-pushed the zedd/upload-files-with-rest branch 2 times, most recently from ff73873 to 4c7842c Compare September 22, 2022 15:24

zdeveloper commented Sep 22, 2022

View reviewed changes

zdeveloper temporarily deployed to Dev2 September 22, 2022 16:35 Inactive

zdeveloper temporarily deployed to Dev2 September 22, 2022 17:10 Inactive

zdeveloper changed the title ~~[Draft] Upload files using Rest~~ Upload files using Rest Sep 22, 2022

zdeveloper marked this pull request as ready for review September 22, 2022 17:27

zdeveloper requested review from DanielSass, nathancrtr, emmastephenson, mehansen, BobanL, emyl3 and georgehager September 22, 2022 17:27

zdeveloper temporarily deployed to Dev2 September 22, 2022 18:13 Inactive

emmastephenson reviewed Sep 22, 2022

View reviewed changes

zdeveloper force-pushed the zedd/upload-files-with-rest branch from f45092f to 451e7fe Compare September 22, 2022 18:49

zdeveloper requested a review from emmastephenson September 22, 2022 18:49

zdeveloper added 9 commits September 22, 2022 13:50

upload files via rest

a016039

update frontend tests

8295a04

add file upload controller test and remove unused resolvers

55a4381

remove unused code

315812f

add tests and enable cors

c41e549

add Patient Upload tests

1da2045

adding sonar feedback

8c665d5

add access-token to FileUploadService

243efa3

update patient upload error alert title

dde8c7e

zdeveloper added 3 commits September 22, 2022 13:50

update uploads error checking

4f9d50c

limit upload file size to 50MB in the backend to match frontend

d3e61b3

only accept csv files and dont send back exception traces

db00372

zdeveloper force-pushed the zedd/upload-files-with-rest branch from 451e7fe to db00372 Compare September 22, 2022 18:50

zdeveloper commented Sep 22, 2022

View reviewed changes

zdeveloper temporarily deployed to Dev2 September 22, 2022 18:58 Inactive

zdeveloper commented Sep 22, 2022

View reviewed changes

georgehager approved these changes Sep 22, 2022

View reviewed changes

emmastephenson approved these changes Sep 22, 2022

View reviewed changes

zdeveloper merged commit 692f679 into main Sep 23, 2022

zdeveloper deleted the zedd/upload-files-with-rest branch September 23, 2022 13:41

nathancrtr mentioned this pull request Oct 31, 2022

File size limit for CSV uploads should match in frontend and backend #4242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upload files using Rest #4300

Upload files using Rest #4300

zdeveloper commented Sep 20, 2022 •

edited

Loading

zdeveloper Sep 22, 2022 •

edited

Loading

zdeveloper Sep 22, 2022

emmastephenson Sep 22, 2022

emmastephenson Sep 22, 2022 •

edited

Loading

zdeveloper Sep 22, 2022

georgehager Sep 22, 2022

georgehager Sep 22, 2022

zdeveloper Sep 22, 2022 •

edited

Loading

georgehager Sep 22, 2022

georgehager Sep 22, 2022

zdeveloper Sep 22, 2022 •

edited

Loading

zdeveloper Sep 22, 2022

zdeveloper Sep 22, 2022

sonarqubecloud bot commented Sep 22, 2022

georgehager left a comment •

edited

Loading

georgehager Sep 22, 2022

georgehager Sep 22, 2022

emmastephenson left a comment


		/** An error thrown when CSV uploads fail for reasons other than validation */
		@ResponseStatus(HttpStatus.BAD_REQUEST)

Upload files using Rest #4300

Upload files using Rest #4300

Conversation

zdeveloper commented Sep 20, 2022 • edited Loading

BACKEND PULL REQUEST

Related Issue

Changes Proposed

Additional Information

Testing

Checklist for Primary Reviewer

zdeveloper Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmastephenson Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdeveloper Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdeveloper Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 22, 2022

georgehager left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmastephenson left a comment

Choose a reason for hiding this comment

zdeveloper commented Sep 20, 2022 •

edited

Loading

zdeveloper Sep 22, 2022 •

edited

Loading

emmastephenson Sep 22, 2022 •

edited

Loading

zdeveloper Sep 22, 2022 •

edited

Loading

zdeveloper Sep 22, 2022 •

edited

Loading

georgehager left a comment •

edited

Loading