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

♻️(backend) allow uploading more types of attachments #309

Merged
merged 2 commits into from
Oct 16, 2024
Merged

♻️(backend) allow uploading more types of attachments #309

merged 2 commits into from
Oct 16, 2024

Conversation

sampaccoud
Copy link
Member

Purpose

We want to allow users to upload files to a document, not just images.

Proposal

We try to enforce coherence between the file extension and the real mime type of its content using magic.

If a file is deemed unsafe, it is still accepted during upload and the information is stored as metadata on the object for display to readers in the frontend.

@sampaccoud sampaccoud requested review from qbey and AntoLC October 7, 2024 18:20
@sampaccoud sampaccoud self-assigned this Oct 7, 2024
@sampaccoud sampaccoud added python Pull requests that update Python code feature backend labels Oct 7, 2024
@sampaccoud sampaccoud force-pushed the accept-file-uploads-beyond-images branch 2 times, most recently from 385085c to 69207ba Compare October 7, 2024 18:25
qbey
qbey reviewed Oct 7, 2024
View reviewed changes
Copy link
Collaborator

@qbey qbey left a comment

Choose a reason for hiding this comment

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

Nice :) I've made few nit picking comments

src/backend/core/api/serializers.py Outdated Show resolved Hide resolved
src/backend/core/api/serializers.py Outdated Show resolved Hide resolved
src/backend/core/api/viewsets.py Outdated Show resolved Hide resolved
src/backend/core/api/serializers.py Outdated Show resolved Hide resolved
@sampaccoud sampaccoud force-pushed the accept-file-uploads-beyond-images branch from 0119d10 to 0171de7 Compare October 11, 2024 19:48
@sampaccoud sampaccoud enabled auto-merge (rebase) October 11, 2024 19:48
@sampaccoud sampaccoud force-pushed the accept-file-uploads-beyond-images branch from 5b0a877 to 9d5139a Compare October 14, 2024 07:53
@sampaccoud
Copy link
Member Author

sampaccoud commented Oct 14, 2024 •
edited
Loading

@qbey I had to change things a little because I realized the mimetype guessed by magic depends on the OS when my tests failed to pass in the CI...
With the new fixup, I stopped trying to correct incoherent extensions and will just mark the file as unsecure... not sure what we will do about it but it seems hard to do more without introducing a lot of issues for peaceful users.
If the extension is missing, I can still add.
Curious to know your thoughts on this refacto!

qbey
qbey approved these changes Oct 14, 2024
View reviewed changes
Copy link
Collaborator

@qbey qbey left a comment

Choose a reason for hiding this comment

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

I was not aware the extension guess depends on the OS, moreover for simple file types... This fix looks good to me then, I don't have a better approach in mind.

src/backend/core/api/serializers.py Show resolved Hide resolved
AntoLC
AntoLC approved these changes Oct 14, 2024
View reviewed changes
Copy link
Collaborator

@AntoLC AntoLC left a comment

Choose a reason for hiding this comment

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

My review is from a frontside perspective.
Works well! Nice to have the video tag automatically added in the doc when we add a video ext.

Bad point but on the frontend side, I can see we have to implement the toolbar to be able to download the files.

scrnli_4VrNcOng6R55mX.webm

@sampaccoud sampaccoud force-pushed the accept-file-uploads-beyond-images branch 3 times, most recently from 96db401 to 509834a Compare October 15, 2024 22:03
@sampaccoud
➖(backend) move freezegun to dev dependencies
9d82b3c 
Freezegun is for testing and should not be installed in the
production image.
@sampaccoud
✨(backend) allow uploading more types of attachments
470d38e 
We want to allow users to upload files to a document, not just images.
We try to enforce coherence between the file extension and the real
mime type of its content. If a file is deemed unsafe, it is still accepted
during upload and the information is stored as metadata on the object
for display to readers.
@sampaccoud sampaccoud force-pushed the accept-file-uploads-beyond-images branch from 509834a to 470d38e Compare October 16, 2024 17:31
@sampaccoud sampaccoud merged commit e8d95fa into main Oct 16, 2024
15 of 16 checks passed
@sampaccoud sampaccoud deleted the accept-file-uploads-beyond-images branch October 16, 2024 17:40
@AntoLC AntoLC mentioned this pull request Oct 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AntoLC AntoLC AntoLC approved these changes

@qbey qbey qbey approved these changes

Assignees

@sampaccoud sampaccoud

Labels
backend feature python Pull requests that update Python code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sampaccoud @qbey @AntoLC