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

setup buckets for file-uploads #503

Merged
merged 10 commits into from
Nov 28, 2023
Merged

setup buckets for file-uploads #503

merged 10 commits into from
Nov 28, 2023

Conversation

fscherf
Copy link
Member

@fscherf fscherf commented Nov 12, 2023

This PR adds a new subsystem called Buckets which can be used to upload files or to make files accessible via HTTP

`ViewRuntime.run_cleanup_hook` already runs in a thread.

Signed-off-by: Florian Scherf <mail@florianscherf.de>
Signed-off-by: Florian Scherf <mail@florianscherf.de>
Signed-off-by: Florian Scherf <mail@florianscherf.de>
Signed-off-by: Florian Scherf <mail@florianscherf.de>
Signed-off-by: Florian Scherf <mail@florianscherf.de>
Signed-off-by: Florian Scherf <mail@florianscherf.de>
Signed-off-by: Florian Scherf <mail@florianscherf.de>
This id is intended to be used as identifier for sub-systems like channels
or buckets.

Signed-off-by: Florian Scherf <mail@florianscherf.de>
This shortcut is intended to make type checks and documentation more readable.

Signed-off-by: Florian Scherf <mail@florianscherf.de>
Signed-off-by: Florian Scherf <mail@florianscherf.de>
@fscherf fscherf linked an issue Nov 12, 2023 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2023

Codecov Report

Attention: 59 lines in your changes are missing coverage. Please review.

Comparison is base (9181128) 73.04% compared to head (9361ac5) 73.19%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
+ Coverage   73.04%   73.19%   +0.14%     
==========================================
  Files          85       88       +3     
  Lines        5869     6103     +234     
  Branches     1275     1326      +51     
==========================================
+ Hits         4287     4467     +180     
- Misses       1314     1355      +41     
- Partials      268      281      +13     
Files Coverage Δ
lona/__init__.py 100.00% <100.00%> (ø)
lona/default_settings.py 100.00% <100.00%> (ø)
lona/html/mixins.py 100.00% <100.00%> (ø)
lona/html/nodes/raw_nodes.py 100.00% <100.00%> (ø)
lona/middleware_controller.py 82.29% <100.00%> (+1.83%) ⬆️
lona/request.py 81.08% <100.00%> (+2.95%) ⬆️
lona/unique_ids.py 100.00% <100.00%> (ø)
lona/server.py 71.19% <66.66%> (+0.76%) ⬆️
lona/view_runtime.py 81.86% <77.77%> (-0.44%) ⬇️
lona/buckets.py 86.03% <86.03%> (ø)
... and 1 more

... and 1 file with indirect coverage changes

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

Copy link
Contributor

@SmithChart SmithChart left a comment

Choose a reason for hiding this comment

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

Hey @fscherf , as far as I can tell: It looks good :-)

Two things I found:

  • Is there a way to set the upload limit? Currently lona_dopzone limits the size to 250MB.
  • Maybe it's a good idea to have a settings to disable the management-interface for a bucket by default? I guess most of the time it's not needed.

Once it's merged I will integrate it into one of my projects to see, how it behaves there.

@fscherf
Copy link
Member Author

fscherf commented Nov 19, 2023

Hi @SmithChart,

nice! Thanks for testing and reviewing! :)

Both is already implemented. Buckets have the arguments max_size, which limits the size in bytes in a bucket, max_files which limits the amount of files in a bucket (can be used together), and index which enables, or disables the HTTP/HTML management interface.

lona-dropzone supports all of these arguments, and you can feed an pre-configured bucket in it.

9361ac5#diff-e6778aa47b2825caf9510d3808762cea575cd372d680e4437970f925c284e35cR62

@SmithChart
Copy link
Contributor

Both is already implemented. Buckets have the arguments max_size, which limits the size in bytes in a bucket, max_files which limits the amount of files in a bucket (can be used together), and index which enables, or disables the HTTP/HTML management interface.

lona-dropzone supports all of these arguments, and you can feed an pre-configured bucket in it.

That's an option. But consider the following situation: Let's assume we are working on a project that is, later on, intended to be public-facing (without any authentication).
For development I would like to have all buckets in debug-mode. But for production I would like to switch these features off.
If this has a preset in the settings I could just add one setting to get debugging on or off.
Otherwise I would need to integrate such setting into every bucket.

And: If the setting would default to "off", this would be a bit more secure by default.

@fscherf
Copy link
Member Author

fscherf commented Nov 19, 2023

I thought about that too and I am still not sure about the security aspect of the index page. On the one hand, I agree, it feels more "secure" not to have that on in production, but on the other hand, it does not allow you anything you can't do using curl or wget. The file and size limits also apply to the index page.

@fscherf
Copy link
Member Author

fscherf commented Nov 22, 2023

@SmithChart
I thought a bit more about the security aspect and rechecked the code. I don't think that the index page is a security concern. It lets you only do what you also could do with curl. The URL, or the token in the URL rather, is the short-lived, shared secret between the view, the user, and the middleware. When you have the token, you can upload files. It is not more or less secure than the session tokens.

@SmithChart
Copy link
Contributor

Totally right. A user could extract the token by hand and do the same.
And: Since the secret is only shared with the user / client, we do not really need to take care of checking a login here, or do we?

@fscherf
Copy link
Member Author

fscherf commented Nov 25, 2023

@SmithChart: I don't think so. I think it is reasonably secure like this

@SmithChart
Copy link
Contributor

OK. From my side: Feel free to merge this :-)

@fscherf
Copy link
Member Author

fscherf commented Nov 28, 2023

@SmithChart: Great! Thanks for your help and patience :)

@fscherf fscherf merged commit 33a2545 into master Nov 28, 2023
5 checks passed
@fscherf fscherf deleted the fscherf/setup-buckets branch November 28, 2023 18:42
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.

Question: How to upload files?
3 participants