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

[#72] Add backend validation for file component #4227

Merged
merged 9 commits into from
May 17, 2024

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Apr 25, 2024

Partly fixes #72, #4222. I don't know if it should also validate more fields in the submitted values for files?

Copy link

codecov bot commented Apr 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.13%. Comparing base (c19b3c7) to head (f13bd39).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4227      +/-   ##
==========================================
+ Coverage   96.12%   96.13%   +0.01%     
==========================================
  Files         730      730              
  Lines       23522    23546      +24     
  Branches     2759     2762       +3     
==========================================
+ Hits        22611    22637      +26     
+ Misses        645      643       -2     
  Partials      266      266              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

Had some time to think about this PR.

This does not go far enough - the nested DictField that expects anything does not at all provide proper input validation. We either implement validation for a component entirely or not all, so please extend this to cover all the validations.

Additionally, I'm wondering how much of the file validation that is currently happening in other places (mime type, extensions, virus check...) can be moved here. I'd like to concentrate that in a single place instead of smearing it out over the entire codebase.

@Viicos
Copy link
Contributor Author

Viicos commented Apr 26, 2024

mime type, extensions, virus check...

From what I saw when implementing this, these checks are done during the creation of the temporary file upload, which is done as soon as a file is added in the fileupload component widget. So moving this here might require some refactoring

@sergei-maertens
Copy link
Member

mime type, extensions, virus check...

From what I saw when implementing this, these checks are done during the creation of the temporary file upload, which is done as soon as a file is added in the fileupload component widget. So moving this here might require some refactoring

Some of them can be moved, some can't - e.g. the virus check and mime type check are fine to keep with the temp upload, but the checks for file types (extensions) for example should be moved.

The "problem" with the temporary uploads is that we have no trusted input pointing to the responsible component for that upload, so we can only perform generic checks there. But once we have the values & the component (so when building the serializer field and running validation), we are able to resolve the (temporary?) uploads belonging to this component and can run the rest of the validation.

@sergei-maertens
Copy link
Member

As mentioned on Slack, I'd relate this PR to #72 instead :)

@Viicos Viicos force-pushed the issue/4222-max-files branch 2 times, most recently from 22761b6 to d9852b0 Compare May 3, 2024 15:24
@Viicos Viicos marked this pull request as draft May 6, 2024 14:21
@Viicos Viicos force-pushed the issue/4222-max-files branch 4 times, most recently from b1d3f74 to 10a84a4 Compare May 7, 2024 13:02
@Viicos Viicos changed the title [#4222] Add backend validation for file component [#72] Add backend validation for file component May 7, 2024
@Viicos Viicos force-pushed the issue/4222-max-files branch 2 times, most recently from 023bfe8 to 7d81cc5 Compare May 7, 2024 14:26
@Viicos Viicos force-pushed the issue/4222-max-files branch 2 times, most recently from 9798391 to 9a007e8 Compare May 7, 2024 14:59
@Viicos Viicos marked this pull request as ready for review May 7, 2024 15:18
src/openforms/formio/components/vanilla.py Outdated Show resolved Hide resolved
src/openforms/formio/components/vanilla.py Outdated Show resolved Hide resolved
src/openforms/formio/tests/validation/test_file.py Outdated Show resolved Hide resolved
src/openforms/formio/tests/validation/test_file.py Outdated Show resolved Hide resolved
src/openforms/formio/tests/validation/test_file.py Outdated Show resolved Hide resolved
src/openforms/submissions/api/serializers.py Show resolved Hide resolved
src/openforms/formio/tests/test_api_fileupload.py Outdated Show resolved Hide resolved
@sergei-maertens
Copy link
Member

I'm also missing the integration tests that show the frontend validation and backend input blocking are aligned.

Validation is now handled by the component serializer
Tests related to MIME type validation are moved to
the file component validation tests, and simplified a bit.
While tweaking tests, made sure a min_length was used
for the list serializer if the component is required.
src/openforms/tests/e2e/input_validation_base.py Outdated Show resolved Hide resolved
src/openforms/tests/e2e/test_input_validation.py Outdated Show resolved Hide resolved
src/openforms/tests/e2e/test_input_validation.py Outdated Show resolved Hide resolved
@@ -133,7 +140,7 @@ def _assertBackendValidation(self, form: Form, key: str, value: JSONValue):
# step data validation is run *if* a value is provided - it ignores empty data
# for fields that are required. So we accept an HTTP 400, or if a 200/201 is
# returned, then we apply additional checks with the _complete endpoint.
assert response.status_code in [201, 200, 400]
assert response.status_code in (201, 200, 400)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that the match statement correctly exhausts all possible cases

Copy link
Member

Choose a reason for hiding this comment

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

TIL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a pyright only thing by the way, mypy will still understand it as a tuple of ints

Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

BEFORE we merge this, I want to manually click through it as a final check :)

@sergei-maertens
Copy link
Member

Seems fine, I created some weird situations where the config of the component is changed after the files have been uploaded/attached and it's all working intuitively.

@sergei-maertens sergei-maertens merged commit 013b5a1 into master May 17, 2024
27 checks passed
@sergei-maertens sergei-maertens deleted the issue/4222-max-files branch May 17, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As developer I want to implement frontend and backend validation of form fields
2 participants